Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: report-unused-disable-directive to report unused eslint-enable #17611

Merged
merged 20 commits into from Oct 17, 2023
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/src/integrate/nodejs-api.md
Expand Up @@ -152,7 +152,7 @@ The `ESLint` constructor takes an `options` object. If you omit the `options` ob
* `options.plugins` (`Record<string, Plugin> | null`)<br>
Default is `null`. The plugin implementations that ESLint uses for the `plugins` setting of your configuration. This is a map-like object. Those keys are plugin IDs and each value is implementation.
* `options.reportUnusedDisableDirectives` (`"error" | "warn" | "off" | null`)<br>
Default is `null`. The severity to report unused eslint-disable directives. If this option is a severity, it overrides the `reportUnusedDisableDirectives` setting in your configurations.
Default is `null`. The severity to report unused eslint-disable and eslint-enable directives. If this option is a severity, it overrides the `reportUnusedDisableDirectives` setting in your configurations.
* `options.resolvePluginsRelativeTo` (`string` | `null`)<br>
Default is `null`. The path to a directory where plugins should be resolved from. If `null` is present, ESLint loads plugins from the location of the configuration file that contains the plugin setting. If a path is present, ESLint loads all plugins from there.
* `options.rulePaths` (`string[]`)<br>
Expand Down Expand Up @@ -537,7 +537,7 @@ The most important method on `Linter` is `verify()`, which initiates linting of
* `filterCodeBlock` - (optional) A function that decides which code blocks the linter should adopt. The function receives two arguments. The first argument is the virtual filename of a code block. The second argument is the text of the code block. If the function returned `true` then the linter adopts the code block. If the function was omitted, the linter adopts only `*.js` code blocks. If you provided a `filterCodeBlock` function, it overrides this default behavior, so the linter doesn't adopt `*.js` code blocks automatically.
* `disableFixes` - (optional) when set to `true`, the linter doesn't make either the `fix` or `suggestions` property of the lint result.
* `allowInlineConfig` - (optional) set to `false` to disable inline comments from changing ESLint rules.
* `reportUnusedDisableDirectives` - (optional) when set to `true`, adds reported errors for unused `eslint-disable` directives when no problems would be reported in the disabled area anyway.
* `reportUnusedDisableDirectives` - (optional) when set to `true`, adds reported errors for unused `eslint-disable` and `eslint-enable` directives when no problems would be reported in the disabled area anyway.

If the third argument is a string, it is interpreted as the `filename`.

Expand Down
4 changes: 2 additions & 2 deletions docs/src/use/command-line-interface.md
Expand Up @@ -97,7 +97,7 @@ Output:

Inline configuration comments:
--no-inline-config Prevent comments from changing config or rules
--report-unused-disable-directives Adds reported errors for unused eslint-disable directives
--report-unused-disable-directives Adds reported errors for unused eslint-disable and eslint-enable directives
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved

