This PR became a lot bigger than I was expecting, and was much more fiddly than I expected too. If we want, we can taked4ce93b 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 usingprefixItems rather thanitems. This was a problem because eslint uses an old version of ajv (v6) and and old version of the JSON schema spec (v4). Seeeslint/eslint#13888 (comment) andhttps://github.com/eslint/eslint/blob/9d1b8fc60cc31f12618e58c10a2669506b7ce9bf/lib/shared/ajv.js#L12

As noted onhttps://json-schema.org/understanding-json-schema/reference/array.html#tuple-validation v4 of the spec didn't actually haveprefixItems, and instead theitems 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 fromprefixItems toitems.

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.

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

Copy link

Contributor

typescript-eslintbot commentedApr 12, 2023

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.

Copy link

netlifybot commentedApr 12, 2023•
edited
Loading

✅ Deploy Preview fortypescript-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 yourNetlify site settings.

domdomegg commented

Apr 12, 2023

View reviewed changes

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

Copy link

ContributorAuthor

domdomeggApr 12, 2023

Choose a reason for hiding this comment

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

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

Copy link

nx-cloudbot commentedApr 12, 2023•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit4d6f65e. 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 💌 fromNxCloud.

domdomegg commented

Apr 12, 2023

View reviewed changes

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

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

Copy link

ContributorAuthor

domdomeggApr 12, 2023

Choose a reason for hiding this comment

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

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.

Copy link

Member

JoshuaKGoldbergApr 16, 2023•
edited
Loading

Choose a reason for hiding this comment

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

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

domdomegg commented

Apr 12, 2023

View reviewed changes

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

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

Copy link

ContributorAuthor

domdomeggApr 12, 2023

Choose a reason for hiding this comment

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

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.

Copy link

Member

JoshuaKGoldbergApr 16, 2023

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

domdomeggApr 16, 2023

Choose a reason for hiding this comment

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

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 commented

Apr 12, 2023

View reviewed changes

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

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

Copy link

ContributorAuthor

domdomeggApr 12, 2023

Choose a reason for hiding this comment

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

These seem incorrect and unnecessary. I think the intention was for this to merge with the baseanyOf 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.

Copy link

ContributorAuthor

domdomeggApr 12, 2023

Choose a reason for hiding this comment

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

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

Copy link

Member

JoshuaKGoldbergApr 16, 2023•
edited
Loading

Choose a reason for hiding this comment

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

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 assertconst baseSchema = baseRule.meta.schema as SomeTypeDescribingWhatWeKnow, since this is making an assumption on the base rule's schema type.

Copy link

ContributorAuthor

domdomeggApr 16, 2023

Choose a reason for hiding this comment

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

Good idea, have done this.

domdomegg commented

Apr 12, 2023

View reviewed changes

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

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

Copy link

ContributorAuthor

domdomeggApr 12, 2023

Choose a reason for hiding this comment

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

Same minItems reasoning as before

domdomegg commented

Apr 12, 2023

View reviewed changes

packages/eslint-plugin/tests/schemas.test.ts OutdatedShow resolvedHide 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 added2 commits

April 12, 2023 23:07

Fix no-restricted-imports schema

ea13617

Add override options for schemas

937a8e9

Copy link

codecovbot commentedApr 12, 2023•
edited
Loading

Codecov Report

Merging#6894 (aca2ead) intomain (276c17b) willincrease coverage by0.00%.
The diff coverage is93.33%.

❗ Current headaca2ead 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% <ø> (ø)`
... and5 more

bradzacher added the bugSomething isn't working label

Apr 14, 2023

JoshuaKGoldberg requested changes

Apr 16, 2023

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

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

Gave a general / high-level review, but sincegenerated-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.tsShow resolvedHide resolved

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

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

Copy link

Member

JoshuaKGoldbergApr 16, 2023•
edited
Loading

Choose a reason for hiding this comment

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

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

packages/eslint-plugin/tests/schemas.test.tsShow resolvedHide resolved

packages/eslint-plugin/tests/schemas.test.ts OutdatedShow resolvedHide resolved

JoshuaKGoldberg marked this pull request as draft

April 16, 2023 16:39

JoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party label

Apr 16, 2023

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

Copy link

ContributorAuthor

domdomegg commentedApr 17, 2023

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

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

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

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 OutdatedShow resolvedHide resolved

JoshuaKGoldberg added 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge and removed awaiting responseIssues waiting for a reply from the OP or another party labels

Apr 17, 2023

domdomegg dismissedJoshuaKGoldberg’sstale review via4d6f65e

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 referencedApr 20, 2023

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

Closed

fix: correct handling of additionalProperties in object schemas#6939

Merged

Copy link

Member

bradzacher commentedApr 20, 2023

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 changed the base branch frommain tov6

April 20, 2023 14:08

Copy link

ContributorAuthor

domdomegg commentedApr 20, 2023

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

domdomegg marked this pull request as draft

April 20, 2023 14:21

Copy link

Member

bradzacher commentedApr 21, 2023

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

domdomegg mentioned this pull request

Apr 22, 2023

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

Merged

3 tasks

Copy link

ContributorAuthor

domdomegg commentedApr 22, 2023

Closing in favour of#6947

domdomegg closed this

Apr 22, 2023

github-actionsbot locked asresolvedand limited conversation to collaborators

Apr 30, 2023

Labels

1 approval

>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge

bug

Something isn't working

3 participants

Movatterモバイル変換

Uh oh!

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

Uh oh!

Conversation

domdomegg commentedApr 12, 2023

PR Checklist

Overview

Adding tests

Note on adding ajv dependency

Uh oh!

typescript-eslintbot commentedApr 12, 2023

Uh oh!

netlifybot commentedApr 12, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

✅ Deploy Preview fortypescript-eslint ready!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nx-cloudbot commentedApr 12, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

☁️ Nx Cloud Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldbergApr 16, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldbergApr 16, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecovbot commentedApr 12, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoshuaKGoldbergApr 16, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

domdomegg commentedApr 17, 2023

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bradzacher commentedApr 20, 2023

Uh oh!

domdomegg commentedApr 20, 2023

Uh oh!

bradzacher commentedApr 21, 2023

Uh oh!

domdomegg commentedApr 22, 2023

Uh oh!

Uh oh!

netlifybot commentedApr 12, 2023•
edited
Loading

nx-cloudbot commentedApr 12, 2023•
edited
Loading

JoshuaKGoldbergApr 16, 2023•
edited
Loading

JoshuaKGoldbergApr 16, 2023•
edited
Loading

codecovbot commentedApr 12, 2023•
edited
Loading

JoshuaKGoldbergApr 16, 2023•
edited
Loading