Repository: eslint/eslint (js)

Issue:
no-var fix isn't safe in the global scope
I'm not sure what the project's policy is about safety of fixes, but the `no-var` can produce unsafe transformations when the var is global which makes it unsuitable for fixing arbitrary files.

This is a minimal example that isn't safe (and here's a [demo page link to it](https://eslint.org/demo/#eyJ0ZXh0IjoiXG52YXIgeCA9IDEiLCJvcHRpb25zIjp7InBhcnNlck9wdGlvbnMiOnsiZWNtYVZlcnNpb24iOjgsInNvdXJjZVR5cGUiOiJzY3JpcHQiLCJlY21hRmVhdHVyZXMiOnt9fSwicnVsZXMiOnsibm8tdmFyIjoyfSwiZW52Ijp7ImJyb3dzZXIiOnRydWV9fX0=)):

```js
var x = 10;
```

When fixed by `eslint` it produces:

```js
let x = 10;
```

However this isn't equivalent as in the global context `let`/`const` statements have different semantics than `var`, take this code for instance:

```html
<script>
    var x = 10;
</script>
<script>
    var x = 20;
</script>
<script>
    console.log(x);
    console.log(window.x);
</script>
```

Running that code will print 20 twice, but in contrast if we run the `eslint` "fixed" version:

```html
<script>
    let x = 10;
</script>
<script>
    let x = 20;
</script>
<script>
    console.log(x);
    console.log(window.x);
</script>
``` 

In this case it will print `10` followed by `undefined`, in addition the second script will simply throw an error that `x` is already defined.

Now I'm not planning on using globals in this way in new code, but a lot of files in the code base are many years old and I know I've seen `var someGlobalName` being used to add things as `window.someGlobalName` so running the `no-var` on the code base is very likely to break things. It would be nice at least to have an option to `no-var` like `ignore-global` or something like that to prevent breaking of such code.

Pull request:
Fix: no-var don't fix globals (fixes #9520)
**What is the purpose of this pull request? (put an "X" next to item)**

[X] Bug fix

Fixes #9520.

**What changes did you make? (Give an overview)**

This PR makes `no-var` rule not fixing global variables.
If the program has special scope (`sourceType: "module"` or `ecmaFeatures: { globalReturn: true }`), the rule keeps the current behavior.

**Is there anything you'd like reviewers to focus on?**

Nothing in particular.


Patch:
diff --git a/lib/rules/no-var.js b/lib/rules/no-var.js
index c74e0b9ad..d3c163e55 100644
--- a/lib/rules/no-var.js
+++ b/lib/rules/no-var.js
@@ -15,6 +15,15 @@ const astUtils = require("../ast-utils");
 // Helpers
 //------------------------------------------------------------------------------
 
+/**
+ * Check whether a given variable is a global variable or not.
+ * @param {eslint-scope.Variable} variable The variable to check.
+ * @returns {boolean} `true` if the variable is a global variable.
+ */
+function isGlobal(variable) {
+    return Boolean(variable.scope) && variable.scope.type === "global";
+}
+
 /**
  * Finds the nearest function scope or global scope walking up the scope
  * hierarchy.
@@ -203,6 +212,7 @@ module.exports = {
          * Checks whether it can fix a given variable declaration or not.
          * It cannot fix if the following cases:
          *
+         * - A variable is a global variable.
          * - A variable is declared on a SwitchCase node.
          * - A variable is redeclared.
          * - A variable is used from outside the scope.
@@ -256,6 +266,7 @@ module.exports = {
 
             if (node.parent.type === "SwitchCase" ||
                 node.declarations.some(hasSelfReferenceInTDZ) ||
+                variables.some(isGlobal) ||
                 variables.some(isRedeclared) ||
                 variables.some(isUsedFromOutsideOf(scopeNode))
             ) {


Test patch:
diff --git a/tests/lib/rules/no-var.js b/tests/lib/rules/no-var.js
index 830104180..48511d825 100644
--- a/tests/lib/rules/no-var.js
+++ b/tests/lib/rules/no-var.js
@@ -21,13 +21,22 @@ const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 6 } });
 ruleTester.run("no-var", rule, {
     valid: [
         "const JOE = 'schmoe';",
-        "let moo = 'car';"
+        "let moo = 'car';",
+        {
+            code: "const JOE = 'schmoe';",
+            parserOptions: { ecmaFeatures: { globalReturn: true } }
+        },
+        {
+            code: "let moo = 'car';",
+            parserOptions: { ecmaFeatures: { globalReturn: true } }
+        }
     ],
 
     invalid: [
         {
             code: "var foo = bar;",
             output: "let foo = bar;",
+            parserOptions: { ecmaFeatures: { globalReturn: true } },
             errors: [
                 {
                     message: "Unexpected var, use let or const instead.",
@@ -38,6 +47,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "var foo = bar, toast = most;",
             output: "let foo = bar, toast = most;",
+            parserOptions: { ecmaFeatures: { globalReturn: true } },
             errors: [
                 {
                     message: "Unexpected var, use let or const instead.",
@@ -48,6 +58,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "var foo = bar; let toast = most;",
             output: "let foo = bar; let toast = most;",
+            parserOptions: { ecmaFeatures: { globalReturn: true } },
             errors: [
                 {
                     message: "Unexpected var, use let or const instead.",
@@ -58,6 +69,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "for (var a of b) { console.log(a); }",
             output: "for (let a of b) { console.log(a); }",
+            parserOptions: { ecmaFeatures: { globalReturn: true } },
             errors: [
                 {
                     message: "Unexpected var, use let or const instead.",
@@ -68,6 +80,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "for (var a in b) { console.log(a); }",
             output: "for (let a in b) { console.log(a); }",
+            parserOptions: { ecmaFeatures: { globalReturn: true } },
             errors: [
                 {
                     message: "Unexpected var, use let or const instead.",
@@ -78,6 +91,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "for (let a of b) { var c = 1; console.log(c); }",
             output: "for (let a of b) { let c = 1; console.log(c); }",
+            parserOptions: { ecmaFeatures: { globalReturn: true } },
             errors: [
                 {
                     message: "Unexpected var, use let or const instead.",
@@ -88,6 +102,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "for (var i = 0; i < list.length; ++i) { foo(i) }",
             output: "for (let i = 0; i < list.length; ++i) { foo(i) }",
+            parserOptions: { ecmaFeatures: { globalReturn: true } },
             errors: [
                 { message: "Unexpected var, use let or const instead.", type: "VariableDeclaration" }
             ]
@@ -95,6 +110,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "for (var i = 0, i = 0; false;);",
             output: null,
+            parserOptions: { ecmaFeatures: { globalReturn: true } },
             errors: [
                 { message: "Unexpected var, use let or const instead.", type: "VariableDeclaration" }
             ]
@@ -102,6 +118,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "var i = 0; for (var i = 1; false;); console.log(i);",
             output: null,
+            parserOptions: { ecmaFeatures: { globalReturn: true } },
             errors: [
                 { message: "Unexpected var, use let or const instead.", type: "VariableDeclaration" },
                 { message: "Unexpected var, use let or const instead.", type: "VariableDeclaration" }
@@ -112,6 +129,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "var a, b, c; var a;",
             output: null,
+            parserOptions: { ecmaFeatures: { globalReturn: true } },
             errors: [
                 "Unexpected var, use let or const instead.",
                 "Unexpected var, use let or const instead."
@@ -120,6 +138,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "var a; if (b) { var a; }",
             output: null,
+            parserOptions: { ecmaFeatures: { globalReturn: true } },
             errors: [
                 "Unexpected var, use let or const instead.",
                 "Unexpected var, use let or const instead."
@@ -128,6 +147,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "if (foo) { var a, b, c; } a;",
             output: null,
+            parserOptions: { ecmaFeatures: { globalReturn: true } },
             errors: [
                 "Unexpected var, use let or const instead."
             ]
@@ -135,6 +155,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "for (var i = 0; i < 10; ++i) {} i;",
             output: null,
+            parserOptions: { ecmaFeatures: { globalReturn: true } },
             errors: [
                 "Unexpected var, use let or const instead."
             ]
@@ -142,6 +163,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "for (var a in obj) {} a;",
             output: null,
+            parserOptions: { ecmaFeatures: { globalReturn: true } },
             errors: [
                 "Unexpected var, use let or const instead."
             ]
@@ -149,6 +171,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "for (var a of list) {} a;",
             output: null,
+            parserOptions: { ecmaFeatures: { globalReturn: true } },
             errors: [
                 "Unexpected var, use let or const instead."
             ]
@@ -156,6 +179,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "switch (a) { case 0: var b = 1 }",
             output: null,
+            parserOptions: { ecmaFeatures: { globalReturn: true } },
             errors: [
                 "Unexpected var, use let or const instead."
             ]
@@ -165,6 +189,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "for (var a of b) { arr.push(() => a); }",
             output: null,
+            parserOptions: { ecmaFeatures: { globalReturn: true } },
             errors: [
                 "Unexpected var, use let or const instead."
             ]
@@ -172,6 +197,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "for (let a of b) { var c; console.log(c); c = 'hello'; }",
             output: null,
+            parserOptions: { ecmaFeatures: { globalReturn: true } },
             errors: [
                 "Unexpected var, use let or const instead."
             ]
@@ -181,6 +207,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "var a = a",
             output: null,
+            parserOptions: { ecmaFeatures: { globalReturn: true } },
             errors: [
                 "Unexpected var, use let or const instead."
             ]
@@ -188,7 +215,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "var {a = a} = {}",
             output: null,
-            parserOptions: { ecmaVersion: 2015 },
+            parserOptions: { ecmaVersion: 2015, ecmaFeatures: { globalReturn: true } },
             errors: [
                 "Unexpected var, use let or const instead."
             ]
@@ -196,7 +223,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "var {a = b, b} = {}",
             output: null,
-            parserOptions: { ecmaVersion: 2015 },
+            parserOptions: { ecmaVersion: 2015, ecmaFeatures: { globalReturn: true } },
             errors: [
                 "Unexpected var, use let or const instead."
             ]
@@ -204,7 +231,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "var {a, b = a} = {}",
             output: "let {a, b = a} = {}",
-            parserOptions: { ecmaVersion: 2015 },
+            parserOptions: { ecmaVersion: 2015, ecmaFeatures: { globalReturn: true } },
             errors: [
                 "Unexpected var, use let or const instead."
             ]
@@ -212,7 +239,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "var a = b, b = 1",
             output: null,
-            parserOptions: { ecmaVersion: 2015 },
+            parserOptions: { ecmaVersion: 2015, ecmaFeatures: { globalReturn: true } },
             errors: [
                 "Unexpected var, use let or const instead."
             ]
@@ -220,7 +247,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "var a = b; var b = 1",
             output: "let a = b; var b = 1",
-            parserOptions: { ecmaVersion: 2015 },
+            parserOptions: { ecmaVersion: 2015, ecmaFeatures: { globalReturn: true } },
             errors: [
                 "Unexpected var, use let or const instead.",
                 "Unexpected var, use let or const instead."
@@ -232,7 +259,7 @@ ruleTester.run("no-var", rule, {
         {
             code: "function foo() { a } var a = 1; foo()",
             output: null,
-            parserOptions: { ecmaVersion: 2015 },
+            parserOptions: { ecmaVersion: 2015, ecmaFeatures: { globalReturn: true } },
             errors: [
                 "Unexpected var, use let or const instead."
             ]
@@ -242,9 +269,33 @@ ruleTester.run("no-var", rule, {
         {
             code: "if (foo) var bar = 1;",
             output: null,
+            parserOptions: { ecmaFeatures: { globalReturn: true } },
             errors: [
                 { message: "Unexpected var, use let or const instead.", type: "VariableDeclaration" }
             ]
+        },
+
+        // https://github.com/eslint/eslint/issues/9520
+        {
+            code: "var foo = 1",
+            output: null,
+            errors: ["Unexpected var, use let or const instead."]
+        },
+        {
+            code: "{ var foo = 1 }",
+            output: null,
+            errors: ["Unexpected var, use let or const instead."]
+        },
+        {
+            code: "if (true) { var foo = 1 }",
+            output: null,
+            errors: ["Unexpected var, use let or const instead."]
+        },
+        {
+            code: "var foo = 1",
+            output: "let foo = 1",
+            parserOptions: { sourceType: "module" },
+            errors: ["Unexpected var, use let or const instead."]
         }
     ]
 });