Breaking: make no-redeclare stricter (fixes #11370, fixes #11405)

docs/developer-guide/working-with-rules.md

@@ -169,6 +169,13 @@ This method returns the scope which has the following types:
 **※2** Only if the `for` statement defines the iteration variable as a block-scoped variable (E.g., `for (let i = 0;;) {}`).<br>
 **※3** The scope of the closest ancestor node which has own scope. If the closest ancestor node has multiple scopes then it chooses the innermost scope (E.g., the `Program` node has a `global` scope and a `module` scope if `Program#sourceType` is `"module"`. The innermost scope is the `module` scope.).
 
+The returned value is a [`Scope` object](scope-manager-interface.md) defined by the `eslint-scope` package. The `Variable` objects of global variables have some additional properties.
+
+* `variable.writeable` (`boolean | undefined`) ... If `true`, this global variable can be assigned arbitrary value. If `false`, this global variable is read-only.
+* `variable.eslintExplicitGlobal` (`boolean | undefined`) ... If `true`, this global variable was defined by a `/* globals */` directive comment in the source code file.
+* `variable.eslintExplicitGlobalComments` (`Comment[] | undefined`) ... The array of `/* globals */` directive comments which defined this global variable in the source code file. This property is `undefined` if there are no `/* globals */` directive comments.
+* `variable.eslintImplicitGlobalSetting` (`"readonly" | "writable" | undefined`) ... The configured value in config files. This can be different from `variable.writeable` if there are `/* globals */` directive comments.
+
 ### context.report()
 
 The main method you'll use is `context.report()`, which publishes a warning or error (depending on the configuration being used). This method accepts a single argument, which is an object containing the following properties:

docs/rules/no-redeclare.md

@@ -27,7 +27,7 @@ a = 10;
 
 ## Options
 
-This rule takes one optional argument, an object with a boolean property `"builtinGlobals"`. It defaults to `false`.
+This rule takes one optional argument, an object with a boolean property `"builtinGlobals"`. It defaults to `true`.
 If set to `true`, this rule also checks redeclaration of built-in globals, such as `Object`, `Array`, `Number`...
 
 ### builtinGlobals

lib/config/config-ops.js

@@ -385,14 +385,14 @@ module.exports = {
             case "true":
             case "writeable":
             case "writable":
-                return "writeable";
+                return "writable";
 
             case null:
             case false:
             case "false":
             case "readable":
             case "readonly":
-                return "readable";
+                return "readonly";
 
             default:
                 throw new Error(`'${configuredValue}' is not a valid configuration for a global (use 'readonly', 'writable', or 'off')`);

lib/linter.js

@@ -70,39 +70,41 @@ const commentParser = new ConfigCommentParser();
  * @param {{exportedVariables: Object, enabledGlobals: Object}} commentDirectives Directives from comment configuration
  * @returns {void}
  */
-function addDeclaredGlobals(globalScope, configGlobals, commentDirectives) {
-    const mergedGlobalsInfo = Object.assign(
-        {},
+function addDeclaredGlobals(globalScope, configGlobals, { exportedVariables, enabledGlobals }) {
+
+    // Define configured global variables.
+    for (const id of new Set([...Object.keys(configGlobals), ...Object.keys(enabledGlobals)])) {
 
         /*
          * `ConfigOps.normalizeConfigGlobal` will throw an error if a configured global value is invalid. However, these errors would
          * typically be caught when validating a config anyway (validity for inline global comments is checked separately).
          */
-        lodash.mapValues(configGlobals, value => ({ sourceComment: null, value: ConfigOps.normalizeConfigGlobal(value) })),
-        lodash.mapValues(commentDirectives.enabledGlobals, ({ comment, value }) => ({ sourceComment: comment, value }))
-    );
+        const configValue = configGlobals[id] === void 0 ? void 0 : ConfigOps.normalizeConfigGlobal(configGlobals[id]);
+        const commentValue = enabledGlobals[id] && enabledGlobals[id].value;
+        const value = commentValue || configValue;
+        const sourceComments = enabledGlobals[id] && enabledGlobals[id].comments;
 
-    Object.keys(mergedGlobalsInfo)
-        .filter(name => mergedGlobalsInfo[name].value !== "off")
-        .forEach(name => {
-            let variable = globalScope.set.get(name);
-
-            if (!variable) {
-                variable = new eslintScope.Variable(name, globalScope);
-                if (mergedGlobalsInfo[name].sourceComment === null) {
-                    variable.eslintExplicitGlobal = false;
-                } else {
-                    variable.eslintExplicitGlobal = true;
-                    variable.eslintExplicitGlobalComment = mergedGlobalsInfo[name].sourceComment;
-                }
-                globalScope.variables.push(variable);
-                globalScope.set.set(name, variable);
-            }
-            variable.writeable = (mergedGlobalsInfo[name].value === "writeable");
-        });
+        if (value === "off") {
+            continue;
+        }
+
+        let variable = globalScope.set.get(id);
+
+        if (!variable) {
+            variable = new eslintScope.Variable(id, globalScope);
+
+            globalScope.variables.push(variable);
+            globalScope.set.set(id, variable);
+        }
+
+        variable.eslintImplicitGlobalSetting = configValue;
+        variable.eslintExplicitGlobal = sourceComments !== void 0;
+        variable.eslintExplicitGlobalComments = sourceComments;
+        variable.writeable = (value === "writable");
+    }
 
     // mark all exported variables as such
-    Object.keys(commentDirectives.exportedVariables).forEach(name => {
+    Object.keys(exportedVariables).forEach(name => {
         const variable = globalScope.set.get(name);
 
         if (variable) {
@@ -157,7 +159,7 @@ function createDisableDirectives(type, loc, value) {
  * @param {string} filename The file being checked.
  * @param {ASTNode} ast The top node of the AST.
  * @param {function(string): {create: Function}} ruleMapper A map from rule IDs to defined rules
- * @returns {{configuredRules: Object, enabledGlobals: Object, exportedVariables: Object, problems: Problem[], disableDirectives: DisableDirective[]}}
+ * @returns {{configuredRules: Object, enabledGlobals: {value:string,comment:Token}[], exportedVariables: Object, problems: Problem[], disableDirectives: DisableDirective[]}}
  * A collection of the directive comments that were found, along with any problems that occurred when parsing
  */
 function getDirectiveComments(filename, ast, ruleMapper) {
@@ -201,11 +203,8 @@ function getDirectiveComments(filename, ast, ruleMapper) {
                     break;
 
                 case "globals":
-                case "global": {
-                    const updatedGlobals = commentParser.parseStringConfig(directiveValue, comment);
-
-                    Object.keys(updatedGlobals).forEach(globalName => {
-                        const { value } = updatedGlobals[globalName];
+                case "global":
+                    for (const [id, { value }] of Object.entries(commentParser.parseStringConfig(directiveValue, comment))) {
                         let normalizedValue;
 
                         try {
@@ -221,13 +220,21 @@ function getDirectiveComments(filename, ast, ruleMapper) {
                                 endColumn: comment.loc.end.column + 1,
                                 nodeType: null
                             });
-                            return;
+                            continue;
                         }
 
-                        enabledGlobals[globalName] = { comment, value: normalizedValue };
-                    });
+                        if (enabledGlobals[id]) {
+                            enabledGlobals[id].comments.push(comment);
+                            enabledGlobals[id].value = normalizedValue;
+                        } else {
+                            enabledGlobals[id] = {
+                                comments: [comment],
+                                value: normalizedValue
+                            };
+                        }
+                    }
                     break;
-                }
+
                 case "eslint-disable":
                     disableDirectives.push(...createDisableDirectives("disable", comment.loc.start, directiveValue));
                     break;

lib/rules/no-redeclare.js

@@ -5,6 +5,12 @@
 
 "use strict";
 
+//------------------------------------------------------------------------------
+// Requirements
+//------------------------------------------------------------------------------
+
+const astUtils = require("../util/ast-utils");
+
 //------------------------------------------------------------------------------
 // Rule Definition
 //------------------------------------------------------------------------------
@@ -20,11 +26,17 @@ module.exports = {
             url: "https://eslint.org/docs/rules/no-redeclare"
         },
 
+        messages: {
+            redeclared: "'{{id}}' is already defined.",
+            redeclaredAsBuiltin: "'{{id}}' is already defined as a built-in global variable.",
+            redeclaredBySyntax: "'{{id}}' is already defined by a variable declaration."
+        },
+
         schema: [
             {
                 type: "object",
                 properties: {
-                    builtinGlobals: { type: "boolean", default: false }
+                    builtinGlobals: { type: "boolean", default: true }
                 },
                 additionalProperties: false
             }
@@ -33,72 +45,128 @@ module.exports = {
 
     create(context) {
         const options = {
-            builtinGlobals: context.options[0] && context.options[0].builtinGlobals
+            builtinGlobals: Boolean(
+                context.options.length === 0 ||
+                context.options[0].builtinGlobals
+            )
         };
+        const sourceCode = context.getSourceCode();
 
         /**
-         * Find variables in a given scope and flag redeclared ones.
-         * @param {Scope} scope - An eslint-scope scope object.
-         * @returns {void}
-         * @private
+         * Iterate declarations of a given variable.
+         * @param {escope.variable} variable The variable object to iterate declarations.
+         * @returns {IterableIterator<{type:string,node:ASTNode,loc:SourceLocation}>} The declarations.
          */
-        function findVariablesInScope(scope) {
-            scope.variables.forEach(variable => {
-                const hasBuiltin = options.builtinGlobals && "writeable" in variable;
-                const count = (hasBuiltin ? 1 : 0) + variable.identifiers.length;
+        function *iterateDeclarations(variable) {
+            if (options.builtinGlobals && (
+                variable.eslintImplicitGlobalSetting === "readonly" ||
+                variable.eslintImplicitGlobalSetting === "writable"
+            )) {
+                yield { type: "builtin" };
+            }
 
-                if (count >= 2) {
-                    variable.identifiers.sort((a, b) => a.range[1] - b.range[1]);
+            for (const id of variable.identifiers) {
+                yield { type: "syntax", node: id, loc: id.loc };
+            }
 
-                    for (let i = (hasBuiltin ? 0 : 1), l = variable.identifiers.length; i < l; i++) {
-                        context.report({ node: variable.identifiers[i], message: "'{{a}}' is already defined.", data: { a: variable.name } });
-                    }
+            if (variable.eslintExplicitGlobalComments) {
+                for (const comment of variable.eslintExplicitGlobalComments) {
+                    yield {
+                        type: "comment",
+                        node: comment,
+                        loc: astUtils.getNameLocationInGlobalDirectiveComment(
+                            sourceCode,
+                            comment,
+                            variable.name
+                        )
+                    };
                 }
-            });
-
+            }
         }
 
         /**
-         * Find variables in the current scope.
-         * @param {ASTNode} node - The Program node.
+         * Find variables in a given scope and flag redeclared ones.
+         * @param {Scope} scope - An eslint-scope scope object.
          * @returns {void}
          * @private
          */
-        function checkForGlobal(node) {
-            const scope = context.getScope(),
-                parserOptions = context.parserOptions,
-                ecmaFeatures = parserOptions.ecmaFeatures || {};
-
-            // Nodejs env or modules has a special scope.
-            if (ecmaFeatures.globalReturn || node.sourceType === "module") {
-                findVariablesInScope(scope.childScopes[0]);
-            } else {
-                findVariablesInScope(scope);
+        function findVariablesInScope(scope) {
+            for (const variable of scope.variables) {
+                const [
+                    declaration,
+                    ...extraDeclarations
+                ] = iterateDeclarations(variable);
+
+                if (extraDeclarations.length === 0) {
+                    continue;
+                }
+
+                /*
+                 * If the type of a declaration is different from the type of
+                 * the first declaration, it shows the location of the first
+                 * declaration.
+                 */
+                const detailMessageId = declaration.type === "builtin"
+                    ? "redeclaredAsBuiltin"
+                    : "redeclaredBySyntax";
+                const data = { id: variable.name };
+
+                // Report extra declarations.
+                for (const { type, node, loc } of extraDeclarations) {
+                    const messageId = type === declaration.type
+                        ? "redeclared"
+                        : detailMessageId;
+
+                    context.report({ node, loc, messageId, data });
+                }
             }
         }
 
         /**
          * Find variables in the current scope.
+         * @param {ASTNode} node The node of the current scope.
          * @returns {void}
          * @private
          */
-        function checkForBlock() {
-            findVariablesInScope(context.getScope());
-        }
+        function checkForBlock(node) {
+            const scope = context.getScope();
 
-        if (context.parserOptions.ecmaVersion >= 6) {
-            return {
-                Program: checkForGlobal,
-                BlockStatement: checkForBlock,
-                SwitchStatement: checkForBlock
-            };
+            /*
+             * In ES5, some node type such as `BlockStatement` doesn't have that scope.
+             * `scope.block` is a different node in such a case.
+             */
+            if (scope.block === node) {
+                findVariablesInScope(scope);
+            }
         }
+
         return {
-            Program: checkForGlobal,
+            Program() {
+                const scope = context.getScope();
+
+                findVariablesInScope(scope);
+
+                // Node.js or ES modules has a special scope.
+                if (
+                    scope.type === "global" &&
+                    scope.childScopes[0] &&
+
+                    // The special scope's block is the Program node.
+                    scope.block === scope.childScopes[0].block
+                ) {
+                    findVariablesInScope(scope.childScopes[0]);
+                }
+            },
+
             FunctionDeclaration: checkForBlock,
             FunctionExpression: checkForBlock,
-            ArrowFunctionExpression: checkForBlock
-        };
+            ArrowFunctionExpression: checkForBlock,
 
+            BlockStatement: checkForBlock,
+            ForStatement: checkForBlock,
+            ForInStatement: checkForBlock,
+            ForOfStatement: checkForBlock,
+            SwitchStatement: checkForBlock
+        };
     }
 };