feat!: Set default `schema: []`, drop support for function-style rules #17792

mdjermanovic · 2023-11-25T15:24:01Z

Prerequisites checklist

I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:

Fixes #14709

Implements RFC Require schemas and object-style rules

This includes changes in the @eslint/eslintrc package: eslint/eslintrc#139

What changes did you make? (Give an overview)

Drop support for function-style rules:

Removed all places where function-style rules were being converted to object-style. Function-style rules will be passed in their original form to the rest of the code and eventually crash in the Linter.
In Linter, I added the following check right before running the rule's create() method:
if (!rule || typeof rule !== "object" || typeof rule.create !== "function") then throw an error.
This is to show a message that is more descriptive than TypeError: rule.create is not a function.
Since rule testers always wrap rules, I added the same check to rule testers.
Removed all the content from custom-rules-deprecated dost, just left a link to custom-rules, and added a note that the function style is no longer supported.
Removed function-style removal warnings from RuleTester, and the check that function-style rules cannot produce fixes from both rule testers.

Schema changes:

Updated all implementations of GetRuleOptionsSchema (flat config, eslintrc, and the one used by RuleTester) to work as follows:
- Top-level schema property is ignored.
- meta.schema:
  - Can be omitted, in which case this function returns a schema that disallows any options so that ESLint throws an error if any options are passed. Effectively the same as schema: []. schema: undefined is treated the same as omitted.
  - If specified, must be an array, object, or false. If any other value is specified (null, true, number...), this function throws an error.
  - Objects and arrays work the same as before.
  - For schema: false, this function returns null, meaning that options validation is disabled for the rule. The same as omitted (or falsy) before this change.
Changed eslintrc and flat config to throw a different error when options validation fails because of an invalid schema.
Changed Linter to recognize that error and rethrow it instead of creating a lint message for invalid inline config.
Changed both rule testers to check and throw an error for schema: {}.
Changed schema of the no-constructor-return rule, from {} (no-op) to [] (disallow passing options).
Removed missing schema warnings from RuleTester.
Updated custom-rules docs with the schema changes.

Is there anything you'd like reviewers to focus on?

Did I miss some places that should also be updated for either of these changes?
Function objects with a create method will also be disallowed. After this change, typeof rule must be "object".
Should the rules be validated (checked if the rule is an object with a create method) at some earlier point?

netlify · 2023-11-25T15:24:07Z

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	`27b4448`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/658c66e74c13ec0008458bfb

bmish · 2023-11-26T19:04:48Z

Thank you for picking this work up! I see you still have a few items on your todo list. My original draft PR may be helpful to reference: #16614

bmish · 2023-11-26T18:39:46Z

docs/src/extend/custom-rules-deprecated.md

In general, should we avoid deleting pages to avoid breaking links, and just add a placeholder page linking to the replacement page?

Good idea, I've added back and updated this document in d14e2b5.

It looks like we also could do a redirect like this: eslint/eslint.org#499

Discussion here: #17840 (comment)

It looks like we also could do a redirect like this: eslint/eslint.org#499

Discussion here: #17840 (comment)

Maybe, but this is a slightly different situation so I'm not sure.

For configuration-files-new, its content is moved to configuration-files, so redirecting makes the most sense.

For custom-rules-deprecated, its content is not moved to custom-rules but deleted, so info that it no longer applies in ESLint 9+ might be more helpful.

bmish · 2023-11-26T18:46:46Z

lib/rule-tester/rule-tester.js

