diff --git a/packages/eslint-plugin/docs/rules/no-unsafe-enum-comparison.md b/packages/eslint-plugin/docs/rules/no-unsafe-enum-comparison.md index f5705826073..5db56d69b32 100644 --- a/packages/eslint-plugin/docs/rules/no-unsafe-enum-comparison.md +++ b/packages/eslint-plugin/docs/rules/no-unsafe-enum-comparison.md @@ -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 @@ -35,7 +37,7 @@ enum Fruit { declare let fruit: Fruit; -fruit === 999; +fruit === 0; ``` ```ts @@ -57,7 +59,7 @@ enum Fruit { declare let fruit: Fruit; -fruit === Fruit.Banana; +fruit === Fruit.Apple; ``` ```ts diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-type-constraint.ts b/packages/eslint-plugin/src/rules/no-unnecessary-type-constraint.ts index 21a98de67a4..6289b7fe39a 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-type-constraint.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-type-constraint.ts @@ -43,7 +43,7 @@ export default 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: diff --git a/packages/eslint-plugin/src/rules/no-unsafe-enum-comparison.ts b/packages/eslint-plugin/src/rules/no-unsafe-enum-comparison.ts index 2366ca54ff4..0d165ae7eb5 100644 --- a/packages/eslint-plugin/src/rules/no-unsafe-enum-comparison.ts +++ b/packages/eslint-plugin/src/rules/no-unsafe-enum-comparison.ts @@ -51,7 +51,9 @@ export default createRule({ requiresTypeChecking: true, }, messages: { - mismatched: + mismatchedCase: + 'The case statement does not have a shared enum type with the switch predicate.', + mismatchedCondition: 'The two values in this comparison do not have a shared enum type.', replaceValueWithEnum: 'Replace with an enum value comparison.', }, @@ -62,56 +64,63 @@ export default createRule({ const parserServices = getParserServices(context); const typeChecker = parserServices.program.getTypeChecker(); - 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; - } + function isMismatchedComparison( + leftType: ts.Type, + rightType: ts.Type, + ): boolean { + // 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; - } + // 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); - 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; - } + // 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 leftType = parserServices.getTypeAtLocation(node.left); + const rightType = parserServices.getTypeAtLocation(node.right); - if ( - typeViolates(leftTypeParts, right) || - typeViolates(rightTypeParts, left) - ) { + if (isMismatchedComparison(leftType, rightType)) { context.report({ - messageId: 'mismatched', + messageId: 'mismatchedCondition', node, suggest: [ { @@ -123,7 +132,7 @@ export default createRule({ // Fruit.Apple === 'apple'; // Fruit.Apple === Fruit.Apple // ``` const leftEnumKey = getEnumKeyForLiteral( - getEnumLiterals(left), + getEnumLiterals(leftType), getStaticValue(node.right)?.value, ); @@ -138,7 +147,7 @@ export default createRule({ // 'apple' === Fruit.Apple; // Fruit.Apple === Fruit.Apple // ``` const rightEnumKey = getEnumKeyForLiteral( - getEnumLiterals(right), + getEnumLiterals(rightType), getStaticValue(node.left)?.value, ); @@ -153,6 +162,32 @@ export default createRule({ }); } }, + + SwitchCase(node): void { + // Ignore `default` cases. + if (node.test == null) { + return; + } + + const { parent } = node; + + /** + * @see https://github.com/typescript-eslint/typescript-eslint/issues/6225 + */ + const switchStatement = parent as TSESTree.SwitchStatement; + + const leftType = parserServices.getTypeAtLocation( + switchStatement.discriminant, + ); + const rightType = parserServices.getTypeAtLocation(node.test); + + if (isMismatchedComparison(leftType, rightType)) { + context.report({ + messageId: 'mismatchedCase', + node, + }); + } + }, }; }, }); diff --git a/packages/eslint-plugin/tests/rules/no-unsafe-enum-comparison.test.ts b/packages/eslint-plugin/tests/rules/no-unsafe-enum-comparison.test.ts index ff1211b257b..b2d4eb907b3 100644 --- a/packages/eslint-plugin/tests/rules/no-unsafe-enum-comparison.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unsafe-enum-comparison.test.ts @@ -306,6 +306,45 @@ ruleTester.run('strict-enums-comparison', rule, { const bitShift = 1 >> Fruit.Apple; `, + ` + enum Fruit { + Apple, + } + + declare const fruit: Fruit; + + switch (fruit) { + case Fruit.Apple: { + break; + } + } + `, + ` + enum Vegetable { + Asparagus = 'asparagus', + } + + declare const vegetable: Vegetable; + + switch (vegetable) { + case Vegetable.Asparagus: { + break; + } + } + `, + ` + enum Vegetable { + Asparagus = 'asparagus', + } + + declare const vegetable: Vegetable; + + switch (vegetable) { + default: { + break; + } + } + `, ], invalid: [ { @@ -315,7 +354,7 @@ ruleTester.run('strict-enums-comparison', rule, { } Fruit.Apple < 1; `, - errors: [{ messageId: 'mismatched' }], + errors: [{ messageId: 'mismatchedCondition' }], }, { code: ` @@ -324,7 +363,7 @@ ruleTester.run('strict-enums-comparison', rule, { } Fruit.Apple > 1; `, - errors: [{ messageId: 'mismatched' }], + errors: [{ messageId: 'mismatchedCondition' }], }, { code: ` @@ -333,7 +372,7 @@ ruleTester.run('strict-enums-comparison', rule, { } Fruit.Apple == 1; `, - errors: [{ messageId: 'mismatched' }], + errors: [{ messageId: 'mismatchedCondition' }], }, { code: ` @@ -342,7 +381,7 @@ ruleTester.run('strict-enums-comparison', rule, { } Fruit.Apple === 1; `, - errors: [{ messageId: 'mismatched' }], + errors: [{ messageId: 'mismatchedCondition' }], }, { code: ` @@ -351,7 +390,7 @@ ruleTester.run('strict-enums-comparison', rule, { } Fruit.Apple != 1; `, - errors: [{ messageId: 'mismatched' }], + errors: [{ messageId: 'mismatchedCondition' }], }, { code: ` @@ -360,7 +399,7 @@ ruleTester.run('strict-enums-comparison', rule, { } Fruit.Apple !== 1; `, - errors: [{ messageId: 'mismatched' }], + errors: [{ messageId: 'mismatchedCondition' }], }, { code: ` @@ -370,7 +409,7 @@ ruleTester.run('strict-enums-comparison', rule, { } Fruit.Apple === 0; `, - errors: [{ messageId: 'mismatched' }], + errors: [{ messageId: 'mismatchedCondition' }], }, { code: ` @@ -380,7 +419,7 @@ ruleTester.run('strict-enums-comparison', rule, { } Fruit.Banana === ''; `, - errors: [{ messageId: 'mismatched' }], + errors: [{ messageId: 'mismatchedCondition' }], }, { code: ` @@ -391,7 +430,7 @@ ruleTester.run('strict-enums-comparison', rule, { } Vegetable.Asparagus === 'beet'; `, - errors: [{ messageId: 'mismatched' }], + errors: [{ messageId: 'mismatchedCondition' }], }, { code: ` @@ -402,7 +441,7 @@ ruleTester.run('strict-enums-comparison', rule, { } 1 === Fruit.Apple; `, - errors: [{ messageId: 'mismatched' }], + errors: [{ messageId: 'mismatchedCondition' }], }, { code: ` @@ -413,7 +452,7 @@ ruleTester.run('strict-enums-comparison', rule, { } 'beet' === Vegetable.Asparagus; `, - errors: [{ messageId: 'mismatched' }], + errors: [{ messageId: 'mismatchedCondition' }], }, { code: ` @@ -425,7 +464,7 @@ ruleTester.run('strict-enums-comparison', rule, { const fruit = Fruit.Apple; fruit === 1; `, - errors: [{ messageId: 'mismatched' }], + errors: [{ messageId: 'mismatchedCondition' }], }, { code: ` @@ -437,7 +476,7 @@ ruleTester.run('strict-enums-comparison', rule, { const vegetable = Vegetable.Asparagus; vegetable === 'beet'; `, - errors: [{ messageId: 'mismatched' }], + errors: [{ messageId: 'mismatchedCondition' }], }, { code: ` @@ -449,7 +488,7 @@ ruleTester.run('strict-enums-comparison', rule, { const fruit = Fruit.Apple; 1 === fruit; `, - errors: [{ messageId: 'mismatched' }], + errors: [{ messageId: 'mismatchedCondition' }], }, { code: ` @@ -461,7 +500,7 @@ ruleTester.run('strict-enums-comparison', rule, { const vegetable = Vegetable.Asparagus; 'beet' === vegetable; `, - errors: [{ messageId: 'mismatched' }], + errors: [{ messageId: 'mismatchedCondition' }], }, { code: @@ -473,7 +512,7 @@ ruleTester.run('strict-enums-comparison', rule, { } Fruit.Apple === Fruit2.Apple2; `, - errors: [{ messageId: 'mismatched' }], + errors: [{ messageId: 'mismatchedCondition' }], }, { code: ` @@ -489,7 +528,7 @@ ruleTester.run('strict-enums-comparison', rule, { } Vegetable.Asparagus === Vegetable2.Asparagus2; `, - errors: [{ messageId: 'mismatched' }], + errors: [{ messageId: 'mismatchedCondition' }], }, { code: @@ -502,7 +541,7 @@ ruleTester.run('strict-enums-comparison', rule, { const fruit = Fruit.Apple; fruit === Fruit2.Apple2; `, - errors: [{ messageId: 'mismatched' }], + errors: [{ messageId: 'mismatchedCondition' }], }, { code: ` @@ -519,7 +558,7 @@ ruleTester.run('strict-enums-comparison', rule, { const vegetable = Vegetable.Asparagus; vegetable === Vegetable2.Asparagus2; `, - errors: [{ messageId: 'mismatched' }], + errors: [{ messageId: 'mismatchedCondition' }], }, { code: ` @@ -545,10 +584,10 @@ ruleTester.run('strict-enums-comparison', rule, { mixed === 1; `, errors: [ - { messageId: 'mismatched' }, - { messageId: 'mismatched' }, - { messageId: 'mismatched' }, - { messageId: 'mismatched' }, + { messageId: 'mismatchedCondition' }, + { messageId: 'mismatchedCondition' }, + { messageId: 'mismatchedCondition' }, + { messageId: 'mismatchedCondition' }, ], }, { @@ -563,7 +602,102 @@ ruleTester.run('strict-enums-comparison', rule, { declare const weirdString: __String; weirdString === 'someArbitraryValue'; `, - errors: [{ messageId: 'mismatched' }], + errors: [{ messageId: 'mismatchedCondition' }], + }, + { + code: ` + enum Fruit { + Apple, + } + + declare const fruit: Fruit; + + switch (fruit) { + case 0: { + break; + } + } + `, + errors: [{ messageId: 'mismatchedCase' }], + }, + { + code: ` + enum Fruit { + Apple, + Banana, + } + + declare const fruit: Fruit; + + switch (fruit) { + case Fruit.Apple: { + break; + } + case 1: { + break; + } + } + `, + errors: [{ messageId: 'mismatchedCase' }], + }, + { + code: ` + enum Vegetable { + Asparagus = 'asparagus', + } + + declare const vegetable: Vegetable; + + switch (vegetable) { + case 'asparagus': { + break; + } + } + `, + errors: [{ messageId: 'mismatchedCase' }], + }, + { + code: ` + enum Vegetable { + Asparagus = 'asparagus', + Beet = 'beet', + } + + declare const vegetable: Vegetable; + + switch (vegetable) { + case Vegetable.Asparagus: { + break; + } + case 'beet': { + break; + } + } + `, + errors: [{ messageId: 'mismatchedCase' }], + }, + { + code: ` + enum Vegetable { + Asparagus = 'asparagus', + Beet = 'beet', + } + + declare const vegetable: Vegetable; + + switch (vegetable) { + case Vegetable.Asparagus: { + break; + } + case 'beet': { + break; + } + default: { + break; + } + } + `, + errors: [{ messageId: 'mismatchedCase' }], }, { code: ` @@ -576,7 +710,7 @@ ruleTester.run('strict-enums-comparison', rule, { `, errors: [ { - messageId: 'mismatched', + messageId: 'mismatchedCondition', suggestions: [ { messageId: 'replaceValueWithEnum', @@ -604,7 +738,7 @@ ruleTester.run('strict-enums-comparison', rule, { `, errors: [ { - messageId: 'mismatched', + messageId: 'mismatchedCondition', suggestions: [ { messageId: 'replaceValueWithEnum', @@ -632,7 +766,7 @@ ruleTester.run('strict-enums-comparison', rule, { `, errors: [ { - messageId: 'mismatched', + messageId: 'mismatchedCondition', suggestions: [ { messageId: 'replaceValueWithEnum', @@ -660,7 +794,7 @@ ruleTester.run('strict-enums-comparison', rule, { `, errors: [ { - messageId: 'mismatched', + messageId: 'mismatchedCondition', suggestions: [ { messageId: 'replaceValueWithEnum', @@ -688,7 +822,7 @@ ruleTester.run('strict-enums-comparison', rule, { `, errors: [ { - messageId: 'mismatched', + messageId: 'mismatchedCondition', suggestions: [ { messageId: 'replaceValueWithEnum', @@ -716,7 +850,7 @@ ruleTester.run('strict-enums-comparison', rule, { `, errors: [ { - messageId: 'mismatched', + messageId: 'mismatchedCondition', suggestions: [ { messageId: 'replaceValueWithEnum', @@ -744,7 +878,7 @@ ruleTester.run('strict-enums-comparison', rule, { `, errors: [ { - messageId: 'mismatched', + messageId: 'mismatchedCondition', suggestions: [ { messageId: 'replaceValueWithEnum', @@ -771,7 +905,7 @@ ruleTester.run('strict-enums-comparison', rule, { `, errors: [ { - messageId: 'mismatched', + messageId: 'mismatchedCondition', suggestions: [ { messageId: 'replaceValueWithEnum', @@ -797,7 +931,7 @@ ruleTester.run('strict-enums-comparison', rule, { `, errors: [ { - messageId: 'mismatched', + messageId: 'mismatchedCondition', suggestions: [ { messageId: 'replaceValueWithEnum', @@ -823,7 +957,7 @@ ruleTester.run('strict-enums-comparison', rule, { `, errors: [ { - messageId: 'mismatched', + messageId: 'mismatchedCondition', suggestions: [ { messageId: 'replaceValueWithEnum', @@ -849,7 +983,7 @@ ruleTester.run('strict-enums-comparison', rule, { `, errors: [ { - messageId: 'mismatched', + messageId: 'mismatchedCondition', suggestions: [ { messageId: 'replaceValueWithEnum', @@ -875,7 +1009,7 @@ ruleTester.run('strict-enums-comparison', rule, { `, errors: [ { - messageId: 'mismatched', + messageId: 'mismatchedCondition', suggestions: [ { messageId: 'replaceValueWithEnum', @@ -901,7 +1035,7 @@ ruleTester.run('strict-enums-comparison', rule, { `, errors: [ { - messageId: 'mismatched', + messageId: 'mismatchedCondition', suggestions: [ { messageId: 'replaceValueWithEnum', @@ -928,7 +1062,7 @@ ruleTester.run('strict-enums-comparison', rule, { `, errors: [ { - messageId: 'mismatched', + messageId: 'mismatchedCondition', suggestions: [ { messageId: 'replaceValueWithEnum',