-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin): add switch-exhaustiveness-check rule #972
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
Conversation
Thanks for the PR, @drets! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
@bradzacher hi. Whom I can ping to get a code review? Have a wonderful day. |
All you need to do is raise the PR. |
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.
Code so far LGTM.
A few things though:
How does this code handle enum
values? Could you please add some tests to validate that they work as expected.
Similarly, could you please add tests for other primitives, ie:
number
:0 | 1 | 2
boolean
:true | false
(yeah this is weird, but might as well it works as expected- mixture of all
0 | 1 | 'two' | 'three' | true
what about symbol
s?
How does this code handle type references?
type A = 'a';
type B = 'b';
type C = 'c';
type Union = A | B | C;
I don't know how checker.typeToString
prints out these cases.
could you please add some tests for this so it's clear and handled properly?
How does this handle typeof
types:
const A = 'a';
const B = 1;
const C = true;
type Union = typeof A | typeof B | typeof C;
How does this implementation handle object type unions: type T = { a: 1 } | { b: 2 }
?
I know it's a weird edge-case, but should the allowed union types be limited in some way so it cannot report in this case?
One question I'd like to discuss: is it worth having the fixer?
I'm wondering how much value there is in inserting case Foo: throw new Exception()
, esp when there's the implementation weirdness with intersection types, and having to deal with indentation (plus any other weirdness that comes from the above cases).
packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts
Outdated
Show resolved
Hide resolved
@drets I'd like to help get this going again: I've opened a PR into your fork with requested changes, let me know if there's anything else I can help with. @bradzacher I think I can also answer your questions.
Added tests for these as well, no surprises there.
This implementation only supports unions/enums of singleton types. As far as I know, TS currently doesn't have literal
we'll return an error:
Only way to satisfy the rule would be to add a
This code handles type references correctly, in a sense that non-exhaustive
As long as values behind
This implementation doesn't handle this case at all and I'm not sure it's applicable: I can't think of a way to
However, if type is a discriminated union, this reduces the problem to union of singleton types (which is handled correctly).
I would agree that a fixer for this would be potentially more trouble than it's worth. For example, if programmer has non-singleton types in the union we construct invalid JS:
which is just asking for trouble. |
@cust0dian wow, thank you for jumping in! I will merge your PR in the next few days. Personally, the fixer is a killer feature of this rule. I'd like to declare union type with N branches, then write |
You're partially correct. Because each symbol is 100% unique, you can't just create a type like You can also key an interface with a symbol variable, and typescript will understand it const x = Symbol('a');
type T = typeof x | true;
declare const y: T;
switch (y) {
case true:
y; // typeof y === true
break;
case x:
y; // typeof y === typeof x
break;
}
if (y === x) {
y; // typeof y === typeof x
}
interface Foo {
[x]: 1;
}
declare const foo: Foo;
const bar = foo[x]; // typeof bar === 1 Important to note that the type of the variable In regards to the fixer, it might be worth adding a suggestion fixer. https://eslint.org/docs/developer-guide/working-with-rules#providing-suggestions They aren't automatically applied by |
instead of autofixing This reverts commit e4a9538.
@bradzacher I see, thank you for explaining it! The code does handle types like that, albeit reporting isn't ideal:
Would it be appropriate to use
in the case of symbols? |
Probably, I'd say that it'd be "good enough" to check the type's flags for the |
Requested changes for switch-exhaustiveness-check
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.
Mostly LGTM, great work.
A few comments to address, mostly tightening tests.
Thanks for doing this.
packages/eslint-plugin/tests/rules/switch-exhaustiveness-check.test.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts
Outdated
Show resolved
Hide resolved
of no cases in switch statement and add tests for suggestions
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.
LGTM - thanks all for working on this!
Hi 👋 ,
I was talking about this thing in microsoft/TypeScript#33160 and Nathan Shively-Sanders suggested to use
typescript-eslint
for such things, and so I did.By introducing this rule people may stop to write small hacks suggested in https://stackoverflow.com/questions/39419170/how-do-i-check-that-a-switch-block-is-exhaustive-in-typescript and use this rule instead.
Let me know if this rule makes sense, thank you.