Breaking: Fixable disable directives (fixes #11815)

docs/developer-guide/nodejs-api.md

@@ -147,7 +147,7 @@ The `ESLint` constructor takes an `options` object. If you omit the `options` ob
 
 * `options.fix` (`boolean | (message: LintMessage) => boolean`)<br>
   Default is `false`. If `true` is present, the [`eslint.lintFiles()`][eslint-lintfiles] and [`eslint.lintText()`][eslint-linttext] methods work in autofix mode. If a predicate function is present, the methods pass each lint message to the function, then use only the lint messages for which the function returned `true`.
-* `options.fixTypes` (`("problem" | "suggestion" | "layout")[] | null`)<br>
+* `options.fixTypes` (`("directive" | "problem" | "suggestion" | "layout")[] | null`)<br>
   Default is `null`. The types of the rules that the [`eslint.lintFiles()`][eslint-lintfiles] and [`eslint.lintText()`][eslint-linttext] methods use for autofix.
 
 ##### Cache-related

lib/cli-engine/cli-engine.js

@@ -41,7 +41,7 @@ const hash = require("./hash");
 const LintResultCache = require("./lint-result-cache");
 
 const debug = require("debug")("eslint:cli-engine");
-const validFixTypes = new Set(["problem", "suggestion", "layout"]);
+const validFixTypes = new Set(["directive", "problem", "suggestion", "layout"]);
 
 //------------------------------------------------------------------------------
 // Typedefs
@@ -325,6 +325,23 @@ function getRule(ruleId, configArrays) {
     return builtInRules.get(ruleId) || null;
 }
 
+/**
+ * Checks whether a message's rule type should be fixed.
+ * @param {LintMessage} message The message to check.
+ * @param {ConfigArray[]} lastConfigArrays The list of config arrays that the last `executeOnFiles` or `executeOnText` used.
+ * @param {string[]} fixTypes An array of fix types to check.
+ * @returns {boolean} Whether the message should be fixed.
+ */
+function shouldMessageBeFixed(message, lastConfigArrays, fixTypes) {
+    if (!message.ruleId) {
+        return fixTypes.has("directive");
+    }
+
+    const rule = message.ruleId && getRule(message.ruleId, lastConfigArrays);
+
+    return rule && rule.meta && fixTypes.has(rule.meta.type);
+}
+
 /**
  * Collect used deprecated rules.
  * @param {ConfigArray[]} usedConfigArrays The config arrays which were used.
@@ -617,12 +634,7 @@ class CLIEngine {
             const originalFix = (typeof options.fix === "function")
                 ? options.fix : () => true;
 
-            options.fix = message => {
-                const rule = message.ruleId && getRule(message.ruleId, lastConfigArrays);
-                const matches = rule && rule.meta && fixTypes.has(rule.meta.type);
-
-                return matches && originalFix(message);
-            };
+            options.fix = message => shouldMessageBeFixed(message, lastConfigArrays, fixTypes) && originalFix(message);
         }
     }
 

lib/eslint/eslint.js

@@ -125,7 +125,7 @@ function isArrayOfNonEmptyString(x) {
  * @returns {boolean} `true` if `x` is valid fix type.
  */
 function isFixType(x) {
-    return x === "problem" || x === "suggestion" || x === "layout";
+    return x === "directive" || x === "problem" || x === "suggestion" || x === "layout";
 }
 
 /**
@@ -237,7 +237,7 @@ function processOptions({
         errors.push("'fix' must be a boolean or a function.");
     }
     if (fixTypes !== null && !isFixTypeArray(fixTypes)) {
-        errors.push("'fixTypes' must be an array of any of \"problem\", \"suggestion\", and \"layout\".");
+        errors.push("'fixTypes' must be an array of any of \"directive\", \"problem\", \"suggestion\", and \"layout\".");
     }
     if (typeof globInputPaths !== "boolean") {
         errors.push("'globInputPaths' must be a boolean.");

lib/linter/apply-disable-directives.js

@@ -18,6 +18,76 @@ function compareLocations(itemA, itemB) {
     return itemA.line - itemB.line || itemA.column - itemB.column;
 }
 
+/**
+ * Returns whether a comment is alone on its line.
+ * @param {Comment} comment The comment node which will be deleted.
+ * @param {string} line Contents of the comment's line.
+ * @returns {boolean} Whether the comment is alone on its line.
+ */
+function commentIsAloneOnLine(comment, line) {
+    return comment.type === "Block" ? line.length === comment.value.length + "/**/".length : line.startsWith("//");
+}
+
+/**
+ * Finds where to remove text for an unused directive.
+ * @param {Directive} directive Unused directive to be removed.
+ * @param {SourceCode} sourceCode A SourceCode object for the given text
+ * @returns {Range} Removal range for the unused directive.
+ * @see https://github.com/eslint/rfcs/pull/78
+ */
+function getDirectiveRemovalRange(directive, sourceCode) {
+    const lineIndex = directive.unprocessedDirective.line - 1;
+    const lineStart = sourceCode.lineStartIndices[lineIndex];
+    const commentStart = lineStart + directive.unprocessedDirective.column - 1;
+    const comment = sourceCode.getTokenByRangeStart(commentStart, { includeComments: true });
+    const line = sourceCode.lines[lineIndex];
+
+    // If the directive's rule isn't the only one in the comment, only remove that rule
+    if (comment.value.includes(",")) {
+        const ruleStart = comment.value.indexOf(directive.ruleId);
+        const ruleEnd = ruleStart + directive.ruleId.length;
+
+        if (comment.value.trimRight().endsWith(directive.ruleId)) {
+            for (const prefix of [", ", ","]) {
+                if (comment.value.slice(ruleStart - prefix.length, ruleStart) === prefix) {
+                    return [commentStart + 2 + ruleStart - prefix.length, commentStart + ruleEnd + 2];
+                }
+            }
+        } else {
+            for (const suffix of [", ", ","]) {
+                if (comment.value.slice(ruleEnd, ruleEnd + suffix.length) === suffix) {
+                    return [commentStart + 2 + ruleStart, commentStart + 2 + ruleEnd + suffix.length];
+                }
+            }
+        }
+
+        return [commentStart + 2 + ruleStart, commentStart + 2 + ruleEnd];
+    }
+
+    // If the comment is alone on its line, remove the entire line
+    if (commentIsAloneOnLine(comment, line)) {
+        return [
+            lineStart,
+            lineIndex === sourceCode.lines.length - 1
+                ? lineStart + line.length
+                : sourceCode.lineStartIndices[lineIndex + 1]
+        ];
+    }
+
+    // If the comment has space between it and whatever else is on its line, collapse the space
+    if (commentStart !== 0 && ["\n", " "].includes(sourceCode.text[commentStart - 1])) {
+        if (comment.range[1] === sourceCode.text.length) {
+            return [comment.range[0] - 1, comment.range[1]];
+        }
+
+        if (sourceCode.text[comment.range[1]] === " ") {
+            return [comment.range[0], comment.range[1] + 1];
+        }
+    }
+
+    return comment.range;
+}
+
 /**
  * This is the same as the exported function, except that it
  * doesn't handle disable-line and disable-next-line directives, and it always reports unused
@@ -87,6 +157,10 @@ function applyDirectives(options) {
     const unusedDisableDirectives = options.directives
         .filter(directive => directive.type === "disable" && !usedDisableDirectives.has(directive))
         .map(directive => ({
+            fix: {
+                range: getDirectiveRemovalRange(directive, options.sourceCode),
+                text: ""
+            },
             ruleId: null,
             message: directive.ruleId
                 ? `Unused eslint-disable directive (no problems were reported from '${directive.ruleId}').`
@@ -115,10 +189,11 @@ function applyDirectives(options) {
  * @param {{ruleId: (string|null), line: number, column: number}[]} options.problems
  * A list of problems reported by rules, sorted by increasing location in the file, with one-based columns.
  * @param {"off" | "warn" | "error"} options.reportUnusedDisableDirectives If `"warn"` or `"error"`, adds additional problems for unused directives
+ * @param {SourceCode} options.sourceCode A SourceCode object for the given text
  * @returns {{ruleId: (string|null), line: number, column: number}[]}
  * A list of reported problems that were not disabled by the directive comments.
  */
-module.exports = ({ directives, problems, reportUnusedDisableDirectives = "off" }) => {
+module.exports = ({ directives, problems, reportUnusedDisableDirectives = "off", sourceCode }) => {
     const blockDirectives = directives
         .filter(directive => directive.type === "disable" || directive.type === "enable")
         .map(directive => Object.assign({}, directive, { unprocessedDirective: directive }))
@@ -150,12 +225,14 @@ module.exports = ({ directives, problems, reportUnusedDisableDirectives = "off"
     const blockDirectivesResult = applyDirectives({
         problems,
         directives: blockDirectives,
-        reportUnusedDisableDirectives
+        reportUnusedDisableDirectives,
+        sourceCode
     });
     const lineDirectivesResult = applyDirectives({
         problems: blockDirectivesResult.problems,
         directives: lineDirectives,
-        reportUnusedDisableDirectives
+        reportUnusedDisableDirectives,
+        sourceCode
     });
 
     return reportUnusedDisableDirectives !== "off"

lib/linter/linter.js

@@ -1204,7 +1204,8 @@ class Linter {
             problems: lintingProblems
                 .concat(commentDirectives.problems)
                 .sort((problemA, problemB) => problemA.line - problemB.line || problemA.column - problemB.column),
-            reportUnusedDisableDirectives: options.reportUnusedDisableDirectives
+            reportUnusedDisableDirectives: options.reportUnusedDisableDirectives,
+            sourceCode
         });
     }
 

lib/options.js

@@ -32,7 +32,7 @@ const optionator = require("optionator");
  * @property {string[]} [ext] Specify JavaScript file extensions
  * @property {boolean} fix Automatically fix problems
  * @property {boolean} fixDryRun Automatically fix problems without saving the changes to the file system
- * @property {("problem" | "suggestion" | "layout")[]} [fixType] Specify the types of fixes to apply (problem, suggestion, layout)
+ * @property {("directive" | "problem" | "suggestion" | "layout")[]} [fixType] Specify the types of fixes to apply (directive, problem, suggestion, layout)
  * @property {string} format Use a specific output format
  * @property {string[]} [global] Define global variables
  * @property {boolean} [help] Show help

lib/shared/types.js

@@ -125,7 +125,7 @@ module.exports = {};
  * @property {Record<string,string>} [messages] The messages the rule reports.
  * @property {string[]} [replacedBy] The IDs of the alternative rules.
  * @property {Array|Object} schema The option schema of the rule.
- * @property {"problem"|"suggestion"|"layout"} type The rule type.
+ * @property {"directive"|"problem"|"suggestion"|"layout"} type The rule type.
  */
 
 /**

tests/lib/cli-engine/cli-engine.js

@@ -5071,20 +5071,24 @@ describe("CLIEngine", () => {
                                     message: "Unused eslint-disable directive (no problems were reported).",
                                     line: 1,
                                     column: 1,
+                                    fix: {
+                                        range: [0, 20],
+                                        text: ""
+                                    },
                                     severity: 2,
                                     nodeType: null
                                 }
                             ],
                             errorCount: 1,
                             warningCount: 0,
-                            fixableErrorCount: 0,
+                            fixableErrorCount: 1,
                             fixableWarningCount: 0,
                             source: "/* eslint-disable */"
                         }
                     ],
                     errorCount: 1,
                     warningCount: 0,
-                    fixableErrorCount: 0,
+                    fixableErrorCount: 1,
                     fixableWarningCount: 0,
                     usedDeprecatedRules: []
                 }

