Repository: eslint/eslint
Pull request #15712 (2022-03-21 14:19:28)

feat: fix logic for top-level `this` in no-invalid-this and no-eval
<!--
    Thank you for contributing!

    ESLint adheres to the [JS Foundation Code of Conduct](https://eslint.org/conduct).
-->

#### Prerequisites checklist

- [x] I have read the [contributing guidelines](https://github.com/eslint/eslint/blob/HEAD/CONTRIBUTING.md).

#### What is the purpose of this pull request? (put an "X" next to an item)

<!--
    The following template is intentionally not a markdown checkbox list for the reasons
    explained in https://github.com/eslint/eslint/pull/12848#issuecomment-580302888
-->

[ ] Documentation update
[x] Bug fix ([template](https://raw.githubusercontent.com/eslint/eslint/HEAD/templates/bug-report.md))
[ ] New rule ([template](https://raw.githubusercontent.com/eslint/eslint/HEAD/templates/rule-proposal.md))
[ ] Changes an existing rule ([template](https://raw.githubusercontent.com/eslint/eslint/HEAD/templates/rule-change-proposal.md))
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

This bug fix produces more warnings from the `no-eval` rule, and thus this change is marked as `feat`.

**Tell us about your environment:**

* **ESLint Version:** v8.11.0
* **Node Version:** 16.14.0
* **npm Version:** 8.3.1

**What parser (default, `@babel/eslint-parser`, `@typescript-eslint/parser`, etc.) are you using?**

default

**Please show your full configuration:**

<details>
<summary>Configuration</summary>

<!-- Paste your configuration below: -->
```js
module.exports = {
    parserOptions: {
        ecmaVersion: 2015
    }
};

```

</details>

**What did you do? Please include the actual source code causing the issue.**

[Online Demo](https://eslint.org/demo#eyJ0ZXh0IjoiLyogZXNsaW50IG5vLWludmFsaWQtdGhpczogXCJlcnJvclwiLCBuby1ldmFsOiBcImVycm9yXCIgKi9cblxuXCJ1c2Ugc3RyaWN0XCI7XG5cbnRoaXMuZXZhbChcImFsZXJ0KCdmb28nKVwiKTtcbiIsIm9wdGlvbnMiOnsicGFyc2VyT3B0aW9ucyI6eyJlY21hVmVyc2lvbiI6ImxhdGVzdCIsInNvdXJjZVR5cGUiOiJzY3JpcHQiLCJlY21hRmVhdHVyZXMiOnt9fSwicnVsZXMiOnt9LCJlbnYiOnt9fX0=)

```js
/* eslint no-invalid-this: "error", no-eval: "error" */

"use strict";

this.eval("alert('foo')");

```

**What did you expect to happen?**

No `no-invalid-this` errors, one `no-eval` error.

At the top level of scripts, `this` always refers to the global object, regardless of `"use strict"`.

In browsers, the above code alerts `foo`, so `this` is valid and `this.eval()` is an `eval` call.

**What actually happened? Please include the actual, raw output from ESLint.**

One `no-invalid-this` error, no `no-eval` errors.

```
  5:1  error  Unexpected 'this'  no-invalid-this

✖ 1 problem (1 error, 0 warnings)
```

<!--
    If the item you've checked above has a template, please paste the template questions below and answer them. (If this pull request is addressing an issue, you can just paste a link to the issue here instead.)
-->

<!--
    Please ensure your pull request is ready:

    - Read the pull request guide (https://eslint.org/docs/developer-guide/contributing/pull-requests)
    - Include tests for this change
    - Update documentation for this change (if appropriate)
-->

<!--
    The following is required for all pull requests:
-->

#### What changes did you make? (Give an overview)

Fixed the `no-invalid-this` to treat `this` at the top level of scripts as valid, regardless of strict mode.

Fixed the `no-eval` rule to treat `this.eval` at the top level of scripts as access to the global `eval`, regardless of strict mode.

#### Is there anything you'd like reviewers to focus on?

<!-- markdownlint-disable-file MD004 -->


aladdin-add: ```js
"use strict";

this;
```
I just had thought `this` should be `undef`. lol.
mdjermanovic: I believe this rule only intends to report `this` that is `undefined` and thus "invalid". Otherwise, it wouldn't check whether or not functions are in strict mode.

For example:

```js
// not strict

function foo() {
    this.alert('foo');
}
```

If the intent was to "disallow `this` keywords outside of classes or class-like objects.", then the above `this` would be reported regardless of `"use strict`", because it is considered to be outside of classes or class-like objects. But it isn't reported, because in this case `this` is the global object. If we add `"use strict"`, that `this` becomes `undefined` and it is reported by this rule.

The same applies to top-level this. The following code is not reported by `no-invalid-this`:

```js
// not strict

this.alert('foo');
```

But the following is:

```js
"use strict";

this.alert('foo');
```

That is the bug, as there's no difference between these two cases, both `this` refer to the global object.

mdjermanovic: In my opinion, we should change "Disallows `this` keywords outside of classes or class-like objects." to something like "Disallows use of `this` where it is `undefined`.". By the current behavior and implementation details, that was the intent for this rule. Otherwise, it wouldn't allow the following code:

```js
/* eslint no-invalid-this: "error" */

this.alert('foo');

() => {
    this.alert('foo');
}

function foo() {
    this.alert('foo');
}
```


mdjermanovic: I'll close-reopen to try to run checks.
mdjermanovic: I updated the docs for `no-invalid-this` to clarify this rule.

I also noticed that `no-eval` doesn't work well with `this` in arrow functions, and wanted to check if that's due to changes from this PR, but it isn't. I'll fix that in another PR.

Patch:
diff --git a/docs/rules/no-invalid-this.md b/docs/rules/no-invalid-this.md
index 91e460da7..bbdc79cbc 100644
--- a/docs/rules/no-invalid-this.md
+++ b/docs/rules/no-invalid-this.md
@@ -1,14 +1,18 @@
 # no-invalid-this
 
-Disallows `this` keywords outside of classes or class-like objects.
+Disallows use of `this` in contexts where the value of `this` is `undefined`.
 
 Under the strict mode, `this` keywords outside of classes or class-like objects might be `undefined` and raise a `TypeError`.
 
 ## Rule Details
 
-This rule aims to flag usage of `this` keywords outside of classes or class-like objects.
+This rule aims to flag usage of `this` keywords in contexts where the value of `this` is `undefined`.
 
-Basically, this rule checks whether or not a function containing `this` keyword is a constructor or a method.
+Top-level `this` in scripts is always considered valid because it refers to the global object regardless of the strict mode.
+
+Top-level `this` in ECMAScript modules is always considered invalid because its value is `undefined`.
+
+For `this` inside functions, this rule basically checks whether or not the function containing `this` keyword is a constructor or a method. Note that arrow functions have lexical `this`, and that therefore this rule checks their enclosing contexts.
 
 This rule judges from following conditions whether or not the function is a constructor:
 
@@ -30,6 +34,7 @@ And this rule allows `this` keywords in functions below:
 
 And this rule always allows `this` keywords in the following contexts:
 
+* At the top level of scripts.
 * In class field initializers.
 * In class static blocks.
 
@@ -46,9 +51,6 @@ Examples of **incorrect** code for this rule in strict mode:
 
 "use strict";
 
-this.a = 0;
-baz(() => this);
-
 (function() {
     this.a = 0;
     baz(() => this);
@@ -69,11 +71,6 @@ foo(function() {
     baz(() => this);
 });
 
-obj.foo = () => {
-    // `this` of arrow functions is the outer scope's.
-    this.a = 0;
-};
-
 var obj = {
     aaa: function() {
         return function foo() {
@@ -98,6 +95,9 @@ Examples of **correct** code for this rule in strict mode:
 
 "use strict";
 
+this.a = 0;
+baz(() => this);
+
 function Foo() {
     // OK, this is in a legacy style constructor.
     this.a = 0;
diff --git a/lib/rules/no-eval.js b/lib/rules/no-eval.js
index ae6d71b80..dab04dd77 100644
--- a/lib/rules/no-eval.js
+++ b/lib/rules/no-eval.js
@@ -87,6 +87,7 @@ module.exports = {
                 upper: funcInfo,
                 node,
                 strict,
+                isTopLevelOfScript: false,
                 defaultThis: false,
                 initialized: strict
             };
@@ -222,12 +223,14 @@ module.exports = {
                     strict =
                         scope.isStrict ||
                         node.sourceType === "module" ||
-                        (features.globalReturn && scope.childScopes[0].isStrict);
+                        (features.globalReturn && scope.childScopes[0].isStrict),
+                    isTopLevelOfScript = node.sourceType !== "module" && !features.globalReturn;
 
                 funcInfo = {
                     upper: null,
                     node,
                     strict,
+                    isTopLevelOfScript,
                     defaultThis: true,
                     initialized: true
                 };
@@ -269,7 +272,8 @@ module.exports = {
                     );
                 }
 
-                if (!funcInfo.strict && funcInfo.defaultThis) {
+                // `this` at the top level of scripts always refers to the global object
+                if (funcInfo.isTopLevelOfScript || (!funcInfo.strict && funcInfo.defaultThis)) {
 
                     // `this.eval` is possible built-in `eval`.
                     report(node.parent);
diff --git a/lib/rules/no-invalid-this.js b/lib/rules/no-invalid-this.js
index 64e4d964a..a97696b85 100644
--- a/lib/rules/no-invalid-this.js
+++ b/lib/rules/no-invalid-this.js
@@ -1,5 +1,5 @@
 /**
- * @fileoverview A rule to disallow `this` keywords outside of classes or class-like objects.
+ * @fileoverview A rule to disallow `this` keywords in contexts where the value of `this` is `undefined`.
  * @author Toru Nagashima
  */
 
@@ -36,7 +36,7 @@ module.exports = {
         type: "suggestion",
 
         docs: {
-            description: "disallow `this` keywords outside of classes or class-like objects",
+            description: "disallow use of `this` in contexts where the value of `this` is `undefined`",
             recommended: false,
             url: "https://eslint.org/docs/rules/no-invalid-this"
         },
@@ -98,11 +98,11 @@ module.exports = {
                     const scope = context.getScope();
                     const features = context.parserOptions.ecmaFeatures || {};
 
+                    // `this` at the top level of scripts always refers to the global object
                     stack.push({
                         init: true,
                         node,
                         valid: !(
-                            scope.isStrict ||
                             node.sourceType === "module" ||
                             (features.globalReturn && scope.childScopes[0].isStrict)
                         )


Test patch:
diff --git a/tests/lib/rules/no-eval.js b/tests/lib/rules/no-eval.js
index b084892cf..fdd961f65 100644
--- a/tests/lib/rules/no-eval.js
+++ b/tests/lib/rules/no-eval.js
@@ -42,6 +42,8 @@ ruleTester.run("no-eval", rule, {
         { code: "function foo() { var eval = 'foo'; globalThis[eval]('foo') }", env: { es2020: true } },
         "this.noeval('foo');",
         "function foo() { 'use strict'; this.eval('foo'); }",
+        { code: "'use strict'; this.eval('foo');", parserOptions: { ecmaFeatures: { globalReturn: true } } },
+        { code: "this.eval('foo');", parserOptions: { ecmaVersion: 6, sourceType: "module" } },
         { code: "function foo() { this.eval('foo'); }", parserOptions: { ecmaVersion: 6, sourceType: "module" } },
         { code: "function foo() { this.eval('foo'); }", parserOptions: { ecmaFeatures: { impliedStrict: true } } },
         "var obj = {foo: function() { this.eval('foo'); }}",
@@ -92,6 +94,7 @@ ruleTester.run("no-eval", rule, {
         { code: "(0, window['eval'])('foo')", env: { browser: true }, errors: [{ messageId: "unexpected", type: "MemberExpression", column: 12, endColumn: 18 }] },
         { code: "var EVAL = eval; EVAL('foo')", errors: [{ messageId: "unexpected", type: "Identifier", column: 12, endColumn: 16 }] },
         { code: "var EVAL = this.eval; EVAL('foo')", errors: [{ messageId: "unexpected", type: "MemberExpression", column: 17, endColumn: 21 }] },
+        { code: "'use strict'; var EVAL = this.eval; EVAL('foo')", errors: [{ messageId: "unexpected", type: "MemberExpression", column: 31, endColumn: 35 }] },
         { code: "(function(exe){ exe('foo') })(eval);", errors: [{ messageId: "unexpected", type: "Identifier", column: 31, endColumn: 35 }] },
         { code: "window.eval('foo')", env: { browser: true }, errors: [{ messageId: "unexpected", type: "CallExpression", column: 8, endColumn: 12 }] },
         { code: "window.window.eval('foo')", env: { browser: true }, errors: [{ messageId: "unexpected", type: "CallExpression", column: 15, endColumn: 19 }] },
@@ -100,6 +103,7 @@ ruleTester.run("no-eval", rule, {
         { code: "global.global.eval('foo')", env: { node: true }, errors: [{ messageId: "unexpected", type: "CallExpression", column: 15, endColumn: 19 }] },
         { code: "global.global[`eval`]('foo')", parserOptions: { ecmaVersion: 6 }, env: { node: true }, errors: [{ messageId: "unexpected", type: "CallExpression", column: 15, endColumn: 21 }] },
         { code: "this.eval('foo')", errors: [{ messageId: "unexpected", type: "CallExpression", column: 6, endColumn: 10 }] },
+        { code: "'use strict'; this.eval('foo')", errors: [{ messageId: "unexpected", type: "CallExpression", column: 20, endColumn: 24 }] },
         { code: "function foo() { this.eval('foo') }", errors: [{ messageId: "unexpected", type: "CallExpression", column: 23, endColumn: 27 }] },
         { code: "var EVAL = globalThis.eval; EVAL('foo')", env: { es2020: true }, errors: [{ messageId: "unexpected", type: "MemberExpression", column: 23, endColumn: 27 }] },
         { code: "globalThis.eval('foo')", env: { es2020: true }, errors: [{ messageId: "unexpected", type: "CallExpression", column: 12, endColumn: 16 }] },
@@ -134,6 +138,11 @@ ruleTester.run("no-eval", rule, {
             parserOptions: { ecmaVersion: 2022 },
             errors: [{ messageId: "unexpected" }]
         },
+        {
+            code: "'use strict'; class C { [this.eval('foo')] }",
+            parserOptions: { ecmaVersion: 2022 },
+            errors: [{ messageId: "unexpected" }]
+        },
 
         {
             code: "class A { static {} [this.eval()]; }",
diff --git a/tests/lib/rules/no-invalid-this.js b/tests/lib/rules/no-invalid-this.js
index 6b54d9a96..b106a706c 100644
--- a/tests/lib/rules/no-invalid-this.js
+++ b/tests/lib/rules/no-invalid-this.js
@@ -106,8 +106,8 @@ const patterns = [
         code: "console.log(this); z(x => console.log(x, this));",
         parserOptions: { ecmaVersion: 6 },
         errors,
-        valid: [NORMAL],
-        invalid: [USE_STRICT, IMPLIED_STRICT, MODULES]
+        valid: [NORMAL, USE_STRICT, IMPLIED_STRICT],
+        invalid: [MODULES]
     },
     {
         code: "console.log(this); z(x => console.log(x, this));",
@@ -125,8 +125,17 @@ const patterns = [
             ecmaVersion: 6
         },
         errors,
-        valid: [NORMAL],
-        invalid: [USE_STRICT, IMPLIED_STRICT, MODULES]
+        valid: [NORMAL, USE_STRICT, IMPLIED_STRICT],
+        invalid: [MODULES]
+    },
+    {
+        code: "this.eval('foo');",
+        parserOptions: {
+            ecmaVersion: 6
+        },
+        errors: [{ messageId: "unexpectedThis", type: "ThisExpression" }],
+        valid: [NORMAL, USE_STRICT, IMPLIED_STRICT],
+        invalid: [MODULES]
     },
 
     // IIFE.
@@ -374,8 +383,8 @@ const patterns = [
     {
         code: "obj.foo = (() => () => { console.log(this); z(x => console.log(x, this)); })();",
         parserOptions: { ecmaVersion: 6 },
-        valid: [NORMAL],
-        invalid: [USE_STRICT, IMPLIED_STRICT, MODULES],
+        valid: [NORMAL, USE_STRICT, IMPLIED_STRICT],
+        invalid: [MODULES],
         errors
     },
     {
@@ -793,8 +802,8 @@ const patterns = [
     {
         code: "class C { [this.foo]; }",
         parserOptions: { ecmaVersion: 2022 },
-        valid: [NORMAL], // the global this in non-strict mode is OK.
-        invalid: [USE_STRICT, IMPLIED_STRICT, MODULES],
+        valid: [NORMAL, USE_STRICT, IMPLIED_STRICT], // `this` is the top-level `this`
+        invalid: [MODULES],
         errors: [{ messageId: "unexpectedThis", type: "ThisExpression" }]
     },
     {
@@ -855,15 +864,15 @@ const patterns = [
     {
         code: "class C { static {} [this]; }",
         parserOptions: { ecmaVersion: 2022 },
-        valid: [NORMAL],
-        invalid: [USE_STRICT, IMPLIED_STRICT, MODULES],
+        valid: [NORMAL, USE_STRICT, IMPLIED_STRICT],
+        invalid: [MODULES],
         errors: [{ messageId: "unexpectedThis", type: "ThisExpression" }]
     },
     {
         code: "class C { static {} [this.x]; }",
         parserOptions: { ecmaVersion: 2022 },
-        valid: [NORMAL],
-        invalid: [USE_STRICT, IMPLIED_STRICT, MODULES],
+        valid: [NORMAL, USE_STRICT, IMPLIED_STRICT],
+        invalid: [MODULES],
         errors: [{ messageId: "unexpectedThis", type: "ThisExpression" }]
     },