From dfc20e9e2e530f7f3af036355e6bf699f6b5183a Mon Sep 17 00:00:00 2001 From: Toru Nagashima Date: Fri, 15 Jul 2016 11:08:43 +0900 Subject: [PATCH] Fix: `no-unused-vars` false positive in loop (fixes #6646) (#6649) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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. https://github.com/eslint/eslint/pull/6649#discussion_r70247810 * Chore: add tests for `isFunctionNode` and `isLoopNode` * Chore: rename `isLoopNode` → `isLoop` and `isFunctionNode` → `isFunction` --- lib/ast-utils.js | 33 ++++++++++ lib/rules/no-empty.js | 10 ++- lib/rules/no-labels.js | 10 ++- lib/rules/no-unmodified-loop-condition.js | 2 +- lib/rules/no-unused-vars.js | 38 +++++++++++- lib/rules/quotes.js | 5 +- tests/lib/ast-utils.js | 76 +++++++++++++++++++++++ tests/lib/rules/no-unused-vars.js | 28 ++++++++- 8 files changed, 186 insertions(+), 16 deletions(-) diff --git a/lib/ast-utils.js b/lib/ast-utils.js index c8d6dcb4915..8ebc064ea76 100644 --- 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 index 1302a907533..973d82058da 100644 --- 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 index da0cd8e7426..93b65f15d9c 100644 --- 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 index eb1b93e26c6..ae73e845ebc 100644 --- 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 index 39b7703c3c1..8050b823b1e 100644 --- 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,9 +196,17 @@ 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; @@ -183,7 +214,7 @@ module.exports = { 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 index d4e9203811c..7d41a2d643e 100644 --- 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 index 0f46fbcede6..4f5edc065c4 100644 --- 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 index 6f277987eba..2be44ef927c 100644 --- 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"}] } ] });