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: Support custom severity when reporting unused disable directives #17212

Merged
merged 61 commits into from Dec 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
61 commits
Select commit Hold shift + click to select a range
8518c04
feat: Support custom severity when reporting unused disable directives
bmish May 22, 2023
9d70807
Merge branch 'main' into rfc-100-part1
bmish Nov 5, 2023
3e3d482
fix test
bmish Nov 6, 2023
3cb8616
remove change to conf/default-cli-options.js
bmish Nov 6, 2023
4b8ce60
improve schema
bmish Nov 6, 2023
d185024
wip
bmish Nov 26, 2023
6e4cb25
Merge branch 'main' into rfc-100-part1
bmish Dec 1, 2023
a1295c0
remove reportUnusedDisableDirectives from processOptions() in favor o…
bmish Dec 2, 2023
6f2bd3c
revert changes to lib/eslint/eslint.js and fix more tests
bmish Dec 2, 2023
7dd444b
remove old type
bmish Dec 2, 2023
82ebdd3
more tests
bmish Dec 2, 2023
964f270
fix comment for ParsedCLIOptions
bmish Dec 2, 2023
56aa528
remove unnecessary exception
bmish Dec 2, 2023
7294056
revert comment
bmish Dec 2, 2023
c774f31
remove number severity
bmish Dec 3, 2023
9f2dc93
fix translateOptions and tests
bmish Dec 3, 2023
abf2ebc
revert comment
bmish Dec 3, 2023
2106b44
Update lib/cli.js
bmish Dec 3, 2023
9818046
Update lib/options.js
bmish Dec 3, 2023
c6a23bf
Update docs/src/use/command-line-interface.md
bmish Dec 3, 2023
321de72
Update lib/cli-engine/cli-engine.js
bmish Dec 3, 2023
9743273
Update lib/cli-engine/cli-engine.js
bmish Dec 3, 2023
de86902
Update lib/shared/types.js
bmish Dec 3, 2023
28341c4
Update lib/shared/types.js
bmish Dec 3, 2023
b4ab419
add arg type to doc
bmish Dec 3, 2023
f0e89d4
mention enable
bmish Dec 3, 2023
4ca4f69
warn with old option
bmish Dec 3, 2023
e7dba74
remove old comment
bmish Dec 3, 2023
13c456d
revert change to tests/lib/eslint/eslint.js
bmish Dec 3, 2023
fa35e5f
add test for removed option
bmish Dec 3, 2023
8951732
revert copy change
bmish Dec 3, 2023
186408d
switch several tests from boolean for reportUnusedDisableDirectives t…
bmish Dec 3, 2023
7c88af0
improve tests
bmish Dec 3, 2023
9fd8d6d
use reportUnusedDisableDirectives: error in eslint-config-eslint
bmish Dec 4, 2023
3c88633
support severity numbers
bmish Dec 4, 2023
b493bbb
refactor
bmish Dec 4, 2023
cadb6d5
revert newline change
bmish Dec 4, 2023
67cfb8f
refactor and fix tests
bmish Dec 4, 2023
7f8d0a8
refactor
bmish Dec 4, 2023
ae363bf
cleanup
bmish Dec 4, 2023
30e3f30
cleanup
bmish Dec 4, 2023
eb25551
use normalizeSeverityToString in another place
bmish Dec 4, 2023
68a9b83
tweak
bmish Dec 4, 2023
dd9861c
attempt to avoid test pollution
bmish Dec 5, 2023
ff813a1
tweak test name
bmish Dec 5, 2023
194c649
remove boolean handling from normalizeSeverityToString
bmish Dec 6, 2023
7fe6ef0
Update docs/src/use/command-line-interface.md
bmish Dec 7, 2023
62ab43c
Update docs/src/use/command-line-interface.md
bmish Dec 7, 2023
e43952e
Update docs/src/use/command-line-interface.md
bmish Dec 7, 2023
9874ae5
cleanup
bmish Dec 7, 2023
bec28fb
normalize flat config reportUnusedDisableDirectives to number
bmish Dec 7, 2023
8c9f33f
Update lib/options.js
bmish Dec 8, 2023
43ad9fd
Update lib/cli.js
bmish Dec 8, 2023
462bc31
Update lib/cli.js
bmish Dec 8, 2023
54cc4a3
remove unused reportUnusedDisableDirectives property
bmish Dec 8, 2023
691b142
run CLI tests for both flat and eslintrc
bmish Dec 8, 2023
4b3d2d8
Update tests/lib/eslint/flat-eslint.js
bmish Dec 12, 2023
f9b60f0
Update tests/lib/eslint/flat-eslint.js
bmish Dec 12, 2023
ba288e6
Update docs/src/use/configure/rules.md
bmish Dec 12, 2023
cd36dd7
address pr feedback
bmish Dec 13, 2023
ddf9aa2
Merge branch 'main' into rfc-100-part1
bmish Dec 13, 2023
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
2 changes: 1 addition & 1 deletion Makefile.js
Expand Up @@ -68,7 +68,7 @@ const NODE = "node ", // intentional extra space

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

