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.

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

feat(eslint-plugin): [no-unsafe-enum-comparison] add switch suggestion #7691

Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
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
57 changes: 53 additions & 4 deletions packages/eslint-plugin/src/rules/enum-utils/shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,23 @@ function getBaseEnumType(typeChecker: ts.TypeChecker, type: ts.Type): ts.Type {
return typeChecker.getTypeAtLocation(symbol.valueDeclaration!.parent);
}

/**
* Retrieve only the Enum literals from a type. for example:
* - 123 --> []
* - {} --> []
* - Fruit.Apple --> [Fruit.Apple]
* - Fruit.Apple | Vegetable.Lettuce --> [Fruit.Apple, Vegetable.Lettuce]
* - Fruit.Apple | Vegetable.Lettuce | 123 --> [Fruit.Apple, Vegetable.Lettuce]
* - T extends Fruit --> [Fruit]
*/
Copy link
Member

Choose a reason for hiding this comment

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

[Praise] Great comments, thanks for continuing the existing standard 🙂

export function getEnumLiterals(type: ts.Type): ts.LiteralType[] {
return tsutils
.unionTypeParts(type)
.filter((subType): subType is ts.LiteralType =>
isTypeFlagSet(subType, ts.TypeFlags.EnumLiteral),
);
}

/**
* A type can have 0 or more enum types. For example:
* - 123 --> []
Expand All @@ -33,8 +50,40 @@ export function getEnumTypes(
typeChecker: ts.TypeChecker,
type: ts.Type,
): ts.Type[] {
return tsutils
.unionTypeParts(type)
.filter(subType => isTypeFlagSet(subType, ts.TypeFlags.EnumLiteral))
.map(type => getBaseEnumType(typeChecker, type));
return getEnumLiterals(type).map(type => getBaseEnumType(typeChecker, type));
}

/**
* Returns the enum key that matches the given literal node, or null if none
* match. For example:
* ```ts
* enum Fruit {
* Apple = 'apple',
* Banana = 'banana',
* }
*
* getEnumKeyForLiteral([Fruit.Apple, Fruit.Banana], 'apple') --> 'Fruit.Apple'
* getEnumKeyForLiteral([Fruit.Apple, Fruit.Banana], 'banana') --> 'Fruit.Banana'
* getEnumKeyForLiteral([Fruit.Apple, Fruit.Banana], 'cherry') --> null
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
* ```
*/
export function getEnumKeyForLiteral(
enumLiterals: ts.LiteralType[],
literal: unknown,
): string | null {
for (const enumLiteral of enumLiterals) {
if (enumLiteral.value === literal) {
const { symbol } = enumLiteral;

const memberDeclaration = symbol.valueDeclaration as ts.EnumMember;
const enumDeclaration = memberDeclaration.parent;

const enumName = enumDeclaration.name.getText();
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
const memberName = memberDeclaration.name.getText();
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

return `${enumName}.${memberName}`;
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
}
}

return null;
}
49 changes: 46 additions & 3 deletions packages/eslint-plugin/src/rules/no-unsafe-enum-comparison.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
import type { TSESTree } from '@typescript-eslint/utils';
import type { TSESLint, TSESTree } from '@typescript-eslint/utils';
import * as tsutils from 'ts-api-utils';
import * as ts from 'typescript';

import { createRule, getParserServices } from '../util';
import { getEnumTypes } from './enum-utils/shared';
import { createRule, getParserServices, getStaticValue } from '../util';
import {
getEnumKeyForLiteral,
getEnumLiterals,
getEnumTypes,
} from './enum-utils/shared';

