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

feat(eslint-plugin): [no-magic-numbers] ignoreTypeIndexes option #4789

Merged
merged 5 commits into from Apr 15, 2022
Merged

feat(eslint-plugin): [no-magic-numbers] ignoreTypeIndexes option #4789

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d0cabc2
feat(eslint-plugin): [no-magic-numbers] ignoreTypeIndexes option
davecardwell Apr 5, 2022
d1e65be
chore: refactor rule to report violations early
davecardwell Apr 5, 2022
8ec2104
Merge branch 'typescript-eslint:main' into feat-no_magic_numbers-igno…
davecardwell Apr 13, 2022
af1ddb7
feat: [no-magic-numbers] handle intersections & nested typed indexed …
davecardwell Apr 15, 2022
88bd7e4
Merge branch 'main' into feat-no_magic_numbers-ignore_array_indexes
JoshuaKGoldberg Apr 15, 2022
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
24 changes: 24 additions & 0 deletions packages/eslint-plugin/docs/rules/no-magic-numbers.md
View file
Open in desktop
Expand Up @@ -36,13 +36,15 @@ interface Options extends BaseNoMagicNumbersOptions {
ignoreEnums?: boolean;
ignoreNumericLiteralTypes?: boolean;
ignoreReadonlyClassProperties?: boolean;
ignoreTypeIndexes?: boolean;
}

const defaultOptions: Options = {
...baseNoMagicNumbersDefaultOptions,
ignoreEnums: false,
ignoreNumericLiteralTypes: false,
ignoreReadonlyClassProperties: false,
ignoreTypeIndexes: false,
};
```

Expand Down Expand Up @@ -118,6 +120,28 @@ class Foo {
}
```

### `ignoreTypeIndexes`

A boolean to specify if numbers used to index types are okay. `false` by default.

Examples of **incorrect** code for the `{ "ignoreTypeIndexes": false }` option:

```ts
/*eslint @typescript-eslint/no-magic-numbers: ["error", { "ignoreTypeIndexes": false }]*/

type Foo = Bar[0];
type Baz = Parameters<Foo>[2];
```

Examples of **correct** code for the `{ "ignoreTypeIndexes": true }` option:

```ts
/*eslint @typescript-eslint/no-magic-numbers: ["error", { "ignoreTypeIndexes": true }]*/

type Foo = Bar[0];
type Baz = Parameters<Foo>[2];
```

Copy link
Member

Choose a reason for hiding this comment

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

Heh, it's a pity this docs page doesn't use the newer tabbed format others do. No need to change in this PR though; I'm going to try to make docs a bit more automated soon.

<sup>