Caching:
--cache Only check changed files - default: false
Expand Down Expand Up @@ -577,7 +577,7 @@ This option causes ESLint to report directive comments like `// eslint-disable-l

* **Argument Type**: No argument.

This can be useful to prevent future errors from unexpectedly being suppressed, by cleaning up old `eslint-disable` comments which are no longer applicable.
This can be useful to prevent future errors from unexpectedly being suppressed, by cleaning up old `eslint-disable` and `eslint-enable` comments which are no longer applicable.

::: warning
When using this option, it is possible that new errors start being reported whenever ESLint or custom rules are upgraded.
Expand Down
6 changes: 3 additions & 3 deletions docs/src/use/configure/configuration-files-new.md
Expand Up @@ -76,7 +76,7 @@ Each configuration object contains all of the information ESLint needs to execut
* `parserOptions` - An object specifying additional options that are passed directly to the `parse()` or `parseForESLint()` method on the parser. The available options are parser-dependent.
* `linterOptions` - An object containing settings related to the linting process.
* `noInlineConfig` - A Boolean value indicating if inline configuration is allowed.
* `reportUnusedDisableDirectives` - A Boolean value indicating if unused disable directives should be tracked and reported.
* `reportUnusedDisableDirectives` - A Boolean value indicating if unused disable and enable directives should be tracked and reported.
* `processor` - Either an object containing `preprocess()` and `postprocess()` methods or a string indicating the name of a processor inside of a plugin (i.e., `"pluginName/processorName"`).
* `plugins` - An object containing a name-value mapping of plugin names to plugin objects. When `files` is specified, these plugins are only available to the matching files.
* `rules` - An object containing the configured rules. When `files` or `ignores` are specified, these rule configurations are only available to the matching files.
Expand Down Expand Up @@ -244,7 +244,7 @@ export default [

#### Reporting unused disable directives

Disable directives such as `/*eslint-disable*/` and `/*eslint-disable-next-line*/` are used to disable ESLint rules around certain portions of code. As code changes, it's possible for these directives to no longer be needed because the code has changed in such a way that the rule is no longer triggered. You can enable reporting of these unused disable directives by setting the `reportUnusedDisableDirectives` option to `true`, as in this example:
Disable and enable directives such as `/*eslint-disable*/`, `/*eslint-enable*/` and `/*eslint-disable-next-line*/` are used to disable ESLint rules around certain portions of code. As code changes, it's possible for these directives to no longer be needed because the code has changed in such a way that the rule is no longer triggered. You can enable reporting of these unused disable directives by setting the `reportUnusedDisableDirectives` option to `true`, as in this example:

```js
export default [
Expand All @@ -257,7 +257,7 @@ export default [
];
```

By default, unused disable directives are reported as warnings. You can change this setting using the `--report-unused-disable-directives` command line option.
By default, unused disable and enable directives are reported as warnings. You can change this setting using the `--report-unused-disable-directives` command line option.

### Configuring language options

Expand Down
123 changes: 112 additions & 11 deletions lib/linter/apply-disable-directives.js
Expand Up @@ -30,7 +30,7 @@ function compareLocations(itemA, itemB) {

/**
* Groups a set of directives into sub-arrays by their parent comment.
* @param {Directive[]} directives Unused directives to be removed.
* @param {Iterable<Directive>} directives Unused directives to be removed.
* @returns {Directive[][]} Directives grouped by their parent comment.
*/
function groupByParentComment(directives) {
Expand Down Expand Up @@ -177,10 +177,10 @@ function createCommentRemoval(directives, commentToken) {

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

return directiveGroups.flatMap(
Expand All @@ -199,6 +199,79 @@ function processUnusedDisableDirectives(allDirectives) {
);
}

/**
* Collect eslint-enable comments that are used after removing the suppression by eslint-disable comments.
ota-meshi marked this conversation as resolved.
Show resolved Hide resolved
* @param {Directive[]} directives The directives to check.
* @returns {Iterable<Directive>} The used eslint-enable comments
*/
function *collectUsedEnableDirectives(directives) {
const withoutRuleIdKey = Symbol("withoutRuleId");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to read this a few times to understand what withoutRuleIdKey meant. Is there a reason we can't just use null to indicate a directive without a rule ID?

If not, could we rename this to emptyRuleId?

const enabledRules = new Map();

for (let index = directives.length - 1; index >= 0; index--) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment here explaining why we're going through the array backwards?

const directive = directives[index];

if (directive.type === "disable") {
if (enabledRules.size === 0) {
continue;
}
if (directive.ruleId === null) {

// If encounter `eslint-disable` without ruleId,
// mark all `eslint-enable` currently held in enabledRules as used.
// e.g.
// /* eslint-disable */ <- current directive
// /* eslint-enable rule-id1 */ <- used
// /* eslint-enable rule-id2 */ <- used
// /* eslint-enable */ <- used
for (const enableDirective of enabledRules.values()) {
yield enableDirective;
}
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
enabledRules.clear();
} else {
const enableDirective = enabledRules.get(directive.ruleId);

if (enableDirective) {

// If encounter `eslint-disable` with ruleId,
// mark it as used if there is an `eslint-enable` with the same ruleId in enabledRules.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to match up with the code. If "it" refers to the disable directive, I don't see where it's being marked as used. Is usedEnableDirectives also used to track disable directives?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought "it" was referring to "an eslint-enable with the same ruleId" 😓. I'll change that comment.

// e.g.
// /* eslint-disable rule-id */ <- current directive
// /* eslint-enable rule-id */ <- used
yield enableDirective;
enabledRules.delete(directive.ruleId);
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
} else {
const enabledDirectiveWithoutRuleId = enabledRules.get(withoutRuleIdKey);

if (enabledDirectiveWithoutRuleId) {

// If encounter `eslint-disable` with ruleId, and there is no `eslint-enable` with the same ruleId in enabledRules,
// mark `eslint-enable` without ruleId as used.
// e.g.
// /* eslint-disable rule-id */ <- current directive
// /* eslint-enable */ <- used
yield enabledDirectiveWithoutRuleId;
}
}
}
} else if (directive.type === "enable") {
if (directive.ruleId === null) {

// If encounter `eslint-enable` without ruleId, the `eslint-enable` that follows it are unused.
// So clear enabledRules.
// e.g.
// /* eslint-enable */ <- current directive
// /* eslint-enable rule-id *// <- unused
// /* eslint-enable */ <- unused
enabledRules.clear();
enabledRules.set(withoutRuleIdKey, directive);
} else {
enabledRules.set(directive.ruleId, directive);
}
}
}
}

/**
* 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
Expand Down Expand Up @@ -258,17 +331,45 @@ function applyDirectives(options) {
const unusedDisableDirectivesToReport = options.directives
.filter(directive => directive.type === "disable" && !usedDisableDirectives.has(directive));

const processed = processUnusedDisableDirectives(unusedDisableDirectivesToReport);

const unusedDisableDirectives = processed
const unusedEnableDirectivesToReport = new Set(
options.directives.filter(directive => directive.unprocessedDirective.type === "enable")
);

if (

/*
* If directives has the eslint-enable directive,
* check whether the eslint-enable comment is used.
*/
ota-meshi marked this conversation as resolved.
Show resolved Hide resolved
unusedEnableDirectivesToReport.size > 0
) {
for (const directive of collectUsedEnableDirectives(options.directives)) {
unusedEnableDirectivesToReport.delete(directive);
}
}

const processed = processUnusedDirectives(unusedDisableDirectivesToReport)
.concat(processUnusedDirectives(unusedEnableDirectivesToReport));

const unusedDirectives = processed
.map(({ description, fix, unprocessedDirective }) => {
const { parentComment, type, line, column } = unprocessedDirective;

let message;

if (type === "enable") {
message = description
? `Unused eslint-enable directive (no suppressed for ${description}).`
: "Unused eslint-enable directive (no suppressed).";
ota-meshi marked this conversation as resolved.
Show resolved Hide resolved
} else {
message = description
? `Unused eslint-disable directive (no problems were reported from ${description}).`
: "Unused eslint-disable directive (no problems were reported).";
}
return {
ruleId: null,
message: description
? `Unused eslint-disable directive (no problems were reported from ${description}).`
: "Unused eslint-disable directive (no problems were reported).",
message,
line: type === "disable-next-line" ? parentComment.commentToken.loc.start.line : line,
column: type === "disable-next-line" ? parentComment.commentToken.loc.start.column + 1 : column,
severity: options.reportUnusedDisableDirectives === "warn" ? 1 : 2,
Expand All @@ -277,7 +378,7 @@ function applyDirectives(options) {
};
});

return { problems, unusedDisableDirectives };
return { problems, unusedDirectives };
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down Expand Up @@ -344,8 +445,8 @@ module.exports = ({ directives, disableFixes, problems, reportUnusedDisableDirec

return reportUnusedDisableDirectives !== "off"
? lineDirectivesResult.problems
.concat(blockDirectivesResult.unusedDisableDirectives)
.concat(lineDirectivesResult.unusedDisableDirectives)
.concat(blockDirectivesResult.unusedDirectives)
.concat(lineDirectivesResult.unusedDirectives)
.sort(compareLocations)
: lineDirectivesResult.problems;
};