Rule proposal: Disallow literals where enums are expected #308
Comments
|
This could be expanded to just "disallow non-enum comparisons when using enum types" So that it covers all of the cases (which are valid typescript, but error-prone): enum FooImplicit {
A,
B,
}
FooImplicit.A === 0
enum FooNum {
A = 1,
B = 2,
}
FooNum.A === 1
enum FooStr {
A = 'a',
B = 'b',
}
FooStr.A === 'a' |
|
TS 4.3 brings in some checks here that solves this for number enums! |
|
@bradzacher @JoshuaKGoldberg if someone makes a PR for this feature, what should the name of the rule be? |
|
It's really hard to name this. Maybe something like Given this rule will cover assignments and comparisons I can't think of a good name which covers both cases succinctly. |
|
@ChocolateLoverRaj are you interested in & have time for sending a PR? No worries if you don't, I can send one soon if not! |
Yes, i can make a PR. it's my first time doing an eslint related PR though. |
|
@ChocolateLoverRaj Any updates on this? |
How about |
|
Also, I would want greater-than and less-than comparison operators to be illegal: enum Foo {
A,
B,
C,
D,
}
// bad
if (foo < Foo.C) {}
// good
if (foo === Foo.A || foo === Foo.B) {} |
|
I'm trying my hand at this, but I'm stuck at comparing the types. const leftType = typeChecker.getTypeAtLocation(leftTSNode);
const rightType = typeChecker.getTypeAtLocation(rightTSNode);
const areTypesEqual = ???What is the correct way to see if the types are equal? I'm looking for an ID field or something, but I don't see anything like that. In the helper functions, I do see |
|
I proceeded with just using the name to compare for now. function fruitFunction(fruit: Fruit) {}
fruitFunction(0); // This line should throw an ESLint warningI'm stuck on how to get the type of the matching function argument. Specifically, on how to write the "getFunctionArgumentType" function: CallExpression(node): void {
const identifier = node.callee;
const functionType = getTypeFromNode(identifier);
for (let i = 0; i < node.arguments.length; i++) {
const expressionArgument = node.arguments[i];
const expressionArgumentType = getTypeFromNode(argument);
const expressionArgumentTypeName = getTypeName(argumentType);
const functionArgumentType = getFunctionArgumentType(functionType, i);
const functionArgumentTypeName = getTypeName(functionArgumentType);
if (expressionArgumentTypeName !== functionArgumentTypeName) {
context.report({ node, messageId: 'incorrectFunctionArgument' });
}
}
}function getFunctionArgumentType(functionType: ts.Type, i: number) {
// ???
}I tried iterating over |
|
I made more progress today. It turns out the trick to get function argument types was to use the Now I'm stuck on a new problem. I don't know how to use ESLint and/or the TypeScript API to get the constituent types of a union type. Until I can figure this out, the rule doesn't catch things like: const fruit: Fruit | null = 0; |
That's a bit catch-all, maybe...
😭 microsoft/TypeScript#9879 strikes yet again.
You could try looking at the declarations with
For the curious, that's a small wrapper around
if (type.isUnion()) {
type.types; // UnionOrIntersectionType.types: ts.Type[]
}Looks like you're making a lot of great progress. Really exciting! |
Currently, the rule does more than look at comparisons. It looks at:
You can look through the strict-enums.test.ts to see. |
I actually found out that you can just use triple equals to compare types directly. |
Yup, that works great, thanks. |
|
Ah, we have an existing |
|
Well, there's also an existing rule called |
|
|
|
For navigating unions you can use For comparing two types - you can do an equality check. If the type is the same type, then TS will return a referentially equal object. For enums because they are a "primitive" type (non object), TS will do the hard work of resolving past aliases and such for you. EG for the following code, the resolved types (via enum Foo { A }
declare const x: Foo.A;
type T = Foo.A;
declare const y: T;You can play around in https://ts-ast-viewer.com/ to validate this assumption. If we were dealing with object types - things would get much harder, but enums are simple! In a nutshell I believe TS will do the following - define a type for each of the enum members, and then define a type for the enum itself which is essentially just a special type which is equivalent to a union of all the enum member types. Re: naming - maybe enum Foo { A }
const x: Foo = 0; // should error
if (x === 0) {} // should error |
|
Also note that If this code was ever submitted as a pull request to TypeScript itself, it would almost certainly be named |
|
Ok, I did more work today, and all the tests are passing now. |
|
The lint rule uncovers some bugs in this monorepo. Should I fix all of the bugs as part of this PR or should that be done in a separate PR? |
|
Could you fix them in the PR please? If you don't have time a TODO comment would suffice. In the past we've found the typescript-eslint codebase to be surprisingly good at exposing some edge cases in PRs. |
|
From #6107: I don't have the time to work on this either, so if anybody else wants to pick it up, please do! |
|
If anyone wants it, I implemented the rule here as part of |
|
Would be nice if it could also handle switch statements... No errors are reported for instance with this code where the @bound private handleKeydown(keyboardEvent: KeyboardEvent): void {
let doStopPropagationAndPreventDefault = false;
assertIsDefined(this.controlledPopupMenu);
switch (keyboardEvent.key) {
case EventKeys.SPACE:
case EventKeys.ENTER:
case E
F438
ventKeys.DOWN:
this.controlledPopupMenu.open();
this.controlledPopupMenu.setFocusToFirstVisibleItem();
doStopPropagationAndPreventDefault = true;
break;
case EventKeys.UP:
this.controlledPopupMenu.open();
this.controlledPopupMenu.setFocusToLastVisibleItem();
doStopPropagationAndPreventDefault = true;
break;
default:
break;
}
if (doStopPropagationAndPreventDefault) {
keyboardEvent.stopPropagation();
keyboardEvent.preventDefault();
}
} |
|
@loicraux I've since stripped out the code in But yes, what you posted is a bug, let's track that in #7509. |
|
I think this should be closed now? |
|
@RedGuy12 It cannot be closed because the rule only checks for comparisons and doesn't check for assignment. e.g. enum Fruit {
Apple,
Banana,
}
let fruit = Fruit.Apple;
fruit = 1; // No error!If you want to prevent this unsafe pattern in your code, use the |
Do not allow number literals to be used instead of enums. I could think of three places that this can happen:
Rule name
I need help with naming it according to conventions
Options
I don't think this rule needs any options. I don't see why anyone wants to use number literals for enums.
The text was updated successfully, but these errors were encountered: