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

.eslintrc.js

@@ -17,7 +17,22 @@ module.exports = {
         "eslint-plugin/prefer-placeholders": "error",
         "eslint-plugin/report-message-format": ["error", "[^a-z].*\\.$"],
         "eslint-plugin/require-meta-type": "error",
-        "eslint-plugin/test-case-property-ordering": "error",
+        "eslint-plugin/test-case-property-ordering": [
+            "error",
+
+            // https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/issues/79
+            [
+                "filename",
+                "code",
+                "output",
+                "options",
+                "parser",
+                "parserOptions",
+                "globals",
+                "env",
+                "errors"
+            ]
+        ],
         "eslint-plugin/test-case-shorthand-strings": "error",
         "internal-rules/multiline-comment-style": "error"
     },

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 that [`eslint-scope` package defined](scope-manager-interface.md). The `Variable` objects of global variables have some additional properties.
+
+* `variable.writeable` (`boolean`) ... If `true`, this global variable can be assigned arbitrary value. Otherwise, this global variable is read-only.
+* `variable.eslintExplicitGlobal` (`boolean`) ... If `true`, this global variable was defined by a `/* globals */` directive comment in the source code file.
+* `variable.eslintExplicitGlobalComments` (`Comment[]`) ... The array of `/* globals */` directive comments which defined this global variable in the source code file.
+* `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

docs/user-guide/migrating-to-6.0.0.md

@@ -0,0 +1,52 @@
+# Migrating to v6.0.0
+
+ESLint v6.0.0 is the fifth major version release. We have made a few breaking changes in this release, but we expect that most users will be able to upgrade without any modifications to their build. This guide is intended to walk you through the breaking changes.
+
+The lists below are ordered roughly by the number of users each change is expected to affect, where the first items are expected to affect the most users.
+
+### Breaking changes for users
+
+* [`no-redeclare` rule now checks the redeclarations by `/* globals */` directive comments](#no-redeclare-and-comments)
+* [`no-redeclare` rule now checks the redeclarations with built-in globals by default](#no-redeclare-and-builtins)
+
+### Breaking changes for plugin/custom rule developers
+
+* [`variable.eslintExplicitGlobalComment` property was removed](#remove-variable-explicit-global-comment)
+
+### Breaking changes for integration developers
+
+---
+
+<!-- Breaking changes for users -->
+
+## <a id="no-redeclare-and-comments"></a> `no-redeclare` rule now checks the redeclarations by `/* globals */` directive comments
+
+[no-redeclare] rule reports the following cases newly:
+
+* Your config file defined a global variable but there is `/* globals */` directive comment of the defined global variable in your source code.
+* There are multiple `/* globals */` directive comments for the same variable.
+
+**To address:** Please remove the redundant `/* globals */` directive comments.
+
+## <a id="no-redeclare-and-builtins"></a> `no-redeclare` rule now checks the redeclarations with built-in globals by default
+
+The `builtinGlobals` option of [no-redeclare] rule is `true` by default. Previously, it was `false` by default.
+
+**To address:** Please remove the redundant declarations or disable `builtinGlobals` option manually.
+
+---
+
+<!-- Breaking changes for plugin/custom rule developers -->
+
+## <a id="remove-variable-explicit-global-comment"></a> `variable.eslintExplicitGlobalComment` property was removed
+
+Undocumented `variable.eslintExplicitGlobalComment` property, only [no-unused-vars] rule had used it, was removed.
+
+**To address:** Please use [`variable.eslintExplicitGlobalComments` property](../working-with-rules.md#contextgetscope) instead.
+
+---
+
+<!-- Breaking changes for integration developers -->
+
+[no-redeclare]: https://eslint.org/docs/rules/no-redeclare
+[no-unused-vars]: https://eslint.org/docs/rules/no-unused-vars

lib/config/config-ops.js

@@ -387,18 +387,18 @@ 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";
 
             // Fallback to minimize compatibility impact
             default:
-                return "writeable";
+                return "writable";
         }
     }
 };

lib/linter.js

@@ -70,34 +70,36 @@ 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(
-        {},
-        lodash.mapValues(configGlobals, value => ({ sourceComment: null, value: ConfigOps.normalizeConfigGlobal(value) })),
-        lodash.mapValues(commentDirectives.enabledGlobals, ({ comment, value }) => ({ sourceComment: comment, value: ConfigOps.normalizeConfigGlobal(value) }))
-    );
+function addDeclaredGlobals(globalScope, configGlobals, { exportedVariables, enabledGlobals }) {
 
-    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");
-        });
+    // Define configured global variables.
+    for (const id of new Set([...Object.keys(configGlobals), ...Object.keys(enabledGlobals)])) {
+        const configValue = configGlobals[id] === void 0 ? void 0 : ConfigOps.normalizeConfigGlobal(configGlobals[id]);
+        const commentValue = enabledGlobals[id] && ConfigOps.normalizeConfigGlobal(enabledGlobals[id].value);
+        const value = commentValue || configValue;
+        const sourceComments = enabledGlobals[id] && enabledGlobals[id].comments;
+
+        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) {
@@ -152,7 +154,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) {
@@ -197,7 +199,14 @@ function getDirectiveComments(filename, ast, ruleMapper) {
 
                 case "globals":
                 case "global":
-                    Object.assign(enabledGlobals, commentParser.parseStringConfig(directiveValue, comment));
+                    for (const [id, { value }] of Object.entries(commentParser.parseStringConfig(directiveValue, comment))) {
+                        if (enabledGlobals[id]) {
+                            enabledGlobals[id].comments.push(comment);
+                            enabledGlobals[id].value = value;
+                        } else {
+                            enabledGlobals[id] = { comments: [comment], value };
+                        }
+                    }
                     break;
 
                 case "eslint-disable":