Skip to content
Permalink
Browse files
Breaking: Fixable disable directives (fixes #11815) (#14617)
* New: Fixable disable directives

* Part of the way there: not ready yet

* Generally fixed up, and just started on unit tests

* lodash.flatMap

* Merge branch 'master'

* Progress...

* Fixed the remaining pre-existing unit tests

* Fixed up some tests

* unprocessedDirective.parentComment

* Avoided rescan by passing commentToken in parentComment

* A lil edge case

* Fix directive grouping the other direction

* Incorrect state directive reference

* Simplified parentComment / commentToken range

* Use Map in lib/linter/apply-disable-directives.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* Used suggestion for individual directives removal

* Boolean in lib/cli-engine/cli-engine.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* Docs fixups

* Added tests

* Tests and fix for multiple rule lists

* Remove unused sourceCode from tests

* Added high-level test in linter.js

* Apply suggestions from code review

Co-authored-by: Brandon Mills <btmills@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* Added require

* Wrong order...

* Also mention in command-line-interface.md

* Respect disableFixes (not yet tested)

* Fixed up existing tests

* Add unit test for options.disableFixes

* Apply suggestions from code review

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Brandon Mills <btmills@users.noreply.github.com>
  • Loading branch information
3 people committed Aug 5, 2021
1 parent 4a7aab7 commit 1d2213deb69c5901c1950bbe648aa819e7e742ed
Show file tree
Hide file tree
Showing 11 changed files with 1,210 additions and 150 deletions.
@@ -148,7 +148,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
@@ -47,7 +47,7 @@ Specifying rules and plugins:
Fixing problems:
--fix Automatically fix problems
--fix-dry-run Automatically fix problems without saving the changes to the file system
--fix-type Array Specify the types of fixes to apply (problem, suggestion, layout)
--fix-type Array Specify the types of fixes to apply (directive, problem, suggestion, layout)
Ignoring files:
--ignore-path path::String Specify path of ignore file
@@ -240,11 +240,12 @@ This flag can be useful for integrations (e.g. editor plugins) which need to aut

#### `--fix-type`

This option allows you to specify the type of fixes to apply when using either `--fix` or `--fix-dry-run`. The three types of fixes are:
This option allows you to specify the type of fixes to apply when using either `--fix` or `--fix-dry-run`. The four types of fixes are:

1. `problem` - fix potential errors in the code
1. `suggestion` - apply fixes to the code that improve it
1. `layout` - apply fixes that do not change the program structure (AST)
1. `directive` - apply fixes to inline directives such as `// eslint-disable`

You can specify one or more fix type on the command line. Here are some examples:

@@ -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
@@ -331,6 +331,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 Boolean(rule && rule.meta && fixTypes.has(rule.meta.type));
}

/**
* Collect used deprecated rules.
* @param {ConfigArray[]} usedConfigArrays The config arrays which were used.
@@ -623,12 +640,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);
}
}

@@ -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.");
@@ -5,6 +5,8 @@

"use strict";

const escapeRegExp = require("escape-string-regexp");

/**
* Compares the locations of two objects in a source file
* @param {{line: number, column: number}} itemA The first object
@@ -16,6 +18,123 @@ function compareLocations(itemA, itemB) {
return itemA.line - itemB.line || itemA.column - itemB.column;
}

/**
* Groups a set of directives into sub-arrays by their parent comment.
* @param {Directive[]} directives Unused directives to be removed.
* @returns {Directive[][]} Directives grouped by their parent comment.
*/
function groupByParentComment(directives) {
const groups = new Map();

for (const directive of directives) {
const { unprocessedDirective: { parentComment } } = directive;

if (groups.has(parentComment)) {
groups.get(parentComment).push(directive);
} else {
groups.set(parentComment, [directive]);
}
}

return [...groups.values()];
}

