Fix: rule deprecation warnings did not consider all rules (#11044)

This fixes a bug where rule deprecation warnings would only be generated for rules passed via the `--rule` flag on the command line, rather for all rules configured in a config file. It also addresses an issue where passing a nonexistent rule on the command line would cause CLIEngine to crash, which broke the eslint-canary build.

lib/cli-engine.js

@@ -22,6 +22,7 @@ const fs = require("fs"),
     lodash = require("lodash"),
     IgnoredPaths = require("./ignored-paths"),
     Config = require("./config"),
+    ConfigOps = require("./config/config-ops"),
     LintResultCache = require("./util/lint-result-cache"),
     globUtils = require("./util/glob-utils"),
     validator = require("./config/config-validator"),
@@ -143,7 +144,7 @@ function calculateStatsPerRun(results) {
  * @param {boolean} allowInlineConfig Allow/ignore comments that change config.
  * @param {boolean} reportUnusedDisableDirectives Allow/ignore comments that change config.
  * @param {Linter} linter Linter context
- * @returns {LintResult} The results for linting on this text.
+ * @returns {{rules: LintResult, config: Object}} The results for linting on this text and the fully-resolved config for it.
  * @private
  */
 function processText(text, configHelper, filename, fix, allowInlineConfig, reportUnusedDisableDirectives, linter) {
@@ -204,7 +205,7 @@ function processText(text, configHelper, filename, fix, allowInlineConfig, repor
         result.source = text;
     }
 
-    return result;
+    return { result, config };
 }
 
 /**
@@ -214,24 +215,22 @@ function processText(text, configHelper, filename, fix, allowInlineConfig, repor
  * @param {Object} configHelper The configuration options for ESLint.
  * @param {Object} options The CLIEngine options object.
  * @param {Linter} linter Linter context
- * @returns {LintResult} The results for linting on this file.
+ * @returns {{rules: LintResult, config: Object}} The results for linting on this text and the fully-resolved config for it.
  * @private
  */
 function processFile(filename, configHelper, options, linter) {
 
-    const text = fs.readFileSync(path.resolve(filename), "utf8"),
-        result = processText(
-            text,
-            configHelper,
-            filename,
-            options.fix,
-            options.allowInlineConfig,
-            options.reportUnusedDisableDirectives,
-            linter
-        );
-
-    return result;
-
+    const text = fs.readFileSync(path.resolve(filename), "utf8");
+
+    return processText(
+        text,
+        configHelper,
+        filename,
+        options.fix,
+        options.allowInlineConfig,
+        options.reportUnusedDisableDirectives,
+        linter
+    );
 }
 
 /**
@@ -275,32 +274,30 @@ function createIgnoreResult(filePath, baseDir) {
 
 /**
  * Produces rule warnings (i.e. deprecation) from configured rules
- * @param {Object} rules - Rules configured
+ * @param {(Array<string>|Set<string>)} usedRules - Rules configured
  * @param {Map} loadedRules - Map of loaded rules
- * @returns {Object} Contains rule warnings
+ * @returns {Array<Object>} Contains rule warnings
  * @private
  */
-function createRuleWarnings(rules, loadedRules) {
-    const ruleWarnings = { usedDeprecatedRules: [] };
-
-    if (rules) {
-        Object.keys(rules).forEach(name => {
-            const loadedRule = loadedRules.get(name);
+function createRuleDeprecationWarnings(usedRules, loadedRules) {
+    const usedDeprecatedRules = [];
 
-            if (loadedRule.meta && loadedRule.meta.deprecated) {
-                const deprecatedRule = { ruleId: name };
-                const replacedBy = lodash.get(loadedRule, "meta.replacedBy", []);
+    usedRules.forEach(name => {
+        const loadedRule = loadedRules.get(name);
 
-                if (replacedBy.every(newRule => lodash.isString(newRule))) {
-                    deprecatedRule.replacedBy = replacedBy;
-                }
+        if (loadedRule && loadedRule.meta && loadedRule.meta.deprecated) {
+            const deprecatedRule = { ruleId: name };
+            const replacedBy = lodash.get(loadedRule, "meta.replacedBy", []);
 
-                ruleWarnings.usedDeprecatedRules.push(deprecatedRule);
+            if (replacedBy.every(newRule => lodash.isString(newRule))) {
+                deprecatedRule.replacedBy = replacedBy;
             }
-        });
-    }
 
-    return ruleWarnings;
+            usedDeprecatedRules.push(deprecatedRule);
+        }
+    });
+
+    return usedDeprecatedRules;
 }
 
 /**
@@ -541,6 +538,7 @@ class CLIEngine {
 
         const startTime = Date.now();
         const fileList = globUtils.listFilesToProcess(patterns, options);
+        const allUsedRules = new Set();
         const results = fileList.map(fileInfo => {
             if (fileInfo.ignored) {
                 return createIgnoreResult(fileInfo.filename, options.cwd);
@@ -564,7 +562,13 @@ class CLIEngine {
 
             debug(`Processing ${fileInfo.filename}`);
 
-            return processFile(fileInfo.filename, configHelper, options, this.linter);
+            const { result, config } = processFile(fileInfo.filename, configHelper, options, this.linter);
+
+            Object.keys(config.rules)
+                .filter(ruleId => ConfigOps.getRuleSeverity(config.rules[ruleId]))
+                .forEach(ruleId => allUsedRules.add(ruleId));
+
+            return result;
         });
 
         if (options.cache) {
@@ -585,7 +589,7 @@ class CLIEngine {
 
         const stats = calculateStatsPerRun(results);
 
-        const ruleWarnings = createRuleWarnings(this.options.rules, this.getRules());
+        const usedDeprecatedRules = createRuleDeprecationWarnings(allUsedRules, this.getRules());
 
         debug(`Linting complete in: ${Date.now() - startTime}ms`);
 
@@ -595,7 +599,7 @@ class CLIEngine {
             warningCount: stats.warningCount,
             fixableErrorCount: stats.fixableErrorCount,
             fixableWarningCount: stats.fixableWarningCount,
-            usedDeprecatedRules: ruleWarnings.usedDeprecatedRules
+            usedDeprecatedRules
         };
     }
 
@@ -618,22 +622,28 @@ class CLIEngine {
         const resolvedFilename = filename && !path.isAbsolute(filename)
             ? path.resolve(options.cwd, filename)
             : filename;
+        let usedDeprecatedRules;
 
         if (resolvedFilename && ignoredPaths.contains(resolvedFilename)) {
             if (warnIgnored) {
                 results.push(createIgnoreResult(resolvedFilename, options.cwd));
             }
+            usedDeprecatedRules = [];
         } else {
-            results.push(
-                processText(
-                    text,
-                    configHelper,
-                    resolvedFilename,
-                    options.fix,
-                    options.allowInlineConfig,
-                    options.reportUnusedDisableDirectives,
-                    this.linter
-                )
+            const { result, config } = processText(
+                text,
+                configHelper,
+                resolvedFilename,
+                options.fix,
+                options.allowInlineConfig,
+                options.reportUnusedDisableDirectives,
+                this.linter
+            );
+
+            results.push(result);
+            usedDeprecatedRules = createRuleDeprecationWarnings(
+                Object.keys(config.rules).filter(rule => ConfigOps.getRuleSeverity(config.rules[rule])),
+                this.getRules()
             );
         }
 
@@ -644,7 +654,8 @@ class CLIEngine {
             errorCount: stats.errorCount,
             warningCount: stats.warningCount,
             fixableErrorCount: stats.fixableErrorCount,
-            fixableWarningCount: stats.fixableWarningCount
+            fixableWarningCount: stats.fixableWarningCount,
+            usedDeprecatedRules
         };
     }
 

packages/eslint-config-eslint/default.yml

@@ -53,7 +53,6 @@ rules:
     no-async-promise-executor: "error"
     no-buffer-constructor: "error"
     no-caller: "error"
-    no-catch-shadow: "error"
     no-confusing-arrow: "error"
     no-console: "error"
     no-delete-var: "error"

tests/fixtures/cli-engine/deprecated-rule-config/.eslintrc.yml

@@ -0,0 +1,2 @@
+rules:
+  indent-legacy: error

tests/lib/cli-engine.js

@@ -299,7 +299,8 @@ describe("CLIEngine", () => {
                 errorCount: 0,
                 warningCount: 0,
                 fixableErrorCount: 0,
-                fixableWarningCount: 0
+                fixableWarningCount: 0,
+                usedDeprecatedRules: []
             });
         });
 
@@ -371,7 +372,8 @@ describe("CLIEngine", () => {
                 errorCount: 1,
                 warningCount: 0,
                 fixableErrorCount: 0,
-                fixableWarningCount: 0
+                fixableWarningCount: 0,
+                usedDeprecatedRules: []
             });
         });
 
@@ -412,7 +414,8 @@ describe("CLIEngine", () => {
                 errorCount: 1,
                 warningCount: 0,
                 fixableErrorCount: 0,
-                fixableWarningCount: 0
+                fixableWarningCount: 0,
+                usedDeprecatedRules: []
             });
         });
 
@@ -453,7 +456,8 @@ describe("CLIEngine", () => {
                 errorCount: 1,
                 warningCount: 0,
                 fixableErrorCount: 0,
-                fixableWarningCount: 0
+                fixableWarningCount: 0,
+                usedDeprecatedRules: []
             });
         });
 
@@ -540,7 +544,8 @@ describe("CLIEngine", () => {
                 errorCount: 1,
                 warningCount: 0,
                 fixableErrorCount: 0,
-                fixableWarningCount: 0
+                fixableWarningCount: 0,
+                usedDeprecatedRules: []
             });
         });
 
@@ -598,6 +603,19 @@ describe("CLIEngine", () => {
                 assert.strictEqual(report.messages[0].message, "OK");
             });
         });
+        it("should warn when deprecated rules are found in a config", () => {
+            engine = new CLIEngine({
+                cwd: originalDir,
+                configFile: "tests/fixtures/cli-engine/deprecated-rule-config/.eslintrc.yml"
+            });
+
+            const report = engine.executeOnText("foo");
+
+            assert.deepStrictEqual(
+                report.usedDeprecatedRules,
+                [{ ruleId: "indent-legacy", replacedBy: ["indent"] }]
+            );
+        });
     });
 
     describe("executeOnFiles()", () => {
@@ -1402,6 +1420,20 @@ describe("CLIEngine", () => {
             assert.deepStrictEqual(report.usedDeprecatedRules, []);
         });
 
+        it("should warn when deprecated rules are found in a config", () => {
+            engine = new CLIEngine({
+                cwd: originalDir,
+                configFile: "tests/fixtures/cli-engine/deprecated-rule-config/.eslintrc.yml"
+            });
+
+            const report = engine.executeOnFiles(["lib/cli*.js"]);
+
+            assert.deepStrictEqual(
+                report.usedDeprecatedRules,
+                [{ ruleId: "indent-legacy", replacedBy: ["indent"] }]
+            );
+        });
+
         describe("Fix Mode", () => {
 
             it("should return fixed text on multiple files when in fix mode", () => {
@@ -3177,7 +3209,8 @@ describe("CLIEngine", () => {
                     errorCount: 1,
                     warningCount: 0,
                     fixableErrorCount: 0,
-                    fixableWarningCount: 0
+                    fixableWarningCount: 0,
+                    usedDeprecatedRules: []
                 }
             );
         });