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

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Sep 18, 2023

Summary

This RFC proposes recursively defaulting rule options based on the rules schema. The behavior changes are that ESLint will now recursively creates {} objects for rule options. Doing so enhances the core Ajv useDefaults for easier rule options defaults.

Related Issues

@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review September 18, 2023 17:04
@mdjermanovic mdjermanovic added the Initial Commenting This RFC is in the initial feedback stage label Sep 18, 2023
@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as draft September 20, 2023 05:15
@JoshuaKGoldberg JoshuaKGoldberg changed the title feat: Rule Options Defaults feat: Rule Options Recursive Defaults Sep 20, 2023
@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review September 20, 2023 07:20
@ljharb
Copy link

ljharb commented Sep 20, 2023

this is a lot to page in, but is there a reason the rule schema can't provide the defaults via default, which is part of JSON Schema?

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 20, 2023

this is a lot to page in, but is there a reason the rule schema can't provide the defaults via default, which is part of JSON Schema?

Rule schemas can already provide defaults via default. This already works everywhere except at the top level but we could fix that.

The concern with using default is that the behavior is specific to Ajv. While the default keyword is indeed part of the JSON Schema, the specification doesn't dictate how it should be used in the validation process. In fact, the recommendation is not to use it in the validation process ("This value is not used to fill in missing values during the validation process"):

https://json-schema.org/understanding-json-schema/reference/generic.html#annotations

@bradzacher
Copy link

As I mentioned on the issue the big problem I've found with the default keyword in the schema is that is really not well handled by Ajv.

Additionally from a type-checker point of view its nearly impossible to strictly type the default value beyond trivial cases.

Finally I also find it can lead to really ambiguous schemas. For example what is the result from this schema:

const schema = {
  type: "object",
  properties: {
    prop: {
      oneOf: [
        { type: "string", default: "foo" },
        { type: "string", default: "bar" },
      ],
    }
  }
};

What does this validate to if I pass it {}? I have no idea!

It's a very vague, wishy-washy way to specify defaults - which isn't what you want.

Which is why (as I mentioned in the issue) we chose an entirely separate property in our typed rule utils for ts-eslint. There's nothing ambiguous or unclear about defaultOption: [{prop: "foo"}] and it's a cinch to validate that the default value is of the correct structure (eg does it pass the schema) and its also a cinch to typecheck it to ensure its of the correct structure.

Thats why personally I think that's the direction we would want to head down here.

@nzakas
Copy link
Member

nzakas commented Sep 22, 2023

In general, I'm not a fan of piggybacking on JSON Schema for this. See my comment for more.

@JoshuaKGoldberg
Copy link
Contributor Author

I threaded the discussion on meta.defaultOptions vs. recursive schema defaults here: https://github.com/eslint/rfcs/pull/113/files#r1331794511

@JoshuaKGoldberg JoshuaKGoldberg changed the title feat: Rule Options Recursive Defaults feat: Support for meta.defaultOptions on rules Oct 16, 2023

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

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Thanks, I like this proposal a lot better. I left some comments on a few points but overall I think this is looking good.

designs/2023-rule-options-defaults/README.md Show resolved Hide resolved
designs/2023-rule-options-defaults/README.md Outdated Show resolved Hide resolved
designs/2023-rule-options-defaults/README.md Outdated Show resolved Hide resolved
designs/2023-rule-options-defaults/README.md Show resolved Hide resolved
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

I'm happy with the current state of this RFC. 👍

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.

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

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

@nzakas
Copy link
Member

nzakas commented Nov 7, 2023

Moving to Final Commenting. @JoshuaKGoldberg please take a look at @mdjermanovic's request for clarification.

@nzakas nzakas added Final Commenting This RFC is in the final week of commenting and removed Initial Commenting This RFC is in the initial feedback stage labels Nov 7, 2023
@nzakas
Copy link
Member

nzakas commented Nov 21, 2023

It looks like we've got most of the high-levels details nailed down, so merging. We can work through edge cases and implementation concerns on the implementation PR.

@Rec0iL99
Copy link
Member

Rec0iL99 commented Feb 7, 2024

It looks like we've got most of the high-levels details nailed down, so merging.

@nzakas did you intend on merging this?

@nzakas
Copy link
Member

nzakas commented Feb 13, 2024

Oops yes! My mistake. Thanks for catching this.

@nzakas nzakas merged commit b67bec5 into eslint:main Feb 13, 2024
5 checks passed
@JoshuaKGoldberg JoshuaKGoldberg deleted the 2023-rule-options-defaults branch February 14, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Final Commenting This RFC is in the final week of commenting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants