fix(eslint-plugin): fix schemas across several rules and add schema tests #6894

domdomegg · 2023-04-12T21:12:47Z

PR Checklist

Addresses an existing open issue: fixes Bug: [array-type] the config simple-array should be disallowed by the schema #6852, Bug: [array-type] Does not validate shape of config #6892
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

This PR became a lot bigger than I was expecting, and was much more fiddly than I expected too. If we want, we can take d4ce93b on its own, and the rest of the changes as a separate PR - but then it won't have tests to enforce that the schema stays fixed.

### Specific array-type problem

The ultimate problem was that we were using prefixItems rather than items. This was a problem because eslint uses an old version of ajv (v6) and and old version of the JSON schema spec (v4). See eslint/eslint#13888 (comment) and https://github.com/eslint/eslint/blob/9d1b8fc60cc31f12618e58c10a2669506b7ce9bf/lib/shared/ajv.js#L12

As noted on https://json-schema.org/understanding-json-schema/reference/array.html#tuple-validation v4 of the spec didn't actually have prefixItems, and instead the items value was assumed to be prefixItems if it was an array of schemas:

In Draft 4 - 2019-09, tuple validation was handled by an alternate form of the items keyword. When items was an array of schemas instead of a single schema, it behaved the way prefixItems behaves.

This in turn I think made the schema fail silently, and it just was okay validating any old random value you'd pass in.

I've fixed this by changing from prefixItems to items.

Adding tests

Actually figuring this out was pretty difficult. To help me get there, I wrote some tests that validated the schemas generally against the same options as eslint.

This helped find other problems with array-type, which was that you could provide any arbitrary arguments as it did not have additionalProperties disabled.

Expanding this across all the rules, using their defaultOptions as sample config (which isn't a perfect solution, but at least is easy and works in every case except 2), managed to catch a few other badly configured rules. I've addressed these problems, applying the same fixes. I've left some comments in the PR to explain changes and why they were necessary too.

The tests are by no means perfect. They don't cover every edge case, and it's very possible to have nested objects with additional arbitrary properties that shouldn't be there. But they are a step in the right direction and what I had time to implement right now :)

Note on adding ajv dependency

This is added as a devDependency, as its only used in tests. Additionally, as eslint uses the same version this doesn't actually add a real dependency, it just makes it explicit - so it doesn't slow down installs etc.

typescript-eslint · 2023-04-12T21:12:50Z

Thanks for the PR, @domdomegg!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

netlify · 2023-04-12T21:12:54Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`4d6f65e`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/643d5eeacd14040008c464f2
😎 Deploy Preview	https://deploy-preview-6894--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

domdomegg · 2023-04-12T21:13:57Z

packages/eslint-plugin/src/rules/array-type.ts

@@ -124,7 +126,6 @@ export default util.createRule<Options, MessageIds>({
                'The array type expected for readonly cases. If omitted, the value for `default` will be used.',
            },
          },
-          type: 'object',


Moved this up to the top for consistency with most of the other rules I saw.

nx-cloud · 2023-04-12T21:16:42Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 4d6f65e. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 25 targets

Sent with 💌 from NxCloud.

domdomegg · 2023-04-12T21:17:27Z

packages/eslint-plugin/src/rules/lines-between-class-members.ts

-      exceptAfterOverload: {
-        type: 'boolean',
-        default: true,
+const schema = Object.values(


Before, we had:

{ "0": { /* schema A */ }, "1": { /* schema B */ } }

AJV is actually not able to interpret this correctly, and no validation actually occurs.

This change fixes it to:

[{ /* schema A */ }, { /* schema B */ }]

Using Object.values here should be safe. The baseRule.meta.schema is an array itself, so will be spread in order (guaranteed by ECMA spec). deepMerge will merge the keys in the order of the first object. Object.values will take the values in order, so we'll get back the array in the right order.

[Praise] Nifty trick to use Object.values to convert the string index style object to an array!

domdomegg · 2023-04-12T21:19:51Z

packages/eslint-plugin/src/rules/no-parameter-properties.ts

@@ -51,7 +51,6 @@ export default util.createRule<Options, MessageIds>({
                'public readonly',
              ],
            },
-            minItems: 1,


Removed this constraint so the default is able to be validated against the schema.

Alternative could be to exclude this from the schema validation list, or have an 'override' schema for testing against the schema.

I don't think this matters for actual usage, and I think it's not harmful: in fact it might be useful in some configs to be able to specify some allowed things by default, and then specify none to be allowed in an override config explicitly to be clear about why it's there.

Sorry, I don't follow - why is this minItems not good? It's a correct restriction, no?

For this PR, the main reason I did it is beacuse it would prefer the default options from validating against the schema itself, and this is a simple way of getting around that.

But also I think semantically it makes sense that it should accept an array of nothing for the exceptions allowed, meaning that no parameter properties are allowed.

domdomegg · 2023-04-12T21:23:35Z

packages/eslint-plugin/src/rules/no-restricted-imports.ts

@@ -27,18 +27,18 @@ const allowTypeImportsOptionSchema = {
 const schemaForMergeArrayOfStringsOrObjects = {
  items: {
    anyOf: [
-      {},


These seem incorrect and unnecessary. I think the intention was for this to merge with the base anyOf items, but deepMerge doesn't handle array items: https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/utils/src/eslint-utils/deepMerge.ts

Although thinking about this now, I'm not certain the updated rule is 100% correct, would appreciate careful eyes on it. Might need to refactor how this extends from the base.

Have redone this. It's fairly horrible how it indexes into the baseRule - happy to take feedback on better approaches.

Yeah seems like you're certainly following the standard practice: https://github.com/eslint/eslint/blob/1fea2797801a82a2718814c83dad641dab092bcc/lib/rules/no-restricted-imports.js#L19.

You might want to assert const baseSchema = baseRule.meta.schema as SomeTypeDescribingWhatWeKnow, since this is making an assumption on the base rule's schema type.

Good idea, have done this.

domdomegg · 2023-04-12T21:23:52Z

packages/eslint-plugin/src/rules/parameter-properties.ts

          properties: {
            allow: {
              type: 'array',
              items: {
                $ref: '#/$defs/modifier',
              },
-              minItems: 1,


Same minItems reasoning as before

packages/eslint-plugin/tests/schemas.test.ts

codecov · 2023-04-12T22:27:16Z

Codecov Report

Merging #6894 (aca2ead) into main (276c17b) will increase coverage by 0.00%.
The diff coverage is 93.33%.

❗ Current head aca2ead differs from pull request most recent head 4d6f65e. Consider uploading reports for the commit 4d6f65e to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6894   +/-   ##
=======================================
  Coverage   87.37%   87.37%           
=======================================
  Files         386      386           
  Lines       13192    13201    +9     
  Branches     3867     3867           
=======================================
+ Hits        11526    11534    +8     
- Misses       1300     1301    +1     
  Partials      366      366

Flag	Coverage Δ
unittest	`87.37% <93.33%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/eslint-plugin/src/rules/array-type.ts	`97.14% <ø> (ø)`
packages/eslint-plugin/src/rules/ban-ts-comment.ts	`96.96% <ø> (ø)`
...-plugin/src/rules/explicit-member-accessibility.ts	`97.43% <ø> (ø)`
...ges/eslint-plugin/src/rules/no-misused-promises.ts	`97.47% <ø> (ø)`
...eslint-plugin/src/rules/no-parameter-properties.ts	`94.11% <ø> (ø)`
...-plugin/src/rules/no-unnecessary-type-assertion.ts	`94.66% <ø> (ø)`
...es/eslint-plugin/src/rules/parameter-properties.ts	`94.11% <ø> (ø)`
...ackages/eslint-plugin/src/rules/prefer-readonly.ts	`99.09% <ø> (ø)`
...int-plugin/src/rules/require-array-sort-compare.ts	`88.23% <ø> (ø)`
...-plugin/src/rules/restrict-template-expressions.ts	`100.00% <ø> (ø)`
... and 5 more

JoshuaKGoldberg

Really digging this direction in general - especially with the added test coverage! Thanks for sending!

Gave a general / high-level review, but since generated-rule-docs.ts is breaking, I'll wait to get more detailed. Hopefully getting that to work isn't too tricky? Explicitly marking this PR as draft till then - happy to re-review once it is (or give input if getting it to work is unclear). Cheers!

packages/eslint-plugin/src/rules/array-type.ts

JoshuaKGoldberg · 2023-04-16T16:34:51Z

packages/eslint-plugin/src/rules/lines-between-class-members.ts

-      exceptAfterOverload: {
-        type: 'boolean',
-        default: true,
+const schema = Object.values(


[Praise] Nifty trick to use Object.values to convert the string index style object to an array!

packages/eslint-plugin/tests/schemas.test.ts

domdomegg · 2023-04-17T10:26:01Z

@JoshuaKGoldberg thanks for the quick review! I think I've got everything working now and have addressed the initial feedback, so it's ready for a more detailed review.

JoshuaKGoldberg

LGTM, thanks! Since @bradzacher has been looking in this area, I'll defer to Brad's second opinion for a review.

packages/eslint-plugin/src/rules/no-restricted-imports.ts

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

bradzacher · 2023-04-20T12:14:17Z

What I'll get you to do is rebase this on top of #6939 (which includes #6899 that's already landed in v6).
I've already merged your schema changes in as I built out the schema gen tooling - so what we're missing now is the explicit schema test cases.

So if you rebase this on my branch, we'll be able to merge your awesome validation tests into v6 and we can release it all together!

Thanks so much for the work you did here - it was a good set of pointers for me to look for in the generated types when writing the schema gen tooling 😄

domdomegg · 2023-04-20T14:21:19Z

What I'll get you to do is rebase this on top of #6939 (which includes #6899 that's already landed in v6). I've already merged your schema changes in as I built out the schema gen tooling - so what we're missing now is the explicit schema test cases.

So if you rebase this on my branch, we'll be able to merge your awesome validation tests into v6 and we can release it all together!

Sounds good! I'll wait for #6939 to be merged first, then rebase on top of v6 as soon as that's done. Just tried rebasing this and it was quite a pain, so probably easiest just to recreate the PR once things are more stable in this area :)

bradzacher · 2023-04-21T05:36:34Z

PRs are both merged so feel free to rebase on v6 at your leisure :)

domdomegg · 2023-04-22T13:12:34Z

Closing in favour of #6947

domdomegg added 5 commits April 12, 2023 20:26

Fix array-type schema validation

68b193d

Actually fix array-type

d4ce93b

Add schema tests

ff0281c

Make version strings consistent

168aa06

Make ajv a devDependency

018163c

domdomegg commented Apr 12, 2023

View reviewed changes

packages/eslint-plugin/tests/schemas.test.ts Outdated Show resolved Hide resolved

domdomegg changed the title ~~fix(eslint-plugin): Fix schemas across several rules, add schema tests~~ fix(eslint-plugin): [multiple] Fix schemas across several rules, and add schema tests Apr 12, 2023

domdomegg changed the title ~~fix(eslint-plugin): [multiple] Fix schemas across several rules, and add schema tests~~ fix(eslint-plugin): [multiple] Fix schemas across several rules and add schema tests Apr 12, 2023

domdomegg changed the title ~~fix(eslint-plugin): [multiple] Fix schemas across several rules and add schema tests~~ fix(eslint-plugin): fix schemas across several rules and add schema tests Apr 12, 2023

domdomegg added 2 commits April 12, 2023 23:07

Fix no-restricted-imports schema

ea13617

Add override options for schemas

937a8e9

bradzacher added the bug Something isn't working label Apr 14, 2023

JoshuaKGoldberg requested changes Apr 16, 2023

View reviewed changes

JoshuaKGoldberg marked this pull request as draft April 16, 2023 16:39

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Apr 16, 2023

domdomegg added 6 commits April 16, 2023 19:03

no-restricted-imports: Assert type once

03102d2

Maybe fix docs build

20ec869

Move rule-specific schema tests

a0b89ae

Fix for ESLint 6

2898df4

Merge branch 'main' into array-type-schema

aca2ead

Merge branch 'main' into array-type-schema

a97f9a1

domdomegg marked this pull request as ready for review April 17, 2023 10:26

JoshuaKGoldberg previously approved these changes Apr 17, 2023

View reviewed changes

packages/eslint-plugin/src/rules/no-restricted-imports.ts Outdated Show resolved Hide resolved

JoshuaKGoldberg added 1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready and removed awaiting response Issues waiting for a reply from the OP or another party labels Apr 17, 2023

domdomegg dismissed JoshuaKGoldberg’s stale review via 4d6f65e April 17, 2023 14:59

Update packages/eslint-plugin/src/rules/no-restricted-imports.ts

4d6f65e

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

This was referenced Apr 20, 2023

fix(eslint-plugin): refactor schemas that use $ref #6896

Closed

fix: correct handling of additionalProperties in object schemas #6939

Merged

domdomegg changed the base branch from main to v6 April 20, 2023 14:08

domdomegg marked this pull request as draft April 20, 2023 14:21

domdomegg mentioned this pull request Apr 22, 2023

fix(eslint-plugin): fix schemas across several rules and add schema tests #6947

Merged

3 tasks

domdomegg closed this Apr 22, 2023

github-actions bot locked as resolved and limited conversation to collaborators Apr 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): fix schemas across several rules and add schema tests #6894

fix(eslint-plugin): fix schemas across several rules and add schema tests #6894

domdomegg commented Apr 12, 2023

typescript-eslint bot commented Apr 12, 2023

netlify bot commented Apr 12, 2023 •

edited

domdomegg Apr 12, 2023

nx-cloud bot commented Apr 12, 2023 •

edited

domdomegg Apr 12, 2023

JoshuaKGoldberg Apr 16, 2023 •

edited

domdomegg Apr 12, 2023

JoshuaKGoldberg Apr 16, 2023

domdomegg Apr 16, 2023

domdomegg Apr 12, 2023

domdomegg Apr 12, 2023

JoshuaKGoldberg Apr 16, 2023 •

edited

domdomegg Apr 16, 2023

domdomegg Apr 12, 2023

codecov bot commented Apr 12, 2023 •

edited

JoshuaKGoldberg left a comment

JoshuaKGoldberg Apr 16, 2023 •

edited

domdomegg commented Apr 17, 2023

JoshuaKGoldberg left a comment

bradzacher commented Apr 20, 2023

domdomegg commented Apr 20, 2023

bradzacher commented Apr 21, 2023

domdomegg commented Apr 22, 2023

fix(eslint-plugin): fix schemas across several rules and add schema tests #6894

fix(eslint-plugin): fix schemas across several rules and add schema tests #6894

Conversation

domdomegg commented Apr 12, 2023

PR Checklist

Overview

Adding tests

Note on adding ajv dependency

typescript-eslint bot commented Apr 12, 2023

netlify bot commented Apr 12, 2023 • edited

✅ Deploy Preview for typescript-eslint ready!

Choose a reason for hiding this comment

nx-cloud bot commented Apr 12, 2023 • edited

☁️ Nx Cloud Report

Choose a reason for hiding this comment

JoshuaKGoldberg Apr 16, 2023 • 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

Choose a reason for hiding this comment

JoshuaKGoldberg Apr 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 12, 2023 • edited

Codecov Report

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Apr 16, 2023 • edited

Choose a reason for hiding this comment

domdomegg commented Apr 17, 2023

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

bradzacher commented Apr 20, 2023

domdomegg commented Apr 20, 2023

bradzacher commented Apr 21, 2023

domdomegg commented Apr 22, 2023

netlify bot commented Apr 12, 2023 •

edited

nx-cloud bot commented Apr 12, 2023 •

edited

JoshuaKGoldberg Apr 16, 2023 •

edited

JoshuaKGoldberg Apr 16, 2023 •

edited

codecov bot commented Apr 12, 2023 •

edited

JoshuaKGoldberg Apr 16, 2023 •

edited