From 427394afcbf850190725e499f218ffc994177bae Mon Sep 17 00:00:00 2001 From: Alan Pierce Date: Fri, 23 Dec 2016 14:12:27 -0800 Subject: [PATCH] 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 | 2 +- lib/ast-utils.js | 87 ++++++++++++++++++------------ lib/rules/no-unused-vars.js | 24 +-------- lib/rules/no-useless-return.js | 20 +------ lib/rules/no-var.js | 99 ++++++++++++++++++++++++++++++++-- tests/lib/rules/no-var.js | 47 ++++++++++++++++ 6 files changed, 198 insertions(+), 81 deletions(-) diff --git a/docs/rules/no-var.md b/docs/rules/no-var.md index f4859d5937a2..c0e193d057fe 100644 --- a/docs/rules/no-var.md +++ b/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 diff --git a/lib/ast-utils.js b/lib/ast-utils.js index 2c175691b36d..d922b766355c 100644 --- a/lib/ast-utils.js +++ b/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); }, /** diff --git a/lib/rules/no-unused-vars.js b/lib/rules/no-unused-vars.js index c8f0edd061e5..ac8f2ed1c0a2 100644 --- a/lib/rules/no-unused-vars.js +++ b/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. diff --git a/lib/rules/no-useless-return.js b/lib/rules/no-useless-return.js index 706cf2758685..259fed2396ad 100644 --- a/lib/rules/no-useless-return.js +++ b/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; } diff --git a/lib/rules/no-var.js b/lib/rules/no-var.js index 0e98170e65e7..3c22f009c63b 100644 --- a/lib/rules/no-var.js +++ b/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,6 +174,25 @@ 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. */ @@ -122,11 +200,22 @@ module.exports = { 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; } /** diff --git a/tests/lib/rules/no-var.js b/tests/lib/rules/no-var.js index 05fdc72cf29a..7d939e2013e2 100644 --- a/tests/lib/rules/no-var.js +++ b/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." + ] + }, ] });