feat(eslint-plugin): [no-mixed-enums] add rule (#6102)

* feat(eslint-plugin): [no-mixed-enums] add rule * Fixed test formatting * Fixed config * Fixed description too * Snazzier query * Account for enum merging * Avoided the type checker where possible (mostly) * Added more type coverage * Apply suggestions from code review Co-authored-by: Brad Zacher <brad.zacher@gmail.com> * Use our own enum for AllowedType * Simplify to Number * Simplify to Number even further --------- Co-authored-by: Brad Zacher <brad.zacher@gmail.com>
typescript-eslint · Feb 16, 2023 · 16144d1 · 16144d1
1 parent 725e7f5
commit 16144d1
Show file tree

Hide file tree

Showing 6 changed files with 977 additions and 0 deletions.
diff --git a/packages/eslint-plugin/docs/rules/no-mixed-enums.md b/packages/eslint-plugin/docs/rules/no-mixed-enums.md
@@ -0,0 +1,88 @@
+---
+description: 'Disallow enums from having both number and string members.'
+---
+
+> 🛑 This file is source code, not the primary documentation location! 🛑
+>
+> See **https://typescript-eslint.io/rules/no-mixed-enums** for documentation.
+
+TypeScript enums are allowed to assign numeric or string values to their members.
+Most enums contain either all numbers or all strings, but in theory you can mix-and-match within the same enum.
+Mixing enum member types is generally considered confusing and a bad practice.
+
+## Examples
+
+<!--tabs-->
+
+### ❌ Incorrect
+
+```ts
+enum Status {
+  Unknown,
+  Closed = 1,
+  Open = 'open',
+}
+```
+
+### ✅ Correct (Explicit Numbers)
+
+```ts
+enum Status {
+  Unknown = 0,
+  Closed = 1,
+  Open = 2,
+}
+```
+
+### ✅ Correct (Implicit Numbers)
+
+```ts
+enum Status {
+  Unknown,
+  Closed,
+  Open,
+}
+```
+
+### ✅ Correct (Strings)
+
+```ts
+enum Status {
+  Unknown = 'unknown',
+  Closed = 'closed',
+  Open = 'open',
+}
+```
+
+## Iteration Pitfalls of Mixed Enum Member Values
+
+Enum values may be iterated over using `Object.entries`/`Object.keys`/`Object.values`.
+
+If all enum members are strings, the number of items will match the number of enum members:
+
+```ts
+enum Status {
+  Closed = 'closed',
+  Open = 'open',
+}
+
+// ['closed', 'open']
+Object.values(Status);
+```
+
+But if the enum contains members that are initialized with numbers -including implicitly initialized numbers— then iteration over that enum will include those numbers as well:
+
+```ts
+enum Status {
+  Unknown,
+  Closed = 1,
+  Open = 'open',
+}
+
+// ["Unknown", "Closed", 0, 1, "open"]
+Object.values(Status);
+```
+
+## When Not To Use It
+
+If you don't mind the confusion of mixed enum member values and don't iterate over enums, you can safely disable this rule.
diff --git a/packages/eslint-plugin/src/configs/all.ts b/packages/eslint-plugin/src/configs/all.ts
@@ -86,6 +86,7 @@ export = {
     '@typescript-eslint/no-meaningless-void-operator': 'error',
     '@typescript-eslint/no-misused-new': 'error',
     '@typescript-eslint/no-misused-promises': 'error',
+    '@typescript-eslint/no-mixed-enums': 'error',
     '@typescript-eslint/no-namespace': 'error',
     '@typescript-eslint/no-non-null-asserted-nullish-coalescing': 'error',
     '@typescript-eslint/no-non-null-asserted-optional-chain': 'error',

diff --git a/packages/eslint-plugin/src/configs/strict.ts b/packages/eslint-plugin/src/configs/strict.ts
@@ -21,6 +21,7 @@ export = {
     '@typescript-eslint/no-extraneous-class': 'warn',
     '@typescript-eslint/no-invalid-void-type': 'warn',
     '@typescript-eslint/no-meaningless-void-operator': 'warn',
+    '@typescript-eslint/no-mixed-enums': 'warn',
     '@typescript-eslint/no-non-null-asserted-nullish-coalescing': 'warn',
     'no-throw-literal': 'off',
     '@typescript-eslint/no-throw-literal': 'warn',

diff --git a/packages/eslint-plugin/src/rules/index.ts b/packages/eslint-plugin/src/rules/index.ts
@@ -59,6 +59,7 @@ import noMagicNumbers from './no-magic-numbers';
 import noMeaninglessVoidOperator from './no-meaningless-void-operator';
 import noMisusedNew from './no-misused-new';
 import noMisusedPromises from './no-misused-promises';
+import noMixedEnums from './no-mixed-enums';
 import noNamespace from './no-namespace';
 import noNonNullAssertedNullishCoalescing from './no-non-null-asserted-nullish-coalescing';
 import noNonNullAssertedOptionalChain from './no-non-null-asserted-optional-chain';
@@ -193,6 +194,7 @@ export default {
   'no-meaningless-void-operator': noMeaninglessVoidOperator,
   'no-misused-new': noMisusedNew,
   'no-misused-promises': noMisusedPromises,
+  'no-mixed-enums': noMixedEnums,
   'no-namespace': noNamespace,
   'no-non-null-asserted-nullish-coalescing': noNonNullAssertedNullishCoalescing,
   'no-non-null-asserted-optional-chain': noNonNullAssertedOptionalChain,

diff --git a/packages/eslint-plugin/src/rules/no-mixed-enums.ts b/packages/eslint-plugin/src/rules/no-mixed-enums.ts
@@ -0,0 +1,224 @@
+import type { Scope } from '@typescript-eslint/scope-manager';
+import { DefinitionType } from '@typescript-eslint/scope-manager';
+import type { TSESTree } from '@typescript-eslint/utils';
+import { AST_NODE_TYPES } from '@typescript-eslint/utils';
+import * as tsutils from 'tsutils';
+import * as ts from 'typescript';
+
+import * as util from '../util';
+
+enum AllowedType {
+  Number,
+  String,
+  Unknown,
+}
+
+export default util.createRule({
+  name: 'no-mixed-enums',
+  meta: {
+    docs: {
+      description: 'Disallow enums from having both number and string members',
+      recommended: 'strict',
+      requiresTypeChecking: true,
+    },
+    messages: {
+      mixed: `Mixing number and string enums can be confusing.`,
+    },
+    schema: [],
+    type: 'problem',
+  },
+  defaultOptions: [],
+  create(context) {
+    const parserServices = util.getParserServices(context);
+    const typeChecker = parserServices.program.getTypeChecker();
+
+    interface CollectedDefinitions {
+      imports: TSESTree.Node[];
+      previousSibling: TSESTree.TSEnumDeclaration | undefined;
+    }
+
+    function collectNodeDefinitions(
+      node: TSESTree.TSEnumDeclaration,
+    ): CollectedDefinitions {
+      const { name } = node.id;
+      const found: CollectedDefinitions = {
+        imports: [],
+        previousSibling: undefined,
+      };
+      let scope: Scope | null = context.getScope();
+
+      for (const definition of scope.upper?.set.get(name)?.defs ?? []) {
+        if (
+          definition.node.type === AST_NODE_TYPES.TSEnumDeclaration &&
+          definition.node.range[0] < node.range[0] &&
+          definition.node.members.length > 0
+        ) {
+          found.previousSibling = definition.node;
+          break;
+        }
+      }
+
+      while (scope) {
+        scope.set.get(name)?.defs?.forEach(definition => {
+          if (definition.type === DefinitionType.ImportBinding) {
+            found.imports.push(definition.node);
+          }
+        });
+
+        scope = scope.upper;
+      }
+
+      return found;
+    }
+
+    function getAllowedTypeForNode(node: ts.Node): AllowedType {
+      return tsutils.isTypeFlagSet(
+        typeChecker.getTypeAtLocation(node),
+        ts.TypeFlags.StringLike,
+      )
+        ? AllowedType.String
+        : AllowedType.Number;
+    }
+
+    function getTypeFromImported(
+      imported: TSESTree.Node,
+    ): AllowedType | undefined {
+      const type = typeChecker.getTypeAtLocation(
+        parserServices.esTreeNodeToTSNodeMap.get(imported),
+      );
+
+      const valueDeclaration = type.getSymbol()?.valueDeclaration;
+      if (
+        !valueDeclaration ||
+        !ts.isEnumDeclaration(valueDeclaration) ||
+        valueDeclaration.members.length === 0
+      ) {
+        return undefined;
+      }
+
+      return getAllowedTypeForNode(valueDeclaration.members[0]);
+    }
+
+    function getMemberType(member: TSESTree.TSEnumMember): AllowedType {
+      if (!member.initializer) {
+        return AllowedType.Number;
+      }
+
+      switch (member.initializer.type) {
+        case AST_NODE_TYPES.Literal:
+          switch (typeof member.initializer.value) {
+            case 'number':
+              return AllowedType.Number;
+            case 'string':
+              return AllowedType.String;
+            default:
+              return AllowedType.Unknown;
+          }
+
+        case AST_NODE_TYPES.TemplateLiteral:
+          return AllowedType.String;
+
+        default:
+          return getAllowedTypeForNode(
+            parserServices.esTreeNodeToTSNodeMap.get(member.initializer),
+          );
+      }
+    }
+
+    function getDesiredTypeForDefinition(
+      node: TSESTree.TSEnumDeclaration,
+    ): AllowedType | ts.TypeFlags.Unknown | undefined {
+      const { imports, previousSibling } = collectNodeDefinitions(node);
+
+      // Case: Merged ambiently via module augmentation
+      // import { MyEnum } from 'other-module';
+      // declare module 'other-module' {
+      //   enum MyEnum { A }
+      // }
+      for (const imported of imports) {
+        const typeFromImported = getTypeFromImported(imported);
+        if (typeFromImported !== undefined) {
+          return typeFromImported;
+        }
+      }
+
+      // Case: Multiple enum declarations in the same file
+      // enum MyEnum { A }
+      // enum MyEnum { B }
+      if (previousSibling) {
+        return getMemberType(previousSibling.members[0]);
+      }
+
+      // Case: Namespace declaration merging
+      // namespace MyNamespace {
+      //   export enum MyEnum { A }
+      // }
+      // namespace MyNamespace {
+      //   export enum MyEnum { B }
+      // }
+      if (
+        node.parent!.type === AST_NODE_TYPES.ExportNamedDeclaration &&
+        node.parent!.parent!.type === AST_NODE_TYPES.TSModuleBlock
+      ) {
+        // TODO: We don't need to dip into the TypeScript type checker here!
+        // Merged namespaces must all exist in the same file.
+        // We could instead compare this file's nodes to find the merges.
+        const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node.id);
+        const declarations = typeChecker
+          .getSymbolAtLocation(tsNode)!
+          .getDeclarations()!;
+
+        for (const declaration of declarations) {
+          for (const member of (declaration as ts.EnumDeclaration).members) {
+            return member.initializer
+              ? tsutils.isTypeFlagSet(
+                  typeChecker.getTypeAtLocation(member.initializer),
+                  ts.TypeFlags.StringLike,
+                )
+                ? AllowedType.String
+                : AllowedType.Number
+              : AllowedType.Number;
+          }
+        }
+      }
+
+      // Finally, we default to the type of the first enum member
+      return getMemberType(node.members[0]);
+    }
+
+    return {
+      TSEnumDeclaration(node): void {
+        if (!node.members.length) {
+          return;
+        }
+
+        let desiredType = getDesiredTypeForDefinition(node);
+        if (desiredType === ts.TypeFlags.Unknown) {
+          return;
+        }
+
+        for (const member of node.members) {
+          const currentType = getMemberType(member);
+          if (currentType === AllowedType.Unknown) {
+            return;
+          }
+
+          if (currentType === AllowedType.Number) {
+            desiredType ??= currentType;
+          }
+
+          if (
+            currentType !== desiredType &&
+            (currentType !== undefined || desiredType === AllowedType.String)
+          ) {
+            context.report({
+              messageId: 'mixed',
+              node: member.initializer ?? member,
+            });
+            return;
+          }
+        }
+      },
+    };
+  },
+});