Skip to content

Commit 0dd9704

Browse files
bmishmdjermanovic
bmish
and
mdjermanovic
authored
feat: Support custom severity when reporting unused disable directives (#17212)
* feat: Support custom severity when reporting unused disable directives * fix test * remove change to conf/default-cli-options.js * improve schema * wip * remove reportUnusedDisableDirectives from processOptions() in favor of overrideConfig * revert changes to lib/eslint/eslint.js and fix more tests * remove old type * more tests * fix comment for ParsedCLIOptions * remove unnecessary exception * revert comment * remove number severity * fix translateOptions and tests * revert comment * Update lib/cli.js Co-authored-by: Milos Djermanovic <[email protected]> * Update lib/options.js Co-authored-by: Milos Djermanovic <[email protected]> * Update docs/src/use/command-line-interface.md Co-authored-by: Milos Djermanovic <[email protected]> * Update lib/cli-engine/cli-engine.js Co-authored-by: Milos Djermanovic <[email protected]> * Update lib/cli-engine/cli-engine.js Co-authored-by: Milos Djermanovic <[email protected]> * Update lib/shared/types.js Co-authored-by: Milos Djermanovic <[email protected]> * Update lib/shared/types.js Co-authored-by: Milos Djermanovic <[email protected]> * add arg type to doc * mention enable * warn with old option * remove old comment * revert change to tests/lib/eslint/eslint.js * add test for removed option * revert copy change * switch several tests from boolean for reportUnusedDisableDirectives to string * improve tests * use reportUnusedDisableDirectives: error in eslint-config-eslint * support severity numbers * refactor * revert newline change * refactor and fix tests * refactor * cleanup * cleanup * use normalizeSeverityToString in another place * tweak * attempt to avoid test pollution * tweak test name * remove boolean handling from normalizeSeverityToString * Update docs/src/use/command-line-interface.md Co-authored-by: Milos Djermanovic <[email protected]> * Update docs/src/use/command-line-interface.md Co-authored-by: Milos Djermanovic <[email protected]> * Update docs/src/use/command-line-interface.md Co-authored-by: Milos Djermanovic <[email protected]> * cleanup * normalize flat config reportUnusedDisableDirectives to number * Update lib/options.js Co-authored-by: Milos Djermanovic <[email protected]> * Update lib/cli.js Co-authored-by: Milos Djermanovic <[email protected]> * Update lib/cli.js Co-authored-by: Milos Djermanovic <[email protected]> * remove unused reportUnusedDisableDirectives property * run CLI tests for both flat and eslintrc * Update tests/lib/eslint/flat-eslint.js Co-authored-by: Milos Djermanovic <[email protected]> * Update tests/lib/eslint/flat-eslint.js Co-authored-by: Milos Djermanovic <[email protected]> * Update docs/src/use/configure/rules.md Co-authored-by: Milos Djermanovic <[email protected]> * address pr feedback --------- Co-authored-by: Milos Djermanovic <[email protected]>
1 parent 31a7e3f commit 0dd9704

18 files changed

Lines changed: 509 additions & 67 deletions

File tree

‎Makefile.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ const NODE = "node ", // intentional extra space
6868

6969
// Utilities - intentional extra space at the end of each string
7070
MOCHA = `${NODE_MODULES}mocha/bin/_mocha `,
71-
ESLINT = `${NODE} bin/eslint.js --report-unused-disable-directives `,
71+
ESLINT = `${NODE} bin/eslint.js `,
7272

7373
// Files
7474
RULE_FILES = glob.sync("lib/rules/*.js").filter(filePath => path.basename(filePath) !== "index.js"),

‎docs/src/use/command-line-interface.md‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ Output:
9898
Inline configuration comments:
9999
--no-inline-config Prevent comments from changing config or rules
100100
--report-unused-disable-directives Adds reported errors for unused eslint-disable and eslint-enable directives
101+
--report-unused-disable-directives-severity String Chooses severity level for reporting unused eslint-disable and eslint-enable directives - either: off, warn, error, 0, 1, or 2
101102
102103
Caching:
103104
--cache Only check changed files - default: false
@@ -582,7 +583,7 @@ This can be useful to prevent future errors from unexpectedly being suppressed,
582583
::: warning
583584
When using this option, it is possible that new errors start being reported whenever ESLint or custom rules are upgraded.
584585

585-
For example, suppose a rule has a bug that causes it to report a false positive, and an `eslint-disable` comment is added to suppress the incorrect report. If the bug is then fixed in a patch release of ESLint, the `eslint-disable` comment becomes unused since ESLint is no longer generating an incorrect report. This results in a new reported error for the unused directive if the `report-unused-disable-directives` option is used.
586+
For example, suppose a rule has a bug that causes it to report a false positive, and an `eslint-disable` comment is added to suppress the incorrect report. If the bug is then fixed in a patch release of ESLint, the `eslint-disable` comment becomes unused since ESLint is no longer generating an incorrect report. This results in a new reported error for the unused directive if the `--report-unused-disable-directives` option is used.
586587
:::
587588

588589
##### `--report-unused-disable-directives` example
@@ -591,6 +592,23 @@ For example, suppose a rule has a bug that causes it to report a false positive,
591592
npx eslint --report-unused-disable-directives file.js
592593
```
593594

595+
#### `--report-unused-disable-directives-severity`
596+
597+
Same as [`--report-unused-disable-directives`](#--report-unused-disable-directives), but allows you to specify the severity level (`error`, `warn`, `off`) of the reported errors. Only one of these two options can be used at a time.
598+
599+
* **Argument Type**: String. One of the following values:
600+
1. `off` (or `0`)
601+
1. `warn` (or `1`)
602+
1. `error` (or `2`)
603+
* **Multiple Arguments**: No
604+
* **Default Value**: By default, `linterOptions.reportUnusedDisableDirectives` configuration setting is used.
605+
606+
##### `--report-unused-disable-directives-severity` example
607+
608+
```shell
609+
npx eslint --report-unused-disable-directives-severity warn file.js
610+
```
611+
594612
### Caching
595613

596614
#### `--cache`

‎docs/src/use/configure/configuration-files-new.md‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ Each configuration object contains all of the information ESLint needs to execut
7676
* `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.
7777
* `linterOptions` - An object containing settings related to the linting process.
7878
* `noInlineConfig` - A Boolean value indicating if inline configuration is allowed.
79-
* `reportUnusedDisableDirectives` - A Boolean value indicating if unused disable and enable directives should be tracked and reported.
79+
* `reportUnusedDisableDirectives` - A severity string indicating if and how unused disable and enable directives should be tracked and reported. For legacy compatibility, `true` is equivalent to `"warn"` and `false` is equivalent to `"off"`. (default: `"off"`)
8080
* `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"`).
8181
* `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.
8282
* `rules` - An object containing the configured rules. When `files` or `ignores` are specified, these rule configurations are only available to the matching files.
@@ -244,20 +244,22 @@ export default [
244244

245245
#### Reporting unused disable directives
246246

247-
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:
247+
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 a severity string, as in this example:
248248

249249
```js
250250
export default [
251251
{
252252
files: ["**/*.js"],
253253
linterOptions: {
254-
reportUnusedDisableDirectives: true
254+
reportUnusedDisableDirectives: "error"
255255
}
256256
}
257257
];
258258
```
259259

260-
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.
260+
You can override this setting using the [`--report-unused-disable-directives`](../command-line-interface#--report-unused-disable-directives) or the [`--report-unused-disable-directives-severity`](../command-line-interface#--report-unused-disable-directives-severity) command line options.
261+
262+
For legacy compatibility, `true` is equivalent to `"warn"` and `false` is equivalent to `"off"`.
261263

262264
### Configuring language options
263265

‎docs/src/use/configure/migration-guide.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -575,7 +575,7 @@ export default [
575575
// ...other config
576576
linterOptions: {
577577
noInlineConfig: true,
578-
reportUnusedDisableDirectives: true
578+
reportUnusedDisableDirectives: "warn"
579579
}
580580
}
581581
];

‎docs/src/use/configure/rules.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -317,4 +317,4 @@ To report unused `eslint-disable` comments, use the `reportUnusedDisableDirectiv
317317
}
318318
```
319319

320-
This setting is similar to [--report-unused-disable-directives](../command-line-interface#--report-unused-disable-directives) CLI option, but doesn't fail linting (reports as `"warn"` severity).
320+
This setting is similar to [--report-unused-disable-directives](../command-line-interface#--report-unused-disable-directives) and [--report-unused-disable-directives-severity](../command-line-interface#--report-unused-disable-directives-severity) CLI options.

‎lib/cli-engine/cli-engine.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ const validFixTypes = new Set(["directive", "problem", "suggestion", "layout"]);
8383
* @property {string[]} [plugins] An array of plugins to load.
8484
* @property {Record<string,RuleConf>} [rules] An object of rules to use.
8585
* @property {string[]} [rulePaths] An array of directories to load custom rules from.
86-
* @property {boolean} [reportUnusedDisableDirectives] `true` adds reports for unused eslint-disable directives
86+
* @property {boolean|string} [reportUnusedDisableDirectives] `true`, `"error"` or '"warn"' adds reports for unused eslint-disable directives
8787
* @property {boolean} [globInputPaths] Set to false to skip glob resolution of input file paths to lint (default: true). If false, each input file paths is assumed to be a non-glob path to an existing file.
8888
* @property {string} [resolvePluginsRelativeTo] The folder where plugins should be resolved from, defaulting to the CWD
8989
*/
@@ -224,7 +224,7 @@ function calculateStatsPerRun(results) {
224224
* @param {ConfigArray} config.config The config.
225225
* @param {boolean} config.fix If `true` then it does fix.
226226
* @param {boolean} config.allowInlineConfig If `true` then it uses directive comments.
227-
* @param {boolean} config.reportUnusedDisableDirectives If `true` then it reports unused `eslint-disable` comments.
227+
* @param {boolean|string} config.reportUnusedDisableDirectives If `true`, `"error"` or '"warn"', then it reports unused `eslint-disable` comments.
228228
* @param {FileEnumerator} config.fileEnumerator The file enumerator to check if a path is a target or not.
229229
* @param {Linter} config.linter The linter instance to verify.
230230
* @returns {LintResult} The result of linting.

‎lib/cli.js‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@ const fs = require("fs"),
2222
{ FlatESLint, shouldUseFlatConfig } = require("./eslint/flat-eslint"),
2323
createCLIOptions = require("./options"),
2424
log = require("./shared/logging"),
25-
RuntimeInfo = require("./shared/runtime-info");
25+
RuntimeInfo = require("./shared/runtime-info"),
26+
{ normalizeSeverityToString } = require("./shared/severity");
2627
const { Legacy: { naming } } = require("@eslint/eslintrc");
2728
const { ModuleImporter } = require("@humanwhocodes/module-importer");
2829

@@ -89,6 +90,7 @@ async function translateOptions({
8990
plugin,
9091
quiet,
9192
reportUnusedDisableDirectives,
93+
reportUnusedDisableDirectivesSeverity,
9294
resolvePluginsRelativeTo,
9395
rule,
9496
rulesdir,
@@ -125,6 +127,14 @@ async function translateOptions({
125127
rules: rule ? rule : {}
126128
}];
127129

130+
if (reportUnusedDisableDirectives || reportUnusedDisableDirectivesSeverity !== void 0) {
131+
overrideConfig[0].linterOptions = {
132+
reportUnusedDisableDirectives: reportUnusedDisableDirectives
133+
? "error"
134+
: normalizeSeverityToString(reportUnusedDisableDirectivesSeverity)
135+
};
136+
}
137+
128138
if (parser) {
129139
overrideConfig[0].languageOptions.parser = await importer.import(parser);
130140
}
@@ -177,8 +187,7 @@ async function translateOptions({
177187
fixTypes: fixType,
178188
ignore,
179189
overrideConfig,
180-
overrideConfigFile,
181-
reportUnusedDisableDirectives: reportUnusedDisableDirectives ? "error" : void 0
190+
overrideConfigFile
182191
};
183192

184193
if (configType === "flat") {
@@ -190,6 +199,11 @@ async function translateOptions({
190199
options.useEslintrc = eslintrc;
191200
options.extensions = ext;
192201
options.ignorePath = ignorePath;
202+
if (reportUnusedDisableDirectives || reportUnusedDisableDirectivesSeverity !== void 0) {
203+
options.reportUnusedDisableDirectives = reportUnusedDisableDirectives
204+
? "error"
205+
: normalizeSeverityToString(reportUnusedDisableDirectivesSeverity);
206+
}
193207
}
194208

195209
return options;
@@ -386,6 +400,11 @@ const cli = {
386400
return 2;
387401
}
388402

403+
if (options.reportUnusedDisableDirectives && options.reportUnusedDisableDirectivesSeverity !== void 0) {
404+
log.error("The --report-unused-disable-directives option and the --report-unused-disable-directives-severity option cannot be used together.");
405+
return 2;
406+
}
407+
389408
const ActiveESLint = usingFlatConfig ? FlatESLint : ESLint;
390409

391410
const engine = new ActiveESLint(await translateOptions(options, usingFlatConfig ? "flat" : "eslintrc"));

‎lib/config/flat-config-schema.js‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
* starting in Node.js v17.
1515
*/
1616
const structuredClone = require("@ungap/structured-clone").default;
17+
const { normalizeSeverityToNumber } = require("../shared/severity");
1718

1819
//-----------------------------------------------------------------------------
1920
// Type Definitions
@@ -262,6 +263,26 @@ const booleanSchema = {
262263
validate: "boolean"
263264
};
264265

266+
const ALLOWED_SEVERITIES = new Set(["error", "warn", "off", 2, 1, 0]);
267+
268+
/** @type {ObjectPropertySchema} */
269+
const disableDirectiveSeveritySchema = {
270+
merge(first, second) {
271+
const value = second === void 0 ? first : second;
272+
273+
if (typeof value === "boolean") {
274+
return value ? "warn" : "off";
275+
}
276+
277+
return normalizeSeverityToNumber(value);
278+
},
279+
validate(value) {
280+
if (!(ALLOWED_SEVERITIES.has(value) || typeof value === "boolean")) {
281+
throw new TypeError("Expected one of: \"error\", \"warn\", \"off\", 0, 1, 2, or a boolean.");
282+
}
283+
}
284+
};
285+
265286
/** @type {ObjectPropertySchema} */
266287
const deepObjectAssignSchema = {
267288
merge(first = {}, second = {}) {
@@ -534,7 +555,7 @@ const flatConfigSchema = {
534555
linterOptions: {
535556
schema: {
536557
noInlineConfig: booleanSchema,
537-
reportUnusedDisableDirectives: booleanSchema
558+
reportUnusedDisableDirectives: disableDirectiveSeveritySchema
538559
}
539560
},
540561
languageOptions: {

‎lib/eslint/eslint-helpers.js‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -675,7 +675,6 @@ function processOptions({
675675
overrideConfig = null,
676676
overrideConfigFile = null,
677677
plugins = {},
678-
reportUnusedDisableDirectives = null, // ← should be null by default because if it's a string then it overrides the 'reportUnusedDisableDirectives' setting in config files. And we cannot use `overrideConfig.reportUnusedDisableDirectives` instead because we cannot configure the `error` severity with that.
679678
warnIgnored = true,
680679
...unknownOptions
681680
}) {
@@ -720,6 +719,9 @@ function processOptions({
720719
if (unknownOptionKeys.includes("rulePaths")) {
721720
errors.push("'rulePaths' has been removed. Please define your rules using plugins.");
722721
}
722+
if (unknownOptionKeys.includes("reportUnusedDisableDirectives")) {
723+
errors.push("'reportUnusedDisableDirectives' has been removed. Please use the 'overrideConfig.linterOptions.reportUnusedDisableDirectives' option instead.");
724+
}
723725
}
724726
if (typeof allowInlineConfig !== "boolean") {
725727
errors.push("'allowInlineConfig' must be a boolean.");
@@ -774,14 +776,6 @@ function processOptions({
774776
if (Array.isArray(plugins)) {
775777
errors.push("'plugins' doesn't add plugins to configuration to load. Please use the 'overrideConfig.plugins' option instead.");
776778
}
777-
if (
778-
reportUnusedDisableDirectives !== "error" &&
779-
reportUnusedDisableDirectives !== "warn" &&
780-
reportUnusedDisableDirectives !== "off" &&
781-
reportUnusedDisableDirectives !== null
782-
) {
783-
errors.push("'reportUnusedDisableDirectives' must be any of \"error\", \"warn\", \"off\", and null.");
784-
}
785779
if (typeof warnIgnored !== "boolean") {
786780
errors.push("'warnIgnored' must be a boolean.");
787781
}
@@ -806,7 +800,6 @@ function processOptions({
806800
globInputPaths,
807801
ignore,
808802
ignorePatterns,
809-
reportUnusedDisableDirectives,
810803
warnIgnored
811804
};
812805
}

‎lib/eslint/flat-eslint.js‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,6 @@ const LintResultCache = require("../cli-engine/lint-result-cache");
8484
* doesn't do any config file lookup when `true`; considered to be a config filename
8585
* when a string.
8686
* @property {Record<string,Plugin>} [plugins] An array of plugin implementations.
87-
* @property {"error" | "warn" | "off"} [reportUnusedDisableDirectives] the severity to report unused eslint-disable directives.
8887
* @property {boolean} warnIgnored Show warnings when the file list includes ignored files
8988
*/
9089

@@ -449,7 +448,6 @@ async function calculateConfigArray(eslint, {
449448
* @param {FlatConfigArray} config.configs The config.
450449
* @param {boolean} config.fix If `true` then it does fix.
451450
* @param {boolean} config.allowInlineConfig If `true` then it uses directive comments.
452-
* @param {boolean} config.reportUnusedDisableDirectives If `true` then it reports unused `eslint-disable` comments.
453451
* @param {Linter} config.linter The linter instance to verify.
454452
* @returns {LintResult} The result of linting.
455453
* @private
@@ -461,7 +459,6 @@ function verifyText({
461459
configs,
462460
fix,
463461
allowInlineConfig,
464-
reportUnusedDisableDirectives,
465462
linter
466463
}) {
467464
const filePath = providedFilePath || "<text>";
@@ -481,7 +478,6 @@ function verifyText({
481478
allowInlineConfig,
482479
filename: filePathToVerify,
483480
fix,
484-
reportUnusedDisableDirectives,
485481

486482
/**
487483
* Check if the linter should adopt a given code block or not.
@@ -749,7 +745,6 @@ class FlatESLint {
749745
cwd,
750746
fix,
751747
fixTypes,
752-
reportUnusedDisableDirectives,
753748
globInputPaths,
754749
errorOnUnmatchedPattern,
755750
warnIgnored
@@ -859,7 +854,6 @@ class FlatESLint {
859854
cwd,
860855
fix: fixer,
861856
allowInlineConfig,
862-
reportUnusedDisableDirectives,
863857
linter
864858
});
865859

@@ -944,7 +938,6 @@ class FlatESLint {
944938
allowInlineConfig,
945939
cwd,
946940
fix,
947-
reportUnusedDisableDirectives,
948941
warnIgnored: constructorWarnIgnored
949942
} = eslintOptions;
950943
const results = [];
@@ -968,7 +961,6 @@ class FlatESLint {
968961
cwd,
969962
fix,
970963
allowInlineConfig,
971-
reportUnusedDisableDirectives,
972964
linter
973965
}));
974966
}

0 commit comments

Comments
 (0)