/**
* Creates removal details for a set of directives within the same comment.
* @param {Directive[]} directives Unused directives to be removed.
* @param {Token} commentToken The backing Comment token.
* @returns {{ description, fix, position }[]} Details for later creation of output Problems.
*/
function createIndividualDirectivesRemoval(directives, commentToken) {
const listOffset = /^\s*\S+\s+/u.exec(commentToken.value)[0].length;
const listText = commentToken.value
.slice(listOffset) // remove eslint-*
.split(/\s-{2,}\s/u)[0] // remove -- directive comment
.trimRight();
const listStart = commentToken.range[0] + 2 + listOffset;

return directives.map(directive => {
const { ruleId } = directive;
const match = new RegExp(String.raw`(?:^|,)\s*${escapeRegExp(ruleId)}\s*(?:$|,)`, "u").exec(listText);
const ruleOffset = match.index;
const ruleEndOffset = ruleOffset + match[0].length;
const ruleText = listText.slice(ruleOffset, ruleEndOffset);

return {
description: `'${ruleId}'`,
fix: {
range: [
listStart + ruleOffset + (ruleText.startsWith(",") && ruleText.endsWith(",") ? 1 : 0),
listStart + ruleEndOffset
],
text: ""
},
position: directive.unprocessedDirective
};
});
}

/**
* Creates a description of deleting an entire unused disable comment.
* @param {Directive[]} directives Unused directives to be removed.
* @param {Token} commentToken The backing Comment token.
* @returns {{ description, fix, position }} Details for later creation of an output Problem.
*/
function createCommentRemoval(directives, commentToken) {
const { range } = commentToken;
const ruleIds = directives.filter(directive => directive.ruleId).map(directive => `'${directive.ruleId}'`);

return {
description: ruleIds.length <= 2
? ruleIds.join(" or ")
: `${ruleIds.slice(0, ruleIds.length - 1).join(", ")}, or ${ruleIds[ruleIds.length - 1]}`,
fix: {
range,
text: " "
},
position: directives[0].unprocessedDirective
};
}

/**
* Returns a new array formed by applying a given callback function to each element of the array, and then flattening the result by one level.
* TODO(stephenwade): Replace this with array.flatMap when we drop support for Node v10
* @param {any[]} array The array to process
* @param {Function} fn The function to use
* @returns {any[]} The result array
*/
function flatMap(array, fn) {
const mapped = array.map(fn);
const flattened = [].concat(...mapped);

return flattened;
}

/**
* Parses details from directives to create output Problems.
* @param {Directive[]} allDirectives Unused directives to be removed.
* @returns {{ description, fix, position }[]} Details for later creation of output Problems.
*/
function processUnusedDisableDirectives(allDirectives) {
const directiveGroups = groupByParentComment(allDirectives);

return flatMap(
directiveGroups,
directives => {
const { parentComment } = directives[0].unprocessedDirective;
const remainingRuleIds = new Set(parentComment.ruleIds);

for (const directive of directives) {
remainingRuleIds.delete(directive.ruleId);
}

return remainingRuleIds.size
? createIndividualDirectivesRemoval(directives, parentComment.commentToken)
: [createCommentRemoval(directives, parentComment.commentToken)];
}
);
}

/**
* 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
@@ -82,17 +201,22 @@ function applyDirectives(options) {
}
}

const unusedDisableDirectives = options.directives
.filter(directive => directive.type === "disable" && !usedDisableDirectives.has(directive))
.map(directive => ({
const unusedDisableDirectivesToReport = options.directives
.filter(directive => directive.type === "disable" && !usedDisableDirectives.has(directive));

const processed = processUnusedDisableDirectives(unusedDisableDirectivesToReport);

const unusedDisableDirectives = processed
.map(({ description, fix, position }) => ({
ruleId: null,
message: directive.ruleId
? `Unused eslint-disable directive (no problems were reported from '${directive.ruleId}').`
message: description
? `Unused eslint-disable directive (no problems were reported from ${description}).`
: "Unused eslint-disable directive (no problems were reported).",
line: directive.unprocessedDirective.line,
column: directive.unprocessedDirective.column,
line: position.line,
column: position.column,
severity: options.reportUnusedDisableDirectives === "warn" ? 1 : 2,
nodeType: null
nodeType: null,
...options.disableFixes ? {} : { fix }
}));

return { problems, unusedDisableDirectives };
@@ -113,29 +237,16 @@ 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 {boolean} options.disableFixes If true, it doesn't make `fix` properties.
* @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, disableFixes, problems, reportUnusedDisableDirectives = "off" }) => {
const blockDirectives = directives
.filter(directive => directive.type === "disable" || directive.type === "enable")
.map(directive => Object.assign({}, directive, { unprocessedDirective: directive }))
.sort(compareLocations);

/**
* Returns a new array formed by applying a given callback function to each element of the array, and then flattening the result by one level.
* TODO(stephenwade): Replace this with array.flatMap when we drop support for Node v10
* @param {any[]} array The array to process
* @param {Function} fn The function to use
* @returns {any[]} The result array
*/
function flatMap(array, fn) {
const mapped = array.map(fn);
const flattened = [].concat(...mapped);

return flattened;
}

