Fix: no-unused-vars false positive in loop (fixes #6646) (#6649)

* Fix: `no-unused-var` false positive in loop (fixes #6646) * Chore: add some private tags to jsdoc comments. * Chore: add `isFunctionNode` and `isLoopNode` to ast-utils. * Chore: add a test for my mistake. #6649 (comment) * Chore: add tests for `isFunctionNode` and `isLoopNode` * Chore: rename `isLoopNode` → `isLoop` and `isFunctionNode` → `isFunction`
eslint · Jul 15, 2016 · dfc20e9 · dfc20e9
1 parent 2ba75d5
commit dfc20e9
Show file tree

Hide file tree

Showing 8 changed files with 186 additions and 16 deletions.
diff --git a/lib/ast-utils.js b/lib/ast-utils.js
@@ -16,6 +16,7 @@ var esutils = require("esutils");
 //------------------------------------------------------------------------------
 
 var anyFunctionPattern = /^(?:Function(?:Declaration|Expression)|ArrowFunctionExpression)$/;
+var anyLoopPattern = /^(?:DoWhile|For|ForIn|ForOf|While)Statement$/;
 var arrayOrTypedArrayPattern = /Array$/;
 var arrayMethodPattern = /^(?:every|filter|find|findIndex|forEach|map|some)$/;
 var bindOrCallOrApplyPattern = /^(?:bind|call|apply)$/;
@@ -551,5 +552,37 @@ module.exports = {
             // no default
         }
         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: function(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: function(node) {
+        return Boolean(node && anyFunctionPattern.test(node.type));
     }
 };
diff --git a/lib/rules/no-empty.js b/lib/rules/no-empty.js
@@ -5,10 +5,14 @@
 "use strict";
 
 //------------------------------------------------------------------------------
-// Rule Definition
+// Requirements
 //------------------------------------------------------------------------------
 
-var FUNCTION_TYPE = /^(?:ArrowFunctionExpression|Function(?:Declaration|Expression))$/;
+var astUtils = require("../ast-utils");
+
+//------------------------------------------------------------------------------
+// Rule Definition
+//------------------------------------------------------------------------------
 
 module.exports = {
     meta: {
@@ -46,7 +50,7 @@ module.exports = {
                 }
 
                 // a function is generally allowed to be empty
-                if (FUNCTION_TYPE.test(node.parent.type)) {
+                if (astUtils.isFunction(node.parent)) {
                     return;
                 }
 

diff --git a/lib/rules/no-labels.js b/lib/rules/no-labels.js
@@ -5,10 +5,10 @@
 "use strict";
 
 //------------------------------------------------------------------------------
-// Constants
+// Requirements
 //------------------------------------------------------------------------------
 
-var LOOP_TYPES = /^(?:While|DoWhile|For|ForIn|ForOf)Statement$/;
+var astUtils = require("../ast-utils");
 
 //------------------------------------------------------------------------------
 // Rule Definition
@@ -51,12 +51,10 @@ module.exports = {
          * @returns {string} The kind of the node.
          */
         function getBodyKind(node) {
-            var type = node.type;
-
-            if (LOOP_TYPES.test(type)) {
+            if (astUtils.isLoop(node)) {
                 return "loop";
             }
-            if (type === "SwitchStatement") {
+            if (node.type === "SwitchStatement") {
                 return "switch";
             }
             return "other";

diff --git a/lib/rules/no-unmodified-loop-condition.js b/lib/rules/no-unmodified-loop-condition.js
@@ -19,7 +19,7 @@ var Map = require("es6-map"),
 
 var pushAll = Function.apply.bind(Array.prototype.push);
 var SENTINEL_PATTERN = /(?:(?:Call|Class|Function|Member|New|Yield)Expression|Statement|Declaration)$/;
-var LOOP_PATTERN = /^(?:DoWhile|For|While)Statement$/;
+var LOOP_PATTERN = /^(?:DoWhile|For|While)Statement$/;  // for-in/of statements don't have `test` property.
 var GROUP_PATTERN = /^(?:BinaryExpression|ConditionalExpression)$/;
 var SKIP_PATTERN = /^(?:ArrowFunction|Class|Function)Expression$/;
 var DYNAMIC_PATTERN = /^(?:Call|Member|New|TaggedTemplate|Yield)Expression$/;

diff --git a/lib/rules/no-unused-vars.js b/lib/rules/no-unused-vars.js
@@ -155,12 +155,35 @@ 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.
          *
          * @param {ASTNode} inner - A node which is expected as inside.
          * @param {ASTNode} outer - A node which is expected as outside.
          * @returns {boolean} `true` if the `inner` node exists in the `outer` node.
+         * @private
          */
         function isInside(inner, outer) {
             return (
@@ -173,17 +196,25 @@ module.exports = {
          * If a given reference is left-hand side of an assignment, this gets
          * the right-hand side node of the assignment.
          *
+         * In the following cases, this returns null.
+         *
+         * - The reference is not the LHS of an assignment expression.
+         * - The reference is inside of a loop.
+         * - The reference is inside of a function scope which is different from
+         *   the declaration.
+         *
          * @param {escope.Reference} ref - A reference to check.
          * @param {ASTNode} prevRhsNode - The previous RHS node.
-         * @returns {ASTNode} The RHS node.
+         * @returns {ASTNode|null} The RHS node or null.
+         * @private
          */
         function getRhsNode(ref, prevRhsNode) {
             var id = ref.identifier;
             var parent = id.parent;
             var granpa = parent.parent;
             var refScope = ref.from.variableScope;
             var varScope = ref.resolved.scope.variableScope;
-            var canBeUsedLater = refScope !== varScope;
+            var canBeUsedLater = refScope !== varScope || isInsideOfLoop(id);
 
             /*
              * Inherits the previous node if this reference is in the node.
@@ -213,6 +244,7 @@ module.exports = {
          *      - the funcNode is assigned to a variable.
          *      - the funcNode is bound as an argument of a function call.
          *      - the function is bound to a property and the object satisfies above conditions.
+         * @private
          */
         function isStorableFunction(funcNode, rhsNode) {
             var node = funcNode;
@@ -266,6 +298,7 @@ module.exports = {
          * @param {ASTNode} id - An Identifier node to check.
          * @param {ASTNode} rhsNode - The RHS node of the previous assignment.
          * @returns {boolean} `true` if the `id` node exists inside of a function node which can be used later.
+         * @private
          */
         function isInsideOfStorableFunction(id, rhsNode) {
             var funcNode = astUtils.getUpperFunction(id);
@@ -283,6 +316,7 @@ module.exports = {
          * @param {escope.Reference} ref - A reference to check.
          * @param {ASTNode} rhsNode - The RHS node of the previous assignment.
          * @returns {boolean} The reference is a read to update itself.
+         * @private
          */
         function isReadForItself(ref, rhsNode) {
             var id = ref.identifier;

diff --git a/lib/rules/quotes.js b/lib/rules/quotes.js
@@ -65,8 +65,7 @@ QUOTE_SETTINGS.backtick.convert = function(str) {
     }) + newQuote;
 };
 
-var AVOID_ESCAPE = "avoid-escape",
-    FUNCTION_TYPE = /^(?:Arrow)?Function(?:Declaration|Expression)$/;
+var AVOID_ESCAPE = "avoid-escape";
 
 //------------------------------------------------------------------------------
 // Rule Definition
@@ -157,7 +156,7 @@ module.exports = {
         function isPartOfDirectivePrologue(node) {
             var block = node.parent.parent;
 
-            if (block.type !== "Program" && (block.type !== "BlockStatement" || !FUNCTION_TYPE.test(block.parent.type))) {
+            if (block.type !== "Program" && (block.type !== "BlockStatement" || !astUtils.isFunction(block.parent))) {
                 return false;
             }
 

diff --git a/tests/lib/ast-utils.js b/tests/lib/ast-utils.js
@@ -345,4 +345,80 @@ describe("ast-utils", function() {
             assert.isTrue(astUtils.isParenthesised(sourceCode, ast.body[0].expression));
         });
     });
+
+    describe("isFunction", function() {
+        it("should return true for FunctionDeclaration", function() {
+            var ast = espree.parse("function a() {}");
+            var node = ast.body[0];
+
+            assert(astUtils.isFunction(node));
+        });
+
+        it("should return true for FunctionExpression", function() {
+            var ast = espree.parse("(function a() {})");
+            var node = ast.body[0].expression;
+
+            assert(astUtils.isFunction(node));
+        });
+
+        it("should return true for AllowFunctionExpression", function() {
+            var ast = espree.parse("(() => {})", {ecmaVersion: 6});
+            var node = ast.body[0].expression;
+
+            assert(astUtils.isFunction(node));
+        });
+
+        it("should return false for Program, VariableDeclaration, BlockStatement", function() {
+            var ast = espree.parse("var a; { }");
+
+            assert(!astUtils.isFunction(ast));
+            assert(!astUtils.isFunction(ast.body[0]));
+            assert(!astUtils.isFunction(ast.body[1]));
+        });
+    });
+
+    describe("isLoop", function() {
+        it("should return true for DoWhileStatement", function() {
+            var ast = espree.parse("do {} while (a)");
+            var node = ast.body[0];
+
+            assert(astUtils.isLoop(node));
+        });
+
+        it("should return true for ForInStatement", function() {
+            var ast = espree.parse("for (var k in obj) {}");
+            var node = ast.body[0];
+
+            assert(astUtils.isLoop(node));
+        });
+
+        it("should return true for ForOfStatement", function() {
+            var ast = espree.parse("for (var x of list) {}", {ecmaVersion: 6});
+            var node = ast.body[0];
+
+            assert(astUtils.isLoop(node));
+        });
+
+        it("should return true for ForStatement", function() {
+            var ast = espree.parse("for (var i = 0; i < 10; ++i) {}");
+            var node = ast.body[0];
+
+            assert(astUtils.isLoop(node));
+        });
+
+        it("should return true for WhileStatement", function() {
+            var ast = espree.parse("while (a) {}");
+            var node = ast.body[0];
+
+            assert(astUtils.isLoop(node));
+        });
+
+        it("should return false for Program, VariableDeclaration, BlockStatement", function() {
+            var ast = espree.parse("var a; { }");
+
+            assert(!astUtils.isLoop(ast));
+            assert(!astUtils.isLoop(ast.body[0]));
+            assert(!astUtils.isLoop(ast.body[1]));
+        });
+    });
 });
diff --git a/tests/lib/rules/no-unused-vars.js b/tests/lib/rules/no-unused-vars.js
@@ -199,7 +199,20 @@ ruleTester.run("no-unused-vars", rule, {
         {code: "function foo(cb) { cb = function() { function something(a) { cb(1 + a); } register(something); }(); } foo();"},
         {code: "function* foo(cb) { cb = yield function(a) { cb(1 + a); }; } foo();", parserOptions: {ecmaVersion: 6}},
         {code: "function foo(cb) { cb = tag`hello${function(a) { cb(1 + a); }}`; } foo();", parserOptions: {ecmaVersion: 6}},
-        {code: "function foo(cb) { var b; cb = b = function(a) { cb(1 + a); }; b(); } foo();"}
+        {code: "function foo(cb) { var b; cb = b = function(a) { cb(1 + a); }; b(); } foo();"},
+
+        // https://github.com/eslint/eslint/issues/6646
+        {
+            code: [
+                "function someFunction() {",
+                "    var a = 0, i;",
+                "    for (i = 0; i < 2; i++) {",
+                "        a = myFunction(a);",
+                "    }",
+                "}",
+                "someFunction();"
+            ].join("\n")
+        }
     ],
     invalid: [
         { code: "function foox() { return foox(); }", errors: [{ message: "'foox' is defined but never used", type: "Identifier"}] },
@@ -425,6 +438,19 @@ ruleTester.run("no-unused-vars", rule, {
         {
             code: "function foo(cb) { cb = (0, function(a) { cb(1 + a); }); } foo();",
             errors: [{message: "'cb' is defined but never used"}]
+        },
+
+        // https://github.com/eslint/eslint/issues/6646
+        {
+            code: [
+                "while (a) {",
+                "    function foo(b) {",
+                "        b = b + 1;",
+                "    }",
+                "    foo()",
+                "}"
+            ].join("\n"),
+            errors: [{message: "'b' is defined but never used"}]
         }
     ]
 });