eslint / eslint-scope

eslint-scope: ECMAScript scope analyzer
BSD 2-Clause "Simplified" License
125 stars 30 forks source link

`"use strict"` in ES3 #86

Closed mdjermanovic closed 2 years ago

mdjermanovic commented 2 years ago

Per https://github.com/eslint/eslint/issues/15456, "use strict" directives should not apply in ES3.

eslint-scope currently does apply "use strict" directives in ES3 mode. Given the above, that behavior looks like a bug.

This is observable with no-invalid-this rule, which uses Scope#isStrict.

/* eslint no-invalid-this: "error" */

function foo() {
    "use strict";
    this;
}

This rule aims to report undefined this. Lowercase functions are assumed to be called without this. If the function is strict code, then this is undefined and it should be reported. If the function is non-strict code, then this refers to the global object and therefore it shouldn't be reported.

Online Demo with ES3 - the rule reports error, which seems wrong because this isn't strict code in ES3.

nzakas commented 2 years ago

It seems like there are multiple areas trying to track strict mode.

isStrictModeSupported: https://github.com/eslint/eslint-scope/blob/d756f1ea974093c3ed7121d17f858254036b9690/lib/scope-manager.js#L80

useDirective: https://github.com/eslint/eslint-scope/blob/d756f1ea974093c3ed7121d17f858254036b9690/lib/scope-manager.js#L56

isStrictScope: https://github.com/eslint/eslint-scope/blob/22a55c01d1a28fd3ffd45c8818b49e65bd3e5005/lib/scope.js#L45

Can you narrow down where the problem is coming from? I’m guessing from useDirective, which doesn’t seem necessary as a separate option from ecmaVersion.

mdjermanovic commented 2 years ago

__useDirective() returns the value of the configuration option directive.

(by "configuration option" I mean an option passed to analyze() on public API)

It doesn't control whether or not "use strict" directives will be processed and applied. It controls only how will eslint-scope technically look for "use strict" directives in given AST.

When the option is true, eslint-scope looks for DirectiveStatement nodes:

https://github.com/eslint/eslint-scope/blob/ceb8bdd2cc31f67255e37a961096f9e3320abac6/lib/scope.js#L86-L97

That's probably an alternative AST spec that was actual when escope was originally written. ESTree spec doesn't have DirectiveStatement nodes.

When the option is false (default), eslint-scope looks for ExpressionStatement nodes:

https://github.com/eslint/eslint-scope/blob/ceb8bdd2cc31f67255e37a961096f9e3320abac6/lib/scope.js#L97-L119

That's per the ESTree spec.

ESLint doesn't set the directive option, so by default eslint-scope works with ESTree AST.

The conclusion is that it doesn't seem that __useDirective() is causing problems with ES3 described in this issue.

Off-topic, we could consider dropping the directive option and the DirectiveStatement branch in a future major version. We could also consider refactoring the other branch to use ESTree's directive property.

mdjermanovic commented 2 years ago

isStrictScope() is the main function that calculates whether a given scope is strict by looking at its upper scope, nodes, directives, etc.

isStrictModeSupported() is a helper function added in https://github.com/estools/escope/pull/93 only to check if the impliedStrict option value should be respected given the other options. It's currently used only here:

https://github.com/eslint/eslint-scope/blob/ceb8bdd2cc31f67255e37a961096f9e3320abac6/lib/referencer.js#L409-L425

I believe this issue will be fixed by setting this.isStrict = false if scopeManager.isStrictModeSupported() returns false, here:

https://github.com/eslint/eslint-scope/blob/ceb8bdd2cc31f67255e37a961096f9e3320abac6/lib/scope.js#L262-L266

mdjermanovic commented 2 years ago

PR https://github.com/eslint/eslint-scope/pull/87