/**
* @returns Whether the right type is an unsafe comparison against any left type.
Expand Down Expand Up @@ -39,6 +43,7 @@ function getEnumValueType(type: ts.Type): ts.TypeFlags | undefined {
export default createRule({
name: 'no-unsafe-enum-comparison',
meta: {
hasSuggestions: true,
type: 'suggestion',
docs: {
description: 'Disallow comparing an enum value with a non-enum value',
Expand All @@ -48,6 +53,7 @@ export default createRule({
messages: {
mismatched:
'The two values in this comparison do not have a shared enum type.',
replaceValueWithEnum: 'Replace with an enum value comparison.',
},
schema: [],
},
Expand Down Expand Up @@ -107,6 +113,43 @@ export default createRule({
context.report({
messageId: 'mismatched',
node,
suggest: [
{
messageId: 'replaceValueWithEnum',
fix(fixer): TSESLint.RuleFix | null {
// Replace the right side with an enum key if possible:
//
// ```ts
// Fruit.Apple === 'apple'; // Fruit.Apple === Fruit.Apple
// ```
const leftEnumKey = getEnumKeyForLiteral(
getEnumLiterals(left),
getStaticValue(node.right)?.value,
);

if (leftEnumKey) {
return fixer.replaceText(node.right, leftEnumKey);
}

// Replace the left side with an enum key if possible:
//
// ```ts
// declare const fruit: Fruit;
// 'apple' === Fruit.Apple; // Fruit.Apple === Fruit.Apple
// ```
const rightEnumKey = getEnumKeyForLiteral(
getEnumLiterals(right),
getStaticValue(node.left)?.value,
);

if (rightEnumKey) {
return fixer.replaceText(node.left, rightEnumKey);
}

return null;
},
},
],
});
}
},
Expand Down
138 changes: 138 additions & 0 deletions packages/eslint-plugin/tests/rules/no-unsafe-enum-comparison.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -565,5 +565,143 @@ ruleTester.run('strict-enums-comparison', rule, {
`,
errors: [{ messageId: 'mismatched' }],
},
{
code: `
enum Str {
A = 'a',
B = 'b',
}
declare const str: Str;
str === 'b';
`,
errors: [
{
messageId: 'mismatched',
suggestions: [
{
messageId: 'replaceValueWithEnum',
output: `
enum Str {
A = 'a',
B = 'b',
}
declare const str: Str;
str === Str.B;
`,
},
],
},
],
},
{
code: `
enum Num {
A = 1,
B = 2,
}
declare const num: Num;
1 === num;
`,
errors: [
{
messageId: 'mismatched',
suggestions: [
{
messageId: 'replaceValueWithEnum',
output: `
enum Num {
A = 1,
B = 2,
}
declare const num: Num;
Num.A === num;
`,
},
],
},
],
},
{
code: `
enum Mixed {
A = 1,
B = 'b',
}
declare const mixed: Mixed;
mixed === 1;
`,
errors: [
{
messageId: 'mismatched',
suggestions: [
{
messageId: 'replaceValueWithEnum',
output: `
enum Mixed {
A = 1,
B = 'b',
}
declare const mixed: Mixed;
mixed === Mixed.A;
`,
},
],
},
],
},
{
code: `
enum Mixed {
A = 1,
B = 'b',
}
declare const mixed: Mixed;
mixed === 'b';
`,
errors: [
{
messageId: 'mismatched',
suggestions: [
{
messageId: 'replaceValueWithEnum',
output: `
enum Mixed {
A = 1,
B = 'b',
}
declare const mixed: Mixed;
mixed === Mixed.B;
`,
},
],
},
],
},
{
code: `
enum StringKey {
'a' = 1,
}
declare const stringKey: StringKey;
stringKey === 1;
`,
errors: [
{
messageId: 'mismatched',
suggestions: [
{
messageId: 'replaceValueWithEnum',
output: `
enum StringKey {
'a' = 1,
}
declare const stringKey: StringKey;
stringKey === StringKey['a'];
Copy link
Member

Choose a reason for hiding this comment

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

StringKey['a'];

Just confirming explicitly in case someone looks at this in the future: I think it's fine to have it look like StringKey['a'] instead of StringKey.a. They're functionally the same, and there are other rules around stylistic concerns.

`,
},
],
},
],
},
],
});