tests/lib/eslint/eslint.js

@@ -198,7 +198,7 @@ describe("ESLint", () => {
                     "- 'errorOnUnmatchedPattern' must be a boolean.",
                     "- 'extensions' must be an array of non-empty strings or null.",
                     "- 'fix' must be a boolean or a function.",
-                    "- 'fixTypes' must be an array of any of \"problem\", \"suggestion\", and \"layout\".",
+                    "- 'fixTypes' must be an array of any of \"directive\", \"problem\", \"suggestion\", and \"layout\".",
                     "- 'globInputPaths' must be a boolean.",
                     "- 'ignore' must be a boolean.",
                     "- 'ignorePath' must be a non-empty string or null.",
@@ -441,7 +441,7 @@ describe("ESLint", () => {
                         fix: true,
                         fixTypes: ["layou"]
                     });
-                }, /'fixTypes' must be an array of any of "problem", "suggestion", and "layout"\./iu);
+                }, /'fixTypes' must be an array of any of "directive", "problem", "suggestion", and "layout"\./iu);
             });
 
             it("should not fix any rules when fixTypes is used without fix", async () => {
@@ -4925,13 +4925,17 @@ describe("ESLint", () => {
                                 message: "Unused eslint-disable directive (no problems were reported).",
                                 line: 1,
                                 column: 1,
+                                fix: {
+                                    range: [0, 20],
+                                    text: ""
+                                },
                                 severity: 2,
                                 nodeType: null
                             }
                         ],
                         errorCount: 1,
                         warningCount: 0,
-                        fixableErrorCount: 0,
+                        fixableErrorCount: 1,
                         fixableWarningCount: 0,
                         source: "/* eslint-disable */",
                         usedDeprecatedRules: []