-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin): [no-mixed-enums] add rule #6102
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
Merged
JoshuaKGoldberg
merged 13 commits into
typescript-eslint:main
from
JoshuaKGoldberg:no-mixed-enums
Feb 16, 2023
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
983ee89
feat(eslint-plugin): [no-mixed-enums] add rule
JoshuaKGoldberg 3be9b29
Fixed test formatting
JoshuaKGoldberg 5e3003a
Fixed config
JoshuaKGoldberg 3e0169e
Fixed description too
JoshuaKGoldberg dcd0c3c
Snazzier query
JoshuaKGoldberg 7f71c10
Merge branch 'main' into no-mixed-enums
JoshuaKGoldberg 961a425
Account for enum merging
JoshuaKGoldberg c5beaa4
Avoided the type checker where possible (mostly)
JoshuaKGoldberg 37d47b2
Added more type coverage
JoshuaKGoldberg 7f3d945
Apply suggestions from code review
JoshuaKGoldberg bc91e7e
Use our own enum for AllowedType
JoshuaKGoldberg ada72bc
Simplify to Number
JoshuaKGoldberg 1de4847
Simplify to Number even further
JoshuaKGoldberg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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]); | ||
} | ||
bradzacher marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// 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. | ||
bradzacher marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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; | ||
} | ||
} | ||
}, | ||
}; | ||
}, | ||
}); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.