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
feat(eslint-plugin): [no-unsafe-enum-comparison] add rule #6107
Merged
JoshuaKGoldberg
merged 101 commits into
typescript-eslint:main
from
JoshuaKGoldberg:no-unsafe-enum-comparison
Apr 5, 2023
Merged
Changes from 100 commits
Commits
Show all changes
101 commits
Select commit
Hold shift + click to select a range
481fb44
feat: adding enums to member-ordering rule
Zamiell 5776a01
Merge branch 'main' into strict-enums
Zamiell 094c90e
feat(eslint-plugin): [strict-enums] adding check for null/undefined
Zamiell 311b1f8
feat(eslint-plugin): [strict-enums] refactoring tests to separate files
Zamiell ac5e6d9
feat(eslint-plugin): [strict-enums] allowing more operators
Zamiell 7d55248
fix(eslint-plugin): [strict-enums] adding union test
Zamiell 74af3d1
fix(eslint-plugin): [strict-enums] refactoring + adding failing class
Zamiell 1eb70ee
fix(eslint-plugin): [strict-enums] adding constraint code
Zamiell cf006fd
fix(eslint-plugin): [strict-enums] better eslint messages
Zamiell 4d7f3fe
Merge branch 'main' into strict-enums
Zamiell 2bbc2b7
fix(eslint-plugin): [strict-enums] removing vscode setting changes
Zamiell 75c2252
Merge branch 'main' into strict-enums
Zamiell 3976fdc
fix: changing function definition to reflect reality
Zamiell 2406d03
fix: pass string enum literal into function that take string
Zamiell 4244376
fix: allow passing enums to functions with any/unknown
Zamiell 8b4dcdc
fix: using flags instead of names
Zamiell 12f8120
fix: adding test that breaks the rule
Zamiell e638718
fix: adding test for variadic functions
Zamiell aef971a
fix: adding isSymbolFlagSet internally
Zamiell 75f60b9
fix: adding ignoring for remaining lint failures
Zamiell d784aa0
fix: better comments
Zamiell 811de98
fix: broken test
Zamiell 9a71d45
fix: adding failing test for generic functions
Zamiell f93a02a
fix: refactoring tests + adding tests
Zamiell 2ced9f8
fix: refactoring enum helper function locations
Zamiell 0695c1b
fix: cleanup
Zamiell 7272a95
fix: refactoring + fixing tests
Zamiell 6ed1f66
fix: more tests
Zamiell 83c70e3
fix: refactoring and making tests pass
Zamiell 0ff3096
fix: adding array code, all tests pass now
Zamiell ddebded
fix: adding failing test
Zamiell c8b239b
fix: allow empty arrays
Zamiell 90981a9
fix: adding logic for arrays with no enums
Zamiell 32dec63
fix: adding more tests
Zamiell d0dbcbb
fix: fixing test
Zamiell c63c518
fix: fixing linter
Zamiell c5cf7f3
Merge branch 'main' into strict-enums
Zamiell 1b26d74
Merge branch 'main' into strict-enums
Zamiell 6e4f705
fix: reverting comment fixes
Zamiell a0a2ee6
fix: removing refactor
Zamiell dbae5db
fix: removing fixes to dot-notation
Zamiell 16965d9
fix: removing semi refactor
Zamiell 8b8f8f8
fix: removing jest logic
Zamiell 5f437cc
fix: removing comparison operators check
Zamiell b1dbeb6
fix: adding failing test
Zamiell 25caa05
fix: making test pass
Zamiell f42de61
fix: adding comment
Zamiell a866ddd
fix: adding back in bitwise operator checks since apparently they are
Zamiell 61cf107
fix: remove bad comment
Zamiell 304d796
fix: removing unnecessary comments
Zamiell 83c5f30
fix: remove types from error messages
Zamiell 23ac918
fix: removing isArray + refactoring
Zamiell 098d732
Merge branch 'main' into strict-enums
Zamiell 99424e1
Update packages/eslint-plugin/src/rules/strict-enums.ts
Zamiell 0de3b26
fix: removing strict-enums from recommended
Zamiell 2d488cc
fix: simplify
Zamiell 15dc3df
fix: undoing refactoring
Zamiell a3d0122
fix: undoing refactoring
Zamiell 5cd8fa7
fix: moving tests into subdirectory
Zamiell a236085
Update packages/eslint-plugin/src/rules/strict-enums.ts
Zamiell c357432
Update packages/eslint-plugin/src/rules/strict-enums.ts
Zamiell 2395998
fix: adding failing test
Zamiell 2851c3b
fix: making boolean tests pass
Zamiell 149c049
Merge branch 'main' into strict-enums
Zamiell cf7c9b7
fix: refactor tests + fix linter
Zamiell 3b2fa57
fix: adding brads tests
Zamiell 6c43f71
fix: brads tests now pass
Zamiell af7d5da
Update packages/eslint-plugin/docs/rules/strict-enums.md
Zamiell a50f6e1
Merge branch 'main' into strict-enums
Zamiell 85563be
Update packages/eslint-plugin/src/rules/strict-enums.ts
Zamiell ee96ce5
Update packages/eslint-plugin/src/rules/strict-enums.ts
Zamiell 943bf9f
Update packages/eslint-plugin/src/rules/strict-enums.ts
Zamiell 7679c4a
fix: make brads updates actually compile
Zamiell 871e9ca
Update strict-enums.ts
Zamiell f861606
Merge branch 'main'
JoshuaKGoldberg cbe9e90
Continued fixing merge conflicts
JoshuaKGoldberg c37165c
Continued fixing the build
JoshuaKGoldberg 3f37c67
Passing build
JoshuaKGoldberg c942261
Update packages/eslint-plugin/src/rules/strict-enums.ts
JoshuaKGoldberg b2947f9
Update packages/eslint-plugin/src/rules/strict-enums.ts
JoshuaKGoldberg a7772d2
A few more reverts
JoshuaKGoldberg 4204a60
Just a bit more changing typeFlagUtils
JoshuaKGoldberg 89bcd73
Fixed strict-enums.md build
JoshuaKGoldberg 4f13db4
Convert tests to not pushing
JoshuaKGoldberg 4706e4f
Simplified the rule a whole bunch
JoshuaKGoldberg be144e3
Add back getEnumNames
JoshuaKGoldberg e4ac325
Even more trimming down
JoshuaKGoldberg 62ccdb1
...and just a bit more
JoshuaKGoldberg da7501a
Undo some JSDoc changes
JoshuaKGoldberg 83c33cb
Progress: no-unsafe-enum-assignment is tested
JoshuaKGoldberg a3e6236
A bit more testing for assignments, as requested
JoshuaKGoldberg f1071fa
Finished testing and sharing
JoshuaKGoldberg 680293a
Added comparison operators
JoshuaKGoldberg 51bc2b1
Merge branch 'main'
JoshuaKGoldberg 14cceea
Added back no-unsafe-enum-comparison
JoshuaKGoldberg bc65e3c
Remove unrelated changes
JoshuaKGoldberg adf6f2e
Merge branch 'main' into no-unsafe-enum-comparison
JoshuaKGoldberg 68ad65c
Reduce coverage
JoshuaKGoldberg 57376a7
Merge branch 'main'
JoshuaKGoldberg 559c0ab
Touched up docs
JoshuaKGoldberg b1c9bec
Merge branch 'main' into no-unsafe-enum-comparison
JoshuaKGoldberg File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
75 changes: 75 additions & 0 deletions
75
packages/eslint-plugin/docs/rules/no-unsafe-enum-comparison.md
This file contains 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,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. |
This file contains 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 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 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,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)); | ||
} |
This file contains 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
121 changes: 121 additions & 0 deletions
121
packages/eslint-plugin/src/rules/no-unsafe-enum-comparison.ts
This file contains 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,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=/=|<|>/]'( | ||
armano2 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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, | ||
}); | ||
} | ||
}, | ||
}; | ||
}, | ||
}); |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changing the recommended configs is considered a breaking change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh shoot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah wait, per https://typescript-eslint.io/linting/configs,
strict
isn't considered stable. It's not in our "recommended" configs list. 😌Still filing #6890 just in case.