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: Reporting unused inline configs #121

Merged
merged 15 commits into from
Sep 5, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
244 changes: 244 additions & 0 deletions designs/2024-report-unused-inline-configs/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,244 @@
- Repo: eslint/eslint
- Start Date: 2024-07-08
- RFC PR:
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
- Authors: [Josh Goldberg](https://github.com/JoshuaKGoldberg)

# Reporting unused inline configs

## Summary

Add an option to report `/* eslint ... */` comments that don't change any settings.

## Motivation

Right now, nothing in ESLint core stops [inline configs (configuration comments)](https://eslint.org/docs/latest/use/configure/rules#using-configuration-comments) from redundantly repeating the same configuration _option_ and/or _severity_ as the file's existing computed configuration.
Unused inline configs suffer from the same drawbacks as [unused disable directives](https://eslint.org/docs/latest/use/configure/rules#report-unused-eslint-disable-comments): they take up space and can be misleading.

For example:

- [Playground of an inline config setting the same severity as the config file](https://eslint.org/play/#eyJ0ZXh0IjoiLyogZXNsaW50IG5vLXVudXNlZC1leHByZXNzaW9uczogXCJlcnJvclwiICovXG5cIuKdjCBkb2VzIG5vdGhpbmc6IGV4aXN0aW5nIHNldmVyaXR5IHZzLiBmaWxlXCJcbiIsIm9wdGlvbnMiOnsicnVsZXMiOnt9LCJsYW5ndWFnZU9wdGlvbnMiOnsiZWNtYVZlcnNpb24iOiJsYXRlc3QiLCJzb3VyY2VUeXBlIjoibW9kdWxlIiwicGFyc2VyT3B0aW9ucyI6eyJlY21hRmVhdHVyZXMiOnt9fX19fQ==)
- [Playground of an inline config setting the same options as the config file](https://eslint.org/play/#eyJ0ZXh0IjoiLyogZXNsaW50IG5vLXVudXNlZC1leHByZXNzaW9uczogW1wiZXJyb3JcIiwgeyBcImFsbG93U2hvcnRDaXJjdWl0XCI6IHRydWUgfV0gKi9cblwi4p2MIGRvZXMgbm90aGluZzogZXhpc3Rpbmcgc2V2ZXJpdHkgYW5kIG9wdGlvbnMgdnMuIGZpbGVcIlxuIiwib3B0aW9ucyI6eyJydWxlcyI6eyJuby11bnVzZWQtZXhwcmVzc2lvbnMiOlsiZXJyb3IiLHsiYWxsb3dTaG9ydENpcmN1aXQiOnRydWUsImFsbG93VGVybmFyeSI6ZmFsc2UsImFsbG93VGFnZ2VkVGVtcGxhdGVzIjpmYWxzZSwiZW5mb3JjZUZvckpTWCI6ZmFsc2V9XX0sImxhbmd1YWdlT3B0aW9ucyI6eyJlY21hVmVyc2lvbiI6ImxhdGVzdCIsInNvdXJjZVR5cGUiOiJtb2R1bGUiLCJwYXJzZXJPcHRpb25zIjp7ImVjbWFGZWF0dXJlcyI6e319fX19)

This RFC proposes adding the ability for ESLint to report on those unused inline configs:

- `--report-unused-inline-configs` CLI option
- `linterOptions.reportUnusedDisableDirectives` configuration file option
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

```shell
npx eslint --report-unused-inline-configs error
```

```js
{
linterOptions: {
reportUnusedDisableDirectives: "error",
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
}
}
```

These new options would be similar to the existing [`--report-unused-disable-directives(-severity)`](https://eslint.org/docs/latest/use/command-line-interface#--report-unused-disable-directives) and [`linterOptions.reportUnusedDisableDirectives`](https://eslint.org/docs/latest/use/configure/configuration-files#reporting-unused-disable-directives) options.

### Examples

The following table uses [`accessor-pairs`](https://eslint.org/docs/latest/rules/accessor-pairs) as an example with inline configs like `/* eslint accessor-pairs: "off" */`.
It assumes the `accessor-pairs` default options from [feat: add meta.defaultOptions](https://github.com/eslint/eslint/pull/17656) of:

```json
{
"enforceForClassMembers": true,
"getWithoutSet": false,
"setWithoutGet": true
}
```

<table>
<thead>
<tr>
<th>Original Config Setting</th>
<th>Inline Config Settings</th>
<th>Report?</th>
</tr>
</thead>
<tbody>
<tr>
<td rowspan="7">
<code>"error"</code>
<br />
<code>2</code>
<br />
<code>["error", { "enforceForClassMembers": true }]</code>
<br />
<code>["error", { "getWithoutSet": false }]</code>
</td>
<td>
<code>"off"</code>
<br />
<code>0</code>
</td>
<td>No</td>
</tr>
<tr>
<td>
<code>"warn"</code>
<br />
<code>1</code>
</td>
<td>No</td>
</tr>
<tr>
<td>
<code>"error"</code>
<br />
<code>2</code>
</td>
<td><strong>Yes</strong></td>
</tr>
<tr>
<td>
<code>["error", { "enforceForClassMembers": false }]</code>
<br />
<code>[2, { "enforceForClassMembers": false }]</code>
</td>
<td>No</td>
</tr>
<tr>
<td>
<code>["error", { "getWithoutSet": true }]</code>
<br />
<code>[2, { "getWithoutSet": true }]</code>
</td>
<td>No</td>
</tr>
<tr>
<td>
<code>["error", { "enforceForClassMembers": true }]</code>
<br />
<code>[2, { "enforceForClassMembers": true }]</code>
</td>
<td><strong>Yes</strong></td>
</tr>
<td>
<code>["error", { "getWithoutSet": false }]</code>
<br />
<code>[2, { "getWithoutSet": false }]</code>
</td>
<td><strong>Yes</strong></td>
</tr>
</tbody>
</table>

## Detailed Design

Additional logic can be added to the existing code points in `Linter` that validate inline config options:

- [`_verifyWithFlatConfigArrayAndWithoutProcessors`](https://github.com/eslint/eslint/blob/7c78ad9d9f896354d557f24e2d37710cf79a27bf/lib/linter/linter.js#L1650): called in the current ("flat") config system
- [`getDirectiveComments`](https://github.com/eslint/eslint/blob/7c78ad9d9f896354d557f24e2d37710cf79a27bf/lib/linter/linter.js#L387): called when `ESLINT_USE_FLAT_CONFIG=true`
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

For a rough code reference, see [`poc: reporting unused inline configs`](https://github.com/JoshuaKGoldberg/eslint/commit/e14e404ed93e6238bdee817923a449f5215eecd8).

This proposed design intentionally not involve any language-specific code changes.
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
How a specific language computes its configuration comments is irrelevant to this proposed feature.

### Computing Option Differences

Each inline config comment will be compared against the existing configuration value(s) it attempts to override:

- If the config comment only specifies a severity, then only the severity will be checked for redundancy
- The new logic will normalize options: `"off"` will be considered equivalent to `0`
- If the config comment also specifies rule options, they will be compared for deep equality to the existing rule options
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

This RFC should wait to begin work until after [feat: add meta.defaultOptions](https://github.com/eslint/eslint/pull/17656) is merged into ESLint.
That way, a rule's `meta.defaultOptions` can be factored into computing whether an inline config's rule options differ from the previously configured options.
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

### Default Values

This RFC proposes a two-step approach to introducing unused inline config reporting:

1. In the current major version upon introduction, don't enable unused inline config reporting by default
2. In the next major version, enable unused inline config reporting by default

Note that the default value in the next major version should be the same as reporting unused disable directives.
See [Change Request: error by default for unused disable directives](https://github.com/eslint/eslint/issues/18665) for an issue on changing that from `"warn"` to `"error"`.

## Documentation

The new settings will be documented similarly to reporting unused disable directives:

- [Configuration Files](https://eslint.org/docs/latest/use/configure/configuration-files):
- List item for `reportUnusedInlineConfig` under _[Configuration Objects](https://eslint.org/docs/latest/use/configure/configuration-files#configuration-objects)_ > `linterOptions`
- Sub-heading alongside _[Reporting Unused Disable Directives](https://eslint.org/docs/latest/use/configure/configuration-files#reporting-unused-disable-directives)_
- [Configure Rules](https://eslint.org/docs/latest/use/configure/rules):
- Sub-heading alongside _[Report unused eslint-disable comments](https://eslint.org/docs/latest/use/configure/rules#report-unused-eslint-disable-comments)_
- [Command Line Interface Reference](https://eslint.org/docs/latest/use/command-line-interface):
- List item under the _[Options](https://eslint.org/docs/latest/use/command-line-interface#options)_ code block, under `Inline Configuration comments:`
- Corresponding sub-headings under _[Inline Configuration Comments](https://eslint.org/docs/latest/use/command-line-interface#inline-configuration-comments)_

## Drawbacks

Any added options come with an added tax on project maintenance and user comprehension.
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
This RFC believes the flagging of unused inline configs is worth that tax.

See [Omitting Legacy Config Support](#omitting-legacy-config-support) for a possible reduction in cost.

## Backwards Compatibility Analysis

The proposed two-step approach introduces the options in a fully backwards-compatible way.
No new warnings or errors will be reported in the current major version without the user explicitly opting into them.

## Alternatives

### Omitting Legacy Config Support

One way to reduce costs could be to wait until ESLint completely removes support for legacy configs.
That way, only the new ("flat") config system would need to be tested with this change.
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

However, it is unclear when the legacy config system will be removed from ESLint core.
This RFC prefers taking action sooner rather than later.

### Separate CLI Option

Unlike the changes discussed in [Change Request: Enable reportUnusedDisableDirectives config option by default](https://github.com/eslint/eslint/issues/15466) -> [feat!: flexible config + default reporting of unused disable directives](https://github.com/eslint/rfcs/pull/100), reporting unused inline configs does not have legacy behavior to keep to.
The existing `--report-unused-disable-directives` (enabling) and `--report-unused-disable-directives-severity` (severity) options were kept separate for backwards compatibility.
Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify, --report-unused-disable-directives is the same as --report-unused-disable-directives-severity error, and it isn't necessary to pass --report-unused-disable-directives to enable reporting when --report-unused-disable-directives-severity is used. In fact, these two options cannot be used together.

https://github.com/eslint/eslint/blob/d1f0831bac173fe3e6e81ff95c5abdbf95b02b65/lib/cli.js#L484-L487


Adding a sole `--report-unused-inline-configs` CLI option presents a discrepency between the two sets of options.
An alternative could be to instead add `--report-unused-inline-configs` an `--report-unused-inline-configs-severity` options for consistency's sake.
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

This RFC's opinion is that the consistency of adding two new options is not worth the excess options logic.
Copy link
Member

Choose a reason for hiding this comment

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

Agreed. 👍

It would instead be preferable to, in a future ESLint major version, deprecate `--report-unused-disable-directives-severity` and merge its logic setting into `--report-unused-disable-directives`.

### Superset Behavior: Unused Disable Directive Reporting

Disable directives can be thought as a subset of inline configs in general.
Reporting on unused disable directives could be thought of as a subset of reporting on unused inline configs.

An additional direction this RFC could propose would be to have the new unused inline config reporting act as a superset of unused disable directive reporting.

However:

- Deprecating `reportUnusedDisableDirectives` would be a distruptive breaking change
- This RFC proposes enabling `reportUnusedInlineConfigs` by default in a subsequent major version anyway: most users will not be manually configuring it

## Help Needed

I would like to implement this RFC.

## Frequently Asked Questions

### Why so many references to reporting unused disable directives?

This RFC's proposed behavior is similar on the surface to the existing behavior around reporting unused disable directives.
It's beneficial for users to have similar behaviors between similar options.

### Will inline configs be compared to previous inline configs in the same file?

As of [Change Request: Disallow multiple configuration comments for the same rule](https://github.com/eslint/eslint/issues/18132) -> [feat!: disallow multiple configuration comments for same rule](https://github.com/eslint/eslint/pull/18157), the same rule cannot be configured by an inline config more than once per file.

## Related Discussions

- <https://github.com/eslint/eslint/issues/18230>: the issue triggering this RFC
- <https://github.com/eslint/eslint/issues/15476>: an older issue that #18230 duplicated
- <https://github.com/eslint/eslint/issues/15466>: previous issue for enabling `reportUnusedDisableDirectives` config option by default
- <https://github.com/eslint/rfcs/pull/100>: the RFC discussion flexible config + default reporting of unused disable directives
- <https://github.com/eslint/eslint/pull/17212>: the PR implementing custom severity when reporting unused disable directives
- <https://github.com/eslint/eslint/issues/18665>: issue suggesting erroring by default for unused disable directives
- <https://github.com/eslint/eslint/issues/18666>: issue suggesting merging `--report-unused-disable-directives-severity` into `--report-unused-disable-directives`
Loading