// Files
RULE_FILES = glob.sync("lib/rules/*.js").filter(filePath => path.basename(filePath) !== "index.js"),
Expand Down
20 changes: 19 additions & 1 deletion docs/src/use/command-line-interface.md
Expand Up @@ -98,6 +98,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 and eslint-enable directives
--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

Caching:
--cache Only check changed files - default: false
Expand Down Expand Up @@ -582,7 +583,7 @@ This can be useful to prevent future errors from unexpectedly being suppressed,
::: warning
When using this option, it is possible that new errors start being reported whenever ESLint or custom rules are upgraded.

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.
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.
:::

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

#### `--report-unused-disable-directives-severity`

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.
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved

* **Argument Type**: String. One of the following values:
1. `off` (or `0`)
1. `warn` (or `1`)
1. `error` (or `2`)
* **Multiple Arguments**: No
* **Default Value**: By default, `linterOptions.reportUnusedDisableDirectives` configuration setting is used.

##### `--report-unused-disable-directives-severity` example

```shell
npx eslint --report-unused-disable-directives-severity warn file.js
```

### Caching

#### `--cache`
Expand Down
10 changes: 6 additions & 4 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 and enable directives should be tracked and reported.
* `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"`)
* `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,20 +244,22 @@ export default [

#### Reporting unused disable directives

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:
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:

```js
export default [
{
files: ["**/*.js"],
linterOptions: {
reportUnusedDisableDirectives: true
reportUnusedDisableDirectives: "error"
}
}
];
```

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.
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.

For legacy compatibility, `true` is equivalent to `"warn"` and `false` is equivalent to `"off"`.
bmish marked this conversation as resolved.
Show resolved Hide resolved

### Configuring language options

Expand Down
2 changes: 1 addition & 1 deletion docs/src/use/configure/migration-guide.md
Expand Up @@ -575,7 +575,7 @@ export default [
// ...other config
linterOptions: {
noInlineConfig: true,
reportUnusedDisableDirectives: true
reportUnusedDisableDirectives: "warn"
}
}
];
Expand Down
2 changes: 1 addition & 1 deletion docs/src/use/configure/rules.md
Expand Up @@ -317,4 +317,4 @@ To report unused `eslint-disable` comments, use the `reportUnusedDisableDirectiv
}
```

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).
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.
4 changes: 2 additions & 2 deletions lib/cli-engine/cli-engine.js
Expand Up @@ -83,7 +83,7 @@ const validFixTypes = new Set(["directive", "problem", "suggestion", "layout"]);
* @property {string[]} [plugins] An array of plugins to load.
* @property {Record<string,RuleConf>} [rules] An object of rules to use.
* @property {string[]} [rulePaths] An array of directories to load custom rules from.
* @property {boolean} [reportUnusedDisableDirectives] `true` adds reports for unused eslint-disable directives
* @property {boolean|string} [reportUnusedDisableDirectives] `true`, `"error"` or '"warn"' adds reports for unused eslint-disable directives
* @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.
* @property {string} [resolvePluginsRelativeTo] The folder where plugins should be resolved from, defaulting to the CWD
*/
Expand Down Expand Up @@ -224,7 +224,7 @@ function calculateStatsPerRun(results) {
* @param {ConfigArray} config.config The config.
* @param {boolean} config.fix If `true` then it does fix.
* @param {boolean} config.allowInlineConfig If `true` then it uses directive comments.
* @param {boolean} config.reportUnusedDisableDirectives If `true` then it reports unused `eslint-disable` comments.
* @param {boolean|string} config.reportUnusedDisableDirectives If `true`, `"error"` or '"warn"', then it reports unused `eslint-disable` comments.
* @param {FileEnumerator} config.fileEnumerator The file enumerator to check if a path is a target or not.
* @param {Linter} config.linter The linter instance to verify.
* @returns {LintResult} The result of linting.
Expand Down
25 changes: 22 additions & 3 deletions lib/cli.js
Expand Up @@ -22,7 +22,8 @@ const fs = require("fs"),
{ FlatESLint, shouldUseFlatConfig } = require("./eslint/flat-eslint"),
createCLIOptions = require("./options"),
log = require("./shared/logging"),
RuntimeInfo = require("./shared/runtime-info");
RuntimeInfo = require("./shared/runtime-info"),
{ normalizeSeverityToString } = require("./shared/severity");
const { Legacy: { naming } } = require("@eslint/eslintrc");
const { ModuleImporter } = require("@humanwhocodes/module-importer");

Expand Down Expand Up @@ -89,6 +90,7 @@ async function translateOptions({
plugin,
quiet,
reportUnusedDisableDirectives,
reportUnusedDisableDirectivesSeverity,
resolvePluginsRelativeTo,
rule,
rulesdir,
Expand Down Expand Up @@ -125,6 +127,14 @@ async function translateOptions({
rules: rule ? rule : {}
}];

if (reportUnusedDisableDirectives || reportUnusedDisableDirectivesSeverity !== void 0) {
overrideConfig[0].linterOptions = {
reportUnusedDisableDirectives: reportUnusedDisableDirectives
? "error"
: normalizeSeverityToString(reportUnusedDisableDirectivesSeverity)
};
}

if (parser) {
overrideConfig[0].languageOptions.parser = await importer.import(parser);
}
Expand Down Expand Up @@ -177,8 +187,7 @@ async function translateOptions({
fixTypes: fixType,
ignore,
overrideConfig,
overrideConfigFile,
reportUnusedDisableDirectives: reportUnusedDisableDirectives ? "error" : void 0
overrideConfigFile
};

if (configType === "flat") {
Expand All @@ -190,6 +199,11 @@ async function translateOptions({
options.useEslintrc = eslintrc;
options.extensions = ext;
options.ignorePath = ignorePath;
if (reportUnusedDisableDirectives || reportUnusedDisableDirectivesSeverity !== void 0) {
options.reportUnusedDisableDirectives = reportUnusedDisableDirectives
? "error"
: normalizeSeverityToString(reportUnusedDisableDirectivesSeverity);
}
}

return options;
Expand Down Expand Up @@ -386,6 +400,11 @@ const cli = {
return 2;
}

if (options.reportUnusedDisableDirectives && options.reportUnusedDisableDirectivesSeverity !== void 0) {
log.error("The --report-unused-disable-directives option and the --report-unused-disable-directives-severity option cannot be used together.");
return 2;
}

const ActiveESLint = usingFlatConfig ? FlatESLint : ESLint;

const engine = new ActiveESLint(await translateOptions(options, usingFlatConfig ? "flat" : "eslintrc"));
Expand Down
23 changes: 22 additions & 1 deletion lib/config/flat-config-schema.js
Expand Up @@ -14,6 +14,7 @@
* starting in Node.js v17.
*/
const structuredClone = require("@ungap/structured-clone").default;
const { normalizeSeverityToNumber } = require("../shared/severity");

//-----------------------------------------------------------------------------
// Type Definitions
Expand Down Expand Up @@ -262,6 +263,26 @@ const booleanSchema = {
validate: "boolean"
};

const ALLOWED_SEVERITIES = new Set(["error", "warn", "off", 2, 1, 0]);

/** @type {ObjectPropertySchema} */
const disableDirectiveSeveritySchema = {
merge(first, second) {
const value = second === void 0 ? first : second;

if (typeof value === "boolean") {
return value ? "warn" : "off";
}

return normalizeSeverityToNumber(value);
},
validate(value) {
if (!(ALLOWED_SEVERITIES.has(value) || typeof value === "boolean")) {
throw new TypeError("Expected one of: \"error\", \"warn\", \"off\", 0, 1, 2, or a boolean.");
}
}
};

/** @type {ObjectPropertySchema} */
const deepObjectAssignSchema = {
merge(first = {}, second = {}) {
Expand Down Expand Up @@ -534,7 +555,7 @@ const flatConfigSchema = {
linterOptions: {
schema: {
noInlineConfig: booleanSchema,
reportUnusedDisableDirectives: booleanSchema
reportUnusedDisableDirectives: disableDirectiveSeveritySchema
}
},
languageOptions: {
Expand Down
13 changes: 3 additions & 10 deletions lib/eslint/eslint-helpers.js
Expand Up @@ -675,7 +675,6 @@ function processOptions({
overrideConfig = null,
overrideConfigFile = null,
plugins = {},
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.
bmish marked this conversation as resolved.
Show resolved Hide resolved
warnIgnored = true,
...unknownOptions
}) {
Expand Down Expand Up @@ -720,6 +719,9 @@ function processOptions({
if (unknownOptionKeys.includes("rulePaths")) {
errors.push("'rulePaths' has been removed. Please define your rules using plugins.");
}
if (unknownOptionKeys.includes("reportUnusedDisableDirectives")) {
errors.push("'reportUnusedDisableDirectives' has been removed. Please use the 'overrideConfig.linterOptions.reportUnusedDisableDirectives' option instead.");
}
}
if (typeof allowInlineConfig !== "boolean") {
errors.push("'allowInlineConfig' must be a boolean.");
Expand Down Expand Up @@ -774,14 +776,6 @@ function processOptions({
if (Array.isArray(plugins)) {
errors.push("'plugins' doesn't add plugins to configuration to load. Please use the 'overrideConfig.plugins' option instead.");
}
if (
reportUnusedDisableDirectives !== "error" &&
reportUnusedDisableDirectives !== "warn" &&
reportUnusedDisableDirectives !== "off" &&
reportUnusedDisableDirectives !== null
) {
errors.push("'reportUnusedDisableDirectives' must be any of \"error\", \"warn\", \"off\", and null.");
}
if (typeof warnIgnored !== "boolean") {
errors.push("'warnIgnored' must be a boolean.");
}
Expand All @@ -806,7 +800,6 @@ function processOptions({
globInputPaths,
ignore,
ignorePatterns,
reportUnusedDisableDirectives,
warnIgnored
};
}
Expand Down
8 changes: 0 additions & 8 deletions lib/eslint/flat-eslint.js
Expand Up @@ -84,7 +84,6 @@ const LintResultCache = require("../cli-engine/lint-result-cache");
* doesn't do any config file lookup when `true`; considered to be a config filename
* when a string.
* @property {Record<string,Plugin>} [plugins] An array of plugin implementations.
* @property {"error" | "warn" | "off"} [reportUnusedDisableDirectives] the severity to report unused eslint-disable directives.
* @property {boolean} warnIgnored Show warnings when the file list includes ignored files
*/

Expand Down Expand Up @@ -449,7 +448,6 @@ async function calculateConfigArray(eslint, {
* @param {FlatConfigArray} config.configs The config.
* @param {boolean} config.fix If `true` then it does fix.
* @param {boolean} config.allowInlineConfig If `true` then it uses directive comments.
* @param {boolean} config.reportUnusedDisableDirectives If `true` then it reports unused `eslint-disable` comments.
* @param {Linter} config.linter The linter instance to verify.
* @returns {LintResult} The result of linting.
* @private
Expand All @@ -461,7 +459,6 @@ function verifyText({
configs,
fix,
allowInlineConfig,
reportUnusedDisableDirectives,
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
linter
}) {
const filePath = providedFilePath || "<text>";
Expand All @@ -481,7 +478,6 @@ function verifyText({
allowInlineConfig,
filename: filePathToVerify,
fix,
reportUnusedDisableDirectives,

/**
* Check if the linter should adopt a given code block or not.
Expand Down Expand Up @@ -749,7 +745,6 @@ class FlatESLint {
cwd,
fix,
fixTypes,
reportUnusedDisableDirectives,
globInputPaths,
errorOnUnmatchedPattern,
warnIgnored
Expand Down Expand Up @@ -859,7 +854,6 @@ class FlatESLint {
cwd,
fix: fixer,
allowInlineConfig,
reportUnusedDisableDirectives,
linter
});

Expand Down Expand Up @@ -944,7 +938,6 @@ class FlatESLint {
allowInlineConfig,
cwd,
fix,
reportUnusedDisableDirectives,
warnIgnored: constructorWarnIgnored
} = eslintOptions;
const results = [];
Expand All @@ -968,7 +961,6 @@ class FlatESLint {
cwd,
fix,
allowInlineConfig,
reportUnusedDisableDirectives,
linter
}));
}
Expand Down
9 changes: 6 additions & 3 deletions lib/linter/linter.js
Expand Up @@ -44,6 +44,7 @@ const { getRuleFromConfig } = require("../config/flat-config-helpers");
const { FlatConfigArray } = require("../config/flat-config-array");
const { RuleValidator } = require("../config/rule-validator");
const { assertIsRuleOptions, assertIsRuleSeverity } = require("../config/flat-config-schema");
const { normalizeSeverityToString } = require("../shared/severity");
const debug = require("debug")("eslint:linter");
const MAX_AUTOFIX_PASSES = 10;
const DEFAULT_PARSER_NAME = "espree";
Expand Down Expand Up @@ -653,9 +654,11 @@ function normalizeVerifyOptions(providedOptions, config) {
reportUnusedDisableDirectives = reportUnusedDisableDirectives ? "error" : "off";
}
if (typeof reportUnusedDisableDirectives !== "string") {
reportUnusedDisableDirectives =
linterOptions.reportUnusedDisableDirectives
? "warn" : "off";
if (typeof linterOptions.reportUnusedDisableDirectives === "boolean") {
reportUnusedDisableDirectives = linterOptions.reportUnusedDisableDirectives ? "warn" : "off";
} else {
reportUnusedDisableDirectives = linterOptions.reportUnusedDisableDirectives === void 0 ? "off" : normalizeSeverityToString(linterOptions.reportUnusedDisableDirectives);
}
}

return {
Expand Down
8 changes: 8 additions & 0 deletions lib/options.js
Expand Up @@ -48,6 +48,7 @@ const optionator = require("optionator");
* @property {string[]} [plugin] Specify plugins
* @property {string} [printConfig] Print the configuration for the given file
* @property {boolean | undefined} reportUnusedDisableDirectives Adds reported errors for unused eslint-disable and eslint-enable directives
* @property {string | undefined} reportUnusedDisableDirectivesSeverity A severity string indicating if and how unused disable and enable directives should be tracked and reported.
* @property {string} [resolvePluginsRelativeTo] A folder where plugins should be resolved from, CWD by default
* @property {Object} [rule] Specify rules
* @property {string[]} [rulesdir] Load additional rules from this directory. Deprecated: Use rules from plugins
Expand Down Expand Up @@ -306,6 +307,13 @@ module.exports = function(usingFlatConfig) {
default: void 0,
description: "Adds reported errors for unused eslint-disable and eslint-enable directives"
},
{
option: "report-unused-disable-directives-severity",
type: "String",
default: void 0,
description: "Chooses severity level for reporting unused eslint-disable and eslint-enable directives",
enum: ["off", "warn", "error", "0", "1", "2"]
},
{
heading: "Caching"
},
Expand Down