From 47ed64f02c0fe2db45d0e3f189d223eed636fa25 Mon Sep 17 00:00:00 2001 From: Toru Nagashima Date: Wed, 22 Jul 2015 22:38:06 +0900 Subject: [PATCH] Fix: `block-scoped-var` issues (fixes #2253, fixes #2747, fixes #2967) --- lib/rules/block-scoped-var.js | 379 ++++++++-------------------- tests/lib/rules/block-scoped-var.js | 20 +- 2 files changed, 118 insertions(+), 281 deletions(-) diff --git a/lib/rules/block-scoped-var.js b/lib/rules/block-scoped-var.js index 61645f4555f4..f0df24343a59 100644 --- a/lib/rules/block-scoped-var.js +++ b/lib/rules/block-scoped-var.js @@ -1,337 +1,158 @@ /** * @fileoverview Rule to check for "block scoped" variables by binding context * @author Matt DuVall + * @copyright 2015 Toru Nagashima. All rights reserved. * @copyright 2015 Mathieu M-Gosselin. All rights reserved. */ "use strict"; //------------------------------------------------------------------------------ -// Rule Definition +// Helpers //------------------------------------------------------------------------------ -module.exports = function(context) { - - var scopeStack = []; - - //-------------------------------------------------------------------------- - // Helpers - //-------------------------------------------------------------------------- - - /** - * Determines whether an identifier is in declaration position or is a non-declaration reference. - * @param {ASTNode} id The identifier. - * @param {ASTNode} parent The identifier's parent AST node. - * @returns {Boolean} true when the identifier is in declaration position. - */ - function isDeclaration(id, parent) { - switch (parent.type) { - case "FunctionDeclaration": - case "FunctionExpression": - return parent.params.indexOf(id) > -1 || id === parent.id; - - case "VariableDeclarator": - return id === parent.id; +/** + * Collects unresolved references from the global scope, then creates a map to references from its name. + * @param {RuleContext} context - The current context. + * @returns {object} A map object. Its key is the variable names. Its value is the references of each variable. + */ +function collectUnresolvedReferences(context) { + var unresolved = Object.create(null); + var references = context.getScope().through; - case "CatchClause": - return id === parent.param; + for (var i = 0; i < references.length; ++i) { + var reference = references[i]; + var name = reference.identifier.name; - default: - return false; + if (name in unresolved === false) { + unresolved[name] = []; } + unresolved[name].push(reference); } - /** - * Determines whether an identifier is in property position. - * @param {ASTNode} id The identifier. - * @param {ASTNode} parent The identifier's parent AST node. - * @returns {Boolean} true when the identifier is in property position. - */ - function isProperty(id, parent) { - switch (parent.type) { - case "MemberExpression": - return id === parent.property && !parent.computed; + return unresolved; +} - case "Property": - return id === parent.key; +/** + * Checks whether or not there is a variable of a given name. + * @param {escope.Variable[]} variables - An array of variables to be found. + * @param {string} name - A name to find. + * @returns {boolean} Whether or not there is a variable of the given name. + */ +function exists(variables, name) { + return variables.some(function(variable) { + return variable.name === name; + }); +} - default: - return false; - } - } +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ - /** - * Pushes a new scope object on the scope stack. - * @returns {void} - */ - function pushScope() { - scopeStack.push([]); - } +module.exports = function(context) { + var unresolvedReferences = Object.create(null); + var stack = []; /** - * Removes the topmost scope object from the scope stack. + * Makes a block scope. + * @param {ASTNode} node - A node of a scope. * @returns {void} */ - function popScope() { - scopeStack.pop(); + function enterScope(node) { + stack.push(node.range); } /** - * Declares the given names in the topmost scope object. - * @param {[String]} names A list of names to declare. + * Pops the last block scope. * @returns {void} */ - function declare(names) { - [].push.apply(scopeStack[scopeStack.length - 1], names); + function exitScope() { + stack.pop(); } - //-------------------------------------------------------------------------- - // Public API - //-------------------------------------------------------------------------- - /** - * Declares all relevant identifiers for module imports. - * @param {ASTNode} node The AST node representing an import. + * Reports a given reference. + * @param {escope.Reference} reference - A reference to report. * @returns {void} - * @private */ - function declareImports(node) { - declare([node.local.name]); - - if (node.imported && node.imported.name !== node.local.name) { - declare([node.imported.name]); - } + function report(reference) { + var identifier = reference.identifier; + context.report( + identifier, + "\"{{name}}\" used outside of binding context.", + {name: identifier.name}); } /** - * Declares all relevant identifiers for module exports. - * @param {ASTNode} node The AST node representing an export. + * Finds and reports references which are outside of valid scopes. + * @param {ASTNode} node - A node to get variables. * @returns {void} - * @private */ - function declareExports(node) { - if (node.exported && node.exported.name) { - declare([node.exported.name]); - - if (node.local) { - declare([node.local.name]); - } + function checkForVariables(node) { + if (node.kind !== "var") { + return; } - } - - /** - * Declares all relevant identifiers for classes. - * @param {ASTNode} node The AST node representing a class. - * @returns {void} - * @private - */ - function declareClass(node) { - if (node.id) { - declare([node.id.name]); + // Defines a predicate to check whether or not a given reference is outside of valid scope. + var scopeRange = stack[stack.length - 1]; + function isOutsideOfScope(reference) { + var idRange = reference.identifier.range; + return idRange[0] < scopeRange[0] || idRange[1] > scopeRange[1]; } - pushScope(); - } - - /** - * Declares all relevant identifiers for classes. - * @param {ASTNode} node The AST node representing a class. - * @returns {void} - * @private - */ - function declareClassMethod(node) { - pushScope(); - - declare([node.key.name]); - } - - /** - * Add declarations based on the type of node being passed. - * @param {ASTNode} node The node containing declarations. - * @returns {void} - * @private - */ - function declareByNodeType(node) { - - var declarations = []; - - switch (node.type) { - case "Identifier": - declarations.push(node.name); - break; - - case "ObjectPattern": - node.properties.forEach(function(property) { - declarations.push(property.key.name); - if (property.value) { - declarations.push(property.value.name); - } - }); - break; - - case "ArrayPattern": - node.elements.forEach(function(element) { - if (element) { - declarations.push(element.name); - } - }); - break; + // Gets declared variables, and checks its references. + var variables = context.getDeclaredVariables(node); + for (var i = 0; i < variables.length; ++i) { + var variable = variables[i]; + var references = variable.references; - case "AssignmentPattern": - declareByNodeType(node.left); - break; + // Some global variables are possibly unresolved. + // In this case, use unresolved references. + if (references.length === 0 && variable.name in unresolvedReferences) { + references = unresolvedReferences[variable.name]; - case "RestElement": - declareByNodeType(node.argument); - break; + // Remove processed references. + // In order to report references of undefined variables later. + delete unresolvedReferences[variable.name]; + } - // no default + // Reports. + references.filter(isOutsideOfScope).forEach(report); } - - declare(declarations); - - } - - /** - * Adds declarations of the function parameters and pushes the scope - * @param {ASTNode} node The node containing declarations. - * @returns {void} - * @private - */ - function functionHandler(node) { - pushScope(); - - node.params.forEach(function(param) { - declareByNodeType(param); - }); - - declare(node.rest ? [node.rest.name] : []); - declare(["arguments"]); - } - - /** - * Adds declaration of the function name in its parent scope then process the function - * @param {ASTNode} node The node containing declarations. - * @returns {void} - * @private - */ - function functionDeclarationHandler(node) { - declare(node.id ? [node.id.name] : []); - functionHandler(node); } /** - * Process function declarations and declares its name in its own scope - * @param {ASTNode} node The node containing declarations. + * Finds and reports references which are not resolved (same as no-undef). * @returns {void} - * @private */ - function functionExpressionHandler(node) { - functionHandler(node); - declare(node.id ? [node.id.name] : []); - } - - function variableDeclarationHandler(node) { - node.declarations.forEach(function(declaration) { - declareByNodeType(declaration.id); - }); - + function checkForUnresolvedReferences() { + var globals = context.getScope().variables; + + for (var name in unresolvedReferences) { + // Skip built-in global variables. + // Those references are valid anywhere. + if (!exists(globals, name)) { + unresolvedReferences[name].forEach(report); + } + } } return { - "Program": function() { - var scope = context.getScope(); - scopeStack = [scope.variables.map(function(v) { - return v.name; - })]; - - // global return creates another scope - if (context.ecmaFeatures.globalReturn) { - scope = scope.childScopes[0]; - scopeStack.push(scope.variables.map(function(v) { - return v.name; - })); - } - }, - - "ImportSpecifier": declareImports, - "ImportDefaultSpecifier": declareImports, - "ImportNamespaceSpecifier": declareImports, - - "ExportSpecifier": declareExports, - - "BlockStatement": function(node) { - var statements = node.body; - pushScope(); - statements.forEach(function(stmt) { - if (stmt.type === "VariableDeclaration") { - variableDeclarationHandler(stmt); - } else if (stmt.type === "FunctionDeclaration") { - declare([stmt.id.name]); - } - }); - }, - - "VariableDeclaration": function(node) { - variableDeclarationHandler(node); - }, - - "BlockStatement:exit": popScope, - - "CatchClause": function(node) { - pushScope(); - declare([node.param.name]); - }, - "CatchClause:exit": popScope, - - "FunctionDeclaration": functionDeclarationHandler, - "FunctionDeclaration:exit": popScope, - - "ClassDeclaration": declareClass, - "ClassDeclaration:exit": popScope, - - "ClassExpression": declareClass, - "ClassExpression:exit": popScope, - - "MethodDefinition": declareClassMethod, - "MethodDefinition:exit": popScope, - - "FunctionExpression": functionExpressionHandler, - "FunctionExpression:exit": popScope, - - // Arrow functions cannot have names - "ArrowFunctionExpression": functionHandler, - "ArrowFunctionExpression:exit": popScope, - - "ForStatement": function() { - pushScope(); - }, - "ForStatement:exit": popScope, - - "ForInStatement": function() { - pushScope(); - }, - "ForInStatement:exit": popScope, - - "ForOfStatement": function() { - pushScope(); + "Program": function(node) { + unresolvedReferences = collectUnresolvedReferences(context); + stack = [node.range]; }, - "ForOfStatement:exit": popScope, - - "Identifier": function(node) { - var ancestor = context.getAncestors().pop(); - if (isDeclaration(node, ancestor) || isProperty(node, ancestor) || ancestor.type === "LabeledStatement" || ancestor.type === "BreakStatement" || ancestor.type === "ContinueStatement") { - return; - } - for (var i = 0, l = scopeStack.length; i < l; i++) { - if (scopeStack[i].indexOf(node.name) > -1) { - return; - } - } - - context.report(node, "\"" + node.name + "\" used outside of binding context."); - } + // Manages scopes. + "BlockStatement": enterScope, + "BlockStatement:exit": exitScope, + "SwitchStatement": enterScope, + "SwitchStatement:exit": exitScope, + "CatchClause": enterScope, + "CatchClause:exit": exitScope, + + // Finds and reports references which are outside of valid scope. + "VariableDeclaration": checkForVariables, + "Program:exit": checkForUnresolvedReferences }; }; diff --git a/tests/lib/rules/block-scoped-var.js b/tests/lib/rules/block-scoped-var.js index 4c1ffc2f586a..5a4f402b89e0 100644 --- a/tests/lib/rules/block-scoped-var.js +++ b/tests/lib/rules/block-scoped-var.js @@ -58,7 +58,7 @@ eslintTester.addRuleTest("lib/rules/block-scoped-var", { "function f(){ for(var i; i; i) i; }", "function f(){ for(var a=0, b=1; a; b) a, b; }", "function f(){ for(var a in {}) a; }", - "function f(){ switch(2) { case 1: var b = 2; b; break; default: b; break;} b; }", + "function f(){ switch(2) { case 1: var b = 2; b; break; default: b; break;} }", "a:;", "foo: while (true) { bar: for (var i = 0; i < 13; ++i) {if (i === 7) break foo; } }", "foo: while (true) { bar: for (var i = 0; i < 13; ++i) {if (i === 7) continue foo; } }", @@ -75,7 +75,19 @@ eslintTester.addRuleTest("lib/rules/block-scoped-var", { { code: "class Test { get flag() { return true; }}", ecmaFeatures: { classes: true }}, { code: "var Test = class { myFunction() { return true; }}", ecmaFeatures: { classes: true }}, { code: "var doStuff; let {x: y} = {x: 1}; doStuff(y);", ecmaFeatures: { blockBindings: true, destructuring: true }}, - { code: "function foo({x: y}) { return y; }", ecmaFeatures: { blockBindings: true, destructuring: true }} + { code: "function foo({x: y}) { return y; }", ecmaFeatures: { blockBindings: true, destructuring: true }}, + + // https://github.com/eslint/eslint/issues/2253 + { code: "/*global React*/ let {PropTypes, addons: {PureRenderMixin}} = React; let Test = React.createClass({mixins: [PureRenderMixin]});", ecmaFeatures: { blockBindings: true, destructuring: true }}, + { code: "/*global prevState*/ const { virtualSize: prevVirtualSize = 0 } = prevState;", ecmaFeatures: { blockBindings: true, destructuring: true }}, + { code: "const { dummy: { data, isLoading }, auth: { isLoggedIn } } = this.props;", ecmaFeatures: { blockBindings: true, destructuring: true }}, + + // https://github.com/eslint/eslint/issues/2747 + { code: "function a(n) { return n > 0 ? b(n - 1) : \"a\"; } function b(n) { return n > 0 ? a(n - 1) : \"b\"; }"}, + + // https://github.com/eslint/eslint/issues/2967 + { code: "(function () { foo(); })(); function foo() {}"}, + { code: "(function () { foo(); })(); function foo() {}", ecmaFeatures: { modules: true }} ], invalid: [ { code: "!function f(){}; f", errors: [{ message: "\"f\" used outside of binding context." }] }, @@ -106,6 +118,10 @@ eslintTester.addRuleTest("lib/rules/block-scoped-var", { code: "function a() { for(var b of {}) { var c = b;} c; }", ecmaFeatures: { forOf: true }, errors: [{ message: "\"c\" used outside of binding context.", type: "Identifier" }] + }, + { + code: "function f(){ switch(2) { case 1: var b = 2; b; break; default: b; break;} b; }", + errors: [{ message: "\"b\" used outside of binding context.", type: "Identifier" }] } ] });