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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 14 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
68b193d
Fix array-type schema validation
domdomegg Apr 12, 2023
d4ce93b
Actually fix array-type
domdomegg Apr 12, 2023
ff0281c
Add schema tests
domdomegg Apr 12, 2023
168aa06
Make version strings consistent
domdomegg Apr 12, 2023
018163c
Make ajv a devDependency
domdomegg Apr 12, 2023
ea13617
Fix no-restricted-imports schema
domdomegg Apr 12, 2023
937a8e9
Add override options for schemas
domdomegg Apr 12, 2023
03102d2
no-restricted-imports: Assert type once
domdomegg Apr 16, 2023
20ec869
Maybe fix docs build
domdomegg Apr 16, 2023
a0b89ae
Move rule-specific schema tests
domdomegg Apr 16, 2023
2898df4
Fix for ESLint 6
domdomegg Apr 16, 2023
aca2ead
Merge branch 'main' into array-type-schema
domdomegg Apr 16, 2023
a97f9a1
Merge branch 'main' into array-type-schema
domdomegg Apr 17, 2023
4d6f65e
Update packages/eslint-plugin/src/rules/no-restricted-imports.ts
domdomegg Apr 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/eslint-plugin/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
"@types/marked": "*",
"@types/natural-compare-lite": "^1.4.0",
"@types/prettier": "*",
"ajv": "^6.12.6",
"chalk": "^5.0.1",
"cross-fetch": "^3.1.5",
"grapheme-splitter": "^1.0.4",
Expand Down
5 changes: 3 additions & 2 deletions packages/eslint-plugin/src/rules/array-type.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,10 @@ export default util.createRule<Options, MessageIds>({
enum: ['array', 'generic', 'array-simple'],
},
},
prefixItems: [
items: [
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
{
type: 'object',
additionalProperties: false,
properties: {
default: {
$ref: '#/$defs/arrayOption',
Expand All @@ -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
Contributor Author

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.

},
],
type: 'array',
Expand Down
5 changes: 3 additions & 2 deletions packages/eslint-plugin/src/rules/ban-ts-comment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,10 @@ export default util.createRule<[Options], MessageIds>({
],
},
},
prefixItems: [
items: [
{
type: 'object',
additionalProperties: false,
properties: {
'ts-expect-error': {
$ref: '#/$defs/directiveConfigSchema',
Expand All @@ -73,7 +75,6 @@ export default util.createRule<[Options], MessageIds>({
default: defaultMinimumDescriptionLength,
},
},
additionalProperties: false,
},
],
type: 'array',
Expand Down
2 changes: 1 addition & 1 deletion packages/eslint-plugin/src/rules/explicit-member-accessibility.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export default util.createRule<Options, MessageIds>({
$defs: {
accessibilityLevel,
},
prefixItems: [
items: [
{
type: 'object',
properties: {
Expand Down
20 changes: 12 additions & 8 deletions packages/eslint-plugin/src/rules/lines-between-class-members.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,20 @@ const baseRule = getESLintCoreRule('lines-between-class-members');
type Options = util.InferOptionsTypeFromRule<typeof baseRule>;
type MessageIds = util.InferMessageIdsTypeFromRule<typeof baseRule>;

const schema = util.deepMerge(
{ ...baseRule.meta.schema },
{
1: {
exceptAfterOverload: {
type: 'boolean',
default: true,
const schema = Object.values(
Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

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

util.deepMerge(
{ ...baseRule.meta.schema },
{
1: {
properties: {
exceptAfterOverload: {
type: 'boolean',
default: true,
},
},
},
},
},
),
);

export default util.createRule<Options, MessageIds>({
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/rules/no-misused-promises.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ export default util.createRule<Options, MessageId>({
schema: [
{
type: 'object',
additionalProperties: false,
properties: {
checksConditionals: {
type: 'boolean',
Expand Down
1 change: 0 additions & 1 deletion packages/eslint-plugin/src/rules/no-parameter-properties.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ export default util.createRule<Options, MessageIds>({
'public readonly',
],
},
minItems: 1,
Copy link
Contributor Author

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

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 this minItems not good? It's a correct restriction, no?

Copy link
Contributor Author

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.

},
},
additionalProperties: false,
Expand Down
5 changes: 3 additions & 2 deletions packages/eslint-plugin/src/rules/no-restricted-imports.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,18 @@ const allowTypeImportsOptionSchema = {
const schemaForMergeArrayOfStringsOrObjects = {
items: {
anyOf: [
{},
Copy link
Contributor Author

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

Copy link
Contributor Author

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

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

Copy link
Contributor Author

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.

{
additionalProperties: false,
properties: allowTypeImportsOptionSchema,
},
],
},
};
const schemaForMergeArrayOfStringsOrObjectPatterns = {
anyOf: [
{},
{
items: {
additionalProperties: false,
properties: allowTypeImportsOptionSchema,
},
},
Expand All @@ -51,6 +51,7 @@ const schema = deepMerge(
schemaForMergeArrayOfStringsOrObjects,
{
items: {
additionalProperties: false,
properties: {
paths: schemaForMergeArrayOfStringsOrObjects,
patterns: schemaForMergeArrayOfStringsOrObjectPatterns,
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export default util.createRule<Options, MessageIds>({
schema: [
{
type: 'object',
additionalProperties: false,
properties: {
typesToIgnore: {
description: 'A list of type names to ignore.',
Expand Down
5 changes: 2 additions & 3 deletions packages/eslint-plugin/src/rules/parameter-properties.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,22 +52,21 @@ export default util.createRule<Options, MessageIds>({
],
},
},
prefixItems: [
items: [
{
type: 'object',
additionalProperties: false,
properties: {
allow: {
type: 'array',
items: {
$ref: '#/$defs/modifier',
},
minItems: 1,
Copy link
Contributor Author

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

},
prefer: {
enum: ['class-property', 'parameter-property'],
},
},
additionalProperties: false,
},
],
type: 'array',
Expand Down
4 changes: 2 additions & 2 deletions packages/eslint-plugin/src/rules/prefer-readonly.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,13 @@ export default util.createRule<Options, MessageIds>({
},
schema: [
{
allowAdditionalProperties: false,
type: 'object',
additionalProperties: false,
properties: {
onlyInlineLambdas: {
type: 'boolean',
},
},
type: 'object',
},
],
type: 'suggestion',
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/rules/require-array-sort-compare.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export default util.createRule<Options, MessageIds>({
schema: [
{
type: 'object',
additionalProperties: false,
properties: {
ignoreStringArrays: {
description:
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/rules/restrict-template-expressions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export default util.createRule<Options, MessageId>({
schema: [
{
type: 'object',
additionalProperties: false,
properties: {
allowNumber: {
description:
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/rules/sort-type-constituents.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ export default util.createRule<Options, MessageIds>({
schema: [
{
type: 'object',
additionalProperties: false,
properties: {
checkIntersections: {
description: 'Whether to check intersection types.',
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/rules/sort-type-union-intersection-members.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ export default util.createRule<Options, MessageIds>({
schema: [
{
type: 'object',
additionalProperties: false,
properties: {
checkIntersections: {
description: 'Whether to check intersection types.',
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/rules/typedef.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export default util.createRule<[Options], MessageIds>({
schema: [
{
type: 'object',
additionalProperties: false,
properties: {
[OptionKeys.ArrayDestructuring]: { type: 'boolean' },
[OptionKeys.ArrowParameter]: { type: 'boolean' },
Expand Down
91 changes: 91 additions & 0 deletions packages/eslint-plugin/tests/schemas.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import Ajv from 'ajv';

Check failure on line 1 in packages/eslint-plugin/tests/schemas.test.ts

View workflow job for this annotation

GitHub Actions / Lint (lint) 

Run autofix to sort these imports!
import type { JSONSchema4 } from 'json-schema';

import eslintPlugin from '../src';
import { RuleModule } from '@typescript-eslint/utils/src/ts-eslint';

Check failure on line 5 in packages/eslint-plugin/tests/schemas.test.ts

View workflow job for this annotation

GitHub Actions / Lint (lint) 

All imports in the declaration are only used as types. Use `import type`

describe("Validating rule's schemas", () => {
// These two have defaults which cover multiple arguments that are incompatible
const excluded = ['semi', 'func-call-spacing'];
domdomegg marked this conversation as resolved.
Show resolved Hide resolved

for (const [ruleName, rule] of Object.entries(eslintPlugin.rules).filter(
([ruleName]) => !excluded.includes(ruleName),
)) {
test(`${ruleName} must have default options satisfying its own schema`, () => {
if (!isValid(rule, rule.defaultOptions)) {
throw new Error(
`Options failed validation against rule's schema, with errors: ${JSON.stringify(
ajv.errors,
)}`,
);
}
});

test(`${ruleName} rejects arbitrary arguments`, () => {
if (isValid(rule, [{ 'arbitrary-schemas.test.ts': true }])) {
console.log(rule.meta.schema);
throw new Error(`Options succeeded validation for arbitrary options`);
}
});
}
});

describe('Individual rule schema validation', () => {
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
// https://github.com/typescript-eslint/typescript-eslint/issues/6852
test("array-type does not accept 'simple-array' option", () => {
const rule = eslintPlugin.rules['array-type'];

if (isValid(rule, [{ default: 'simple-array' }])) {
throw new Error(`Options succeeded validation for bad options`);
}
});

// https://github.com/typescript-eslint/typescript-eslint/issues/6892
test('array-type does not accept non object option', () => {
const rule = eslintPlugin.rules['array-type'];

if (isValid(rule, ['array'])) {
throw new Error(`Options succeeded validation for bad options`);
}
});
});

const ajv = new Ajv({ async: false });

function isValid(
rule: RuleModule<string, unknown[]>,
options: unknown,
): boolean {
const normalizedSchema = normalizeSchema(rule.meta.schema);

const valid = ajv.validate(normalizedSchema, options);
if (typeof valid !== 'boolean') {
// Schema could not validate options synchronously. This is not allowed for ESLint rules.
return false;
}

return valid;
}

function normalizeSchema(
schema: JSONSchema4 | readonly JSONSchema4[],
): JSONSchema4 {
if (!Array.isArray(schema)) {
return schema;
}

if (schema.length === 0) {
return {
type: 'array',
minItems: 0,
maxItems: 0,
};
}

return {
type: 'array',
items: schema,
minItems: 0,
maxItems: schema.length,
};
}
5 changes: 4 additions & 1 deletion packages/utils/src/ts-eslint/Rule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,11 @@ interface RuleMetaData<TMessageIds extends string> {
replacedBy?: readonly string[];
/**
* The options schema. Supply an empty array if there are no options.
* $ref is not supported in arrays, so is omitted.
* See: https://github.com/typescript-eslint/typescript-eslint/pull/5531
* See: https://eslint.org/docs/latest/extend/custom-rules#options-schemas
*/
schema: JSONSchema4 | readonly JSONSchema4[];
schema: JSONSchema4 | readonly Omit<JSONSchema4, '$ref'>[];
}

interface RuleFix {
Expand Down
2 changes: 1 addition & 1 deletion yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4307,7 +4307,7 @@ ajv-keywords@^5.0.0:
dependencies:
fast-deep-equal "^3.1.3"

ajv@^6.10.0, ajv@^6.12.2, ajv@^6.12.4, ajv@^6.12.5, ajv@~6.12.6:
ajv@^6.10.0, ajv@^6.12.2, ajv@^6.12.4, ajv@^6.12.5, ajv@^6.12.6, ajv@~6.12.6:
version "6.12.6"
resolved "https://registry.yarnpkg.com/ajv/-/ajv-6.12.6.tgz#baf5a62e802b07d977034586f8c3baf5adf26df4"
integrity sha512-j3fVLgvTo527anyYyJOGTYJbG+vnnQYvE0m5mmkc1TK+nxAppkCLMIL0aZ4dblVCNoGShhm+kzE4ZUykBoMg4g==
Expand Down