Repository: eslint/eslint
Pull request #11044 (2018-10-31 19:33:14)

Fix: rule deprecation warnings did not consider all rules
<!--
    ESLint adheres to the [JS Foundation Code of Conduct](https://js.foundation/community/code-of-conduct).
-->

**What is the purpose of this pull request? (put an "X" next to item)**

[x] Bug fix

<!--
    If the item you've checked above has a template, please paste the template questions below and answer them. (If this pull request is addressing an issue, you can just paste a link to the issue here instead.)
-->

<!--
    Please ensure your pull request is ready:

    - Read the pull request guide (https://eslint.org/docs/developer-guide/contributing/pull-requests)
    - Include tests for this change
    - Update documentation for this change (if appropriate)
-->

<!--
    The following is required for all pull requests:
-->

**Tell us about your environment**

* **ESLint Version:** master
* **Node Version:** 11.0.0
* **npm Version:** 6.4.1

**What parser (default, Babel-ESLint, etc.) are you using?**

N/A

**Please show your full configuration:**

N/A

**What did you do? Please include the actual source code causing the issue.**

From within the ESLint repository:

```js
const CLIEngine = require(".").CLIEngine;
const cliEngine = new CLIEngine();

console.log(cliEngine.executeOnFiles(["Makefile.js"]));
```

Ran `eslint Makefile.js --format json` in the ESLint repository

**What did you expect to happen?**

I expected the output to contain a `usedDeprecatedRules` property indicating that the `no-catch-shadow` rule (which we enable in `eslint-config-eslint`) has been replaced by `no-shadow`.

**What actually happened? Please include the actual, raw output from ESLint.**

```
{ results:
   [ { filePath: '/path/to/eslint/Makefile.js',
       messages: [],
       errorCount: 0,
       warningCount: 0,
       fixableErrorCount: 0,
       fixableWarningCount: 0 } ],
  errorCount: 0,
  warningCount: 0,
  fixableErrorCount: 0,
  fixableWarningCount: 0,
  usedDeprecatedRules: [] }
```

**What changes did you make? (Give an overview)**

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.

Additionally, `usedDeprecatedRules` was only appearing on the result of `CLIEngine#executeOnFiles` before. I updated it to also appear on the result of `CLIEngine#executeOnText`, since the two functions are documented to have the same output format.

**Is there anything you'd like reviewers to focus on?**

Nothing in particular


Patch:
diff --git a/docs/maintainer-guide/governance.md b/docs/maintainer-guide/governance.md
index 81d5908ea..0016d1890 100644
--- a/docs/maintainer-guide/governance.md
+++ b/docs/maintainer-guide/governance.md
@@ -91,7 +91,7 @@ A Committer is invited to become a TSC member by existing TSC members. A nominat
 
 #### Process for Adding TSC Members
 
-1. Add the GitHub user to the "ESLint TSC" team
+1. Add the GitHub user to the "ESLint TSC" GitHub team
 1. Set the GitHub user to be have the "Owner" role for the ESLint organization
 1. Send a welcome email with a link to the [maintainer guide](./) and the [npm 2FA guide](./npm-2fa).
 1. Add the TSC member to the README
@@ -99,6 +99,7 @@ A Committer is invited to become a TSC member by existing TSC members. A nominat
 1. Make the TSC member an admin on the ESLint team mailing list
 1. Add the TSC member to the recurring TSC meeting event on Google Calendar
 1. Add the TSC member as an admin to ESLint Twitter Account on Tweetdeck
+1. Add the TSC member to the ESLint TSC mailing list as an "Owner"
 1. Tweet congratulations to the new TSC member from the ESLint Twitter account
 
 #### TSC Meetings
diff --git a/lib/cli-engine.js b/lib/cli-engine.js
index 1545bf4c0..f801aee48 100644
--- a/lib/cli-engine.js
+++ b/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
         };
     }
 
diff --git a/packages/eslint-config-eslint/default.yml b/packages/eslint-config-eslint/default.yml
index 579b53abc..30444d640 100644
--- a/packages/eslint-config-eslint/default.yml
+++ b/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"


Test patch:
diff --git a/tests/fixtures/cli-engine/deprecated-rule-config/.eslintrc.yml b/tests/fixtures/cli-engine/deprecated-rule-config/.eslintrc.yml
new file mode 100644
index 000000000..e4ea290db
--- /dev/null
+++ b/tests/fixtures/cli-engine/deprecated-rule-config/.eslintrc.yml
@@ -0,0 +1,2 @@
+rules:
+  indent-legacy: error
diff --git a/tests/lib/cli-engine.js b/tests/lib/cli-engine.js
index 92458b59a..acc093b31 100644
--- a/tests/lib/cli-engine.js
+++ b/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: []
                 }
             );
         });