Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support for meta.defaultOptions on rules #113

Merged
merged 12 commits into from
Feb 13, 2024
294 changes: 294 additions & 0 deletions designs/2023-rule-options-defaults/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,294 @@
- Repo: eslint/eslint
- Start Date: 2023-09-18
- RFC PR: https://github.com/eslint/rfcs/pull/113
- Authors: [Josh Goldberg](https://github.com/JoshuaKGoldberg)

# Support for `meta.defaultOptions` on rules

## Summary

Enable rules to provide default options programmatically with a new `meta.defaultOptions` property.

## Motivation

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)

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.

For example:

- [`array-callback-return`](https://eslint.org/docs/latest/rules/array-callback-return#options) phrases its options as _`"<key>": <value> (default) <explanation>`_
- [`accessor-pairs`](https://eslint.org/docs/latest/rules/accessor-pairs#options) phrases its options as _`<key> <explanation> (Default <default>)`_

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

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

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.
Comment on lines +44 to +45
Copy link
Member

Choose a reason for hiding this comment

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

Note that this is different from the current approach with Ajv defaults, where FlatConfigArray applies them directly in the config objects.

For example, currently:

// eslint.config.js
module.exports = [{
    rules: {
        camelcase: ["error", {}]
    }
}];
$ npx eslint --print-config foo.js
{
  "languageOptions": {
    "ecmaVersion": "latest",
    "sourceType": "module",
    "parser": "[email protected]",
    "parserOptions": {},
    "globals": {}
  },
  "plugins": [
    "@"
  ],
  "rules": {
    "camelcase": [
      2,
      {
        "ignoreDestructuring": false,
        "ignoreImports": false,
        "ignoreGlobals": false
      }
    ]
  }
}

After this change, it would be:

$ npx eslint --print-config foo.js
{
  "languageOptions": {
    "ecmaVersion": "latest",
    "sourceType": "module",
    "parser": "[email protected]",
    "parserOptions": {},
    "globals": {}
  },
  "plugins": [
    "@"
  ],
  "rules": {
    "camelcase": [
      2,
      {}
    ]
  }
}

An advantage of applying defaults in FlatConfigArray is performance, as config-array caches config objects so the defaults are not applied again for each file being linted.

Would it make sense to apply meta.defaultOptions in FlatConfigArray rather than when linting each file? Aside from possible performance gains, an observable difference would be in the output of --print-config which would show the final merged options (which seems useful, though it may be confusing as the output wouldn't match what user sees in their config file).

Copy link
Contributor Author

@JoshuaKGoldberg JoshuaKGoldberg Nov 21, 2023

Choose a reason for hiding this comment

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

This makes a lot of sense to me, thanks. I implemented the logic inside RuleValidator's validate() method in eslint/eslint#17656.

https://github.com/eslint/eslint/actions/runs/6939046423/job/18875742661?pr=17656 shows finding two typos in rule default options (fixed in 629f691c7):

     Error: Key "rules": Key "accessor-pairs": 	Value {"enforceForClassMembers":true,"getWithoutGet":false,"setWithoutGet":true} should NOT have additional properties.
     Error: Key "rules": Key "camelcase": 	Value {"allow":[],"ignoreDestructuring":false,"ignoreGlobals":false,"ignoreIMports":false,"properties":"always"} should NOT have additional properties.

Also updated eslint/eslint#17656 to mostly apply default options through config-validator and rule-validator. There are a few remaining tests I haven't had the time to stamp out just yet, I think it still functions as a general proof of concept.


Defaulting logic could go with the following logic to approximate user intent:

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
Copy link
Member

Choose a reason for hiding this comment

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

What if the rule schema has different object schemas in oneOf/anyOf, for example as in prefer-destructuring. The default for this would be an object with VariableDeclarator and AssignmentExpression properties, but when user config provides the other alternative with array and object properties, merging would result in an object with a mix of properties that would fail validation. Is this a case where the rule should not use meta.defaultOptions?

Choose a reason for hiding this comment

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

In general the advice we give when using the current ts-eslint tooling is that if you've got some complex options schema - eg oneOf where the schemas are truly disjoint like your example, or rules like @typescript-eslint/ban-types - they are just usecases where the standard logic cannot apply and thus you need to write your own options merging logic.

Copy link
Member

Choose a reason for hiding this comment

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

I think at this point we should be discouraging the use of oneOf and anyOf. We ended up doing this in core rules because we didn't realize just how many exceptions and options people would want on rules, so early rules would often just use a string value as an option. In order to allow more options, we'd also allow an object. In general, I think we should be encouraging folks to always use an object with multiple properties for their rule options and avoid oneOf and anyOf in their schemas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should be encouraging folks to always use an object with multiple properties for their rule options and avoid oneOf and anyOf in their schemas.

Aha! typescript-eslint/typescript-eslint#6040. I would be interested in filing an issue/discussion on ESLint core to discuss establishing this in core development docs.

Copy link

Choose a reason for hiding this comment

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

Even with that as a constraint, i'd still have many use cases for anyOf/oneOf, because there are often multiple disjoint sets of options that may or may not be mutually exclusive.

Copy link
Member

Choose a reason for hiding this comment

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

@ljharb you can always ignore recommendations. I just think it's helpful to give people guardrails that cover the 80% case. If you fall into the 20% case, then void the warranty and do what you need to. I think it's fine for the 20% case to not get the 80% case DX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you provide some examples?

Copy link

Choose a reason for hiding this comment

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

@JoshuaKGoldberg for example, https://github.com/jsx-eslint/eslint-plugin-react/blob/9da1bb0f4aa44849b56e8c2064afd582ea8b36f8/lib/rules/jsx-indent-props.js#L64-L71 - that object property takes a number or two enum values.

https://github.com/jsx-eslint/eslint-plugin-react/blob/9da1bb0f4aa44849b56e8c2064afd582ea8b36f8/lib/rules/jsx-max-props-per-line.js#L39-L70 is more complex; you can specify a maximum with single and/or multi, xor you can specify a maximum with "when".

There's a bunch more in this plugin, and I haven't looked at the other two yet, but suffice to say it's pretty much impossible to avoid with a complex rule that evolves over time.

3. Arrays and non-`undefined` literals -including `null`- are used as-is

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

### Rules with Differing Options Behavior

The PoC PR 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`
- Include the search string `Object.assign(`
- Experience test failures when Ajv's `useDefaults` is option is disabled (see [Schema Defaults](#schema-defaults))

<details>
<summary>
The following matched rules use <code>context.optionsRaw</code> because their current defaulting logic is incompatible with <code>context.options</code> + <code>meta.defaultOptions</code>.
</summary>

- `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`
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
- `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' }`

</details>

<details>
<summary>
Additionally, the following rule has default logic that doesn't map well to <code>meta.defaultOptions</code>.
</summary>

- `strict`: The mode depends on -and can be overridden based on- `ecmaFeatures.impliedStrict`

</details>

### Schema 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).
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.
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

### Out of Scope

#### 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<TMessageIds, TOptions>` 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` doesn't factor in root-level `defaultOptions` to the type system (https://github.com/typescript-eslint/typescript-eslint/issues/5439)

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

Heads up that I implemented a basic version of rule option lists in bmish/eslint-doc-generator#481. It currently pulls option defaults from the schema. I imagine we can just switch it to use meta.defaultOptions whenever a rule provides the new property.


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 explicitly moves away from the JSON Schema / ajv style description of putting `default` values in `meta.schema`
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

## Backwards Compatibility Analysis
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

If a rule does not specify `meta.defaultOptions`, its behavior will not change.
This change is backwards compatible.

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

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:

<table>
<thead>
<tr>
<th>Approach</th>
<th>Advantages</th>
<th>Disadvantages</th>
</tr>
</thead>
<tbody>
<tr>
<th><code>meta.defaultOptions</code></th>
<td>
<ul>
<li>Easier to change and understand the defaults</li>
<li>Not tied to Ajv implementation / details</li>
</ul>
</td>
<td>
<ul>
<li>More complex deep merging behavior</li>
<li>Two locations for option descriptions</li>
</ul>
</td>
</tr>
<tr>
<th>Recursive Schema Defaults</th>
<td>
<ul>
<li>One idiomatic location for option descriptions</li>
<li>Aligns closer to common Ajv paradigms</li>
</ul>
</td>
<td>
<ul>
<li>More difficult to statically type</li>
<li>Complex schemas can be ambiguous</li>
</ul>
</td>
</tr>
</tbody>
</table>

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

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

See [this Runkit with simulated Ajv objects](https://runkit.com/joshuakgoldberg/650a826da5839400082514ff) for examples of how rules would need to provide those default values.
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

### 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 `RuleCreator`'s `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.
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:

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

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`?

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.

## Related Discussions

- <https://github.com/eslint/eslint/issues/17448>: the issue triggering this RFC
- <https://github.com/typescript-eslint/typescript-eslint/pull/6963>: typescript-eslint forking types for `JSONSchemaV4`