From 27700cf13c60a22e49aa56e4753da656fbbe6376 Mon Sep 17 00:00:00 2001 From: Toru Nagashima Date: Wed, 6 Jul 2016 02:43:57 +0900 Subject: [PATCH] Fix: `no-unused-vars` false positive around callback (fixes #6576) (#6579) * Fix: `no-unused-vars` false positive around callback (fixes #6576) * Fix: `no-unused-vars` false positive about reuse (fixes #6576) --- docs/rules/no-unused-vars.md | 7 ++ lib/rules/no-unused-vars.js | 106 ++++++++++++++++++++++++++++-- tests/lib/rules/no-unused-vars.js | 56 +++++++++++++++- 3 files changed, 160 insertions(+), 9 deletions(-) diff --git a/docs/rules/no-unused-vars.md b/docs/rules/no-unused-vars.md index 89b91a82aea..df409bc15ce 100644 --- a/docs/rules/no-unused-vars.md +++ b/docs/rules/no-unused-vars.md @@ -11,6 +11,7 @@ A variable is considered to be used if any of the following are true: * It represents a function that is called (`doSomething()`) * It is read (`var y = x`) * It is passed into a function as an argument (`doSomething(x)`) +* It is read inside of a function that is passed to another function (`doSomething(function() { foo(); })`) A variable is *not* considered to be used if it is only ever assigned to (`var x = 5`) or declared. @@ -61,6 +62,12 @@ myFunc(function foo() { (function(foo) { return foo; })(); + +var myFunc; +myFunc = setTimeout(function() { + // myFunc is considered used + myFunc(); +}, 50); ``` ### exported diff --git a/lib/rules/no-unused-vars.js b/lib/rules/no-unused-vars.js index 87bec297074..39b7703c3c1 100644 --- a/lib/rules/no-unused-vars.js +++ b/lib/rules/no-unused-vars.js @@ -10,6 +10,7 @@ //------------------------------------------------------------------------------ var lodash = require("lodash"); +var astUtils = require("../ast-utils"); //------------------------------------------------------------------------------ // Rule Definition @@ -95,6 +96,8 @@ module.exports = { // Helpers //-------------------------------------------------------------------------- + var STATEMENT_TYPE = /(?:Statement|Declaration)$/; + /** * Determines if a given variable is being exported from a module. * @param {Variable} variable - EScope variable object. @@ -152,6 +155,20 @@ module.exports = { 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. + */ + function isInside(inner, outer) { + return ( + inner.range[0] >= outer.range[0] && + inner.range[1] <= outer.range[1] + ); + } + /** * If a given reference is left-hand side of an assignment, this gets * the right-hand side node of the assignment. @@ -164,27 +181,102 @@ module.exports = { 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; /* * Inherits the previous node if this reference is in the node. * This is for `a = a + a`-like code. */ - if (prevRhsNode && - prevRhsNode.range[0] <= id.range[0] && - prevRhsNode.range[1] >= id.range[1] - ) { + if (prevRhsNode && isInside(id, prevRhsNode)) { return prevRhsNode; } if (parent.type === "AssignmentExpression" && granpa.type === "ExpressionStatement" && - id === parent.left + id === parent.left && + !canBeUsedLater ) { return parent.right; } return null; } + /** + * Checks whether a given function node is stored to somewhere or not. + * If the function node is stored, the function can be used later. + * + * @param {ASTNode} funcNode - A function node to check. + * @param {ASTNode} rhsNode - The RHS node of the previous assignment. + * @returns {boolean} `true` if under the following conditions: + * - 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. + */ + function isStorableFunction(funcNode, rhsNode) { + var node = funcNode; + var parent = funcNode.parent; + + while (parent && isInside(parent, rhsNode)) { + switch (parent.type) { + case "SequenceExpression": + if (parent.expressions[parent.expressions.length - 1] !== node) { + return false; + } + break; + + case "CallExpression": + case "NewExpression": + return parent.callee !== node; + + case "AssignmentExpression": + case "TaggedTemplateExpression": + case "YieldExpression": + return true; + + default: + if (STATEMENT_TYPE.test(parent.type)) { + + /* + * If it encountered statements, this is a complex pattern. + * Since analyzeing complex patterns is hard, this returns `true` to avoid false positive. + */ + return true; + } + } + + node = parent; + parent = parent.parent; + } + + return false; + } + + /** + * Checks whether a given Identifier node exists inside of a function node which can be used later. + * + * "can be used later" means: + * - the function is assigned to a variable. + * - the function is bound to a property and the object can be used later. + * - the function is bound as an argument of a function call. + * + * If a reference exists in a function which can be used later, the reference is read when the function is called. + * + * @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. + */ + function isInsideOfStorableFunction(id, rhsNode) { + var funcNode = astUtils.getUpperFunction(id); + + return ( + funcNode && + isInside(funcNode, rhsNode) && + isStorableFunction(funcNode, rhsNode) + ); + } + /** * Checks whether a given reference is a read to update itself or not. * @@ -213,8 +305,8 @@ module.exports = { // in RHS of an assignment for itself. e.g. `a = a + 1` ( rhsNode && - rhsNode.range[0] <= id.range[0] && - rhsNode.range[1] >= id.range[1] + isInside(id, rhsNode) && + !isInsideOfStorableFunction(id, rhsNode) ) ); } diff --git a/tests/lib/rules/no-unused-vars.js b/tests/lib/rules/no-unused-vars.js index e641f208171..6f277987eba 100644 --- a/tests/lib/rules/no-unused-vars.js +++ b/tests/lib/rules/no-unused-vars.js @@ -165,7 +165,41 @@ ruleTester.run("no-unused-vars", rule, { {code: "var a = 0, b; b = a++; foo(b);"}, {code: "function foo(a) { var b = a = a + 1; bar(b) } foo();"}, {code: "function foo(a) { var b = a += a + 1; bar(b) } foo();"}, - {code: "function foo(a) { var b = a++; bar(b) } foo();"} + {code: "function foo(a) { var b = a++; bar(b) } foo();"}, + + // https://github.com/eslint/eslint/issues/6576 + { + code: [ + "var unregisterFooWatcher;", + "// ...", + "unregisterFooWatcher = $scope.$watch( \"foo\", function() {", + " // ...some code..", + " unregisterFooWatcher();", + "});" + ].join("\n") + }, + { + code: [ + "var ref;", + "ref = setInterval(", + " function(){", + " clearInterval(ref);", + " }, 10);", + ].join("\n") + }, + { + code: [ + "var _timer;", + "function f() {", + " _timer = setTimeout(function () {}, _timer ? 100 : 0);", + "}", + "f();", + ].join("\n") + }, + {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();"} ], invalid: [ { code: "function foox() { return foox(); }", errors: [{ message: "'foox' is defined but never used", type: "Identifier"}] }, @@ -373,6 +407,24 @@ ruleTester.run("no-unused-vars", rule, { {code: "function foo(a) { a += a + 1 } foo();", errors: [{message: "'a' is defined but never used"}]}, {code: "function foo(a) { a++ } foo();", errors: [{message: "'a' is defined but never used"}]}, {code: "var a = 3; a = a * 5 + 6;", errors: [{message: "'a' is defined but never used"}]}, - {code: "var a = 2, b = 4; a = a * 2 + b;", errors: [{message: "'a' is defined but never used"}]} + {code: "var a = 2, b = 4; a = a * 2 + b;", errors: [{message: "'a' is defined but never used"}]}, + + // https://github.com/eslint/eslint/issues/6576 (For coverage) + { + code: "function foo(cb) { cb = function(a) { cb(1 + a); }; bar(not_cb); } foo();", + errors: [{message: "'cb' is defined but never used"}] + }, + { + code: "function foo(cb) { cb = function(a) { return cb(1 + a); }(); } foo();", + errors: [{message: "'cb' is defined but never used"}] + }, + { + code: "function foo(cb) { cb = (function(a) { cb(1 + a); }, cb); } foo();", + errors: [{message: "'cb' is defined but never used"}] + }, + { + code: "function foo(cb) { cb = (0, function(a) { cb(1 + a); }); } foo();", + errors: [{message: "'cb' is defined but never used"}] + } ] });