Taken with ❤️ [from ESLint core](https://github.com/eslint/eslint/blob/main/docs/rules/no-magic-numbers.md)
Expand Down
64 changes: 48 additions & 16 deletions packages/eslint-plugin/src/rules/no-magic-numbers.ts
View file
Open in desktop
Expand Up @@ -24,6 +24,9 @@ const schema = util.deepMerge(
ignoreReadonlyClassProperties: {
type: 'boolean',
},
ignoreTypeIndexes: {
type: 'boolean',
},
},
},
);
Expand Down Expand Up @@ -56,29 +59,40 @@ export default util.createRule<Options, MessageIds>({

return {
Literal(node): void {
// Check if the node is a TypeScript enum declaration
if (options.ignoreEnums && isParentTSEnumDeclaration(node)) {
// If it’s not a numeric literal we’re not interested
if (typeof node.value !== 'number' && typeof node.value !== 'bigint') {
return;
}

// This will be `true` if we’re configured to ignore this case (eg. it’s
// an enum and `ignoreEnums` is `true`). It will be `false` if we’re not
// configured to ignore this case. It will remain `undefined` if this is
// not one of our exception cases, and we’ll fall back to the base rule.
let isAllowed: boolean | undefined;

// Check if the node is a TypeScript enum declaration
if (isParentTSEnumDeclaration(node)) {
isAllowed = options.ignoreEnums === true;
}
// Check TypeScript specific nodes for Numeric Literal
if (
options.ignoreNumericLiteralTypes &&
typeof node.value === 'number' &&
isTSNumericLiteralType(node)
) {
return;
else if (isTSNumericLiteralType(node)) {
isAllowed = options.ignoreNumericLiteralTypes === true;
}
// Check if the node is a type index
else if (isAncestorTSIndexedAccessType(node)) {
isAllowed = options.ignoreTypeIndexes === true;
}

// Check if the node is a readonly class property
if (
(typeof node.value === 'number' || typeof node.value === 'bigint') &&
isParentTSReadonlyPropertyDefinition(node)
) {
if (options.ignoreReadonlyClassProperties) {
return;
}
else if (isParentTSReadonlyPropertyDefinition(node)) {
isAllowed = options.ignoreReadonlyClassProperties === true;
}

// If we’ve hit a case where the ignore option is true we can return now
if (isAllowed === true) {
return;
}
// If the ignore option is *not* set we can report it now
else if (isAllowed === false) {
let fullNumberNode: TSESTree.Literal | TSESTree.UnaryExpression =
node;
let raw = node.raw;
Expand Down Expand Up @@ -216,3 +230,21 @@ function isParentTSReadonlyPropertyDefinition(node: TSESTree.Literal): boolean {

return false;
}

/**
* Checks if the node is part of a type indexed access (eg. Foo[4])
* @param node the node to be validated.
* @returns true if the node is part of an indexed access
* @private
*/
function isAncestorTSIndexedAccessType(node: TSESTree.Literal): boolean {
// Handle unary expressions (eg. -4)
let ancestor = getLiteralParent(node);

// Go up another level if we’re part of a type union (eg. 1 | 2)
if (ancestor?.parent?.type === AST_NODE_TYPES.TSUnionType) {
ancestor = ancestor.parent;
}

return ancestor?.parent?.type === AST_NODE_TYPES.TSIndexedAccessType;
}
142 changes: 142 additions & 0 deletions packages/eslint-plugin/tests/rules/no-magic-numbers.test.ts
View file
Open in desktop
Expand Up @@ -58,6 +58,42 @@ class Foo {
`,
options: [{ ignoreReadonlyClassProperties: true }],
},
{
code: 'type Foo = Bar[0];',
options: [{ ignoreTypeIndexes: true }],
},
{
code: 'type Foo = Bar[-1];',
options: [{ ignoreTypeIndexes: true }],
},
{
code: 'type Foo = Bar[0xab];',
options: [{ ignoreTypeIndexes: true }],
},
{
code: 'type Foo = Bar[5.6e1];',
options: [{ ignoreTypeIndexes: true }],
},
{
code: 'type Foo = Bar[10n];',
options: [{ ignoreTypeIndexes: true }],
},
{
code: 'type Foo = Bar[1 | -2];',
options: [{ ignoreTypeIndexes: true }],
},
{
code: 'type Foo = Parameters<Bar>[2];',
options: [{ ignoreTypeIndexes: true }],
},
{
code: "type Foo = Bar['baz'];",
options: [{ ignoreTypeIndexes: true }],
},
{
code: "type Foo = Bar['baz'];",
options: [{ ignoreTypeIndexes: false }],
},
Copy link
Member

Choose a reason for hiding this comment

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

A few edge cases we'll also want to test for:

type Foo = Bar[1 & number];
type Other = {
    [0]: 3;
}

type Foo = {
  [K in keyof Other]: `${K & number}`;
};
type Foo = {
  [K in 0 | 1 | 2]: 0;
};
type Others = [ ['a'], ['b'] ];

type Foo = {
  [K in keyof Others[0]]: Others[K];
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JoshuaKGoldberg couple of quick questions on those:

type Foo = Bar[1 & number];

Sure, I check for union types so intersection should be covered too! Thanks.

type Other = {
    [0]: 3;
}

type Foo = {
  [K in keyof Other]: `${K & number}`;
};

Since the 0 is a TSPropertySignature, not a TSIndexedAccessType, would you still want it to be covered by this new ignoreTypeIndexes config? Seems like that should be a separate config?

Also, the 3 there should be an error, right? So perhaps string or some other non-numeric literal would be more appropriate for this test case?

type Foo = {
  [K in 0 | 1 | 2]: 0;
};

As above, TSPropertySignature.

type Others = [ ['a'], ['b'] ];

type Foo = {
  [K in keyof Others[0]]: Others[K];
};

I’ll add a test to ensure the Others[0] part is covered.

Copy link
Member

Choose a reason for hiding this comment

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

Great questions!

check for union types so intersection should be covered too

Bar[1 & number] is a bit different syntax from ${K & number}. I think it'd be good to have an explicit test for both. The code right now might not differentiate, but it's ripe for future refactors to miss it.

Since the 0 is a TSPropertySignature, not a TSIndexedAccessType, would you still want it to be covered by this new ignoreTypeIndexes config?

I'm not requesting changes to logic, just that it be covered. Agreed that they're different things -- the tests should validate that ignoreTypeIndexes doesn't remove the complaint on it.

Also, the 3 there should be an error, right?

Yes. That's already covered though; I'm really more interested in the TSPropertySignature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JoshuaKGoldberg Thank you for clarifying. In af1ddb7 I added support for intersection types, nesting of union/intersection types, and the additional invalid test cases you suggested.

],

invalid: [
Expand Down Expand Up @@ -268,5 +304,111 @@ class Foo {
},
],
},
{
code: 'type Foo = Bar[0];',
options: [{ ignoreTypeIndexes: false }],
errors: [
{
messageId: 'noMagic',
data: {
raw: '0',
},
line: 1,
column: 16,
},
],
},
{
code: 'type Foo = Bar[-1];',
options: [{ ignoreTypeIndexes: false }],
errors: [
{
messageId: 'noMagic',
data: {
raw: '-1',
},
line: 1,
column: 16,
},
],
},
{
code: 'type Foo = Bar[0xab];',
options: [{ ignoreTypeIndexes: false }],
errors: [
{
messageId: 'noMagic',
data: {
raw: '0xab',
},
line: 1,
column: 16,
},
],
},
{
code: 'type Foo = Bar[5.6e1];',
options: [{ ignoreTypeIndexes: false }],
errors: [
{
messageId: 'noMagic',
data: {
raw: '5.6e1',
},
line: 1,
column: 16,
},
],
},
{
code: 'type Foo = Bar[10n];',
options: [{ ignoreTypeIndexes: false }],
errors: [
{
messageId: 'noMagic',
data: {
raw: '10n',
},
line: 1,
column: 16,
},
],
},
{
code: 'type Foo = Bar[1 | -2];',
options: [{ ignoreTypeIndexes: false }],
errors: [
{
messageId: 'noMagic',
data: {
raw: '1',
},
line: 1,
column: 16,
},
{
messageId: 'noMagic',
data: {
raw: '-2',
},
line: 1,
column: 20,
},
],
},
{
code: 'type Foo = Parameters<Bar>[2];',
options: [{ ignoreTypeIndexes: false }],
errors: [
{
messageId: 'noMagic',
data: {
raw: '2',
},
line: 1,
column: 28,
},
],
},
],
});
1 change: 1 addition & 0 deletions packages/eslint-plugin/typings/eslint-rules.d.ts
View file
Open in desktop
Expand Up @@ -319,6 +319,7 @@ declare module 'eslint/lib/rules/no-magic-numbers' {
ignoreNumericLiteralTypes?: boolean;
ignoreEnums?: boolean;
ignoreReadonlyClassProperties?: boolean;
ignoreTypeIndexes?: boolean;
},
],
{
Expand Down