const lineDirectives = flatMap(directives, directive => {
switch (directive.type) {
case "disable":
@@ -162,11 +273,13 @@ module.exports = ({ directives, problems, reportUnusedDisableDirectives = "off"
const blockDirectivesResult = applyDirectives({
problems,
directives: blockDirectives,
disableFixes,
reportUnusedDisableDirectives
});
const lineDirectivesResult = applyDirectives({
problems: blockDirectivesResult.problems,
directives: lineDirectives,
disableFixes,
reportUnusedDisableDirectives
});

@@ -242,28 +242,30 @@ function createLintingProblem(options) {
* Creates a collection of disable directives from a comment
* @param {Object} options to create disable directives
* @param {("disable"|"enable"|"disable-line"|"disable-next-line")} options.type The type of directive comment
* @param {{line: number, column: number}} options.loc The 0-based location of the comment token
* @param {token} options.commentToken The Comment token
* @param {string} options.value The value after the directive in the comment
* comment specified no specific rules, so it applies to all rules (e.g. `eslint-disable`)
* @param {function(string): {create: Function}} options.ruleMapper A map from rule IDs to defined rules
* @returns {Object} Directives and problems from the comment
*/
function createDisableDirectives(options) {
const { type, loc, value, ruleMapper } = options;
const { commentToken, type, value, ruleMapper } = options;
const ruleIds = Object.keys(commentParser.parseListConfig(value));
const directiveRules = ruleIds.length ? ruleIds : [null];
const result = {
directives: [], // valid disable directives
directiveProblems: [] // problems in directives
};

const parentComment = { commentToken, ruleIds };

for (const ruleId of directiveRules) {

// push to directives, if the rule is defined(including null, e.g. /*eslint enable*/)
if (ruleId === null || ruleMapper(ruleId) !== null) {
result.directives.push({ type, line: loc.start.line, column: loc.start.column + 1, ruleId });
result.directives.push({ parentComment, type, line: commentToken.loc.start.line, column: commentToken.loc.start.column + 1, ruleId });
} else {
result.directiveProblems.push(createLintingProblem({ ruleId, loc }));
result.directiveProblems.push(createLintingProblem({ ruleId, loc: commentToken.loc }));
}
}
return result;
@@ -340,7 +342,7 @@ function getDirectiveComments(filename, ast, ruleMapper, warnInlineConfig) {
case "eslint-disable-next-line":
case "eslint-disable-line": {
const directiveType = directiveText.slice("eslint-".length);
const options = { type: directiveType, loc: comment.loc, value: directiveValue, ruleMapper };
const options = { commentToken: comment, type: directiveType, value: directiveValue, ruleMapper };
const { directives, directiveProblems } = createDisableDirectives(options);

disableDirectives.push(...directives);
@@ -1216,6 +1218,7 @@ class Linter {

return applyDisableDirectives({
directives: commentDirectives.disableDirectives,
disableFixes: options.disableFixes,
problems: lintingProblems
.concat(commentDirectives.problems)
.sort((problemA, problemB) => problemA.line - problemB.line || problemA.column - problemB.column),
@@ -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
@@ -151,7 +151,7 @@ module.exports = optionator({
{
option: "fix-type",
type: "Array",
description: "Specify the types of fixes to apply (problem, suggestion, layout)"
description: "Specify the types of fixes to apply (directive, problem, suggestion, layout)"
},
{
heading: "Ignoring files"

0 comments on commit 1d2213d

Please sign in to comment.