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 6 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
51 changes: 47 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,21 @@
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.Type[] {
return tsutils
.unionTypeParts(type)
.filter(subType => util.isTypeFlagSet(subType, ts.TypeFlags.EnumLiteral));
}

/**
* A type can have 0 or more enum types. For example:
* - 123 --> []
Expand All @@ -33,8 +48,36 @@
typeChecker: ts.TypeChecker,
type: ts.Type,
): ts.Type[] {
return tsutils
.unionTypeParts(type)
.filter(subType => util.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], 'orange') --> null
* ```
*/
export function getEnumKeyForLiteral(
enumLiterals: ts.Type[],
literal: unknown,
): string | null {
for (const enumLiteral of enumLiterals) {
// @ts-expect-error Not sure why `value` is not on `enumLiteral`.
if (enumLiteral.value === literal) {
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
const { symbol } = enumLiteral;

// @ts-expect-error Not sure why `parent` is not on `symbol`.
return `${symbol.parent.name}.${symbol.name}`;

Check failure on line 78 in packages/eslint-plugin/src/rules/enum-utils/shared.ts

View workflow job for this annotation

GitHub Actions / Lint (lint)

Unsafe member access .name on an `any` value
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
}
}

return null;
}
57 changes: 55 additions & 2 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 * as util from '../util';
import { getEnumTypes } from './enum-utils/shared';
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 util.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 util.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,53 @@ export default util.createRule({
context.report({
messageId: 'mismatched',
node,
suggest: [
{
messageId: 'replaceValueWithEnum',
fix(fixer): TSESLint.RuleFix | null {
const sourceCode = context.getSourceCode();
const leftExpression = sourceCode.getText(node.left);
const rightExpression = sourceCode.getText(node.right);

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

if (leftEnumKey != null) {
return fixer.replaceText(
node,
`${leftExpression} ${node.operator} ${leftEnumKey}`,
);
}
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

// 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),
util.getStaticValue(node.left)?.value,
);

if (rightEnumKey != null) {
return fixer.replaceText(
node,
`${rightEnumKey} ${node.operator} ${rightExpression}`,
);
}

return null;
},
},
],
});
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -565,5 +565,63 @@ 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; ` + `num === 1;`,
errors: [
{
messageId: 'mismatched',
suggestions: [
{
messageId: 'replaceValueWithEnum',
output: `enum Num {A = 1, B = 2} declare const num: Num; num === Num.A;`,
},
],
},
],
},
{
code:
`enum Mixed {A = 1, B = 'b'} ` +
`declare const mixed: Mixed; ` +
`mixed === 1 || mixed === 'b';`,
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
errors: [
{
messageId: 'mismatched',
suggestions: [
{
messageId: 'replaceValueWithEnum',
output: `enum Mixed {A = 1, B = 'b'} declare const mixed: Mixed; mixed === Mixed.A || mixed === 'b';`,
},
],
},
{
messageId: 'mismatched',
suggestions: [
{
messageId: 'replaceValueWithEnum',
output: `enum Mixed {A = 1, B = 'b'} declare const mixed: Mixed; mixed === 1 || mixed === Mixed.B;`,
},
],
},
],
},
],
});