From 983ee89ed7d988d5a541f60076853ccfadc616d2 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sat, 26 Nov 2022 21:27:17 -0500 Subject: [PATCH 01/12] feat(eslint-plugin): [no-mixed-enums] add rule --- .../docs/rules/no-mixed-enums.md | 85 ++++++++ packages/eslint-plugin/src/configs/all.ts | 1 + packages/eslint-plugin/src/configs/strict.ts | 1 + packages/eslint-plugin/src/rules/index.ts | 2 + .../eslint-plugin/src/rules/no-mixed-enums.ts | 62 ++++++ .../tests/rules/no-mixed-enums.test.ts | 202 ++++++++++++++++++ 6 files changed, 353 insertions(+) create mode 100644 packages/eslint-plugin/docs/rules/no-mixed-enums.md create mode 100644 packages/eslint-plugin/src/rules/no-mixed-enums.ts create mode 100644 packages/eslint-plugin/tests/rules/no-mixed-enums.test.ts diff --git a/packages/eslint-plugin/docs/rules/no-mixed-enums.md b/packages/eslint-plugin/docs/rules/no-mixed-enums.md new file mode 100644 index 00000000000..08d2c8cbb5f --- /dev/null +++ b/packages/eslint-plugin/docs/rules/no-mixed-enums.md @@ -0,0 +1,85 @@ +--- +description: 'Disallow enums to have 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 + + + +### ❌ 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 index 20ea892f581..1b652837ddb 100644 --- a/packages/eslint-plugin/src/configs/all.ts +++ b/packages/eslint-plugin/src/configs/all.ts @@ -127,6 +127,7 @@ export = { 'padding-line-between-statements': 'off', '@typescript-eslint/padding-line-between-statements': 'error', '@typescript-eslint/prefer-as-const': 'error', + '@typescript-eslint/prefer-consistent-enums': 'error', '@typescript-eslint/prefer-enum-initializers': 'error', '@typescript-eslint/prefer-for-of': 'error', '@typescript-eslint/prefer-function-type': 'error', diff --git a/packages/eslint-plugin/src/configs/strict.ts b/packages/eslint-plugin/src/configs/strict.ts index ccd44b85a0a..0ec4dc07660 100644 --- 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 index 8a3c2bbf437..3bd96d9309a 100644 --- a/packages/eslint-plugin/src/rules/index.ts +++ b/packages/eslint-plugin/src/rules/index.ts @@ -56,6 +56,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'; @@ -187,6 +188,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 new file mode 100644 index 00000000000..73111b4723e --- /dev/null +++ b/packages/eslint-plugin/src/rules/no-mixed-enums.ts @@ -0,0 +1,62 @@ +import type { TSESTree } from '@typescript-eslint/utils'; +import * as tsutils from 'tsutils'; +import * as ts from 'typescript'; + +import * as util from '../util'; + +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(); + + function getMemberType( + initializer: TSESTree.Expression | undefined, + ): ts.TypeFlags.Number | ts.TypeFlags.String { + if (!initializer) { + return ts.TypeFlags.Number; + } + + return tsutils.isTypeFlagSet( + typeChecker.getTypeAtLocation( + parserServices.esTreeNodeToTSNodeMap.get(initializer), + ), + ts.TypeFlags.StringLike, + ) + ? ts.TypeFlags.String + : ts.TypeFlags.Number; + } + + return { + 'TSEnumDeclaration:has(TSEnumMember ~ TSEnumMember)'( + node: TSESTree.TSEnumDeclaration, + ): void { + const [firstMember, ...remainingMembers] = node.members; + const allowedMemberType = getMemberType(firstMember.initializer); + + for (const member of remainingMembers) { + if (getMemberType(member.initializer) !== allowedMemberType) { + context.report({ + messageId: 'mixed', + node: member.initializer!, + }); + return; + } + } + }, + }; + }, +}); diff --git a/packages/eslint-plugin/tests/rules/no-mixed-enums.test.ts b/packages/eslint-plugin/tests/rules/no-mixed-enums.test.ts new file mode 100644 index 00000000000..4a55bd52796 --- /dev/null +++ b/packages/eslint-plugin/tests/rules/no-mixed-enums.test.ts @@ -0,0 +1,202 @@ +import rule from '../../src/rules/no-mixed-enums'; +import { getFixturesRootDir, RuleTester } from '../RuleTester'; + +const rootDir = getFixturesRootDir(); +const ruleTester = new RuleTester({ + parserOptions: { + tsconfigRootDir: rootDir, + project: './tsconfig.json', + }, + parser: '@typescript-eslint/parser', +}); + +ruleTester.run('no-mixed-enums', rule, { + valid: [ + ` + enum Fruit {} + `, + ` + enum Fruit { + Apple, + } + `, + ` + enum Fruit { + Apple, + Banana, + } + `, + ` + enum Fruit { + Apple = 0, + Banana, + } + `, + ` + enum Fruit { + Apple, + Banana = 1, + } + `, + ` + enum Fruit { + Apple = 0, + Banana = 1, + } + `, + ` + const getValue = () => 0; + enum Fruit { + Apple, + Banana = getValue(), + } + `, + ` + const getValue = () => 0; + enum Fruit { + Apple = getValue(), + Banana = getValue(), + } + `, + ` + const getValue = () => ''; + enum Fruit { + Apple = '', + Banana = getValue(), + } + `, + ` + const getValue = () => ''; + enum Fruit { + Apple = getValue(), + Banana = '', + } + `, + ` + const getValue = () => ''; + enum Fruit { + Apple = getValue(), + Banana = getValue(), + } + `, + ], + invalid: [ + { + code: ` + enum Fruit { + Apple, + Banana = 'banana', + } + `, + errors: [ + { + endColumn: 28, + column: 20, + line: 4, + messageId: 'mixed', + }, + ], + }, + { + code: ` + enum Fruit { + Apple, + Banana = 'banana', + Cherry = 'cherry', + } + `, + errors: [ + { + endColumn: 28, + column: 20, + line: 4, + messageId: 'mixed', + }, + ], + }, + { + code: ` + enum Fruit { + Apple, + Banana, + Cherry = 'cherry', + } + `, + errors: [ + { + endColumn: 28, + column: 20, + line: 5, + messageId: 'mixed', + }, + ], + }, + { + code: ` + enum Fruit { + Apple = 0, + Banana = 'banana', + } + `, + errors: [ + { + endColumn: 28, + column: 20, + line: 4, + messageId: 'mixed', + }, + ], + }, + { + code: ` + const getValue = () => 0, + enum Fruit { + Apple = getValue(), + Banana = 'banana', + } + `, + errors: [ + { + endColumn: 28, + column: 20, + line: 5, + messageId: 'mixed', + }, + ], + }, + { + code: ` + const getValue = () => '', + enum Fruit { + Apple, + Banana = getValue(), + } + `, + errors: [ + { + endColumn: 30, + column: 20, + line: 5, + messageId: 'mixed', + }, + ], + }, + { + code: ` + const getValue = () => '', + enum Fruit { + Apple = getValue(), + Banana = 0, + } + `, + errors: [ + { + endColumn: 21, + column: 20, + line: 5, + messageId: 'mixed', + }, + ], + }, + ], +}); From 3be9b296a05e64f37a080b8c6987ca84c73c50d7 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sat, 26 Nov 2022 21:40:52 -0500 Subject: [PATCH 02/12] Fixed test formatting --- packages/eslint-plugin/tests/rules/no-mixed-enums.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/eslint-plugin/tests/rules/no-mixed-enums.test.ts b/packages/eslint-plugin/tests/rules/no-mixed-enums.test.ts index 4a55bd52796..7a4662e6ab8 100644 --- a/packages/eslint-plugin/tests/rules/no-mixed-enums.test.ts +++ b/packages/eslint-plugin/tests/rules/no-mixed-enums.test.ts @@ -149,7 +149,7 @@ ruleTester.run('no-mixed-enums', rule, { }, { code: ` - const getValue = () => 0, + const getValue = () => 0; enum Fruit { Apple = getValue(), Banana = 'banana', @@ -166,7 +166,7 @@ ruleTester.run('no-mixed-enums', rule, { }, { code: ` - const getValue = () => '', + const getValue = () => ''; enum Fruit { Apple, Banana = getValue(), @@ -183,7 +183,7 @@ ruleTester.run('no-mixed-enums', rule, { }, { code: ` - const getValue = () => '', + const getValue = () => ''; enum Fruit { Apple = getValue(), Banana = 0, From 5e3003adf82e859fafb3a5c74b24c1e002826d96 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sat, 26 Nov 2022 21:41:27 -0500 Subject: [PATCH 03/12] Fixed config --- packages/eslint-plugin/src/configs/all.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/src/configs/all.ts b/packages/eslint-plugin/src/configs/all.ts index 1b652837ddb..fadf9b3a292 100644 --- a/packages/eslint-plugin/src/configs/all.ts +++ b/packages/eslint-plugin/src/configs/all.ts @@ -82,6 +82,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', @@ -127,7 +128,6 @@ export = { 'padding-line-between-statements': 'off', '@typescript-eslint/padding-line-between-statements': 'error', '@typescript-eslint/prefer-as-const': 'error', - '@typescript-eslint/prefer-consistent-enums': 'error', '@typescript-eslint/prefer-enum-initializers': 'error', '@typescript-eslint/prefer-for-of': 'error', '@typescript-eslint/prefer-function-type': 'error', From 3e0169e9f2a4395e76f15541f7ac720a36d10dc5 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sat, 26 Nov 2022 21:41:49 -0500 Subject: [PATCH 04/12] Fixed description too --- packages/eslint-plugin/docs/rules/no-mixed-enums.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/docs/rules/no-mixed-enums.md b/packages/eslint-plugin/docs/rules/no-mixed-enums.md index 08d2c8cbb5f..41d11e6c26a 100644 --- a/packages/eslint-plugin/docs/rules/no-mixed-enums.md +++ b/packages/eslint-plugin/docs/rules/no-mixed-enums.md @@ -1,5 +1,5 @@ --- -description: 'Disallow enums to have both number and string members.' +description: 'Disallow enums from having both number and string members.' --- > πŸ›‘ This file is source code, not the primary documentation location! πŸ›‘ From dcd0c3c09a7f897e397d39bf94bd31a6b62228c1 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sat, 26 Nov 2022 22:49:11 -0500 Subject: [PATCH 05/12] Snazzier query --- packages/eslint-plugin/src/rules/no-mixed-enums.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-mixed-enums.ts b/packages/eslint-plugin/src/rules/no-mixed-enums.ts index 73111b4723e..a9f6b3e237b 100644 --- a/packages/eslint-plugin/src/rules/no-mixed-enums.ts +++ b/packages/eslint-plugin/src/rules/no-mixed-enums.ts @@ -41,9 +41,7 @@ export default util.createRule({ } return { - 'TSEnumDeclaration:has(TSEnumMember ~ TSEnumMember)'( - node: TSESTree.TSEnumDeclaration, - ): void { + 'TSEnumDeclaration[members.1]'(node: TSESTree.TSEnumDeclaration): void { const [firstMember, ...remainingMembers] = node.members; const allowedMemberType = getMemberType(firstMember.initializer); From 961a4259c8ef343d483937a4faefeff4a17d5697 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sat, 4 Feb 2023 19:35:06 -0500 Subject: [PATCH 06/12] Account for enum merging --- .../eslint-plugin/src/rules/no-mixed-enums.ts | 55 ++-- .../tests/rules/no-mixed-enums.test.ts | 247 ++++++++++++++++++ 2 files changed, 286 insertions(+), 16 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-mixed-enums.ts b/packages/eslint-plugin/src/rules/no-mixed-enums.ts index a9f6b3e237b..5bab57b9901 100644 --- a/packages/eslint-plugin/src/rules/no-mixed-enums.ts +++ b/packages/eslint-plugin/src/rules/no-mixed-enums.ts @@ -4,6 +4,8 @@ import * as ts from 'typescript'; import * as util from '../util'; +type AllowedType = ts.TypeFlags.Number | ts.TypeFlags.String; + export default util.createRule({ name: 'no-mixed-enums', meta: { @@ -23,33 +25,54 @@ export default util.createRule({ const parserServices = util.getParserServices(context); const typeChecker = parserServices.program.getTypeChecker(); - function getMemberType( - initializer: TSESTree.Expression | undefined, - ): ts.TypeFlags.Number | ts.TypeFlags.String { - if (!initializer) { - return ts.TypeFlags.Number; - } - + function getAllowedType(node: ts.Node): AllowedType { return tsutils.isTypeFlagSet( - typeChecker.getTypeAtLocation( - parserServices.esTreeNodeToTSNodeMap.get(initializer), - ), + typeChecker.getTypeAtLocation(node), ts.TypeFlags.StringLike, ) ? ts.TypeFlags.String : ts.TypeFlags.Number; } + function getAllowedTypeOfEnum( + node: TSESTree.TSEnumDeclaration, + ): AllowedType | undefined { + 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 + ? getAllowedType(member.initializer) + : ts.TypeFlags.Number; + } + } + + return undefined; + } + + function getAllowedTypeOfInitializer( + initializer: TSESTree.Expression | undefined, + ): ts.TypeFlags.Number | ts.TypeFlags.String { + return initializer + ? getAllowedType(parserServices.esTreeNodeToTSNodeMap.get(initializer)) + : ts.TypeFlags.Number; + } + return { - 'TSEnumDeclaration[members.1]'(node: TSESTree.TSEnumDeclaration): void { - const [firstMember, ...remainingMembers] = node.members; - const allowedMemberType = getMemberType(firstMember.initializer); + TSEnumDeclaration(node: TSESTree.TSEnumDeclaration): void { + const allowedType = getAllowedTypeOfEnum(node); + if (allowedType === undefined) { + return; + } - for (const member of remainingMembers) { - if (getMemberType(member.initializer) !== allowedMemberType) { + for (const member of node.members) { + if (getAllowedTypeOfInitializer(member.initializer) !== allowedType) { context.report({ messageId: 'mixed', - node: member.initializer!, + node: member.initializer ?? member, }); return; } diff --git a/packages/eslint-plugin/tests/rules/no-mixed-enums.test.ts b/packages/eslint-plugin/tests/rules/no-mixed-enums.test.ts index 7a4662e6ab8..fee010548a2 100644 --- a/packages/eslint-plugin/tests/rules/no-mixed-enums.test.ts +++ b/packages/eslint-plugin/tests/rules/no-mixed-enums.test.ts @@ -79,6 +79,72 @@ ruleTester.run('no-mixed-enums', rule, { Banana = getValue(), } `, + ` + enum First { + A = 1, + } + + enum Second { + A = First.A, + B = 2, + } + `, + ` + enum First { + A = '', + } + + enum Second { + A = First.A, + B = 'b', + } + `, + ` + enum Foo { + A, + } + enum Foo { + B, + } + `, + ` + enum Foo { + A = 0, + } + enum Foo { + B, + } + `, + ` + enum Foo { + A, + } + enum Foo { + B = 1, + } + `, + ` + enum Foo { + A = 0, + } + enum Foo { + B = 1, + } + `, + ` + enum Foo { + A = 'a', + } + enum Foo { + B = 'b', + } + `, + ` + declare const Foo: any; + enum Foo { + A, + } + `, ], invalid: [ { @@ -198,5 +264,186 @@ ruleTester.run('no-mixed-enums', rule, { }, ], }, + { + code: ` + enum First { + A = 1, + } + + enum Second { + A = First.A, + B = 'b', + } + `, + errors: [ + { + endColumn: 18, + column: 15, + line: 8, + messageId: 'mixed', + }, + ], + }, + { + code: ` + enum First { + A = 'a', + } + + enum Second { + A = First.A, + B = 1, + } + `, + errors: [ + { + endColumn: 16, + column: 15, + line: 8, + messageId: 'mixed', + }, + ], + }, + { + code: ` + enum Foo { + A, + } + enum Foo { + B = 'b', + } + `, + errors: [ + { + endColumn: 18, + column: 15, + line: 6, + messageId: 'mixed', + }, + ], + }, + { + code: ` + enum Foo { + A = 1, + } + enum Foo { + B = 'b', + } + `, + errors: [ + { + endColumn: 18, + column: 15, + line: 6, + messageId: 'mixed', + }, + ], + }, + { + code: ` + enum Foo { + A = 'a', + } + enum Foo { + B, + } + `, + errors: [ + { + endColumn: 12, + column: 11, + line: 6, + messageId: 'mixed', + }, + ], + }, + { + code: ` + enum Foo { + A = 'a', + } + enum Foo { + B = 0, + } + `, + errors: [ + { + endColumn: 16, + column: 15, + line: 6, + messageId: 'mixed', + }, + ], + }, + { + code: ` + enum Foo { + A, + } + enum Foo { + B = 'b', + } + enum Foo { + C = 'c', + } + `, + errors: [ + { + endColumn: 18, + column: 15, + line: 6, + messageId: 'mixed', + }, + { + endColumn: 18, + column: 15, + line: 9, + messageId: 'mixed', + }, + ], + }, + { + code: ` + enum Foo { + A, + } + enum Foo { + B = 'b', + } + enum Foo { + C, + } + `, + errors: [ + { + endColumn: 18, + column: 15, + line: 6, + messageId: 'mixed', + }, + ], + }, + { + code: ` + enum Foo { + A, + } + enum Foo { + B, + } + enum Foo { + C = 'c', + } + `, + errors: [ + { + endColumn: 18, + column: 15, + line: 9, + messageId: 'mixed', + }, + ], + }, ], }); From c5beaa4bab18d86a99fcb83ce4cb7af5cb94c27f Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sun, 12 Feb 2023 16:43:23 -0500 Subject: [PATCH 07/12] Avoided the type checker where possible (mostly) --- .../eslint-plugin/src/rules/no-mixed-enums.ts | 181 +++++++++++++++--- .../tests/rules/no-mixed-enums.test.ts | 146 ++++++++++++++ 2 files changed, 303 insertions(+), 24 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-mixed-enums.ts b/packages/eslint-plugin/src/rules/no-mixed-enums.ts index 5bab57b9901..4e1d3b8f92b 100644 --- a/packages/eslint-plugin/src/rules/no-mixed-enums.ts +++ b/packages/eslint-plugin/src/rules/no-mixed-enums.ts @@ -1,4 +1,7 @@ +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'; @@ -25,7 +28,46 @@ export default util.createRule({ const parserServices = util.getParserServices(context); const typeChecker = parserServices.program.getTypeChecker(); - function getAllowedType(node: ts.Node): AllowedType { + 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 | undefined { return tsutils.isTypeFlagSet( typeChecker.getTypeAtLocation(node), ts.TypeFlags.StringLike, @@ -34,42 +76,133 @@ export default util.createRule({ : ts.TypeFlags.Number; } - function getAllowedTypeOfEnum( + 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 | undefined { + if (!member.initializer) { + return undefined; + } + + switch (member.initializer.type) { + case AST_NODE_TYPES.Literal: + switch (typeof member.initializer.value) { + case 'number': + return ts.TypeFlags.Number; + case 'string': + return ts.TypeFlags.String; + default: + return undefined; + } + + case AST_NODE_TYPES.TemplateLiteral: + return ts.TypeFlags.String; + + default: + return getAllowedTypeForNode( + parserServices.esTreeNodeToTSNodeMap.get(member.initializer), + ); + } + } + + function getDesiredTypeForDefinition( node: TSESTree.TSEnumDeclaration, ): AllowedType | undefined { - 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 - ? getAllowedType(member.initializer) - : ts.TypeFlags.Number; + 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; } } - return undefined; - } + // Case: Multiple enum declarations in the same file + // enum MyEnum { A } + // enum MyEnum { B } + if (previousSibling) { + return getMemberType(previousSibling.members[0]); + } - function getAllowedTypeOfInitializer( - initializer: TSESTree.Expression | undefined, - ): ts.TypeFlags.Number | ts.TypeFlags.String { - return initializer - ? getAllowedType(parserServices.esTreeNodeToTSNodeMap.get(initializer)) - : ts.TypeFlags.Number; + // 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, + ) + ? ts.TypeFlags.String + : ts.TypeFlags.Number + : ts.TypeFlags.Number; + } + } + } + + // Finally, we default to the type of the first enum member + return getMemberType(node.members[0]); } return { - TSEnumDeclaration(node: TSESTree.TSEnumDeclaration): void { - const allowedType = getAllowedTypeOfEnum(node); - if (allowedType === undefined) { + TSEnumDeclaration(node): void { + if (!node.members.length) { return; } + let desiredType = getDesiredTypeForDefinition(node); + for (const member of node.members) { - if (getAllowedTypeOfInitializer(member.initializer) !== allowedType) { + const currentType = getMemberType(member); + + if (currentType === ts.TypeFlags.Number) { + desiredType ??= currentType; + } + + if ( + currentType !== desiredType && + (currentType !== undefined || desiredType === ts.TypeFlags.String) + ) { context.report({ messageId: 'mixed', node: member.initializer ?? member, diff --git a/packages/eslint-plugin/tests/rules/no-mixed-enums.test.ts b/packages/eslint-plugin/tests/rules/no-mixed-enums.test.ts index fee010548a2..539708d78e0 100644 --- a/packages/eslint-plugin/tests/rules/no-mixed-enums.test.ts +++ b/packages/eslint-plugin/tests/rules/no-mixed-enums.test.ts @@ -145,6 +145,67 @@ ruleTester.run('no-mixed-enums', rule, { A, } `, + ` +enum Foo { + A = 1, +} +enum Foo { + B = 2, +} + `, + ` +import { AST_NODE_TYPES } from '@typescript-eslint/types'; + +declare module '@typescript-eslint/types' { + enum AST_NODE_TYPES { + StringLike = 'StringLike', + } +} + `, + ` +namespace Test { + export enum Bar { + A = 1, + } +} +namespace Test { + export enum Bar { + B = 2, + } +} + `, + ` +namespace Outer { + namespace Test { + export enum Bar { + A = 1, + } + } +} +namespace Outer { + namespace Test { + export enum Bar { + B = 'B', + } + } +} + `, + ` +namespace Outer { + namespace Test { + export enum Bar { + A = 1, + } + } +} +namespace Different { + namespace Test { + export enum Bar { + B = 'B', + } + } +} + `, ], invalid: [ { @@ -445,5 +506,90 @@ ruleTester.run('no-mixed-enums', rule, { }, ], }, + { + code: ` +import { AST_NODE_TYPES } from '@typescript-eslint/types'; + +declare module '@typescript-eslint/types' { + enum AST_NODE_TYPES { + Numeric = 0, + } +} + `, + errors: [ + { + endColumn: 16, + column: 15, + line: 6, + messageId: 'mixed', + }, + ], + }, + { + code: ` +enum Foo { + A = 1, +} +enum Foo { + B = 'B', +} + `, + errors: [ + { + endColumn: 10, + column: 7, + line: 6, + messageId: 'mixed', + }, + ], + }, + { + code: ` +namespace Test { + export enum Bar { + A = 1, + } +} +namespace Test { + export enum Bar { + B = 'B', + } +} + `, + errors: [ + { + endColumn: 12, + column: 9, + line: 9, + messageId: 'mixed', + }, + ], + }, + { + code: ` +namespace Outer { + export namespace Test { + export enum Bar { + A = 1, + } + } +} +namespace Outer { + export namespace Test { + export enum Bar { + B = 'B', + } + } +} + `, + errors: [ + { + endColumn: 14, + column: 11, + line: 12, + messageId: 'mixed', + }, + ], + }, ], }); From 37d47b2faa430ccc47a69ff2cfd84c531d89f0b0 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sun, 12 Feb 2023 17:12:46 -0500 Subject: [PATCH 08/12] Added more type coverage --- .../eslint-plugin/src/rules/no-mixed-enums.ts | 12 +++- .../tests/rules/no-mixed-enums.test.ts | 66 +++++++++++++++++++ 2 files changed, 75 insertions(+), 3 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-mixed-enums.ts b/packages/eslint-plugin/src/rules/no-mixed-enums.ts index 4e1d3b8f92b..fe0b7f8b54f 100644 --- a/packages/eslint-plugin/src/rules/no-mixed-enums.ts +++ b/packages/eslint-plugin/src/rules/no-mixed-enums.ts @@ -97,7 +97,7 @@ export default util.createRule({ function getMemberType( member: TSESTree.TSEnumMember, - ): AllowedType | undefined { + ): AllowedType | ts.TypeFlags.Unknown | undefined { if (!member.initializer) { return undefined; } @@ -110,7 +110,7 @@ export default util.createRule({ case 'string': return ts.TypeFlags.String; default: - return undefined; + return ts.TypeFlags.Unknown; } case AST_NODE_TYPES.TemplateLiteral: @@ -125,7 +125,7 @@ export default util.createRule({ function getDesiredTypeForDefinition( node: TSESTree.TSEnumDeclaration, - ): AllowedType | undefined { + ): AllowedType | ts.TypeFlags.Unknown | undefined { const { imports, previousSibling } = collectNodeDefinitions(node); // Case: Merged ambiently via module augmentation @@ -191,9 +191,15 @@ export default util.createRule({ } let desiredType = getDesiredTypeForDefinition(node); + if (desiredType === ts.TypeFlags.Unknown) { + return; + } for (const member of node.members) { const currentType = getMemberType(member); + if (currentType === ts.TypeFlags.Unknown) { + return; + } if (currentType === ts.TypeFlags.Number) { desiredType ??= currentType; diff --git a/packages/eslint-plugin/tests/rules/no-mixed-enums.test.ts b/packages/eslint-plugin/tests/rules/no-mixed-enums.test.ts index 539708d78e0..3847fda0131 100644 --- a/packages/eslint-plugin/tests/rules/no-mixed-enums.test.ts +++ b/packages/eslint-plugin/tests/rules/no-mixed-enums.test.ts @@ -20,6 +20,11 @@ ruleTester.run('no-mixed-enums', rule, { Apple, } `, + ` + enum Fruit { + Apple = false, + } + `, ` enum Fruit { Apple, @@ -44,6 +49,12 @@ ruleTester.run('no-mixed-enums', rule, { Banana = 1, } `, + ` + enum Fruit { + Apple, + Banana = false, + } + `, ` const getValue = () => 0; enum Fruit { @@ -154,6 +165,30 @@ enum Foo { } `, ` +enum Foo { + A = \`A\`, +} +enum Foo { + B = \`B\`, +} + `, + ` +enum Foo { + A = false, // (TS error) +} +enum Foo { + B = \`B\`, +} + `, + ` +enum Foo { + A = 'A', +} +enum Foo { + B = false, // (TS error) +} + `, + ` import { AST_NODE_TYPES } from '@typescript-eslint/types'; declare module '@typescript-eslint/types' { @@ -163,6 +198,15 @@ declare module '@typescript-eslint/types' { } `, ` +import { TSESTree } from '@typescript-eslint/types'; + +declare module '@typescript-eslint/types' { + enum TSESTree { + StringLike = 'StringLike', + } +} + `, + ` namespace Test { export enum Bar { A = 1, @@ -550,6 +594,28 @@ namespace Test { A = 1, } } +namespace Test { + export enum Bar { + B = 'B', + } +} + `, + errors: [ + { + endColumn: 12, + column: 9, + line: 9, + messageId: 'mixed', + }, + ], + }, + { + code: ` +namespace Test { + export enum Bar { + A, + } +} namespace Test { export enum Bar { B = 'B', From 7f3d94505990c08f34a77254a2e17270b6832c87 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Thu, 16 Feb 2023 14:02:40 -0500 Subject: [PATCH 09/12] Apply suggestions from code review Co-authored-by: Brad Zacher --- packages/eslint-plugin/docs/rules/no-mixed-enums.md | 7 +++++-- packages/eslint-plugin/src/rules/no-mixed-enums.ts | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-mixed-enums.md b/packages/eslint-plugin/docs/rules/no-mixed-enums.md index 41d11e6c26a..96ccbddf4e0 100644 --- a/packages/eslint-plugin/docs/rules/no-mixed-enums.md +++ b/packages/eslint-plugin/docs/rules/no-mixed-enums.md @@ -30,7 +30,8 @@ enum Status { enum Status { Unknown = 0, Closed = 1, - Open = 2 + Open = 2, +} ``` ### βœ… Correct (Implicit Numbers) @@ -39,7 +40,8 @@ enum Status { enum Status { Unknown, Closed, - Open + Open, +} ``` ### βœ… Correct (Strings) @@ -49,6 +51,7 @@ enum Status { Unknown = 'unknown', Closed = 'closed', Open = 'open', +} ``` ## Iteration Pitfalls of Mixed Enum Member Values diff --git a/packages/eslint-plugin/src/rules/no-mixed-enums.ts b/packages/eslint-plugin/src/rules/no-mixed-enums.ts index fe0b7f8b54f..8b7418b0769 100644 --- a/packages/eslint-plugin/src/rules/no-mixed-enums.ts +++ b/packages/eslint-plugin/src/rules/no-mixed-enums.ts @@ -67,7 +67,7 @@ export default util.createRule({ return found; } - function getAllowedTypeForNode(node: ts.Node): AllowedType | undefined { + function getAllowedTypeForNode(node: ts.Node): AllowedType { return tsutils.isTypeFlagSet( typeChecker.getTypeAtLocation(node), ts.TypeFlags.StringLike, From bc91e7ebe8cc3a78e97a13f21e7af131ad51dcf7 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Thu, 16 Feb 2023 14:03:54 -0500 Subject: [PATCH 10/12] Use our own enum for AllowedType --- .../eslint-plugin/src/rules/no-mixed-enums.ts | 28 +++++++++++-------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-mixed-enums.ts b/packages/eslint-plugin/src/rules/no-mixed-enums.ts index 8b7418b0769..465a8091992 100644 --- a/packages/eslint-plugin/src/rules/no-mixed-enums.ts +++ b/packages/eslint-plugin/src/rules/no-mixed-enums.ts @@ -7,7 +7,11 @@ import * as ts from 'typescript'; import * as util from '../util'; -type AllowedType = ts.TypeFlags.Number | ts.TypeFlags.String; +enum AllowedType { + Number, + String, + Unknown, +} export default util.createRule({ name: 'no-mixed-enums', @@ -72,8 +76,8 @@ export default util.createRule({ typeChecker.getTypeAtLocation(node), ts.TypeFlags.StringLike, ) - ? ts.TypeFlags.String - : ts.TypeFlags.Number; + ? AllowedType.String + : AllowedType.Number; } function getTypeFromImported( @@ -106,15 +110,15 @@ export default util.createRule({ case AST_NODE_TYPES.Literal: switch (typeof member.initializer.value) { case 'number': - return ts.TypeFlags.Number; + return AllowedType.Number; case 'string': - return ts.TypeFlags.String; + return AllowedType.String; default: - return ts.TypeFlags.Unknown; + return AllowedType.Unknown; } case AST_NODE_TYPES.TemplateLiteral: - return ts.TypeFlags.String; + return AllowedType.String; default: return getAllowedTypeForNode( @@ -173,9 +177,9 @@ export default util.createRule({ typeChecker.getTypeAtLocation(member.initializer), ts.TypeFlags.StringLike, ) - ? ts.TypeFlags.String - : ts.TypeFlags.Number - : ts.TypeFlags.Number; + ? AllowedType.String + : AllowedType.Number + : AllowedType.Number; } } } @@ -201,13 +205,13 @@ export default util.createRule({ return; } - if (currentType === ts.TypeFlags.Number) { + if (currentType === AllowedType.Number) { desiredType ??= currentType; } if ( currentType !== desiredType && - (currentType !== undefined || desiredType === ts.TypeFlags.String) + (currentType !== undefined || desiredType === AllowedType.String) ) { context.report({ messageId: 'mixed', From ada72bc8bd6641a4c68d578cd53a259f7a9597d8 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Thu, 16 Feb 2023 14:09:41 -0500 Subject: [PATCH 11/12] Simplify to Number --- packages/eslint-plugin/src/rules/no-mixed-enums.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-mixed-enums.ts b/packages/eslint-plugin/src/rules/no-mixed-enums.ts index 465a8091992..918c33c5c51 100644 --- a/packages/eslint-plugin/src/rules/no-mixed-enums.ts +++ b/packages/eslint-plugin/src/rules/no-mixed-enums.ts @@ -101,9 +101,9 @@ export default util.createRule({ function getMemberType( member: TSESTree.TSEnumMember, - ): AllowedType | ts.TypeFlags.Unknown | undefined { + ): AllowedType | undefined { if (!member.initializer) { - return undefined; + return AllowedType.Number; } switch (member.initializer.type) { @@ -201,7 +201,7 @@ export default util.createRule({ for (const member of node.members) { const currentType = getMemberType(member); - if (currentType === ts.TypeFlags.Unknown) { + if (currentType === AllowedType.Unknown) { return; } From 1de48473565fe0b846a9da790db73187691658bc Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Thu, 16 Feb 2023 14:09:58 -0500 Subject: [PATCH 12/12] Simplify to Number even further --- packages/eslint-plugin/src/rules/no-mixed-enums.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-mixed-enums.ts b/packages/eslint-plugin/src/rules/no-mixed-enums.ts index 918c33c5c51..047471d4b26 100644 --- a/packages/eslint-plugin/src/rules/no-mixed-enums.ts +++ b/packages/eslint-plugin/src/rules/no-mixed-enums.ts @@ -99,9 +99,7 @@ export default util.createRule({ return getAllowedTypeForNode(valueDeclaration.members[0]); } - function getMemberType( - member: TSESTree.TSEnumMember, - ): AllowedType | undefined { + function getMemberType(member: TSESTree.TSEnumMember): AllowedType { if (!member.initializer) { return AllowedType.Number; }