Skip to content

feat(eslint-plugin): [no-unsafe-enum-comparison] handles switch cases #7541

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

Closed
wants to merge 12 commits into from
22 changes: 12 additions & 10 deletions packages/eslint-plugin/docs/rules/no-unsafe-enum-comparison.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,23 @@ description: 'Disallow comparing an enum value with a non-enum value.'
>
> See **https://typescript-eslint.io/rules/no-unsafe-enum-comparison** for documentation.

The TypeScript compiler can be surprisingly lenient when working with enums.
For example, it will allow you to compare enum values against numbers even though they might not have any type overlap:
The TypeScript compiler can be surprisingly lenient when working with enums. String enums are widely considered to be safer than number enums, but even string enums have some pitfalls. For example, it is allowed to compare enum values against literals:

```ts
enum Fruit {
Apple,
Banana,
enum Vegetable {
Asparagus = 'asparagus',
}

declare let fruit: Fruit;
declare const vegetable: Vegetable;

fruit === 999; // No error
vegetable === 'asparagus'; // No error
```

This rule flags when an enum typed value is compared to a non-enum `number`.
The above code snippet should instead be written as `vegetable === Vegetable.Asparagus`. Allowing literals in comparisons subverts the point of using enums in the first place. By enforcing comparisons with properly typed enums:

- It makes a codebase more resilient to enum members changing values.
- It allows for code IDEs to use the "Rename Symbol" feature to quickly rename an enum.
- It aligns code to the proper enum semantics of referring to them by name and treating their values as implementation details.

## Examples

Expand All @@ -35,7 +37,7 @@ enum Fruit {

declare let fruit: Fruit;

fruit === 999;
fruit === 0;
```

```ts
Expand All @@ -57,7 +59,7 @@ enum Fruit {

declare let fruit: Fruit;

fruit === Fruit.Banana;
fruit === Fruit.Apple;
```

```ts
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export default util.createRule({
function checkRequiresGenericDeclarationDisambiguation(
filename: string,
): boolean {
const pathExt = extname(filename).toLocaleLowerCase();
const pathExt = extname(filename).toLocaleLowerCase() as ts.Extension;
switch (pathExt) {
case ts.Extension.Cts:
case ts.Extension.Mts:
Expand Down
112 changes: 71 additions & 41 deletions packages/eslint-plugin/src/rules/no-unsafe-enum-comparison.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,10 @@ export default util.createRule({
requiresTypeChecking: true,
},
messages: {
mismatched:
mismatchedCondition:
'The two values in this comparison do not have a shared enum type.',
mismatchedCase:
'The case statement does not have a shared enum type with the switch predicate.',
},
schema: [],
},
Expand All @@ -56,56 +58,84 @@ export default util.createRule({
const parserServices = util.getParserServices(context);
const typeChecker = parserServices.program.getTypeChecker();

function isMismatchedComparison(
leftNode: TSESTree.Node,
rightNode: TSESTree.Node,
): boolean {
const leftType = parserServices.getTypeAtLocation(leftNode);
const rightType = parserServices.getTypeAtLocation(rightNode);

// Allow comparisons that don't have anything to do with enums:
//
// ```ts
// 1 === 2;
// ```
const leftEnumTypes = getEnumTypes(typeChecker, leftType);
const rightEnumTypes = new Set(getEnumTypes(typeChecker, rightType));
if (leftEnumTypes.length === 0 && rightEnumTypes.size === 0) {
return false;
}

// Allow comparisons that share an enum type:
//
// ```ts
// Fruit.Apple === Fruit.Banana;
// ```
for (const leftEnumType of leftEnumTypes) {
if (rightEnumTypes.has(leftEnumType)) {
return false;
}
}

const leftTypeParts = tsutils.unionTypeParts(leftType);
const rightTypeParts = tsutils.unionTypeParts(rightType);

// If a type exists in both sides, we consider this comparison safe:
//
// ```ts
// declare const fruit: Fruit.Apple | 0;
// fruit === 0;
// ```
for (const leftTypePart of leftTypeParts) {
if (rightTypeParts.includes(leftTypePart)) {
return false;
}
}

return (
typeViolates(leftTypeParts, rightType) ||
typeViolates(rightTypeParts, leftType)
);
}

return {
'BinaryExpression[operator=/^[<>!=]?={0,2}$/]'(
node: TSESTree.BinaryExpression,
): void {
const left = parserServices.getTypeAtLocation(node.left);
const right = parserServices.getTypeAtLocation(node.right);

// Allow comparisons that don't have anything to do with enums:
//
// ```ts
// 1 === 2;
// ```
const leftEnumTypes = getEnumTypes(typeChecker, left);
const rightEnumTypes = new Set(getEnumTypes(typeChecker, right));
if (leftEnumTypes.length === 0 && rightEnumTypes.size === 0) {
return;
if (isMismatchedComparison(node.left, node.right)) {
context.report({
messageId: 'mismatchedCondition',
node,
});
}
},

// Allow comparisons that share an enum type:
//
// ```ts
// Fruit.Apple === Fruit.Banana;
// ```
for (const leftEnumType of leftEnumTypes) {
if (rightEnumTypes.has(leftEnumType)) {
return;
}
SwitchCase(node): void {
// Ignore `default` cases.
if (node.test == null) {
return;
}

const leftTypeParts = tsutils.unionTypeParts(left);
const rightTypeParts = tsutils.unionTypeParts(right);

// If a type exists in both sides, we consider this comparison safe:
//
// ```ts
// declare const fruit: Fruit.Apple | 0;
// fruit === 0;
// ```
for (const leftTypePart of leftTypeParts) {
if (rightTypeParts.includes(leftTypePart)) {
return;
}
}
const { parent } = node;

/**
* @see https://github.com/typescript-eslint/typescript-eslint/issues/6225
*/
const switchStatement = parent as TSESTree.SwitchStatement;

if (
typeViolates(leftTypeParts, right) ||
typeViolates(rightTypeParts, left)
) {
if (isMismatchedComparison(switchStatement.discriminant, node.test)) {
context.report({
messageId: 'mismatched',
messageId: 'mismatchedCase',
node,
});
}
Expand Down
Loading