-        );
-    }
-}
-
 /**
 * Emit a deprecation warning if rule has options but is missing the "meta.schema" property


Should be able to delete emitMissingSchemaWarning too.

Yes, I kept that for now to see if the tests for it will fail as expected after the schema default change (wip).

Done in 24144d4

bmish · 2023-11-26T18:49:11Z

tests/lib/linter/rules.js

@@ -32,18 +32,6 @@ describe("rules", () => {
            assert.ok(rules.get(ruleId));
        });

-        it("should return the rule as an object with a create() method if the rule was defined as a function", () => {


In my draft implementation, I updated this test to check that it throws: https://github.com/eslint/eslint/pull/16614/files#diff-0c938400b183b70017038162a127cef7bc753f98c6e2ccc97800bce0a0effcc8 But maybe you omitted this because it's tested in several other places?

Yes, I added a more general check (rule must be an object with a create method) in Linter. A test case where Rules#define is used (via Linter#defineRule) is here.

mdjermanovic · 2023-11-30T14:23:46Z

This is ready for review.

bmish

Looks great! Thanks for picking up this work.

bmish · 2023-11-30T22:24:47Z

lib/rule-tester/flat-rule-tester.js

+             * At this point, `schema` can only be an object or `null`.
+             */
+            if (schema && Object.keys(schema).length === 0) {
+                throw new Error(`\`schema: {}\` is a no-op${metaSchemaDescription}`);


Disallowing schema: {} is most important since it's the most common, accidental, no-op schema.

I'm fine with this one check for now, but just want to ask if any other obvious/common no-op schemas come to mind? In the RFC, I mentioned schema: { type: "array" } as another no-op schema we could disallow, with the intention being to leave the door open to any others that we judge worth checking for. Obviously, we can only add update the banned no-op list during major versions.

is there a related discussion in ajv repo? it could benefit all the dependents (not just eslint) - if it could be supported by ajv.

I did a brief search and didn't immediately find a relevant discussion in in ajv. But I have included a lot more context in this issue discussing a potential lint rule:

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

Overall, we should probably just allow any schema in ESLint except extremely common and obvious mistakes like {}, and consider linting to detect further possible problems.

Newer versions of Ajv have some built-in validations, but there were backwards compatibility issues when we tried to switch to Ajv 7+ (#13911). However, I believe schema: { type: "array" } would be considered valid by Ajv. It's just useless in ESLint because we're always passing arrays to these validations.

I agree that for now RuleTester should only check for the common and obvious mistake schema: {}.

github-actions · 2023-12-10T22:31:16Z

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

nzakas · 2023-12-20T23:10:53Z

Looks like there are some merge conflicts here.

mdjermanovic · 2023-12-21T09:12:06Z

I'll rebase when we merge a few more PRs that target the same files. This shouldn't be merged until we release @eslint/eslintrc (eslint/eslintrc#139) anyway.

…eRule

mdjermanovic · 2023-12-26T08:57:54Z

Merge conflicts are fixed now.

package.json

mdjermanovic · 2023-12-27T18:29:05Z

This is ready now. @nzakas would you like to take a look before merging?

nzakas

LGTM. Please be sure to update the migration guide with info about this change.

eslint-github-bot bot added breaking This change is backwards-incompatible feature This change adds a new feature to ESLint labels Nov 25, 2023

bmish reviewed Nov 26, 2023

View reviewed changes

mdjermanovic changed the title ~~feat!: Add default schema: [], drop support for function-style rules~~ feat!: Set default schema: [], drop support for function-style rules Nov 29, 2023

mdjermanovic mentioned this pull request Nov 29, 2023

feat!: Set default schema: [], drop support for function-style rules eslint/eslintrc#139

Merged

bmish approved these changes Nov 30, 2023

View reviewed changes

bmish mentioned this pull request Dec 3, 2023

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

Closed

1 task

github-actions bot added the Stale label Dec 10, 2023

bmish assigned mdjermanovic Dec 10, 2023

bmish removed the Stale label Dec 10, 2023

bmish mentioned this pull request Dec 11, 2023

docs: Switch to flat config by default #17840

Merged

1 task

mdjermanovic mentioned this pull request Dec 19, 2023

Bug: fix rule schemas #16879

Closed

3 tasks

mdjermanovic added core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion labels Dec 19, 2023

nzakas marked this pull request as ready for review December 20, 2023 23:10

nzakas requested a review from a team as a code owner December 20, 2023 23:10

mdjermanovic added 8 commits December 26, 2023 09:16

drop support for function-style rules in flat-rule-tester

9d924e3

drop support for function-style rules in rule-tester

c7a0b42

drop support for function-style rules in config-rule (eslint-fuzzer)

0964578

add rule object checks to rule testers

b37aeb4

drop support for function-style rules in flat config and Linter#defin…

1658e6f

…eRule

update JSDoc

d868180

add rule object checks to Linter

47c060b

drop support for function-style rules in eslintrc

cd2caa0

mdjermanovic added 12 commits December 26, 2023 09:23

remove custom-rules-deprecated.md

3cc25bb

update flat config getRuleOptionsSchema for schema changes

f7c4f8d

add back and update custom-rules-deprecated.md

1671198

show ruleId in error messages for invalid schema

e3ba846

throw error in Linter if rule has invalid schema

91fe762

add short description of meta.schema to type errors in flat-rule-tester

5d6f96f

throw error for empty object schema in flat-rule-tester

b3fa339

update rule-tester for schema changes

dabe3ef

add integration tests for eslintrc schema changes

e078476

add more flat-config-array unit tests for schema changes

78145ec

throw error in Linter in eslintrc mode if rule has invalid schema

d268d81

update docs for schema changes

a92d4ab

mdjermanovic force-pushed the issue14709 branch from ea58146 to a92d4ab Compare December 26, 2023 08:35

mdjermanovic commented Dec 26, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

bmish approved these changes Dec 27, 2023

View reviewed changes

use eslintrc v3

27b4448

nzakas approved these changes Dec 27, 2023

View reviewed changes

nzakas merged commit fb81b1c into main Dec 27, 2023
17 checks passed

nzakas deleted the issue14709 branch December 27, 2023 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Set default `schema: []`, drop support for function-style rules #17792

feat!: Set default `schema: []`, drop support for function-style rules #17792

mdjermanovic commented Nov 25, 2023 •

edited

netlify bot commented Nov 25, 2023 •

edited

bmish commented Nov 26, 2023

bmish Nov 26, 2023

mdjermanovic Nov 27, 2023

bmish Dec 12, 2023

mdjermanovic Dec 21, 2023

bmish Nov 26, 2023

mdjermanovic Nov 27, 2023

mdjermanovic Nov 29, 2023

bmish Nov 26, 2023

mdjermanovic Nov 28, 2023

mdjermanovic commented Nov 30, 2023

bmish left a comment

bmish Nov 30, 2023

aladdin-add Dec 18, 2023

bmish Dec 18, 2023

mdjermanovic Dec 21, 2023

github-actions bot commented Dec 10, 2023

nzakas commented Dec 20, 2023

mdjermanovic commented Dec 21, 2023

mdjermanovic commented Dec 26, 2023

mdjermanovic commented Dec 27, 2023

nzakas left a comment

feat!: Set default schema: [], drop support for function-style rules #17792

feat!: Set default schema: [], drop support for function-style rules #17792

Conversation

mdjermanovic commented Nov 25, 2023 • edited

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

netlify bot commented Nov 25, 2023 • edited

✅ Deploy Preview for docs-eslint canceled.

bmish commented Nov 26, 2023

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 commented Nov 30, 2023

bmish 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

github-actions bot commented Dec 10, 2023

nzakas commented Dec 20, 2023

mdjermanovic commented Dec 21, 2023

mdjermanovic commented Dec 26, 2023

mdjermanovic commented Dec 27, 2023

nzakas left a comment

Choose a reason for hiding this comment

feat!: Set default `schema: []`, drop support for function-style rules #17792

feat!: Set default `schema: []`, drop support for function-style rules #17792

mdjermanovic commented Nov 25, 2023 •

edited

netlify bot commented Nov 25, 2023 •

edited