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 all 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
72 changes: 68 additions & 4 deletions packages/eslint-plugin/src/rules/enum-utils/shared.ts
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,55 @@ 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 memberNameIdentifier = memberDeclaration.name;
const enumName = enumDeclaration.name.text;

switch (memberNameIdentifier.kind) {
case ts.SyntaxKind.Identifier:
return `${enumName}.${memberNameIdentifier.text}`;

case ts.SyntaxKind.StringLiteral: {
const memberName = memberNameIdentifier.text.replace(/'/g, "\\'");

return `${enumName}['${memberName}']`;
}

case ts.SyntaxKind.ComputedPropertyName:
return `${enumName}[${memberNameIdentifier.expression.getText()}]`;
Copy link
Contributor Author

@StyleShit StyleShit Oct 17, 2023

Choose a reason for hiding this comment

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

had to use .getText() here, there is no .text for some reason
seems like it's fine? See the tests for that as a reference

Copy link
Member

Choose a reason for hiding this comment

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

The type of memberNameIdentifier.expression is ts.Expression, which is a pretty wide type. You could as assert it to the union of types that are actually allowed as enum member names... or just go with .getText(). If the unit tests aren't showing any bad behavior, it's probably fine. 😎

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would probably keep it like this, seems fine in the tests (unless I'm missing some cases?)

Copy link
Member

Choose a reason for hiding this comment

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

The existing bugs in #7768 make me not too motivated to get every single possible edge case in this PR. The tests you've added are pretty great. Thanks!


default:
break;
}
}
}

return null;
}
49 changes: 46 additions & 3 deletions packages/eslint-plugin/src/rules/no-unsafe-enum-comparison.ts
@@ -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