feat(eslint-plugin): [no-unsafe-enum-comparison] add rule (#6107)

* feat: adding enums to member-ordering rule * feat(eslint-plugin): [strict-enums] adding check for null/undefined * feat(eslint-plugin): [strict-enums] refactoring tests to separate files * feat(eslint-plugin): [strict-enums] allowing more operators * fix(eslint-plugin): [strict-enums] adding union test * fix(eslint-plugin): [strict-enums] refactoring + adding failing class test * fix(eslint-plugin): [strict-enums] adding constraint code * fix(eslint-plugin): [strict-enums] better eslint messages * fix(eslint-plugin): [strict-enums] removing vscode setting changes * fix: changing function definition to reflect reality * fix: pass string enum literal into function that take string * fix: allow passing enums to functions with any/unknown * fix: using flags instead of names also fixes for codebase * fix: adding test that breaks the rule * fix: adding test for variadic functions * fix: adding isSymbolFlagSet internally * fix: adding ignoring for remaining lint failures * fix: better comments * fix: broken test * fix: adding failing test for generic functions * fix: refactoring tests + adding tests * fix: refactoring enum helper function locations * fix: cleanup * fix: refactoring + fixing tests * fix: more tests * fix: refactoring and making tests pass * fix: adding array code, all tests pass now * fix: adding failing test * fix: allow empty arrays * fix: adding logic for arrays with no enums * fix: adding more tests * fix: fixing test * fix: fixing linter * fix: reverting comment fixes * fix: removing refactor * fix: removing fixes to dot-notation * fix: removing semi refactor * fix: removing jest logic * fix: removing comparison operators check * fix: adding failing test * fix: making test pass * fix: adding comment * fix: adding back in bitwise operator checks since apparently they are needed * fix: remove bad comment * fix: removing unnecessary comments * fix: remove types from error messages * fix: removing isArray + refactoring * Update packages/eslint-plugin/src/rules/strict-enums.ts Co-authored-by: Brad Zacher <brad.zacher@gmail.com> * fix: removing strict-enums from recommended * fix: simplify * fix: undoing refactoring * fix: undoing refactoring * fix: moving tests into subdirectory * Update packages/eslint-plugin/src/rules/strict-enums.ts Co-authored-by: Brad Zacher <brad.zacher@gmail.com> * Update packages/eslint-plugin/src/rules/strict-enums.ts Co-authored-by: Brad Zacher <brad.zacher@gmail.com> * fix: adding failing test * fix: making boolean tests pass * fix: refactor tests + fix linter * fix: adding brads tests * fix: brads tests now pass * Update packages/eslint-plugin/docs/rules/strict-enums.md Co-authored-by: Joshua Chen <sidachen2003@gmail.com> * Update packages/eslint-plugin/src/rules/strict-enums.ts Co-authored-by: Brad Zacher <brad.zacher@gmail.com> * Update packages/eslint-plugin/src/rules/strict-enums.ts Co-authored-by: Brad Zacher <brad.zacher@gmail.com> * Update packages/eslint-plugin/src/rules/strict-enums.ts Co-authored-by: Brad Zacher <brad.zacher@gmail.com> * fix: make brads updates actually compile * Update strict-enums.ts * Continued fixing merge conflicts * Continued fixing the build * Passing build * Update packages/eslint-plugin/src/rules/strict-enums.ts * Update packages/eslint-plugin/src/rules/strict-enums.ts * A few more reverts * Just a bit more changing typeFlagUtils * Fixed strict-enums.md build * Convert tests to not pushing * Simplified the rule a whole bunch * Add back getEnumNames * Even more trimming down * ...and just a bit more * Undo some JSDoc changes * Progress: no-unsafe-enum-assignment is tested * A bit more testing for assignments, as requested * Finished testing and sharing * Added comparison operators * Added back no-unsafe-enum-comparison * Remove unrelated changes * Reduce coverage * Touched up docs --------- Co-authored-by: James <5511220+Zamiell@users.noreply.github.com> Co-authored-by: Brad Zacher <brad.zacher@gmail.com> Co-authored-by: Joshua Chen <sidachen2003@gmail.com>
typescript-eslint · Apr 5, 2023 · 915f9c2 · 915f9c2
1 parent 75806c9
commit 915f9c2
Show file tree

Hide file tree

Showing 9 changed files with 806 additions and 4 deletions.
diff --git a/packages/eslint-plugin/docs/rules/no-unsafe-enum-comparison.md b/packages/eslint-plugin/docs/rules/no-unsafe-enum-comparison.md
@@ -0,0 +1,75 @@
+---
+description: 'Disallow comparing an enum value with a non-enum value.'
+---
+
+> 🛑 This file is source code, not the primary documentation location! 🛑
+>
+> 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:
+
+```ts
+enum Fruit {
+  Apple,
+  Banana,
+}
+
+declare let fruit: Fruit;
+
+fruit === 999; // No error
+```
+
+This rule flags when an enum typed value is compared to a non-enum `number`.
+
+<!--tabs-->
+
+### ❌ Incorrect
+
+```ts
+enum Fruit {
+  Apple,
+}
+
+declare let fruit: Fruit;
+
+fruit === 999;
+```
+
+```ts
+enum Vegetable {
+  Asparagus = 'asparagus',
+}
+
+declare let vegetable: Vegetable;
+
+vegetable === 'asparagus';
+```
+
+### ✅ Correct
+
+```ts
+enum Fruit {
+  Apple,
+}
+
+declare let fruit: Fruit;
+
+fruit === Fruit.Banana;
+```
+
+```ts
+enum Vegetable {
+  Asparagus = 'asparagus',
+}
+
+declare let vegetable: Vegetable;
+
+vegetable === Vegetable.Asparagus;
+```
+
+<!--/tabs-->
+
+## When Not to Use It
+
+If you don't mind number and/or literal string constants being compared against enums, you likely don't need this rule.
diff --git a/packages/eslint-plugin/src/configs/all.ts b/packages/eslint-plugin/src/configs/all.ts
@@ -116,6 +116,7 @@ export = {
     '@typescript-eslint/no-unsafe-assignment': 'error',
     '@typescript-eslint/no-unsafe-call': 'error',
     '@typescript-eslint/no-unsafe-declaration-merging': 'error',
+    '@typescript-eslint/no-unsafe-enum-comparison': 'error',
     '@typescript-eslint/no-unsafe-member-access': 'error',
     '@typescript-eslint/no-unsafe-return': 'error',
     'no-unused-expressions': 'off',

diff --git a/packages/eslint-plugin/src/configs/strict.ts b/packages/eslint-plugin/src/configs/strict.ts
@@ -29,6 +29,7 @@ export = {
     '@typescript-eslint/no-unnecessary-condition': 'warn',
     '@typescript-eslint/no-unnecessary-type-arguments': 'warn',
     '@typescript-eslint/no-unsafe-declaration-merging': 'warn',
+    '@typescript-eslint/no-unsafe-enum-comparison': 'warn',
     'no-useless-constructor': 'off',
     '@typescript-eslint/no-useless-constructor': 'warn',
     '@typescript-eslint/non-nullable-type-assertion-style': 'warn',

diff --git a/packages/eslint-plugin/src/rules/enum-utils/shared.ts b/packages/eslint-plugin/src/rules/enum-utils/shared.ts
@@ -0,0 +1,40 @@
+import * as tsutils from 'tsutils';
+import * as ts from 'typescript';
+
+import * as util from '../../util';
+
+/*
+ * If passed an enum member, returns the type of the parent. Otherwise,
+ * returns itself.
+ *
+ * For example:
+ * - `Fruit` --> `Fruit`
+ * - `Fruit.Apple` --> `Fruit`
+ */
+function getBaseEnumType(typeChecker: ts.TypeChecker, type: ts.Type): ts.Type {
+  const symbol = type.getSymbol()!;
+  if (!tsutils.isSymbolFlagSet(symbol, ts.SymbolFlags.EnumMember)) {
+    return type;
+  }
+
+  return typeChecker.getTypeAtLocation(symbol.valueDeclaration!.parent);
+}
+
+/**
+ * A type can have 0 or more enum types. For example:
+ * - 123 --> []
+ * - {} --> []
+ * - Fruit.Apple --> [Fruit]
+ * - Fruit.Apple | Vegetable.Lettuce --> [Fruit, Vegetable]
+ * - Fruit.Apple | Vegetable.Lettuce | 123 --> [Fruit, Vegetable]
+ * - T extends Fruit --> [Fruit]
+ */
+export function getEnumTypes(
+  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));
+}
diff --git a/packages/eslint-plugin/src/rules/index.ts b/packages/eslint-plugin/src/rules/index.ts
@@ -85,6 +85,7 @@ import noUnsafeArgument from './no-unsafe-argument';
 import noUnsafeAssignment from './no-unsafe-assignment';
 import noUnsafeCall from './no-unsafe-call';
 import noUnsafeDeclarationMerging from './no-unsafe-declaration-merging';
+import noUnsafeEnumComparison from './no-unsafe-enum-comparison';
 import noUnsafeMemberAccess from './no-unsafe-member-access';
 import noUnsafeReturn from './no-unsafe-return';
 import noUnusedExpressions from './no-unused-expressions';
@@ -222,6 +223,7 @@ export default {
   'no-unsafe-assignment': noUnsafeAssignment,
   'no-unsafe-call': noUnsafeCall,
   'no-unsafe-declaration-merging': noUnsafeDeclarationMerging,
+  'no-unsafe-enum-comparison': noUnsafeEnumComparison,
   'no-unsafe-member-access': noUnsafeMemberAccess,
   'no-unsafe-return': noUnsafeReturn,
   'no-unused-expressions': noUnusedExpressions,

diff --git a/packages/eslint-plugin/src/rules/no-unsafe-enum-comparison.ts b/packages/eslint-plugin/src/rules/no-unsafe-enum-comparison.ts
@@ -0,0 +1,121 @@
+import type { TSESTree } from '@typescript-eslint/utils';
+import * as tsutils from 'tsutils';
+import * as ts from 'typescript';
+
+import * as util from '../util';
+import { getEnumTypes } from './enum-utils/shared';
+
+/**
+ * @returns Whether the right type is an unsafe comparison against any left type.
+ */
+function typeViolates(leftTypeParts: ts.Type[], right: ts.Type): boolean {
+  const leftValueKinds = new Set(leftTypeParts.map(getEnumValueType));
+
+  return (
+    (leftValueKinds.has(ts.TypeFlags.Number) &&
+      tsutils.isTypeFlagSet(
+        right,
+        ts.TypeFlags.Number | ts.TypeFlags.NumberLike,
+      )) ||
+    (leftValueKinds.has(ts.TypeFlags.String) &&
+      tsutils.isTypeFlagSet(
+        right,
+        ts.TypeFlags.String | ts.TypeFlags.StringLike,
+      ))
+  );
+}
+
+/**
+ * @returns What type a type's enum value is (number or string), if either.
+ */
+function getEnumValueType(type: ts.Type): ts.TypeFlags | undefined {
+  return util.isTypeFlagSet(type, ts.TypeFlags.EnumLike)
+    ? util.isTypeFlagSet(type, ts.TypeFlags.NumberLiteral)
+      ? ts.TypeFlags.Number
+      : ts.TypeFlags.String
+    : undefined;
+}
+
+export default util.createRule({
+  name: 'no-unsafe-enum-comparison',
+  meta: {
+    type: 'suggestion',
+    docs: {
+      description: 'Disallow comparing an enum value with a non-enum value',
+      recommended: 'strict',
+      requiresTypeChecking: true,
+    },
+    messages: {
+      mismatched:
+        'The two values in this comparison do not have a shared enum type.',
+    },
+    schema: [],
+  },
+  defaultOptions: [],
+  create(context) {
+    const parserServices = util.getParserServices(context);
+    const typeChecker = parserServices.program.getTypeChecker();
+
+    function getTypeFromNode(node: TSESTree.Node): ts.Type {
+      return typeChecker.getTypeAtLocation(
+        parserServices.esTreeNodeToTSNodeMap.get(node),
+      );
+    }
+
+    return {
+      'BinaryExpression[operator=/=|<|>/]'(
+        node: TSESTree.BinaryExpression,
+      ): void {
+        const left = getTypeFromNode(node.left);
+        const right = getTypeFromNode(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;
+        }
+
+        // Allow comparisons that share an enum type:
+        //
+        // ```ts
+        // Fruit.Apple === Fruit.Banana;
+        // ```
+        for (const leftEnumType of leftEnumTypes) {
+          if (rightEnumTypes.has(leftEnumType)) {
+            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;
+          }
+        }
+
+        if (
+          typeViolates(leftTypeParts, right) ||
+          typeViolates(rightTypeParts, left)
+        ) {
+          context.report({
+            messageId: 'mismatched',
+            node,
+          });
+        }
+      },
+    };
+  },
+});