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
302 changes: 302 additions & 0 deletions designs/2023-rule-options-defaults/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,302 @@
- 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.

### 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:

- 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 will not use <code>meta.defaultOptions</code> because their current defaulting logic is incompatible with the new merging logic:
</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 } ]`
- `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 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:

- [`@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 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`
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
- This RFC believes that schema-oriented defaults are too convoluted to justify their usage

## 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 therefore completely backwards compatible to start.

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

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

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

Choose a reason for hiding this comment

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

In the PoC, it's possible to provide the default options that are invalid for the schema because merging happens after the validation of options.

For example, ignoreIMports is invalid property name (typo uppercase M).

Copy link
Member

Choose a reason for hiding this comment

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

This seems like something that can be worked out during the implementation phase. I'd recommend we move into Final Commenting now. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Works for me, I'd just like to clarify #113 (comment) as that may affect this and I'm not sure whether the intent was to always pass raw or merged options to Ajv validations.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, that's what the final commenting period is for: small edits and clarifications. Because we're aligned on the overall approach I think we're ready.


## 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`
Loading