Fix: Disable no-var autofixer in some incorrect cases in loops

The autofixer for the no-var rule was converting `var` to `let` within loops,
but in some cases that can introduce incorrect behavior because `let` variables
in loops only live for the lifetime of their loop iteration, while `var`
variables within loops use the same variable across all iterations.

We can still convert to `let` in typical cases as long as we check for cases
that might cause a behavior difference:
* If the variable is referenced from a closure, then that closure might be
  called after the current loop iteration ends. For `var` declarations, the
  closure refers to the shared variable across all iterations, and for `let`
  declarations, the closure refers to the variable just from that one iteration.
* If the variable is used before its first assignment in the loop body, then for
  `var` declarations it will retain its value from the previous iteration, but
  for `let` declarations it will start as undefined.

This change skips the autofixer for any variables referenced by any closure, and
for any variables that are not initialized right when they are declared. Some
additional static analysis can make both of these cases smarter, but this should
handle most common cases.

docs/rules/no-var.md

@@ -1,6 +1,6 @@
 # require `let` or `const` instead of `var` (no-var)
 
-(fixable) The `--fix` option on the [command line](../user-guide/command-line-interface#fix) automatically fixed problems reported by this rule.
+(fixable) The `--fix` option on the [command line](../user-guide/command-line-interface#fix) automatically fixes some instances of problems reported by this rule.
 
 ECMAScript 6 allows programmers to create variables with block scope instead of function scope using the `let`
 and `const` keywords. Block scope is common in many other programming languages and helps programmers avoid mistakes

lib/ast-utils.js

@@ -84,6 +84,56 @@ function getUpperFunction(node) {
     return null;
 }
 
+/**
+ * Checks whether a given node is a function node or not.
+ * The following types are function nodes:
+ *
+ * - ArrowFunctionExpression
+ * - FunctionDeclaration
+ * - FunctionExpression
+ *
+ * @param {ASTNode|null} node - A node to check.
+ * @returns {boolean} `true` if the node is a function node.
+ */
+function isFunction(node) {
+    return Boolean(node && anyFunctionPattern.test(node.type));
+}
+
+/**
+ * Checks whether a given node is a loop node or not.
+ * The following types are loop nodes:
+ *
+ * - DoWhileStatement
+ * - ForInStatement
+ * - ForOfStatement
+ * - ForStatement
+ * - WhileStatement
+ *
+ * @param {ASTNode|null} node - A node to check.
+ * @returns {boolean} `true` if the node is a loop node.
+ */
+function isLoop(node) {
+    return Boolean(node && anyLoopPattern.test(node.type));
+}
+
+/**
+ * Checks whether the given node is in a loop or not.
+ *
+ * @param {ASTNode} node - The node to check.
+ * @returns {boolean} `true` if the node is in a loop.
+ */
+function isInLoop(node) {
+    while (node && !isFunction(node)) {
+        if (isLoop(node)) {
+            return true;
+        }
+
+        node = node.parent;
+    }
+
+    return false;
+}
+
 /**
  * Checks whether or not a node is `null` or `undefined`.
  * @param {ASTNode} node - A node to check.
@@ -270,6 +320,9 @@ module.exports = {
     isCallee,
     isES5Constructor,
     getUpperFunction,
+    isFunction,
+    isLoop,
+    isInLoop,
     isArrayFromMethod,
     isParenthesised,
 
@@ -634,38 +687,6 @@ module.exports = {
         return 18;
     },
 
-    /**
-     * Checks whether a given node is a loop node or not.
-     * The following types are loop nodes:
-     *
-     * - DoWhileStatement
-     * - ForInStatement
-     * - ForOfStatement
-     * - ForStatement
-     * - WhileStatement
-     *
-     * @param {ASTNode|null} node - A node to check.
-     * @returns {boolean} `true` if the node is a loop node.
-     */
-    isLoop(node) {
-        return Boolean(node && anyLoopPattern.test(node.type));
-    },
-
-    /**
-     * Checks whether a given node is a function node or not.
-     * The following types are function nodes:
-     *
-     * - ArrowFunctionExpression
-     * - FunctionDeclaration
-     * - FunctionExpression
-     *
-     * @param {ASTNode|null} node - A node to check.
-     * @returns {boolean} `true` if the node is a function node.
-     */
-    isFunction(node) {
-        return Boolean(node && anyFunctionPattern.test(node.type));
-    },
-
     /**
      * Checks whether the given node is an empty block node or not.
      *
@@ -683,7 +704,7 @@ module.exports = {
      * @returns {boolean} `true` if the node is an empty function.
      */
     isEmptyFunction(node) {
-        return module.exports.isFunction(node) && module.exports.isEmptyBlock(node.body);
+        return isFunction(node) && module.exports.isEmptyBlock(node.body);
     },
 
     /**

lib/rules/no-unused-vars.js

@@ -156,28 +156,6 @@ module.exports = {
             return false;
         }
 
-        /**
-         * Checks whether a given node is inside of a loop or not.
-         *
-         * @param {ASTNode} node - A node to check.
-         * @returns {boolean} `true` if the node is inside of a loop.
-         * @private
-         */
-        function isInsideOfLoop(node) {
-            while (node) {
-                if (astUtils.isLoop(node)) {
-                    return true;
-                }
-                if (astUtils.isFunction(node)) {
-                    return false;
-                }
-
-                node = node.parent;
-            }
-
-            return false;
-        }
-
         /**
          * Checks the position of given nodes.
          *
@@ -215,7 +193,7 @@ module.exports = {
             const granpa = parent.parent;
             const refScope = ref.from.variableScope;
             const varScope = ref.resolved.scope.variableScope;
-            const canBeUsedLater = refScope !== varScope || isInsideOfLoop(id);
+            const canBeUsedLater = refScope !== varScope || astUtils.isInLoop(id);
 
             /*
              * Inherits the previous node if this reference is in the node.

lib/rules/no-useless-return.js

@@ -54,24 +54,6 @@ function isRemovable(node) {
     );
 }
 
-/**
- * Checks whether the given return statement is in a loop or not.
- *
- * @param {ASTNode} node - The return statement node to check.
- * @returns {boolean} `true` if the node is in a loop.
- */
-function isInLoop(node) {
-    while (node && !astUtils.isFunction(node)) {
-        if (astUtils.isLoop(node)) {
-            return true;
-        }
-
-        node = node.parent;
-    }
-
-    return false;
-}
-
 /**
  * Checks whether the given return statement is in a `finally` block or not.
  *
@@ -260,7 +242,7 @@ module.exports = {
                 if (node.argument) {
                     markReturnStatementsOnCurrentSegmentsAsUsed();
                 }
-                if (node.argument || isInLoop(node) || isInFinally(node)) {
+                if (node.argument || astUtils.isInLoop(node) || isInFinally(node)) {
                     return;
                 }
 

lib/rules/no-var.js

@@ -5,10 +5,67 @@
 
 "use strict";
 
+//------------------------------------------------------------------------------
+// Requirements
+//------------------------------------------------------------------------------
+
+const astUtils = require("../ast-utils");
+
 //------------------------------------------------------------------------------
 // Helpers
 //------------------------------------------------------------------------------
 
+/**
+ * Finds the nearest function scope or global scope walking up the scope
+ * hierarchy.
+ *
+ * @param {escope.Scope} scope - The scope to traverse.
+ * @returns {escope.Scope} a function scope or global scope containing the given
+ *      scope.
+ */
+function getEnclosingFunctionScope(scope) {
+    while (scope.type !== "function" && scope.type !== "global") {
+        scope = scope.upper;
+    }
+    return scope;
+}
+
+/**
+ * Checks whether the given variable has any references from a more specific
+ * function expression (i.e. a closure).
+ *
+ * @param {escope.Variable} variable - A variable to check.
+ * @returns {boolean} `true` if the variable is used from a closure.
+ */
+function isReferencedInClosure(variable) {
+    const enclosingFunctionScope = getEnclosingFunctionScope(variable.scope);
+
+    return variable.references.some(reference =>
+        getEnclosingFunctionScope(reference.from) !== enclosingFunctionScope);
+}
+
+/**
+ * Checks whether the given node is the assignee of a loop.
+ *
+ * @param {ASTNode} node - A VariableDeclaration node to check.
+ * @returns {boolean} `true` if the declaration is assigned as part of loop
+ *      iteration.
+ */
+function isLoopAssignee(node) {
+    return (node.parent.type === "ForOfStatement" || node.parent.type === "ForInStatement") &&
+        node === node.parent.left;
+}
+
+/**
+ * Checks whether the given variable declaration is immediately initialized.
+ *
+ * @param {ASTNode} node - A VariableDeclaration node to check.
+ * @returns {boolean} `true` if the declaration has an initializer.
+ */
+function isDeclarationInitialized(node) {
+    return node.declarations.every(declarator => declarator.init !== null);
+}
+
 const SCOPE_NODE_TYPE = /^(?:Program|BlockStatement|SwitchStatement|ForStatement|ForInStatement|ForOfStatement)$/;
 
 /**
@@ -97,6 +154,8 @@ module.exports = {
          * - A variable is declared on a SwitchCase node.
          * - A variable is redeclared.
          * - A variable is used from outside the scope.
+         * - A variable is used from a closure within a loop.
+         * - A variable might be used before it is assigned within a loop.
          *
          * ## A variable is declared on a SwitchCase node.
          *
@@ -115,18 +174,48 @@ module.exports = {
          * The language spec disallows accesses from outside of the scope for
          * `let` declarations. Those variables would cause reference errors.
          *
+         * ## A variable is used from a closure within a loop.
+         *
+         * A `var` declaration within a loop shares the same variable instance
+         * across all loop iterations, while a `let` declaration creates a new
+         * instance for each iteration. This means if a variable in a loop is
+         * referenced by any closure, changing it from `var` to `let` would
+         * change the behavior in a way that is generally unsafe.
+         *
+         * ## A variable might be used before it is assigned within a loop.
+         *
+         * Within a loop, a `let` declaration without an initializer will be
+         * initialized to null, while a `var` declaration will retain its value
+         * from the previous iteration, so it is only safe to change `var` to
+         * `let` if we can statically determine that the variable is always
+         * assigned a value before its first access in the loop body. To keep
+         * the implementation simple, we only convert `var` to `let` within
+         * loops when the variable is a loop assignee or the declaration has an
+         * initializer.
+         *
          * @param {ASTNode} node - A variable declaration node to check.
          * @returns {boolean} `true` if it can fix the node.
          */
         function canFix(node) {
             const variables = context.getDeclaredVariables(node);
             const scopeNode = getScopeNode(node);
 
-            return !(
-                node.parent.type === "SwitchCase" ||
-                variables.some(isRedeclared) ||
-                variables.some(isUsedFromOutsideOf(scopeNode))
-            );
+            if (node.parent.type === "SwitchCase" ||
+                    variables.some(isRedeclared) ||
+                    variables.some(isUsedFromOutsideOf(scopeNode))) {
+                return false;
+            }
+
+            if (astUtils.isInLoop(node)) {
+                if (variables.some(isReferencedInClosure)) {
+                    return false;
+                }
+                if (!isLoopAssignee(node) && !isDeclarationInitialized(node)) {
+                    return false;
+                }
+            }
+
+            return true;
         }
 
         /**

tests/lib/rules/no-var.js

@@ -55,6 +55,37 @@ ruleTester.run("no-var", rule, {
                 }
             ]
         },
+        {
+            code: "for (var a of b) { console.log(a); }",
+            output: "for (let a of b) { console.log(a); }",
+            errors: [
+                {
+                    message: "Unexpected var, use let or const instead.",
+                    type: "VariableDeclaration"
+                }
+            ]
+        },
+        {
+            code: "for (var a in b) { console.log(a); }",
+            output: "for (let a in b) { console.log(a); }",
+            errors: [
+                {
+                    message: "Unexpected var, use let or const instead.",
+                    type: "VariableDeclaration"
+                }
+            ]
+        },
+        {
+            code: "for (let a of b) { var c = 1; console.log(c); }",
+            output: "for (let a of b) { let c = 1; console.log(c); }",
+            errors: [
+                {
+                    message: "Unexpected var, use let or const instead.",
+                    type: "VariableDeclaration"
+                }
+            ]
+        },
+
 
         // Not fix if it's redeclared or it's used from outside of the scope or it's declared on a case chunk.
         {
@@ -108,5 +139,21 @@ ruleTester.run("no-var", rule, {
                 "Unexpected var, use let or const instead."
             ]
         },
+
+        // Don't fix if the variable is in a loop and the behavior might change.
+        {
+            code: "for (var a of b) { arr.push(() => a); }",
+            output: "for (var a of b) { arr.push(() => a); }",
+            errors: [
+                "Unexpected var, use let or const instead."
+            ]
+        },
+        {
+            code: "for (let a of b) { var c; console.log(c); c = 'hello'; }",
+            output: "for (let a of b) { var c; console.log(c); c = 'hello'; }",
+            errors: [
+                "Unexpected var, use let or const instead."
+            ]
+        },
     ]
 });