From 3b4181a36d21aad83c2558416a711f7b118395eb Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Mon, 18 Sep 2023 12:58:33 -0400 Subject: [PATCH 01/12] feat: Rule Options Defaults --- designs/2023-rule-options-defaults/README.md | 282 +++++++++++++++++++ 1 file changed, 282 insertions(+) create mode 100644 designs/2023-rule-options-defaults/README.md diff --git a/designs/2023-rule-options-defaults/README.md b/designs/2023-rule-options-defaults/README.md new file mode 100644 index 00000000..f12839ce --- /dev/null +++ b/designs/2023-rule-options-defaults/README.md @@ -0,0 +1,282 @@ +- Repo: eslint/eslint +- Start Date: 2023-09-18 +- RFC PR: +- Authors: [Josh Goldberg](https://github.com/JoshuaKGoldberg) + +# Support for including options defaults in rules + +## Summary + +Enable rule authors to describe default values for rule options in a formal schema. + +## Motivation + +Right now, there's no programmatic way to determine what an ESLint rule's default options are. +This has resulted in a proliferation of per-repository or per-rule strategies for normalizing options with defaults. + +- Some rules define functions with names like `normalizeOptions` ([`array-bracket-newline`](https://github.com/eslint/eslint/blob/da09f4e641141f585ef611c6e9d63d4331054706/lib/rules/array-bracket-newline.js#L102), [`object-curly-newline`](https://github.com/eslint/eslint/blob/da09f4e641141f585ef611c6e9d63d4331054706/lib/rules/object-curly-newline.js#L99)) or `parseOptions` ([`no-implicit-coercion`](https://github.com/eslint/eslint/blob/da09f4e641141f585ef611c6e9d63d4331054706/lib/rules/no-implicit-coercion.js#L22), [`use-before-define`](https://github.com/eslint/eslint/blob/da09f4e641141f585ef611c6e9d63d4331054706/lib/rules/no-use-before-define.js#L20)) that apply various, subtly different value defaults to the provided `context.options`. +- Some rules define individual option purely with inline logic ([`accessor-pairs`](https://github.com/eslint/eslint/blob/da09f4e641141f585ef611c6e9d63d4331054706/lib/rules/accessor-pairs.js#L177-L180), [`n/file-extension-in-import`](https://github.com/eslint-community/eslint-plugin-n/blob/150b34fa60287b088fc51cf754ff716e4862883c/lib/rules/file-extension-in-import.js#L67-L68)) or inline with a helper ([`array-bracket-spacing`](https://github.com/eslint/eslint/blob/da09f4e641141f585ef611c6e9d63d4331054706/lib/rules/array-bracket-spacing.js#L65-L74)) +- `@typescript-eslint/eslint-plugin` has its own [`applyDefault`](https://github.com/typescript-eslint/typescript-eslint/blob/da09f4e641141f585ef611c6e9d63d4331054706/packages/utils/src/eslint-utils/applyDefault.ts#L10) used in a wrapping [`createRule`](https://github.com/typescript-eslint/typescript-eslint/blob/da09f4e641141f585ef611c6e9d63d4331054706/packages/utils/src/eslint-utils/RuleCreator.ts#L84) + +In addition to increasing development complexity for rules, the lack of a standard options parsing strategy means users don't have a consistent or programmatic way to determine rule defaults. +The only user-facing method to determine a rule's default options is to read the documentation, or if it either doesn't exist or doesn't describe default values, read the rule's source. +Even if a plugin does have documentation, there is no guaranteed consistency in phrasing. + +For example: + +- [`array-callback-return`](https://eslint.org/docs/latest/rules/array-callback-return#options) phrases its options as _`"": (default) `_ +- [`accessor-pairs`](https://eslint.org/docs/latest/rules/accessor-pairs#options) phrases its options as _` (Default )`_ + +This RFC proposes allowing rules to opt into declaring the default values of their options. +Doing so would: + +- Streamline the process of creating and maintaining rules that take in options +- Standardize how both developers and end-users can reason about default rule options +- Allow tooling such as [`eslint-doc-generator`](https://github.com/bmish/eslint-doc-generator) to describe rule options programmatically - and therefore more consistently + +This RFC proposes defaulting options passed to rules: + +- Schemas that include `default` values will cause the `context.options` provided to rules to use that `default` if a value is `undefined` +- Options will always recursively default to at least an empty `{}`, to make behavior of nested object definitions with or without `default` properties consist +- The original, non-defaulted options object will be available as `context.optionsRaw` to reduce code churn in rules that currently rely on original (raw) values + +Doing so would be a breaking change as it impacts what values are passed to the `create` method of rules. + +## Detailed Design + +This RFC proposes using the already-existing optional `default` field for each property in a rule's `meta.schema` when resolving what options values are provided to rules. +For each config value defined in `meta.schema`: + +- If `default` is defined and the rule config's value is `undefined`, the value of `default` is used instead +- If the config value contains `type: "object"`, each property's `default` will recursively go through this same defaulting logic + - If a rule config has not provided a value for the object, a new empty object will be created + +Objects created by this defaulting logic should always be shallow cloned (`{ ...original }`) before being passed to rules. + +The original `options` provided to rules will be available on a new `context.optionsRaw` property. +That property will be equivalent to what current versions of ESLint provide as `context.options`. + +### Example: Primitive Value Default + +Given a rule schema that defines a default value: + +```json5 +{ + default: "implicit", + type: "string", +} +``` + +The proposed change would impact only cases when the provided value is `undefined`: + +| Rule Config | Current `context.options` | Proposed `context.options` | +| ----------------------- | ------------------------- | -------------------------- | +| `"error"` | `[]` | `["implicit"]` | +| `["error"]` | `[]` | `["implicit"]` | +| `["error", undefined]` | `[undefined]` | `["implicit"]` | +| `["error", null]` | `[null]` | `[null]` | +| `["error", "explicit"]` | `["explicit"]` | `["explicit"]` | + +### Example: Nested Object Defaults + +Using [`class-methods-use-this`](https://github.com/eslint/eslint/blob/da09f4e641141f585ef611c6e9d63d4331054706/lib/rules/class-methods-use-this.js)'s schema as an example: + +```json5 +[ + { + type: "object", + properties: { + exceptMethods: { + type: "array", + items: { + type: "string", + }, + }, + enforceForClassFields: { + type: "boolean", + default: true, + }, + }, + additionalProperties: false, + }, +] +``` + +- The rule's options parsing logic will no longer need to manually default to `{}`: + + ```diff + - const config = Object.assign({}, context.options[0]); + + const config = context.options[0]; + ``` + +- Options default checks, such as `!==` against `default: true` properties, will no longer be necessary: + + ```diff + - const enforceForClassFields = config.enforceForClassFields !== false; + + const enforceForClassFields = config.enforceForClassFields; + ``` + +- Options whose provided values don't provide a default don't change behavior: + + ```diff + const exceptMethods = new Set(config.exceptMethods || []); + ``` + +The proposed behavior would impact cases when the provided value is `undefined` or is an object without an explicit value for `enforceForClassFields`: + +| Rule Config | Current `context.options` | Proposed `context.options` | +| --------------------------------------------- | ------------------------------------ | -------------------------------------- | +| `"error"` | `[]` | `[ { enforceForClassFields: true } ]` | +| `["error"]` | `[]` | `[ { enforceForClassFields: true } ]` | +| `["error", undefined]` | `[undefined]` | `[ { enforceForClassFields: true } ]` | +| `["error", null]` | `[null]` | `[null]` | +| `["error", {}]` | `[{}]` | `[ { enforceForClassFields: true } ]` | +| `["error", { enforceForClassFields: false }]` | `[{ enforceForClassFields: false }]` | `[ { enforceForClassFields: false } ]` | +| `["error", { enforceForClassFields: true }]` | `[{ enforceForClassFields: true }]` | `[ { enforceForClassFields: true } ]` | + +### Implementation + +Currently, ESLint rule options are passed through a [`getRuleOptions` function](https://github.com/eslint/eslint/blob/da09f4e641141f585ef611c6e9d63d4331054706/lib/linter/linter.js#L740) whose only processing is to remove severity. +That function could be augmented to also take in the `rule.meta.schema`. +Its "worker" logic could recursively parse the rule config and schema: + +1. If the rule config is `null`, return that directly +2. If the schema is an array: + 1. Recurse on the range of elements from 0 to the smaller of the rule config and schema's length + 2. If there are remaining rule config values (so, there were more rule configs than schema descriptions), add them directly to the resolved options + 3. If there are remaining schema (so, there were more schema descriptions than rule configs), fill in any options values for schemas containining either `default` or `type: "object"` +3. Create an options object that is a shallow clone of the first non-`undefined` from `ruleConfig` or `schema.default`, or `{}` if the schema contains `type: "object"` +4. If the options is truthy and schema is an object: + 1. Recursively apply defaulting logic for each of the schema's `properties` + +Draft PR: [[Reference] feat!: factor in schema defaults for rule options](https://github.com/eslint/eslint/pull/17580) + +### Out of Scope + +#### Default `anyOf` Values + +JSON schemas can use `anyOf` to indicate values whose types are analogous to TypeScript union types. +Resolving which `anyOf` constituent a runtime config aligns to can be complex. +It is out of scope for this RFC to investigate heuristics for that problem. + +#### TypeScript Types + +Right now, there are two community strategies for declaring rule options types, both with drawbacks: + +- [`@types/eslint`](https://npmjs.com/package/@types/eslint) (the community-authored package describing ESLint's types for TypeScript users) [declares `RuleContext`'s `options` as `any[]`](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/ca231578e79ab1c2f01a308615e60a432061c97a/types/eslint/index.d.ts#L703), indicating it can be an array of any value types. + - Drawback: `any` is generally considered a bad practice for TypeScript users as it provides no type descriptions or safety checks. +- [`@typescript-eslint/utils` > `RuleCreator`](https://typescript-eslint.io/developers/custom-rules#rulecreator) (the recommended TypeScript-enabled rule development utility) declares a [generic `RuleContext` type with an `options: TOptions` property](https://github.com/typescript-eslint/typescript-eslint/blob/72149e1fe8f57fd217dfc59295a51df68187aad4/packages/utils/src/ts-eslint/Rule.ts#L175). + - Drawback: `RuleContext` already doesn't factor in default options to the type system (https://github.com/typescript-eslint/typescript-eslint/issues/5439) + +Resolving the type system type resulting from a fixed JSON schema -with or without `default`- and a user-provided object can be complex. +It is out of scope for this RFC to investigate heuristics for that problem. + +## Documentation + +We'll want to augment the following docs generators in: + +- The core ESLint docs: automating and therefore standardizing how rule docs pages phrase default values +- typescript-eslint's docs: the same as the core ESLint docs +- []`eslint-doc-generator`](https://github.com/bmish/eslint-doc-generator) + +## Drawbacks + +- This increases both conceptual and implementation complexity around rule options +- As a breaking change, this may break user rules - especially private rules we don't have visibility to + +## Backwards Compatibility Analysis + +This proposal contains two breaking changes: + +- If a schema declares `default` values, rule options that were `undefined` will become those defaults instead +- Options will recursively default to `{}` based on schemas even if the schema has no `default` properties + +Those breaking changes will impact rules that rely on the exact length or existing of properties in `context.options`. +Rules that wish to preserve the legacy options behavior can switch to referring to the new `context.optionsRaw` property. + +In the draft PR containing this change, only 3 test files -rule tests for `indent`, `max-len`, and `no-multiple-empty-lines`- failed during `npm run test`. +All were fixed with a direct find-and-replace from `context.options` to `context.optionsRaw` in those rules' implementations. + +## Alternatives + +### Non-Recursive Object Defaults + +An alternate, lower-impact change would be to _not_ options to `{}` if there is no `default` in its schema. +However: + +- Defaulting to `{}` allows rules logic to not need to `|| {}` or `?? {}` as much in source code. +- Consider the case of an options schema declaring a `default` for a child property inside an object whose parent does not have a `default` introduces complexity: + - If the defaulting logic didn't recursively create default objects, it could be confusing why the `default` value wasn't being used. + - If the defaulting logic were to recursively create default objects only when a descendent property contains `default`, the inconsistency could also be confusing to users (and would be more code to implement in ESLint). + +This RFC proposes always defaulting objects to `{}` for consistency. + +### Prior Art + +The only standardized options application I could find was `@typescript-eslint/eslint-plugin`'s `defaultOptions` property on rules. +It's visible in the plugin's [Custom Rules guide](https://typescript-eslint.io/developers/custom-rules#rulecreator), which suggests users wrap rule creation with a `createRule`: + +```ts +import { ESLintUtils } from "@typescript-eslint/utils"; + +const createRule = ESLintUtils.RuleCreator( + (name) => `https://example.com/rule/${name}` +); + +export const rule = createRule({ + create(context) { + /* ... */ + }, + defaultOptions: [ + /* ... */ + ], + meta: { + /* ... */ + }, + name: "...", +}); +``` + +`createRule` applies a _deep_ merge rather than a shallow `Object.assign`: https://github.com/typescript-eslint/typescript-eslint/blob/324c4d31e46cbc95e8eb67d71792de9f49e65606/packages/utils/src/eslint-utils/applyDefault.ts. +This has the benefit of allowing user-provided options to "merge" into complex objects, rather than requiring rewriting larger objects. + +However, deep merges are a more nuanced behavior, and may surprise users who assume the more common shallow merging or a different strategy in edge cases such as arrays. +This proposal goes with a less opinionated _shallow_ merge. + +## Open Questions + +1. Does the developer benefit of nested object defaulting work well enough to justify the added complexity? +2. Would we want to eventually deprecate/delete `context.optionsRaw`? Alternately, should we avoid it altogether, in favor of rewriting the options logic in the core rules? +3. Should ESLint eventually write its own schema parsing package? +4. Should figuring out TypeScript types block this proposal? + +## Help Needed + +I expect to implement this change. + +I plan to file issues on popular community plugins suggesting using it and offering to implement. + +## Frequently Asked Questions + +### Does this change how we write rules? + +Not significantly. +It should be a little less code to handle optional options. + +### How does this impact typescript-eslint's `RuleCreator` and `defaultOptions`? + +We can roll out this change over the current and next two major versions of typescript-eslint: + +1. Current major version (v6): no code changes; mention ESLint's new options defaulting in documentation +2. Next major version (v7): mark `defaultOptions` as deprecated +3. Subsequent major version (v8): remove `defaultOptions` + +This RFC should ideally at least have participation from typescript-eslint maintainers besides its author. + +Note that [typescript-eslint's Building Custom Rules documentation](https://typescript-eslint.io/developers/custom-rules) only shows an empty `defaultOptions` in code snippets. +It never explains the presence of `defaultOptions` or suggests using it. + +## Related Discussions + +- : the issue triggering this RFC +- : typescript-eslint forking types for `JSONSchemaV4` From 84aa4870502137878b878d6021d51abff4a088f7 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Mon, 18 Sep 2023 13:01:47 -0400 Subject: [PATCH 02/12] Added PR link --- designs/2023-rule-options-defaults/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/designs/2023-rule-options-defaults/README.md b/designs/2023-rule-options-defaults/README.md index f12839ce..10b7d0b5 100644 --- a/designs/2023-rule-options-defaults/README.md +++ b/designs/2023-rule-options-defaults/README.md @@ -1,6 +1,6 @@ - Repo: eslint/eslint - Start Date: 2023-09-18 -- RFC PR: +- RFC PR: https://github.com/eslint/rfcs/pull/113 - Authors: [Josh Goldberg](https://github.com/JoshuaKGoldberg) # Support for including options defaults in rules From b3ce30790ac315df3b2aae04016d9a0c6d99364d Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Mon, 18 Sep 2023 13:04:00 -0400 Subject: [PATCH 03/12] Fill out remaining Documentation section bits --- designs/2023-rule-options-defaults/README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/designs/2023-rule-options-defaults/README.md b/designs/2023-rule-options-defaults/README.md index 10b7d0b5..4f5269ec 100644 --- a/designs/2023-rule-options-defaults/README.md +++ b/designs/2023-rule-options-defaults/README.md @@ -177,7 +177,9 @@ We'll want to augment the following docs generators in: - The core ESLint docs: automating and therefore standardizing how rule docs pages phrase default values - typescript-eslint's docs: the same as the core ESLint docs -- []`eslint-doc-generator`](https://github.com/bmish/eslint-doc-generator) +- [`eslint-doc-generator`](https://github.com/bmish/eslint-doc-generator) + +We'll also want to mention this in the ESLint core custom rule documentation. ## Drawbacks From 238677748c9bb1635d6e94a79ccae1ce77736efe Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Wed, 20 Sep 2023 03:08:48 -0400 Subject: [PATCH 04/12] Mentioned Ajv's defaults and removed invalid example cases --- designs/2023-rule-options-defaults/README.md | 63 ++++++++++---------- 1 file changed, 33 insertions(+), 30 deletions(-) diff --git a/designs/2023-rule-options-defaults/README.md b/designs/2023-rule-options-defaults/README.md index 4f5269ec..dfc51a3c 100644 --- a/designs/2023-rule-options-defaults/README.md +++ b/designs/2023-rule-options-defaults/README.md @@ -11,14 +11,17 @@ Enable rule authors to describe default values for rule options in a formal sche ## Motivation -Right now, there's no programmatic way to determine what an ESLint rule's default options are. +Right now, most popular ESLint rules do not adhere to a single programmatic way to determine their default options. This has resulted in a proliferation of per-repository or per-rule strategies for normalizing options with defaults. - Some rules define functions with names like `normalizeOptions` ([`array-bracket-newline`](https://github.com/eslint/eslint/blob/da09f4e641141f585ef611c6e9d63d4331054706/lib/rules/array-bracket-newline.js#L102), [`object-curly-newline`](https://github.com/eslint/eslint/blob/da09f4e641141f585ef611c6e9d63d4331054706/lib/rules/object-curly-newline.js#L99)) or `parseOptions` ([`no-implicit-coercion`](https://github.com/eslint/eslint/blob/da09f4e641141f585ef611c6e9d63d4331054706/lib/rules/no-implicit-coercion.js#L22), [`use-before-define`](https://github.com/eslint/eslint/blob/da09f4e641141f585ef611c6e9d63d4331054706/lib/rules/no-use-before-define.js#L20)) that apply various, subtly different value defaults to the provided `context.options`. - Some rules define individual option purely with inline logic ([`accessor-pairs`](https://github.com/eslint/eslint/blob/da09f4e641141f585ef611c6e9d63d4331054706/lib/rules/accessor-pairs.js#L177-L180), [`n/file-extension-in-import`](https://github.com/eslint-community/eslint-plugin-n/blob/150b34fa60287b088fc51cf754ff716e4862883c/lib/rules/file-extension-in-import.js#L67-L68)) or inline with a helper ([`array-bracket-spacing`](https://github.com/eslint/eslint/blob/da09f4e641141f585ef611c6e9d63d4331054706/lib/rules/array-bracket-spacing.js#L65-L74)) - `@typescript-eslint/eslint-plugin` has its own [`applyDefault`](https://github.com/typescript-eslint/typescript-eslint/blob/da09f4e641141f585ef611c6e9d63d4331054706/packages/utils/src/eslint-utils/applyDefault.ts#L10) used in a wrapping [`createRule`](https://github.com/typescript-eslint/typescript-eslint/blob/da09f4e641141f585ef611c6e9d63d4331054706/packages/utils/src/eslint-utils/RuleCreator.ts#L84) -In addition to increasing development complexity for rules, the lack of a standard options parsing strategy means users don't have a consistent or programmatic way to determine rule defaults. +Although the currently used Ajv package does provide a [`useDefaults` option](https://ajv.js.org/guide/modifying-data.html#assigning-defaults), rule developers typically have not used it. +Ajv does not recursively fill in objects for rule defaults - an ergonomics issue for rule authors. + +In addition to increasing development complexity for rules, the lack of ergonomic options defaulting means end-users haven't had a consistent or programmatic way to determine rule defaults. The only user-facing method to determine a rule's default options is to read the documentation, or if it either doesn't exist or doesn't describe default values, read the rule's source. Even if a plugin does have documentation, there is no guaranteed consistency in phrasing. @@ -27,25 +30,24 @@ For example: - [`array-callback-return`](https://eslint.org/docs/latest/rules/array-callback-return#options) phrases its options as _`"": (default) `_ - [`accessor-pairs`](https://eslint.org/docs/latest/rules/accessor-pairs#options) phrases its options as _` (Default )`_ -This RFC proposes allowing rules to opt into declaring the default values of their options. +This RFC proposes augmenting Ajv's defaulting logic to recursively fill in objects. Doing so would: - Streamline the process of creating and maintaining rules that take in options - Standardize how both developers and end-users can reason about default rule options -- Allow tooling such as [`eslint-doc-generator`](https://github.com/bmish/eslint-doc-generator) to describe rule options programmatically - and therefore more consistently - -This RFC proposes defaulting options passed to rules: - -- Schemas that include `default` values will cause the `context.options` provided to rules to use that `default` if a value is `undefined` -- Options will always recursively default to at least an empty `{}`, to make behavior of nested object definitions with or without `default` properties consist -- The original, non-defaulted options object will be available as `context.optionsRaw` to reduce code churn in rules that currently rely on original (raw) values +- Encourage writing rules that allow tooling such as [`eslint-doc-generator`](https://github.com/bmish/eslint-doc-generator) to describe rule options programmatically - and therefore more consistently Doing so would be a breaking change as it impacts what values are passed to the `create` method of rules. ## Detailed Design -This RFC proposes using the already-existing optional `default` field for each property in a rule's `meta.schema` when resolving what options values are provided to rules. -For each config value defined in `meta.schema`: +This RFC proposes defaulting options passed to rules: + +- Schemas that include `default` values will continue to allow those default values using Ajv's `useDefaults` +- Object options will always recursively default to at least an empty `{}`, to make behavior of nested object definitions with or without `default` properties consist +- The options object created by Ajv's parsing without any recursive object creation will be available as `context.optionsRaw` to reduce code churn in rules that currently rely on original (raw) values + +Specifically, for each config value defined in `meta.schema`: - If `default` is defined and the rule config's value is `undefined`, the value of `default` is used instead - If the config value contains `type: "object"`, each property's `default` will recursively go through this same defaulting logic @@ -73,8 +75,6 @@ The proposed change would impact only cases when the provided value is `undefine | ----------------------- | ------------------------- | -------------------------- | | `"error"` | `[]` | `["implicit"]` | | `["error"]` | `[]` | `["implicit"]` | -| `["error", undefined]` | `[undefined]` | `["implicit"]` | -| `["error", null]` | `[null]` | `[null]` | | `["error", "explicit"]` | `["explicit"]` | `["explicit"]` | ### Example: Nested Object Defaults @@ -128,9 +128,7 @@ The proposed behavior would impact cases when the provided value is `undefined` | --------------------------------------------- | ------------------------------------ | -------------------------------------- | | `"error"` | `[]` | `[ { enforceForClassFields: true } ]` | | `["error"]` | `[]` | `[ { enforceForClassFields: true } ]` | -| `["error", undefined]` | `[undefined]` | `[ { enforceForClassFields: true } ]` | -| `["error", null]` | `[null]` | `[null]` | -| `["error", {}]` | `[{}]` | `[ { enforceForClassFields: true } ]` | +| `["error", {}]` | `[{ enforceForClassFields: true }]` | `[ { enforceForClassFields: true } ]` | | `["error", { enforceForClassFields: false }]` | `[{ enforceForClassFields: false }]` | `[ { enforceForClassFields: false } ]` | | `["error", { enforceForClassFields: true }]` | `[{ enforceForClassFields: true }]` | `[ { enforceForClassFields: true } ]` | @@ -188,12 +186,8 @@ We'll also want to mention this in the ESLint core custom rule documentation. ## Backwards Compatibility Analysis -This proposal contains two breaking changes: - -- If a schema declares `default` values, rule options that were `undefined` will become those defaults instead -- Options will recursively default to `{}` based on schemas even if the schema has no `default` properties - -Those breaking changes will impact rules that rely on the exact length or existing of properties in `context.options`. +Options will recursively default to `{}` based on schemas even if the schema has no `default` properties +That breaking change will impact rules which rely on the exact length or existing of properties in `context.options`. Rules that wish to preserve the legacy options behavior can switch to referring to the new `context.optionsRaw` property. In the draft PR containing this change, only 3 test files -rule tests for `indent`, `max-len`, and `no-multiple-empty-lines`- failed during `npm run test`. @@ -203,15 +197,24 @@ All were fixed with a direct find-and-replace from `context.options` to `context ### Non-Recursive Object Defaults -An alternate, lower-impact change would be to _not_ options to `{}` if there is no `default` in its schema. -However: +Ajv provides its own support for `default` values with a [`useDefaults` option](https://ajv.js.org/guide/modifying-data.html#assigning-defaults). +It's used in ESLint core already, in [`lib/shared/ajv.js`](https://github.com/eslint/eslint/blob/22a558228ff98f478fa308c9ecde361acc4caf20/lib/shared/ajv.js#L21). +Sticking Ajv's own defaults logic would bring two benefits: + +- Aligning ESLint schema defaulting with the standard logic used by Ajv +- Avoiding a custom defaulting implementation in ESLint by using Ajv's instead + +However, [`useDefaults` requires explicit empty object for intermediate object entries to work for nested properties](https://github.com/ajv-validator/ajv/issues/1710). +That means: + +- Schemas with optional objects containing properties with default need to provide an explicit `default: {}` for those objects. +- If those properties are themselves optional objects containing properties, both the outer and inner objects need explicit `default` values. -- Defaulting to `{}` allows rules logic to not need to `|| {}` or `?? {}` as much in source code. -- Consider the case of an options schema declaring a `default` for a child property inside an object whose parent does not have a `default` introduces complexity: - - If the defaulting logic didn't recursively create default objects, it could be confusing why the `default` value wasn't being used. - - If the defaulting logic were to recursively create default objects only when a descendent property contains `default`, the inconsistency could also be confusing to users (and would be more code to implement in ESLint). +An alternative to this RFC could be to fill in those default values in ESLint core rules and plugins. +See [this Runkit with simulated Ajv objects](https://runkit.com/joshuakgoldberg/650a826da5839400082514ff) for examples of how rules would need to provide those default values. -This RFC proposes always defaulting objects to `{}` for consistency. +This RFC attempts to optimize for common cases of how rule authors would write ESLint rules. +As rules have typically omitted `default: {}`, the path of least inconvenience for rule authors seems to be to fill in those values for them recursively. ### Prior Art From e636adb43950f5324019a0e66394c4d9b1927750 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Wed, 20 Sep 2023 03:16:51 -0400 Subject: [PATCH 05/12] useRecursiveOptionDefaults --- designs/2023-rule-options-defaults/README.md | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/designs/2023-rule-options-defaults/README.md b/designs/2023-rule-options-defaults/README.md index dfc51a3c..1951720f 100644 --- a/designs/2023-rule-options-defaults/README.md +++ b/designs/2023-rule-options-defaults/README.md @@ -37,7 +37,7 @@ Doing so would: - Standardize how both developers and end-users can reason about default rule options - Encourage writing rules that allow tooling such as [`eslint-doc-generator`](https://github.com/bmish/eslint-doc-generator) to describe rule options programmatically - and therefore more consistently -Doing so would be a breaking change as it impacts what values are passed to the `create` method of rules. +A new `useRecursiveOptionDefaults` option would be made available to ESLint plugins to let them opt into the behavior. ## Detailed Design @@ -182,11 +182,18 @@ We'll also want to mention this in the ESLint core custom rule documentation. ## Drawbacks - This increases both conceptual and implementation complexity around rule options -- As a breaking change, this may break user rules - especially private rules we don't have visibility to +- As a future breaking change, this may break user rules - especially private rules we don't have visibility to ## Backwards Compatibility Analysis -Options will recursively default to `{}` based on schemas even if the schema has no `default` properties +The new `useRecursiveOptionDefaults` option being opt-in means this would not be a breaking change in the current version of ESLint. +This RFC's intent is for that option to gradually be removed over time: + +1. ESLint v8: opt-in (default to `false`) +2. ESLint v9: opt-out (default to `true`) +3. ESLint v10: remove both `useRecursiveOptionDefaults` and `context.optionsRaw` + +Changing `useRecursiveOptionDefaults` to opt-out would be a breaking change as it impacts what values are passed to the `create` method of rules. That breaking change will impact rules which rely on the exact length or existing of properties in `context.options`. Rules that wish to preserve the legacy options behavior can switch to referring to the new `context.optionsRaw` property. From 638e05b6b616397ca2a3c6b2f2b51265c97d3b95 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Wed, 20 Sep 2023 03:20:23 -0400 Subject: [PATCH 06/12] Adjust title --- designs/2023-rule-options-defaults/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/designs/2023-rule-options-defaults/README.md b/designs/2023-rule-options-defaults/README.md index 1951720f..97680d6f 100644 --- a/designs/2023-rule-options-defaults/README.md +++ b/designs/2023-rule-options-defaults/README.md @@ -3,7 +3,7 @@ - RFC PR: https://github.com/eslint/rfcs/pull/113 - Authors: [Josh Goldberg](https://github.com/JoshuaKGoldberg) -# Support for including options defaults in rules +# Support for recursively filling in options defaults in rules ## Summary From 753f82c22d308efde8321a4015391799ba36caeb Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Wed, 20 Sep 2023 03:20:59 -0400 Subject: [PATCH 07/12] Adjust description too --- designs/2023-rule-options-defaults/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/designs/2023-rule-options-defaults/README.md b/designs/2023-rule-options-defaults/README.md index 97680d6f..e7c8d138 100644 --- a/designs/2023-rule-options-defaults/README.md +++ b/designs/2023-rule-options-defaults/README.md @@ -7,7 +7,7 @@ ## Summary -Enable rule authors to describe default values for rule options in a formal schema. +Enable rule options defaulting logic to recursively fill in objects and their default properties. ## Motivation From 291ea5ebb4c1a6c1411f8d60622647aa0ec42688 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Wed, 20 Sep 2023 03:25:34 -0400 Subject: [PATCH 08/12] Small touchups --- designs/2023-rule-options-defaults/README.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/designs/2023-rule-options-defaults/README.md b/designs/2023-rule-options-defaults/README.md index e7c8d138..06452c2d 100644 --- a/designs/2023-rule-options-defaults/README.md +++ b/designs/2023-rule-options-defaults/README.md @@ -43,8 +43,8 @@ A new `useRecursiveOptionDefaults` option would be made available to ESLint plug This RFC proposes defaulting options passed to rules: -- Schemas that include `default` values will continue to allow those default values using Ajv's `useDefaults` -- Object options will always recursively default to at least an empty `{}`, to make behavior of nested object definitions with or without `default` properties consist +- Schemas that include `default` values will continue to allow those default values similarly to Ajv's `useDefaults` +- Object options will always recursively default to at least an empty `{}`, to make behavior of nested object definitions with or without `default` properties consistent - The options object created by Ajv's parsing without any recursive object creation will be available as `context.optionsRaw` to reduce code churn in rules that currently rely on original (raw) values Specifically, for each config value defined in `meta.schema`: @@ -261,6 +261,7 @@ This proposal goes with a less opinionated _shallow_ merge. 2. Would we want to eventually deprecate/delete `context.optionsRaw`? Alternately, should we avoid it altogether, in favor of rewriting the options logic in the core rules? 3. Should ESLint eventually write its own schema parsing package? 4. Should figuring out TypeScript types block this proposal? +5. The three core rules that would want `optionsRaw` -indent, max-len, and no-multiple-empty-lines- are all formatting rules. Is it safe to assume they'll be removed by ESLint 10 (https://github.com/eslint/eslint/issues/17522)? ## Help Needed From dca6c9f81b90d77328441ce6bad2982b2533b4fc Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Mon, 16 Oct 2023 12:53:02 -0400 Subject: [PATCH 09/12] Switched to meta.defaultOptions approach --- designs/2023-rule-options-defaults/README.md | 279 +++++++++---------- 1 file changed, 139 insertions(+), 140 deletions(-) diff --git a/designs/2023-rule-options-defaults/README.md b/designs/2023-rule-options-defaults/README.md index 06452c2d..dbff608b 100644 --- a/designs/2023-rule-options-defaults/README.md +++ b/designs/2023-rule-options-defaults/README.md @@ -3,11 +3,11 @@ - RFC PR: https://github.com/eslint/rfcs/pull/113 - Authors: [Josh Goldberg](https://github.com/JoshuaKGoldberg) -# Support for recursively filling in options defaults in rules +# Support for `meta.defaultOptions` on rules ## Summary -Enable rule options defaulting logic to recursively fill in objects and their default properties. +Enable rules to provide default options programmatically with a new `meta.defaultOptions` property. ## Motivation @@ -30,133 +30,89 @@ For example: - [`array-callback-return`](https://eslint.org/docs/latest/rules/array-callback-return#options) phrases its options as _`"": (default) `_ - [`accessor-pairs`](https://eslint.org/docs/latest/rules/accessor-pairs#options) phrases its options as _` (Default )`_ -This RFC proposes augmenting Ajv's defaulting logic to recursively fill in objects. +This RFC proposes adding a `defaultOptions` property to rules' `meta`. Doing so would: - Streamline the process of creating and maintaining rules that take in options - Standardize how both developers and end-users can reason about default rule options - Encourage writing rules that allow tooling such as [`eslint-doc-generator`](https://github.com/bmish/eslint-doc-generator) to describe rule options programmatically - and therefore more consistently -A new `useRecursiveOptionDefaults` option would be made available to ESLint plugins to let them opt into the behavior. +Options provided to rules would be the result of a deep (recursive) clone and merge of the rule's default options and user-provided options. ## Detailed Design -This RFC proposes defaulting options passed to rules: - -- Schemas that include `default` values will continue to allow those default values similarly to Ajv's `useDefaults` -- Object options will always recursively default to at least an empty `{}`, to make behavior of nested object definitions with or without `default` properties consistent -- The options object created by Ajv's parsing without any recursive object creation will be available as `context.optionsRaw` to reduce code churn in rules that currently rely on original (raw) values - -Specifically, for each config value defined in `meta.schema`: - -- If `default` is defined and the rule config's value is `undefined`, the value of `default` is used instead -- If the config value contains `type: "object"`, each property's `default` will recursively go through this same defaulting logic - - If a rule config has not provided a value for the object, a new empty object will be created - -Objects created by this defaulting logic should always be shallow cloned (`{ ...original }`) before being passed to rules. - -The original `options` provided to rules will be available on a new `context.optionsRaw` property. -That property will be equivalent to what current versions of ESLint provide as `context.options`. +Currently, ESLint rule options are passed through a [`getRuleOptions` function](https://github.com/eslint/eslint/blob/da09f4e641141f585ef611c6e9d63d4331054706/lib/linter/linter.js#L740) whose only processing is to remove severity. +That function could be augmented to also take in the `rule.meta.defaultOptions`, if it exists. -### Example: Primitive Value Default +Defaulting logic could go with the following logic to approximate user intent: -Given a rule schema that defines a default value: +1. If the user-provided value is `undefined`, go with the default option +2. If the user-provided value is an object, a new `{}` object is created with values recursively merged +3. Arrays and non-`undefined` literals -including `null`- are used as-is -```json5 -{ - default: "implicit", - type: "string", -} -``` +See [[Reference] feat: add meta.defaultOptions](https://github.com/eslint/eslint/pull/17656) > [`function deepMerge`](https://github.com/eslint/eslint/pull/17656/files#diff-9bbcc1ca9625b99554a055ac028190e5bd28432207a7f1a519690c5d0484287fR24) and [`describe("deepMerge")`](https://github.com/eslint/eslint/pull/17656/files#diff-fa70d7d4c142a6c3e727cf7280f981cb80a1db10bd6cc85c205e47e47da05cfeR25) for the proposed defaulting logic. -The proposed change would impact only cases when the provided value is `undefined`: - -| Rule Config | Current `context.options` | Proposed `context.options` | -| ----------------------- | ------------------------- | -------------------------- | -| `"error"` | `[]` | `["implicit"]` | -| `["error"]` | `[]` | `["implicit"]` | -| `["error", "explicit"]` | `["explicit"]` | `["explicit"]` | - -### Example: Nested Object Defaults - -Using [`class-methods-use-this`](https://github.com/eslint/eslint/blob/da09f4e641141f585ef611c6e9d63d4331054706/lib/rules/class-methods-use-this.js)'s schema as an example: - -```json5 -[ - { - type: "object", - properties: { - exceptMethods: { - type: "array", - items: { - type: "string", - }, - }, - enforceForClassFields: { - type: "boolean", - default: true, - }, - }, - additionalProperties: false, - }, -] -``` +The original `options` provided to rules will temporarily be available on a new `context.optionsRaw` property. +That property exists solely as a legacy compatibility layer for rules that would want the behavioral and/or documentation benefits of `meta.defaultOptions` but have nonstandard options parsing logic. +`context.optionsRaw` will be removed in the next major version of ESLint. -- The rule's options parsing logic will no longer need to manually default to `{}`: +### Rules with Differing Options Behavior - ```diff - - const config = Object.assign({}, context.options[0]); - + const config = context.options[0]; - ``` +The PoC PR implements `meta.defaultOptions` on rules that include options, are not deprecated, and match any of: -- Options default checks, such as `!==` against `default: true` properties, will no longer be necessary: +- Are mentioned earlier in this RFC +- Have a name starting with `A-C` and include the search string `context.options` +- Include the search string `Object.assign(` +- Experience test failures when Ajv's `useDefaults` is option is disabled (see [Schema Defaults](#schema-defaults)) - ```diff - - const enforceForClassFields = config.enforceForClassFields !== false; - + const enforceForClassFields = config.enforceForClassFields; - ``` +
+ +The following matched rules use context.optionsRaw because their current defaulting logic is incompatible with context.options + meta.defaultOptions. + -- Options whose provided values don't provide a default don't change behavior: +- `array-bracket-newline`: + - Presumed `defaultOptions: [{ minItems: null, multiline: true }]` + - When given `options: [{ minItems: null }]`: + - Current normalized options: `[ { minItems: null } ]`, with implicit `multiline: false` + - Updated normalized options: `[ { minItems: null, multiline: true } ]` +- `computed-property-spacing`: + - Presumed `defaultOptions: ["never"]` + - Note that `default: true` should also be removed from its `meta.schema`'s object `enforceForClassMembers` property + - When given `["always", {}]`: + - Current normalized `options.enforceForClassMembers`: `true` + - Updated normalized `options.enforceForClassMembers`: `undefined` +- `object-curly-newline`: + - Presumed `defaultOptions: [{ consistent: true }]` + - When given `options: [{ multiline: true, minProperties: 2 }]`: + - Current normalized `options.ObjectExpression`: `{ multiline: true, minProperties: 2, consistent: false` + - Updated normalized `options.ObjectExpression`: `{ multiline: true, minProperties: 2, consistent: true` +- `operator-linebreak`: + - Presumed `defaultOptions: ["after", { "overrides": { "?": "before", ":": "before" } }]` + - When given `options: ["none"]`: - Current normalized `styleOverrides`: `{}` - Updated normalized `styleOverrides`: `{ '?': 'before', ':': 'before' }` - ```diff - const exceptMethods = new Set(config.exceptMethods || []); - ``` +
-The proposed behavior would impact cases when the provided value is `undefined` or is an object without an explicit value for `enforceForClassFields`: +
+ +Additionally, the following rule has default logic that doesn't map well to meta.defaultOptions. + -| Rule Config | Current `context.options` | Proposed `context.options` | -| --------------------------------------------- | ------------------------------------ | -------------------------------------- | -| `"error"` | `[]` | `[ { enforceForClassFields: true } ]` | -| `["error"]` | `[]` | `[ { enforceForClassFields: true } ]` | -| `["error", {}]` | `[{ enforceForClassFields: true }]` | `[ { enforceForClassFields: true } ]` | -| `["error", { enforceForClassFields: false }]` | `[{ enforceForClassFields: false }]` | `[ { enforceForClassFields: false } ]` | -| `["error", { enforceForClassFields: true }]` | `[{ enforceForClassFields: true }]` | `[ { enforceForClassFields: true } ]` | +- `strict`: The mode depends on -and can be overridden based on- `ecmaFeatures.impliedStrict` -### Implementation +
-Currently, ESLint rule options are passed through a [`getRuleOptions` function](https://github.com/eslint/eslint/blob/da09f4e641141f585ef611c6e9d63d4331054706/lib/linter/linter.js#L740) whose only processing is to remove severity. -That function could be augmented to also take in the `rule.meta.schema`. -Its "worker" logic could recursively parse the rule config and schema: +### Schema Defaults -1. If the rule config is `null`, return that directly -2. If the schema is an array: - 1. Recurse on the range of elements from 0 to the smaller of the rule config and schema's length - 2. If there are remaining rule config values (so, there were more rule configs than schema descriptions), add them directly to the resolved options - 3. If there are remaining schema (so, there were more schema descriptions than rule configs), fill in any options values for schemas containining either `default` or `type: "object"` -3. Create an options object that is a shallow clone of the first non-`undefined` from `ruleConfig` or `schema.default`, or `{}` if the schema contains `type: "object"` -4. If the options is truthy and schema is an object: - 1. Recursively apply defaulting logic for each of the schema's `properties` +Ajv provides its own support for `default` values with a [`useDefaults` option](https://ajv.js.org/guide/modifying-data.html#assigning-defaults). +It's used in ESLint core already, in [`lib/shared/ajv.js`](https://github.com/eslint/eslint/blob/22a558228ff98f478fa308c9ecde361acc4caf20/lib/shared/ajv.js#L21). +Some core ESLint rules describe `default:` values in that schema. -Draft PR: [[Reference] feat!: factor in schema defaults for rule options](https://github.com/eslint/eslint/pull/17580) +Having two places to describe `default` values can be confusing for developers. +It can be unclear where a value comes from when its default may be specified in two separate locations. +This RFC proposes removing the `useDefaults: true` setting in the next major version of ESLint. ### Out of Scope -#### Default `anyOf` Values - -JSON schemas can use `anyOf` to indicate values whose types are analogous to TypeScript union types. -Resolving which `anyOf` constituent a runtime config aligns to can be complex. -It is out of scope for this RFC to investigate heuristics for that problem. - #### TypeScript Types Right now, there are two community strategies for declaring rule options types, both with drawbacks: @@ -164,10 +120,10 @@ Right now, there are two community strategies for declaring rule options types, - [`@types/eslint`](https://npmjs.com/package/@types/eslint) (the community-authored package describing ESLint's types for TypeScript users) [declares `RuleContext`'s `options` as `any[]`](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/ca231578e79ab1c2f01a308615e60a432061c97a/types/eslint/index.d.ts#L703), indicating it can be an array of any value types. - Drawback: `any` is generally considered a bad practice for TypeScript users as it provides no type descriptions or safety checks. - [`@typescript-eslint/utils` > `RuleCreator`](https://typescript-eslint.io/developers/custom-rules#rulecreator) (the recommended TypeScript-enabled rule development utility) declares a [generic `RuleContext` type with an `options: TOptions` property](https://github.com/typescript-eslint/typescript-eslint/blob/72149e1fe8f57fd217dfc59295a51df68187aad4/packages/utils/src/ts-eslint/Rule.ts#L175). - - Drawback: `RuleContext` already doesn't factor in default options to the type system (https://github.com/typescript-eslint/typescript-eslint/issues/5439) + - Drawback: `RuleContext` doesn't factor in root-level `defaultOptions` to the type system (https://github.com/typescript-eslint/typescript-eslint/issues/5439) -Resolving the type system type resulting from a fixed JSON schema -with or without `default`- and a user-provided object can be complex. -It is out of scope for this RFC to investigate heuristics for that problem. +This change is limited to the ESLint core repository, which doesn't provide TypeScript types. +It is out of scope for this RFC to apply `RuleCreator`-style types to `@types/node`, remove `default` from typescript-eslint's schema types, and/or resolve typescript-eslint's factoring default options issue. ## Documentation @@ -182,51 +138,93 @@ We'll also want to mention this in the ESLint core custom rule documentation. ## Drawbacks - This increases both conceptual and implementation complexity around rule options -- As a future breaking change, this may break user rules - especially private rules we don't have visibility to +- This explicitly moves away from the JSON Schema / ajv style description of putting `default` values in `meta.schema` ## Backwards Compatibility Analysis -The new `useRecursiveOptionDefaults` option being opt-in means this would not be a breaking change in the current version of ESLint. -This RFC's intent is for that option to gradually be removed over time: - -1. ESLint v8: opt-in (default to `false`) -2. ESLint v9: opt-out (default to `true`) -3. ESLint v10: remove both `useRecursiveOptionDefaults` and `context.optionsRaw` +If a rule does not specify `meta.defaultOptions`, its behavior will not change. +This change is backwards compatible. -Changing `useRecursiveOptionDefaults` to opt-out would be a breaking change as it impacts what values are passed to the `create` method of rules. -That breaking change will impact rules which rely on the exact length or existing of properties in `context.options`. -Rules that wish to preserve the legacy options behavior can switch to referring to the new `context.optionsRaw` property. - -In the draft PR containing this change, only 3 test files -rule tests for `indent`, `max-len`, and `no-multiple-empty-lines`- failed during `npm run test`. -All were fixed with a direct find-and-replace from `context.options` to `context.optionsRaw` in those rules' implementations. +The removals of `context.optionsRaw` and Ajv `useDefaults: true` will be breaking changes in the next major version. +This RFC believes the change will be minor and not create significant use pain. +Most ESLint core rules did not change behavior when switched to `meta.defaultOptions`. ## Alternatives +This RFC originally proposed including recursive object defaults in the `meta.schema`. +See [[Reference] feat!: factor in schema defaults for rule options](https://github.com/eslint/eslint/pull/17580). + +Doing so aligned with existing schema usage; however, its drawbacks were noted as increased user-land complexity for defining options: + + + + + + + + + + + + + + + + + + + + + +
ApproachAdvantagesDisadvantages
meta.defaultOptions +
    +
  • Easier to change and understand the defaults
    • +
  • Not tied to Ajv implementation / details
    • +
+		 +
    +
  • More complex deep merging behavior
    • +
  • Two locations for option descriptions
    • +
+
Recursive Schema Defaults +
    +
  • One idiomatic location for option descriptions
    • +
  • Aligns closer to common Ajv paradigms
    • +
+		 +
    +
  • More difficult to statically type
    • +
  • Complex schemas can be ambiguous
    • +
+
+ +As a data point, typescript-eslint's `defaultOptions` approach has been in wide use for several years: + +1. `eslint-plugin-typescript` originally added its `defaultOptions` in [December 2018](https://github.com/bradzacher/eslint-plugin-typescript/pull/206) +2. `defaultOptions` were made the standard for typescript-eslint's rules in [February 2019](https://github.com/typescript-eslint/typescript-eslint/pull/120) +3. `defaultOptions` became the standard for rules written with typescript-eslint in [May 2019](https://github.com/typescript-eslint/typescript-eslint/pull/425) + +The lack of user pushback against typescript-eslint's approach is considered evidence that its noted disadvantages are not severe enough to prevent using it. + ### Non-Recursive Object Defaults -Ajv provides its own support for `default` values with a [`useDefaults` option](https://ajv.js.org/guide/modifying-data.html#assigning-defaults). -It's used in ESLint core already, in [`lib/shared/ajv.js`](https://github.com/eslint/eslint/blob/22a558228ff98f478fa308c9ecde361acc4caf20/lib/shared/ajv.js#L21). -Sticking Ajv's own defaults logic would bring two benefits: +Sticking with Ajv's `useDefaults: true` would bring two benefits: - Aligning ESLint schema defaulting with the standard logic used by Ajv - Avoiding a custom defaulting implementation in ESLint by using Ajv's instead -However, [`useDefaults` requires explicit empty object for intermediate object entries to work for nested properties](https://github.com/ajv-validator/ajv/issues/1710). +However, in addition to the drawbacks mentioned in the alternatives table, [`useDefaults` requires explicit empty object for intermediate object entries to work for nested properties](https://github.com/ajv-validator/ajv/issues/1710). That means: - Schemas with optional objects containing properties with default need to provide an explicit `default: {}` for those objects. - If those properties are themselves optional objects containing properties, both the outer and inner objects need explicit `default` values. -An alternative to this RFC could be to fill in those default values in ESLint core rules and plugins. See [this Runkit with simulated Ajv objects](https://runkit.com/joshuakgoldberg/650a826da5839400082514ff) for examples of how rules would need to provide those default values. -This RFC attempts to optimize for common cases of how rule authors would write ESLint rules. -As rules have typically omitted `default: {}`, the path of least inconvenience for rule authors seems to be to fill in those values for them recursively. - ### Prior Art The only standardized options application I could find was `@typescript-eslint/eslint-plugin`'s `defaultOptions` property on rules. -It's visible in the plugin's [Custom Rules guide](https://typescript-eslint.io/developers/custom-rules#rulecreator), which suggests users wrap rule creation with a `createRule`: +It's visible in the plugin's [Custom Rules guide](https://typescript-eslint.io/developers/custom-rules#rulecreator), which suggests users wrap rule creation with a `RuleCreator`'s `createRule`: ```ts import { ESLintUtils } from "@typescript-eslint/utils"; @@ -251,17 +249,22 @@ export const rule = createRule({ `createRule` applies a _deep_ merge rather than a shallow `Object.assign`: https://github.com/typescript-eslint/typescript-eslint/blob/324c4d31e46cbc95e8eb67d71792de9f49e65606/packages/utils/src/eslint-utils/applyDefault.ts. This has the benefit of allowing user-provided options to "merge" into complex objects, rather than requiring rewriting larger objects. +Most rules with nontrivial options ask for those options to be specified with at least one object. + +This RFC proposes roughly the same merging strategy as `RuleCreator`. +This RFC's differences are: -However, deep merges are a more nuanced behavior, and may surprise users who assume the more common shallow merging or a different strategy in edge cases such as arrays. -This proposal goes with a less opinionated _shallow_ merge. +- Moving `defaultOptions` inside of `meta` +- Not calling `JSON.parse(JSON.stringify(...))` on options to remove `undefined` values + +Updating `RuleCreator` inside typescript-eslint is out of scope for this RFC. +The typescript-eslint team will take on that work. ## Open Questions -1. Does the developer benefit of nested object defaulting work well enough to justify the added complexity? -2. Would we want to eventually deprecate/delete `context.optionsRaw`? Alternately, should we avoid it altogether, in favor of rewriting the options logic in the core rules? -3. Should ESLint eventually write its own schema parsing package? -4. Should figuring out TypeScript types block this proposal? -5. The three core rules that would want `optionsRaw` -indent, max-len, and no-multiple-empty-lines- are all formatting rules. Is it safe to assume they'll be removed by ESLint 10 (https://github.com/eslint/eslint/issues/17522)? +1. Is adding `context.optionsRaw` for backwards compatibility until the next major version worth the cost? +2. Should ESLint eventually write its own schema parsing package - i.e. formalizing its fork from ajv? +3. _"`RuleTester` should validate the default options against the rule options schema."_ was mentioned in the original issue prompting this RFC. `RuleTester` in the PoC validates the results of merging `meta.defaultOptions` with test-provided `options`. Is this enough? ## Help Needed @@ -278,13 +281,9 @@ It should be a little less code to handle optional options. ### How does this impact typescript-eslint's `RuleCreator` and `defaultOptions`? -We can roll out this change over the current and next two major versions of typescript-eslint: - -1. Current major version (v6): no code changes; mention ESLint's new options defaulting in documentation -2. Next major version (v7): mark `defaultOptions` as deprecated -3. Subsequent major version (v8): remove `defaultOptions` - -This RFC should ideally at least have participation from typescript-eslint maintainers besides its author. +This RFC includes participation from multiple typescript-eslint maintainers. +The exact rollout plan will be discussed on typescript-eslint's side. +We can roll out this change over the current and next 1-2 major versions of typescript-eslint. Note that [typescript-eslint's Building Custom Rules documentation](https://typescript-eslint.io/developers/custom-rules) only shows an empty `defaultOptions` in code snippets. It never explains the presence of `defaultOptions` or suggests using it. From e69aa90e8f225d77e7bd123aee0e3723ebd8185f Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Mon, 16 Oct 2023 14:40:59 -0400 Subject: [PATCH 10/12] More proofreading --- designs/2023-rule-options-defaults/README.md | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/designs/2023-rule-options-defaults/README.md b/designs/2023-rule-options-defaults/README.md index dbff608b..20a22a76 100644 --- a/designs/2023-rule-options-defaults/README.md +++ b/designs/2023-rule-options-defaults/README.md @@ -53,12 +53,12 @@ Defaulting logic could go with the following logic to approximate user intent: See [[Reference] feat: add meta.defaultOptions](https://github.com/eslint/eslint/pull/17656) > [`function deepMerge`](https://github.com/eslint/eslint/pull/17656/files#diff-9bbcc1ca9625b99554a055ac028190e5bd28432207a7f1a519690c5d0484287fR24) and [`describe("deepMerge")`](https://github.com/eslint/eslint/pull/17656/files#diff-fa70d7d4c142a6c3e727cf7280f981cb80a1db10bd6cc85c205e47e47da05cfeR25) for the proposed defaulting logic. The original `options` provided to rules will temporarily be available on a new `context.optionsRaw` property. -That property exists solely as a legacy compatibility layer for rules that would want the behavioral and/or documentation benefits of `meta.defaultOptions` but have nonstandard options parsing logic. +That property will exist solely as a legacy compatibility layer for rules that would want the behavioral and/or documentation benefits of `meta.defaultOptions` but have nonstandard options parsing logic. `context.optionsRaw` will be removed in the next major version of ESLint. ### Rules with Differing Options Behavior -The PoC PR implements `meta.defaultOptions` on rules that include options, are not deprecated, and match any of: +[[Reference] feat: add meta.defaultOptions](https://github.com/eslint/eslint/pull/17656) implements `meta.defaultOptions` on rules that include options, are not deprecated, and match any of: - Are mentioned earlier in this RFC - Have a name starting with `A-C` and include the search string `context.options` @@ -137,17 +137,20 @@ We'll also want to mention this in the ESLint core custom rule documentation. ## Drawbacks -- This increases both conceptual and implementation complexity around rule options +- This increases both conceptual and implementation complexity around built-in rule options logic + - This RFC believes that enough rules were already implemented with ad-hoc logic to make standardizing rule options logic a net reduction of complexity - This explicitly moves away from the JSON Schema / ajv style description of putting `default` values in `meta.schema` + - This RFC believes that schema-oriented defaults are too convoluted to justify their usage ## Backwards Compatibility Analysis If a rule does not specify `meta.defaultOptions`, its behavior will not change. -This change is backwards compatible. +This change is therefore completely backwards compatible to start. The removals of `context.optionsRaw` and Ajv `useDefaults: true` will be breaking changes in the next major version. -This RFC believes the change will be minor and not create significant use pain. +This RFC believes the impact of those removals will be minor and not create significant user pain. Most ESLint core rules did not change behavior when switched to `meta.defaultOptions`. +Popular third-party plugins will be notified of the change and helped move to it. ## Alternatives @@ -198,6 +201,8 @@ Doing so aligned with existing schema usage; however, its drawbacks were noted a +This RFC believes that the simplicity of defining option defaults in `meta.defaultOptions` is the best outcome for rule authors and end-users. + As a data point, typescript-eslint's `defaultOptions` approach has been in wide use for several years: 1. `eslint-plugin-typescript` originally added its `defaultOptions` in [December 2018](https://github.com/bradzacher/eslint-plugin-typescript/pull/206) @@ -223,7 +228,7 @@ See [this Runkit with simulated Ajv objects](https://runkit.com/joshuakgoldberg/ ### Prior Art -The only standardized options application I could find was `@typescript-eslint/eslint-plugin`'s `defaultOptions` property on rules. +The only standardized options application found in the wild is `@typescript-eslint/eslint-plugin`'s `defaultOptions` property on rules. It's visible in the plugin's [Custom Rules guide](https://typescript-eslint.io/developers/custom-rules#rulecreator), which suggests users wrap rule creation with a `RuleCreator`'s `createRule`: ```ts @@ -247,7 +252,7 @@ export const rule = createRule({ }); ``` -`createRule` applies a _deep_ merge rather than a shallow `Object.assign`: https://github.com/typescript-eslint/typescript-eslint/blob/324c4d31e46cbc95e8eb67d71792de9f49e65606/packages/utils/src/eslint-utils/applyDefault.ts. +`createRule` also applies a deep merge rather than a shallow `Object.assign`: https://github.com/typescript-eslint/typescript-eslint/blob/324c4d31e46cbc95e8eb67d71792de9f49e65606/packages/utils/src/eslint-utils/applyDefault.ts. This has the benefit of allowing user-provided options to "merge" into complex objects, rather than requiring rewriting larger objects. Most rules with nontrivial options ask for those options to be specified with at least one object. From b182b786e75b2fb631231560134d531fe283ee18 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Wed, 18 Oct 2023 23:22:40 -0400 Subject: [PATCH 11/12] Removed optionsRaw and postponed useDefaults: true removal --- designs/2023-rule-options-defaults/README.md | 33 +++++++++++++------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/designs/2023-rule-options-defaults/README.md b/designs/2023-rule-options-defaults/README.md index 20a22a76..bd087005 100644 --- a/designs/2023-rule-options-defaults/README.md +++ b/designs/2023-rule-options-defaults/README.md @@ -52,10 +52,6 @@ Defaulting logic could go with the following logic to approximate user intent: See [[Reference] feat: add meta.defaultOptions](https://github.com/eslint/eslint/pull/17656) > [`function deepMerge`](https://github.com/eslint/eslint/pull/17656/files#diff-9bbcc1ca9625b99554a055ac028190e5bd28432207a7f1a519690c5d0484287fR24) and [`describe("deepMerge")`](https://github.com/eslint/eslint/pull/17656/files#diff-fa70d7d4c142a6c3e727cf7280f981cb80a1db10bd6cc85c205e47e47da05cfeR25) for the proposed defaulting logic. -The original `options` provided to rules will temporarily be available on a new `context.optionsRaw` property. -That property will exist solely as a legacy compatibility layer for rules that would want the behavioral and/or documentation benefits of `meta.defaultOptions` but have nonstandard options parsing logic. -`context.optionsRaw` will be removed in the next major version of ESLint. - ### Rules with Differing Options Behavior [[Reference] feat: add meta.defaultOptions](https://github.com/eslint/eslint/pull/17656) implements `meta.defaultOptions` on rules that include options, are not deprecated, and match any of: @@ -67,7 +63,7 @@ That property will exist solely as a legacy compatibility layer for rules that w
-The following matched rules use context.optionsRaw because their current defaulting logic is incompatible with context.options + meta.defaultOptions. +The following matched rules will not use meta.defaultOptions because their current defaulting logic is incompatible with the new merging logic: - `array-bracket-newline`: @@ -109,10 +105,22 @@ Some core ESLint rules describe `default:` values in that schema. Having two places to describe `default` values can be confusing for developers. It can be unclear where a value comes from when its default may be specified in two separate locations. -This RFC proposes removing the `useDefaults: true` setting in the next major version of ESLint. +This RFC proposes removing the `useDefaults: true` setting in a future major version of ESLint. ### Out of Scope +#### Preserving Original Options + +The original `options` provided to rules could be made available on a new property with a name like `context.optionsRaw`. +That property could exist solely as a legacy compatibility layer for rules that would want the behavioral and/or documentation benefits of `meta.defaultOptions` but have nonstandard options parsing logic. + +However: + +- The overlap of rules that would want the documentation benefits of `meta.defaultOptions` but would not be able to work with the new defaulting logic is quite small. +- Using non-standard defaulting logic is detrimental to users being able to reliably understand how rules' options generally work. + +This RFC considers `context.optionsRaw` or an equivalent as not worth the code addition. + #### TypeScript Types Right now, there are two community strategies for declaring rule options types, both with drawbacks: @@ -147,10 +155,12 @@ We'll also want to mention this in the ESLint core custom rule documentation. If a rule does not specify `meta.defaultOptions`, its behavior will not change. This change is therefore completely backwards compatible to start. -The removals of `context.optionsRaw` and Ajv `useDefaults: true` will be breaking changes in the next major version. -This RFC believes the impact of those removals will be minor and not create significant user pain. +The removal of Ajv `useDefaults: true` will be a breaking change in a future major version. +This RFC believes the impact of that removal will be minor and not create significant user pain. Most ESLint core rules did not change behavior when switched to `meta.defaultOptions`. -Popular third-party plugins will be notified of the change and helped move to it. + +Popular third-party plugins will be notified of the upcoming change and helped move to `meta.defaultOptions`. +Given the improvements to rule implementations and documentation `eslint-doc-generator`, this RFC expects plugins to be generally receptive. ## Alternatives @@ -267,9 +277,8 @@ The typescript-eslint team will take on that work. ## Open Questions -1. Is adding `context.optionsRaw` for backwards compatibility until the next major version worth the cost? -2. Should ESLint eventually write its own schema parsing package - i.e. formalizing its fork from ajv? -3. _"`RuleTester` should validate the default options against the rule options schema."_ was mentioned in the original issue prompting this RFC. `RuleTester` in the PoC validates the results of merging `meta.defaultOptions` with test-provided `options`. Is this enough? +1. Should ESLint eventually write its own schema parsing package - i.e. formalizing its fork from ajv? +2. _"`RuleTester` should validate the default options against the rule options schema."_ was mentioned in the original issue prompting this RFC. `RuleTester` in the PoC validates the results of merging `meta.defaultOptions` with test-provided `options`. Is this enough? ## Help Needed From 2634a11f26680961b164dd020fee7b72ef3f42d9 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Thu, 26 Oct 2023 13:09:27 +0300 Subject: [PATCH 12/12] Updated computed-property-spacing --- designs/2023-rule-options-defaults/README.md | 6 ------ 1 file changed, 6 deletions(-) diff --git a/designs/2023-rule-options-defaults/README.md b/designs/2023-rule-options-defaults/README.md index bd087005..d29e620a 100644 --- a/designs/2023-rule-options-defaults/README.md +++ b/designs/2023-rule-options-defaults/README.md @@ -71,12 +71,6 @@ The following matched rules will not use meta.defaultOptions becaus - When given `options: [{ minItems: null }]`: - Current normalized options: `[ { minItems: null } ]`, with implicit `multiline: false` - Updated normalized options: `[ { minItems: null, multiline: true } ]` -- `computed-property-spacing`: - - Presumed `defaultOptions: ["never"]` - - Note that `default: true` should also be removed from its `meta.schema`'s object `enforceForClassMembers` property - - When given `["always", {}]`: - - Current normalized `options.enforceForClassMembers`: `true` - - Updated normalized `options.enforceForClassMembers`: `undefined` - `object-curly-newline`: - Presumed `defaultOptions: [{ consistent: true }]` - When given `options: [{ multiline: true, minProperties: 2 }]`: