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 1 commit
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
1 change: 1 addition & 0 deletions conf/default-cli-options.js
Expand Up @@ -28,5 +28,6 @@ module.exports = {
fix: false,
allowInlineConfig: true,
reportUnusedDisableDirectives: void 0,
reportUnusedDisableDirectivesSeverity: void 0,
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
globInputPaths: true
};
15 changes: 13 additions & 2 deletions 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 directives
--report-unused-disable-directives-severity String Choose what severity level that eslint-disable directives should be reported as
bmish marked this conversation as resolved.
Show resolved Hide resolved

Caching:
--cache Only check changed files - default: false
Expand Down Expand Up @@ -572,7 +573,7 @@ npx eslint --no-inline-config file.js

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

This option causes ESLint to report directive comments like `// eslint-disable-line` when no errors would have been reported on that line anyway.
This option causes ESLint to report disable directive comments like `// eslint-disable-line` as errors when there are no violations present.

* **Argument Type**: No argument.

Expand All @@ -581,7 +582,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 @@ -590,6 +591,16 @@ 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

##### `--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 @@ -45,7 +45,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 severity string indicating if and how unused disable 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 @@ -208,20 +208,22 @@ 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 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 a severity string, as in this example:

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

By default, unused disable 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
4 changes: 2 additions & 2 deletions docs/src/use/configure/rules.md
Expand Up @@ -302,8 +302,8 @@ To report unused `eslint-disable` comments, use the `reportUnusedDisableDirectiv
```json
{
"rules": {...},
"reportUnusedDisableDirectives": true
"reportUnusedDisableDirectives": "error"
bmish marked this conversation as resolved.
Show resolved Hide resolved
}
```

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` or `error` adds reports for unused eslint-disable directives
bmish marked this conversation as resolved.
Show resolved Hide resolved
* @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 @@ -215,7 +215,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` or `error`, then it reports unused `eslint-disable` comments.
bmish marked this conversation as resolved.
Show resolved Hide resolved
* @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
8 changes: 7 additions & 1 deletion lib/cli.js
Expand Up @@ -89,6 +89,7 @@ async function translateOptions({
plugin,
quiet,
reportUnusedDisableDirectives,
reportUnusedDisableDirectivesSeverity,
resolvePluginsRelativeTo,
rule,
rulesdir
Expand Down Expand Up @@ -177,7 +178,7 @@ async function translateOptions({
ignore,
overrideConfig,
overrideConfigFile,
reportUnusedDisableDirectives: reportUnusedDisableDirectives ? "error" : void 0
reportUnusedDisableDirectives: reportUnusedDisableDirectivesSeverity || (reportUnusedDisableDirectives ? "error" : void 0)
};

if (configType === "flat") {
Expand Down Expand Up @@ -377,6 +378,11 @@ const cli = {
return 2;
}

if (options.reportUnusedDisableDirectives && options.reportUnusedDisableDirectivesSeverity) {
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
12 changes: 11 additions & 1 deletion lib/config/flat-config-schema.js
Expand Up @@ -222,6 +222,16 @@ const booleanSchema = {
validate: "boolean"
};

/** @type {ObjectPropertySchema} */
const booleanOrStringSchema = {
merge: "replace",
validate(value) {
if (typeof value !== "string" && typeof value !== "boolean") {
throw new TypeError("Expected a string or a boolean.");
}
}
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
};

/** @type {ObjectPropertySchema} */
const deepObjectAssignSchema = {
merge(first = {}, second = {}) {
Expand Down Expand Up @@ -447,7 +457,7 @@ exports.flatConfigSchema = {
linterOptions: {
schema: {
noInlineConfig: booleanSchema,
reportUnusedDisableDirectives: booleanSchema
reportUnusedDisableDirectives: booleanOrStringSchema
}
},
languageOptions: {
Expand Down
2 changes: 1 addition & 1 deletion lib/eslint/eslint-helpers.js
Expand Up @@ -675,7 +675,7 @@ 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
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. TODO: should anything change based on this comment?
bmish marked this conversation as resolved.
Show resolved Hide resolved
...unknownOptions
}) {
const errors = [];
Expand Down
2 changes: 1 addition & 1 deletion lib/eslint/eslint.js
Expand Up @@ -165,7 +165,7 @@ 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.
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. TODO: should anything change based on this comment?
resolvePluginsRelativeTo = null, // ← should be null by default because if it's a string then it suppresses RFC47 feature.
rulePaths = [],
useEslintrc = true,
Expand Down
2 changes: 1 addition & 1 deletion lib/eslint/flat-eslint.js
Expand Up @@ -466,7 +466,7 @@ 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 {boolean|string} config.reportUnusedDisableDirectives If `true` or `error`, then it reports unused `eslint-disable` comments.
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
* @param {Linter} config.linter The linter instance to verify.
* @returns {LintResult} The result of linting.
* @private
Expand Down
16 changes: 13 additions & 3 deletions lib/linter/linter.js
Expand Up @@ -613,6 +613,7 @@ function normalizeFilename(filename) {
* @param {VerifyOptions} providedOptions Options
* @param {ConfigData} config Config.
* @returns {Required<VerifyOptions> & InternalOptions} Normalized options
* @throws an error upon invalid options
*/
function normalizeVerifyOptions(providedOptions, config) {

Expand All @@ -631,9 +632,18 @@ function normalizeVerifyOptions(providedOptions, config) {
reportUnusedDisableDirectives = reportUnusedDisableDirectives ? "error" : "off";
}
if (typeof reportUnusedDisableDirectives !== "string") {
reportUnusedDisableDirectives =
linterOptions.reportUnusedDisableDirectives
? "warn" : "off";

// Handle either a boolean or severity string in linterOptions.reportUnusedDisableDirectives.
if (typeof linterOptions.reportUnusedDisableDirectives === "string") {
if (!["error", "warn", "off"].includes(linterOptions.reportUnusedDisableDirectives)) {
throw new Error("Invalid value for 'reportUnusedDisableDirectives' in config.");
}
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
reportUnusedDisableDirectives = linterOptions.reportUnusedDisableDirectives;
} else if (linterOptions.reportUnusedDisableDirectives === true) {
reportUnusedDisableDirectives = "warn";
} else {
reportUnusedDisableDirectives = "off";
}
}

return {
Expand Down
9 changes: 8 additions & 1 deletion lib/options.js
Expand Up @@ -47,7 +47,7 @@ const optionator = require("optionator");
* @property {Object} [parserOptions] Specify parser options
* @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 directives
* @property {boolean|string|undefined} reportUnusedDisableDirectives Adds reported errors for unused eslint-disable directives
* @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 @@ -294,6 +294,13 @@ module.exports = function(usingFlatConfig) {
default: void 0,
description: "Adds reported errors for unused eslint-disable directives"
},
{
option: "report-unused-disable-directives-severity",
type: "String",
default: void 0,
description: "Chooses severity level for reporting unused eslint-disable directives",
bmish marked this conversation as resolved.
Show resolved Hide resolved
enum: ["off", "warn", "error"]
},
{
heading: "Caching"
},
Expand Down
4 changes: 2 additions & 2 deletions lib/shared/types.js
Expand Up @@ -47,7 +47,7 @@ module.exports = {};
* @property {ParserOptions} [parserOptions] The parser options.
* @property {string[]} [plugins] The plugin specifiers.
* @property {string} [processor] The processor specifier.
* @property {boolean} [reportUnusedDisableDirectives] The flag to report unused `eslint-disable` comments.
* @property {boolean|string} [reportUnusedDisableDirectives] The flag or severity to report unused `eslint-disable` comments.
bmish marked this conversation as resolved.
Show resolved Hide resolved
* @property {boolean} [root] The root flag.
* @property {Record<string, RuleConf>} [rules] The rule settings.
* @property {Object} [settings] The shared settings.
Expand All @@ -66,7 +66,7 @@ module.exports = {};
* @property {ParserOptions} [parserOptions] The parser options.
* @property {string[]} [plugins] The plugin specifiers.
* @property {string} [processor] The processor specifier.
* @property {boolean} [reportUnusedDisableDirectives] The flag to report unused `eslint-disable` comments.
* @property {boolean|string} [reportUnusedDisableDirectives] The flag or severity to report unused `eslint-disable` comments.
bmish marked this conversation as resolved.
Show resolved Hide resolved
* @property {Record<string, RuleConf>} [rules] The rule settings.
* @property {Object} [settings] The shared settings.
*/
Expand Down