-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin): [switch-exhaustiveness-check] add considerDefaultExhaustiveForUnions option #9954
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): [switch-exhaustiveness-check] add considerDefaultExhaustiveForUnions option #9954
Conversation
Thanks for the PR, @developer-bandi! 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. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9954 +/- ##
=======================================
Coverage 86.19% 86.19%
=======================================
Files 430 430
Lines 15064 15071 +7
Branches 4371 4373 +2
=======================================
+ Hits 12984 12991 +7
Misses 1728 1728
Partials 352 352
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
🆒
packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.mdx
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.mdx
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/tests/rules/switch-exhaustiveness-check.test.ts
Outdated
Show resolved
Hide resolved
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.
The code logic looks great to me, but some cleanup is needed for the option naming and docs.
allowDefaultCaseMatchUnionMember
is ungrammatical, and requireDefaultCaseForUnions
is not accurate, to my understanding.
Maybe requireExplicitCasesForUnion
or considerDefaultExhaustiveForUnion
depending on which direction the true/false goes?
@@ -53,6 +53,29 @@ switch (value) { | |||
|
|||
Since `value` is a non-union type it requires the switch case to have a default clause only with `requireDefaultForNonUnion` enabled. | |||
|
|||
### `allowDefaultCaseMatchUnionMember` |
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.
option name in docs differs from code
@@ -155,10 +173,13 @@ export default createRule<Options, MessageIds>({ | |||
const { missingLiteralBranchTypes, symbolName, defaultCase } = | |||
switchMetadata; | |||
|
|||
// We only trigger the rule if a `default` case does not exist, since that | |||
// would disqualify the switch statement from having cases that exactly | |||
// We only trigger the rule if a `default` case does not exist when requireDefaultCaseForUnions option |
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.
(clarity nit)
Maybe just have a standalone condition like
// Unless requireDefaultCaseForUnions is enabled, the presence of a default case
// always makes the switch exhaustive.
if (!requireDefaultCaseForUnions && defaultCase != null) {
return;
}
@@ -243,6 +264,15 @@ export default createRule<Options, MessageIds>({ | |||
.join('\n'); | |||
|
|||
if (lastCase) { | |||
const isLastCaseDefaultCase = lastCase.test == null; | |||
if (isLastCaseDefaultCase) { |
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.
(optional)
food for thought - what if the default case isn't the last case?
This change makes it so that we avoid suggesting to fix
declare const literal: 'a' | 'b';
switch (literal) {
case 'b':
default:
}
to
declare const literal: 'a' | 'b';
switch (literal) {
case 'b':
default:
case "a": { throw new Error('Not implemented yet: "a" case') }
}
and instead fix it to
declare const literal: 'a' | 'b';
switch (literal) {
case 'b':
case "a": { throw new Error('Not implemented yet: "a" case') }
default:
}
. (also note that this has its own issue of no longer having case 'b'
be handled the same as default
, instead having it throw as not implemented yet).
But it will happily fix
declare const literal: 'a' | 'b';
switch (literal) {
default:
case 'b':
}
to
declare const literal: 'a' | 'b';
switch (literal) {
default:
case 'b':
case "a": { throw new Error('Not implemented yet: "a" case') }
}
which still has the added case as part of the default 🤔, even though the first examples implies this code is trying to avoid it. Maybe to be more consistent we could say "if default case is present, add it above it, else add it at the end"?
I don't think that this is super important since the user will have to do manual cleanup regardless, but just throwing this out there in case you want to tweak some edge cases. I'm ok with either way though.
@@ -65,6 +72,10 @@ export default createRule<Options, MessageIds>({ | |||
description: `If 'true', require a 'default' clause for switches on non-union types.`, | |||
type: 'boolean', | |||
}, | |||
requireDefaultCaseForUnions: { | |||
description: `If 'true', the 'default' clause is used to determine whether the switch statement is Exhaustive for union type`, |
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.
description: `If 'true', the 'default' clause is used to determine whether the switch statement is Exhaustive for union type`, | |
description: `If 'true', the 'default' clause is used to determine whether the switch statement is exhaustive for union type`, |
(Further thoughts on naming and options) - My understanding is that there are two independent things going on with union types and defaults.
Perhaps thinking about it in this framework can help clarify the docs and the naming. When I read |
As for the option name, the It seems that including both
|
packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.mdx
Outdated
Show resolved
Hide resolved
looks pretty close. Let's just change the option name to |
|
||
Defaults to false. Whether a `default` clause will be considered as making a switch statement over a union type exhaustive. | ||
|
||
By default, if a `switch` statement over a union type includes a `default` case, then it's considered exhaustive. |
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.
this is the opposite way (since the option defaults to false). By default, every branch will be required to be explicitly handled, and one may opt out by enabling considerDefaultExhaustiveForUnions
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.
thank you I think I missed changing it at the same time when changing the default value.
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.
packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.mdx
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.mdx
Outdated
Show resolved
Hide resolved
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.
Good stuff! I'm super excited to get this change released! This has been a pain point for me as a user of the rule, and I'm glad to see it addressed 🙂
FYI - I've made a few edits to your branch primarily in order to get the website to build (we added in the {/* insert option description */}
thing recently, which prevented the CI from building).
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 :)
packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.mdx
Outdated
Show resolved
Hide resolved
3e1a24e
##### [v8.12.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8120-2024-10-28) ##### 🚀 Features - **eslint-plugin:** \[no-base-to-string] handle String() ([#10005](typescript-eslint/typescript-eslint#10005)) - **eslint-plugin:** \[switch-exhaustiveness-check] add allowDefaultCaseMatchUnionMember option ([#9954](typescript-eslint/typescript-eslint#9954)) - **eslint-plugin:** \[consistent-indexed-object-style] report mapped types ([#10160](typescript-eslint/typescript-eslint#10160)) - **eslint-plugin:** \[prefer-nullish-coalescing] add support for assignment expressions ([#10152](typescript-eslint/typescript-eslint#10152)) ##### ❤️ Thank You - Abraham Guo - Kim Sang Du [@developer-bandi](https://github.com/developer-bandi) - Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger) - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.
| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.11.0 | 8.12.2 | | npm | @typescript-eslint/parser | 8.11.0 | 8.12.2 | ## [v8.12.2](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8122-2024-10-29) ##### 🩹 Fixes - **eslint-plugin:** \[switch-exhaustiveness-check] invert `considerDefaultExhaustiveForUnions` ([#10223](typescript-eslint/typescript-eslint#10223)) ##### ❤️ Thank You - Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website. ## [v8.12.1](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8121-2024-10-28) This was a version bump only for eslint-plugin to align it with other projects, there were no code changes. You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website. ## [v8.12.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8120-2024-10-28) ##### 🚀 Features - **eslint-plugin:** \[no-base-to-string] handle String() ([#10005](typescript-eslint/typescript-eslint#10005)) - **eslint-plugin:** \[switch-exhaustiveness-check] add allowDefaultCaseMatchUnionMember option ([#9954](typescript-eslint/typescript-eslint#9954)) - **eslint-plugin:** \[consistent-indexed-object-style] report mapped types ([#10160](typescript-eslint/typescript-eslint#10160)) - **eslint-plugin:** \[prefer-nullish-coalescing] add support for assignment expressions ([#10152](typescript-eslint/typescript-eslint#10152)) ##### ❤️ Thank You - Abraham Guo - Kim Sang Du [@developer-bandi](https://github.com/developer-bandi) - Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger) - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.
| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.11.0 | 8.12.2 | | npm | @typescript-eslint/parser | 8.11.0 | 8.12.2 | ## [v8.12.2](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8122-2024-10-29) ##### 🩹 Fixes - **eslint-plugin:** \[switch-exhaustiveness-check] invert `considerDefaultExhaustiveForUnions` ([#10223](typescript-eslint/typescript-eslint#10223)) ##### ❤️ Thank You - Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website. ## [v8.12.1](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8121-2024-10-28) This was a version bump only for eslint-plugin to align it with other projects, there were no code changes. You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website. ## [v8.12.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8120-2024-10-28) ##### 🚀 Features - **eslint-plugin:** \[no-base-to-string] handle String() ([#10005](typescript-eslint/typescript-eslint#10005)) - **eslint-plugin:** \[switch-exhaustiveness-check] add allowDefaultCaseMatchUnionMember option ([#9954](typescript-eslint/typescript-eslint#9954)) - **eslint-plugin:** \[consistent-indexed-object-style] report mapped types ([#10160](typescript-eslint/typescript-eslint#10160)) - **eslint-plugin:** \[prefer-nullish-coalescing] add support for assignment expressions ([#10152](typescript-eslint/typescript-eslint#10152)) ##### ❤️ Thank You - Abraham Guo - Kim Sang Du [@developer-bandi](https://github.com/developer-bandi) - Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger) - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.
| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.11.0 | 8.12.2 | | npm | @typescript-eslint/parser | 8.11.0 | 8.12.2 | ## [v8.12.2](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8122-2024-10-29) ##### 🩹 Fixes - **eslint-plugin:** \[switch-exhaustiveness-check] invert `considerDefaultExhaustiveForUnions` ([#10223](typescript-eslint/typescript-eslint#10223)) ##### ❤️ Thank You - Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website. ## [v8.12.1](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8121-2024-10-28) This was a version bump only for eslint-plugin to align it with other projects, there were no code changes. You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website. ## [v8.12.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8120-2024-10-28) ##### 🚀 Features - **eslint-plugin:** \[no-base-to-string] handle String() ([#10005](typescript-eslint/typescript-eslint#10005)) - **eslint-plugin:** \[switch-exhaustiveness-check] add allowDefaultCaseMatchUnionMember option ([#9954](typescript-eslint/typescript-eslint#9954)) - **eslint-plugin:** \[consistent-indexed-object-style] report mapped types ([#10160](typescript-eslint/typescript-eslint#10160)) - **eslint-plugin:** \[prefer-nullish-coalescing] add support for assignment expressions ([#10152](typescript-eslint/typescript-eslint#10152)) ##### ❤️ Thank You - Abraham Guo - Kim Sang Du [@developer-bandi](https://github.com/developer-bandi) - Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger) - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.
PR Checklist
Overview