From 5c1b771ce006086989c3d0497027f4e3d079f500 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sun, 7 Jul 2024 17:53:51 -0400 Subject: [PATCH] feat: Reporting unused inline configs --- .../README.md | 244 ++++++++++++++++++ 1 file changed, 244 insertions(+) create mode 100644 designs/2024-report-unused-inline-configs/README.md diff --git a/designs/2024-report-unused-inline-configs/README.md b/designs/2024-report-unused-inline-configs/README.md new file mode 100644 index 00000000..49c21faa --- /dev/null +++ b/designs/2024-report-unused-inline-configs/README.md @@ -0,0 +1,244 @@ +- Repo: eslint/eslint +- Start Date: 2024-07-08 +- RFC PR: +- 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 + +```shell +npx eslint --report-unused-inline-configs error +``` + +```js +{ + linterOptions: { + reportUnusedDisableDirectives: "error", + } +} +``` + +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 +} +``` + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
Original Config SettingInline Config SettingsReport?
+ "error" +
+ 2 +
+ ["error", { "enforceForClassMembers": true }] +
+ ["error", { "getWithoutSet": false }] + 		+ "off" +
+ 0 + 		No
+ "warn" +
+ 1 + 		No
+ "error" +
+ 2 + 		Yes
+ ["error", { "enforceForClassMembers": false }] +
+ [2, { "enforceForClassMembers": false }] + 		No
+ ["error", { "getWithoutSet": true }] +
+ [2, { "getWithoutSet": true }] + 		No
+ ["error", { "enforceForClassMembers": true }] +
+ [2, { "enforceForClassMembers": true }] + 		Yes
+ ["error", { "getWithoutSet": false }] +
+ [2, { "getWithoutSet": false }] + 		Yes
+ +## 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` + +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. +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 + +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. + +### 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. +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. + +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. + +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. + +This RFC's opinion is that the consistency of adding two new options is not worth the excess options logic. +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 + +- : the issue triggering this RFC + - : an older issue that #18230 duplicated +- : previous issue for enabling `reportUnusedDisableDirectives` config option by default + - : the RFC discussion flexible config + default reporting of unused disable directives + - : the PR implementing custom severity when reporting unused disable directives +- : issue suggesting erroring by default for unused disable directives +- : issue suggesting merging `--report-unused-disable-directives-severity` into `--report-unused-disable-directives`