feat!: Require schemas and object-style rules #85

bmish · 2021-12-13T17:35:42Z

Summary

We propose two changes to rules:

Requiring rules with options to specify schemas (removing support for rules with options that are missing schemas)
Requiring rule implementations to use the object-style format (removing support for the deprecated function-style format)

Demonstrations

Deprecation warnings when running rule tests: https://github.com/bmish/eslint/tree/RFC-85-deprecation-warning

Related Issues

Change default rule schema to schema: [] and drop support for function-style rules eslint#14709

nzakas

Overall very much in favor of these changes. Just a few notes.

nzakas · 2021-12-18T01:15:17Z

designs/2021-schema-object-rules/README.md

+The two proposals in this RFC should be implemented separately.
+
+### Detailed design for requiring schemas
+


Note that these changes will need to be made in the flat config files as we aren’t making any further changes to the eslintrc format.

designs/2021-schema-object-rules/README.md

aladdin-add · 2021-12-25T05:39:05Z

designs/2021-schema-object-rules/README.md

+Today, rule schemas ([meta.schema](https://eslint.org/docs/developer-guide/working-with-rules#rule-basics)) are optional. This has the following effects:
+
+- If a rule does not specify a schema, nothing is enforced.
+- Rules with options may not have a schema specified, despite it being best practice to specify a schema for rule options. This increases the chances of developers passing invalid rule options and encountering unexpected behavior / bugs in rules.
+- If a rule author wants to enforce that no options are passed to their rule, they have to manually specify `schema: []`, otherwise it's possible that developers could accidentally provide useless rule options to their rule without knowing it.
+
+So we will change the default rule schema to `schema: []` to enforce that no rule options are passed to rules that do not specify a schema.
+
+This has the desirable consequence of requiring rules with options to begin specifying their schema if they did not already.
+
+Making ESLint's default behavior more strict like this goes along with many recent changes to tighten validation (i.e. more [RuleTester class validation in ESLint 7](https://eslint.org/docs/user-guide/migrating-to-7.0.0#rule-tester-strict), [increased rule configuration validation in ESLint 6](https://eslint.org/docs/user-guide/migrating-to-6.0.0#rule-config-validating), etc) to improve rule quality and usability and reduce the chance of mistakes.
+
+And in addition to the obvious benefits of increased rule schema usage such as reduced chance of silent mistakes/bugs, being able to rely on having rule schemas available could unlock new features/improvements:
+
+- Improved/auto-generated documentation regarding rule options
+- TypeScript-style auto-complete/IntelliSense when configuring a rule
+
+There is an existing, third-party lint rule [eslint-plugin/require-meta-schema](https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/master/docs/rules/require-meta-schema.md) to enforce that rules have schemas.


I'm 👍to change the default to [], but do we want to add a new way to allow rule authors explicitly skipping the validating - e.g. schema: false?
I'm asking as it could happen in some rare cases, e.g. writing an internal rule.

I'm to change the default to [], but do we want to add a new way to allow rule authors explicitly skipping the validating - e.g. schema: false?

A workaround can be schema: {} or schema: { type: "array" }

I could see both sides, but my current preference is to not provide an explicit opt-out.

Why we shouldn't give users an explicit schema opt-out:

An easy, well-known option to opt-out is just welcoming laziness, and many plugins authors may default to always using it, despite the benefits and ease of writing a real schema, defeating many of the benefits described in this RFC

An explicit opt-out option is essentially encouraging/recommending opting-out as an acceptable approach

As @mdjermanovic suggested, users can use the the workaround of providing an incomplete schema likeschema: {} or schema: { type: "array" } if they really want to opt-out

In my opinion, schemas should be low-effort and valuable for all rules, even internal rules

Why we should give users an explicit schema opt-out like schema: false:

If users are going to abuse obscure workarounds like schema: {} or schema: { type: "array" } anyway, then it would be clearer for everyone if they could explicitly specify that they don't want a schema

I added a paragraph about this workaround in the Drawbacks of requiring schemas section including touching on why I am against an explicit opt-out.

Both arguments for and arguments against schema: false are good.

Why we should give users an explicit schema opt-out like schema: false:

If users are going to abuse obscure workarounds like schema: {} or schema: { type: "array" } anyway, then it would be clearer for everyone if they could explicitly specify that they don't want a schema

I suggested the workarounds, but this is a very good argument and I'm now for an explicit opt-out like schema: false.

As described in the Motivation section of this RFC, one of the benefits of rule schemas (and thus forcing rules to define schemas) is automated analysis of rule options, like auto-generated docs, auto-complete, maybe even detecting breaking changes as mentioned in eslint/eslint#15428, etc., For that purpose, when some rule doesn't provide a schema for whatever reason, it seems better to see that right away from schema: false than to analyze a meaningless workaround schema.

Also, some schema validations we might want to add to RuleTester in the future could report these workaround schemas as potentially incorrect, since they're essentially a no-op.

One follow-up to this, I think we should disallow schema: {}. That is the simplest no-op schema and can be easily confused with schema: [] which actually does enforce that there are no options. Anyone using schema: {} is likely making a mistake, so we can throw an error if we detect that:

`schema: {}` is a no-op. For rules with options, please fill in a complete schema. For rules without options, please omit `schema` or use `schema: []`.

We could also disallow other obvious ones like schema: { type: "array" } but I'm not too worried about catching all possible no-op schemas right now.

I agree, i had no idea that {} works this way.

would { additionalProperties: false } be the same as []?

schema: { additionalProperties: false } is also a no-op.

ah, maybe { maxProperties: 0 } or something, but clearly [] is simpler

I updated the Opt-out section to mention that we will disallow a few common no-op schemas.

mdjermanovic · 2021-12-25T13:55:25Z

designs/2021-schema-object-rules/README.md

+- Adding deprecation notices inside ESLint in a minor version release, showing a warning when executing linting for an offending rule:
+  > DEPRECATION WARNING: This rule has options but is missing `meta.schema` and will stop working in ESLint v9.
+  > The maintainer of this rule needs to release an updated version with a schema added.
+  >
+  > DEPRECATION WARNING: This rule is using the deprecated function-style format and will stop working in ESLint v9.
+  > The maintainer of this rule needs to release an updated version using object-style format.


Is this necessary, since we will have deprecation warnings when running tests?

Showing warnings when executing linting could be annoying to end users, as there's nothing they can do to fix the issue.

I was hoping that showing the deprecation warning to end users would result in them pinging the maintainer / filing an issue on github / or even opening a PR themselves with the fix. This would mainly be necessary for stagnant eslint plugins where the maintainer might not otherwise notice the deprecation warning since they aren't regularly running tests.

Perhaps I can propose a different timeline however:

When this RFC is approved, add the deprecation warning when running tests, in the hopes of most affected plugins seeing this.

In the final minor version of ESLint 8, add the deprecation warning for end users when running linting to try to catch any last holdouts.

In ESLint 9, change the deprecation warnings into fatal assertions.

But if we still think the deprecation warning for end users is too obtrusive, I'm not attached to it and we can skip it.

I'd vote for not showing warnings to end users since we have other ways to notify maintainers: deprecation warnings when running tests, blog posts, and prerelease versions. If all of that fails to notify some plugins, the worst case is that they'll be notified by users when we release ESLint 9 final.

Agreed with @mdjermanovic. The prerelease versions should give users/plugins time to discover any remaining problems. Updated RFC to only add a deprecation warning when running tests.

mdjermanovic · 2021-12-25T14:32:48Z

designs/2021-schema-object-rules/README.md

+    to existing users?
+-->
+
+This RFC describes a set of breaking changes that can be released in the next major version of ESLint. Both ESLint plugin authors and ESLint users will be affected and will be unable to use offending rules until they are updated. This will mostly affect a small number of older ESLint plugins that haven't been updated nor maintained in years.


This will be also a breaking change for end users who have invalid configurations for non-offending rules (those that don't expect any options), as they'll have to remove useless options from config files.

I added a bullet point to denote this.

nzakas

I’m happy with the overall direction at this point. 👍

mdjermanovic

Aside from resolving the discussion about schema opt-outs (#85 (comment)), everything else looks good to me, and the analysis of plugins is awesome!

mdjermanovic · 2022-02-18T12:37:26Z

designs/2021-schema-object-rules/README.md

+  > DEPRECATION WARNING: This rule has options but is missing `meta.schema` and will stop working in ESLint v9.
+  > Please add `meta.schema`: <https://eslint.org/docs/developer-guide/working-with-rules#options-schemas>
+  > This lint rule can assist with the conversion: <https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/main/docs/rules/require-meta-schema.md>


Can we clarify here in the RFC document that this check is triggered by test cases that have options property? (as implemented in bmish/eslint@b7bb2c0)

Updated deprecation warning text to clarify when it is triggered.

btmills

LGTM pending addition of schema: false. Thank you for building the ecosystem analyzer; this is the sort of change I would be very nervous about making without data!

nzakas · 2022-03-15T00:28:41Z

@mdjermanovic do you feel comfortable moving to final commenting?

bmish · 2022-03-15T20:38:05Z

Addressed all comments.

mdjermanovic

Looks good to me, thanks!

mdjermanovic · 2022-03-16T15:31:58Z

@mdjermanovic do you feel comfortable moving to final commenting?

Yes, moving to final commenting now.

ljharb · 2022-03-16T15:33:48Z

designs/2021-schema-object-rules/README.md

+
+We propose two changes to rules:
+
+- Requiring rules with options to specify schemas (removing support for rules with options that are missing schemas)


This is a good change; a few of my plugins have rules that lack a schema because it’s a breaking change, but my planned workaround for those is to conditionally use an empty schema for eslint 9+

ljharb · 2022-03-16T15:34:16Z

designs/2021-schema-object-rules/README.md

+We propose two changes to rules:
+
+- Requiring rules with options to specify schemas (removing support for rules with options that are missing schemas)
+- Requiring rule implementations to use the object-style format (removing support for the deprecated function-style format)


Which versions of eslint, if any, only supported the function format? What capabilities, if any, are only available in the function format?

I just did some testing and found that ESLint v1 only supported function-style rules (object-style rules will throw an error rule is not a function). It looks like ESLint v2 began supporting object-style rules too.

I'm pretty confident that function-style rules do not offer any extra functionality over object-style rules.

ljharb · 2022-03-16T15:34:45Z

designs/2021-schema-object-rules/README.md

+
+- If a rule does not specify a schema, nothing is enforced.
+- Rules with options may not have a schema specified, despite it being best practice to specify a schema for rule options. This increases the chances of developers passing invalid rule options and encountering unexpected behavior / bugs in rules.
+- If a rule author wants to enforce that no options are passed to their rule, they have to manually specify `schema: []`, otherwise it's possible that developers could accidentally provide useless rule options to their rule without knowing it.


I believe schema: {} works as well?

I just tested to confirm that schema: {} does not actually enforce anything. As mentioned in comments like #85 (comment), we should probably disallow no-op schemas like that. If someone wants to enforce that there are no options, they should omit the schema (after this RFC) or use schema: []. If someone wants to skip writing a schema, they will be able to use schema: false.

aladdin-add

LGTM~

mdjermanovic · 2022-03-17T09:53:27Z

designs/2021-schema-object-rules/README.md

+We believe this explicit opt-out is preferred over users resorting to hacky workarounds such as providing no-op/fake/minimal/incomplete schemas like `schema: {}` or `schema: { type: "array" }`. No-op schemas like these can confuse both humans (who might mistake it with `schema: []` which is not a no-op) and automated analysis/tooling built around schemas. So we will throw an error when encountering a few common versions of these no-op schemas:
+
+```pt
+`schema: {}` is a no-op. For rules with options, please fill in a complete schema. For rules without options, please omit `schema` or use `schema: []`.
+```


We had a consensus on all changes proposed by this RFC, but this looks like an entirely new addition to the proposal so we should either revert to the approved version or dismiss all approvals and seek consensus again.

I'm not opposed to evaluating this, but we should clarify details like:

Where will this check be performed? (RuleTester or core, and if it's in the core then where exactly).

Which schemas will be reported?

Should we log a warning in ESLint v8?

If it causes a problem, I'm happy to revert this new exception. I figured this RFC would be a good chance to include it since it's very related, and you suggested it originally in the other thread. Definitely want to get the full TSC's opinion on it. I can fill in more details soon.

In general, once an RFC is in final commenting, we shouldn’t be making any further additions, only adding clarifications.

I think it makes sense to bundle this change with the rest of the RFC but the additional details @mdjermanovic mentioned would be helpful.

I also think it makes sense to include this change in this RFC, just waiting for details :)

We can throw the exception in both core and rule tester. In core, we can throw it next to ajv.compile where the existing schema validation happens.

Honestly, I'm fine just checking for schema: {} since that's the most-common mistake and most-easy to mix up with schema: []. It should be easy to add more to the list later if there's an interest or need.

Yes I think we can log a warning in ESLint v8 when running tests for a rule with such a schema.

I used eslint-ecosystem-analyzer and found that about ~7 of the top 1,000 plugins have an occurrence of schema: {}.

Thanks for the additional details, I completely agree with 2. and 3.

As for 1., reporting no-op schemas looks to me more like useful info for rule developers than a catastrophic error that should prevent the rule from working in production, so I would be more inclined to have this check only in RuleTester. We already have a somewhat similar check for no-op defaults in the RuleTester (strictDefaults: true) but not in the core. However, I don't have a strong opinion on this - if there is any benefit in throwing this error in the core, I could agree with that.

Either way, there's no need to duplicate checks. If we decide to implement this check in the core, then it doesn't have to be duplicated in the RuleTester, because the tests will fail anyway.

I agreed, there is nothing the plugin users can do to avoid the error. they could only disable the rule totally and report to the plugin author.

I'm fine with only throwing the exception in rule tester for now. Although note that other schema validation happens in core already, so it could be argued that all validation should be done together for consistency instead of having different types of validation in different places.

I have updated this section of the RFC.

Although note that other schema validation happens in core already

Do we have validations of rule schemas in core? I think we've disabled Ajv validations and that we don't have any custom validations.

My bad, I just confirmed that we don't validate schemas in core. We do ajv.compile in core but ajv.validateSchema only in rule tester.

mdjermanovic

LGTM, thanks!

* feat!: Require schemas and object-style rules * add more detail on deprecation warnings * tweak deprecation notice * address PR feedback * schema: false * clarify when deprecation warning shows * mention when object-style rules were introduced * disallow no-op schemas * no-op schema rule tester only

eslint-github-bot bot added breaking triage feature labels Dec 13, 2021

feat!: Require schemas and object-style rules

dbbba75

bmish force-pushed the schema-object-rules branch from fc0626e to dbbba75 Compare December 13, 2021 17:36

bmish mentioned this pull request Dec 13, 2021

Change default rule schema to schema: [] and drop support for function-style rules eslint/eslint#14709

Closed

nzakas added Initial Commenting This RFC is in the initial feedback stage and removed triage labels Dec 16, 2021

nzakas requested changes Dec 18, 2021

View reviewed changes

bmish added 2 commits December 24, 2021 12:07

add more detail on deprecation warnings

f185b06

tweak deprecation notice

a6a36f4

aladdin-add reviewed Dec 25, 2021

View reviewed changes

mdjermanovic reviewed Dec 25, 2021

View reviewed changes

address PR feedback

93ffd2e

bmish mentioned this pull request Jan 2, 2022

Roadmap planning 2022 eslint/eslint#15383

Closed

nzakas approved these changes Jan 7, 2022

View reviewed changes

mdjermanovic reviewed Feb 18, 2022

View reviewed changes

btmills approved these changes Feb 21, 2022

View reviewed changes

bmish added 2 commits March 15, 2022 16:36

schema: false

064335c

clarify when deprecation warning shows

591a4b6

mdjermanovic approved these changes Mar 16, 2022

View reviewed changes

mdjermanovic 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 Mar 16, 2022

ljharb reviewed Mar 16, 2022

View reviewed changes

bmish added 2 commits March 16, 2022 11:56

mention when object-style rules were introduced

e61973e

disallow no-op schemas

db58afb

aladdin-add approved these changes Mar 17, 2022

View reviewed changes

mdjermanovic requested changes Mar 17, 2022

View reviewed changes

no-op schema rule tester only

65ea9ee

mdjermanovic approved these changes Mar 24, 2022

View reviewed changes

btmills merged commit 53d1b2d into eslint:main Mar 24, 2022

bmish mentioned this pull request Apr 5, 2022

feat: add rule test deprecation warnings: fn-style rules/missing schema eslint/eslint#15761

Closed

1 task

snitin315 mentioned this pull request Jun 28, 2022

feat: add deprecation warnings for legacy API in RuleTester eslint/eslint#16063

Merged

1 task

bmish mentioned this pull request Sep 6, 2022

New Rule: no-incomplete-schema eslint-community/eslint-plugin-eslint-plugin#296

Open

This was referenced Dec 5, 2022

feat!: drop support for function-style rules and rules missing schemas eslint/eslint#16614

Closed

refactor: migrate off deprecated function-style rules in all tests eslint/eslint#16618

Merged

bmish mentioned this pull request Dec 17, 2022

Drop support for deprecated, function-style rules bmish/eslint-doc-generator#353

Closed

keithamus mentioned this pull request Jan 13, 2023

move schemas into meta github/eslint-plugin-custom-elements#55

Merged

bmish mentioned this pull request Feb 13, 2024

eslint: tweak documentation for rule meta properties DefinitelyTyped/DefinitelyTyped#68619

Merged

8 tasks

		The two proposals in this RFC should be implemented separately.

		### Detailed design for requiring schemas


		We propose two changes to rules:

		- Requiring rules with options to specify schemas (removing support for rules with options that are missing schemas)

feat!: Require schemas and object-style rules #85

feat!: Require schemas and object-style rules #85

Conversation

bmish commented Dec 13, 2021 • edited

Summary

Demonstrations

Related Issues

nzakas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb Mar 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmish Dec 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzakas left a comment

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btmills left a comment

Choose a reason for hiding this comment

nzakas commented Mar 15, 2022

bmish commented Mar 15, 2022

mdjermanovic left a comment

Choose a reason for hiding this comment

mdjermanovic commented Mar 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmish Mar 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aladdin-add left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment

bmish commented Dec 13, 2021 •

edited

ljharb Mar 16, 2022 •

edited

bmish Dec 25, 2021 •

edited

bmish Mar 16, 2022 •

edited