Fix missed errors in switch when using union of literal and non-literal types (#38686) #51373
Conversation
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
|
@microsoft-github-policy-service agree |
|
@typescript-bot test this |
|
Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at 83bf937. You can monitor the build here. |
|
Heya @RyanCavanaugh, I've started to run the diff-based user code test suite on this PR at 83bf937. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the diff-based top-repos suite on this PR at 83bf937. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the perf test suite on this PR at 83bf937. You can monitor the build here. Update: The results are in! |
|
@RyanCavanaugh Here are the results of running the user test suite comparing Everything looks good! |
|
Heya @RyanCavanaugh, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@RyanCavanaugh Here they are:
CompilerComparison Report - main..51373
System
Hosts
Scenarios
TSServerComparison Report - main..51373
System
Hosts
Scenarios
StartupComparison Report - main..51373
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@RyanCavanaugh Here are the results of running the top-repos suite comparing Everything looks good! |
|
@typescript-bot perf test this faster |
|
Heya @RyanCavanaugh, I've started to run the abridged perf test suite on this PR at 83bf937. You can monitor the build here. Update: The results are in! |
|
@RyanCavanaugh Here they are:Comparison Report - main..51373
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
andrewbranch
approved these changes
Nov 15, 2022
|
Should I resolve the conflicts? Or someone else will handle that? |
|
You’ll need to resolve them. We migrated the codebase to ES modules so the indentation of every line has change. We’ve found that git is very bad at figuring out where the meaningful changes are in these conflicts, but VS Code’s “Resolve in Merge Editor” functionality usually gets it right. |
|
@andrewbranch cool, I already did that locally, but it looks like some tests are failing on the latest main branch. |
…al types (microsoft#38686) This commit makes it so we don’t use the base type of literals when checking comparability in switch. The comparability checks handle that case already, is my understanding, so we don’t need to clobber the type before actually doing the check, causing missed errors. When comparing the types in switch, if a union with a literal and a non-literal was used, the compiler in `checker.ts` would automatically get the base type of all parts of the union, resulting in missed errors. For example, if the union of the non-literal `number` and literal `"hello"` was compared to the literal `"world"` in a switch case, the compiler would miss that they’re actually not comparable. Maybe someone can tell me why we were getting the base type before checking comparability, rather than relying on the logic within the comparability checks to handle literal/base type comparability?
|
I rebased this branch onto the latest |
|
Looks like worked fine—cool beans. |
|
@sandersn you wrote the code this is deleting (albeit 6 years ago) if you want to add a review |
gabritto
approved these changes
Nov 18, 2022
|
I think this is good, but before merging I'd like to hear from @sandersn when he gets back to see if there was any particular reason why we were using the base types for this comparison check. |
|
Is @sandersn around yet? |
|
@typescript-bot pack this |
|
Hey @gabritto, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
I talked to @sandersn offline about the old implementation. The use of base types for the case and expression types was implemented in #11633 as a fix to a bug (#10892). As seen in this bug, the way we checked type comparability involving literal types was inadequate in some cases, and thus motivated the original PR that used the base types instead. But we have made several improvements to type (equality) comparability in the meantime, such that using the literal types for case and expression types and checking for equality comparability in the original code of the bug #10892 doesn't lead to an error anymore. It seems that we simply didn't update the type comparability implementation for switch statements, leading to the problem this PR fixed. Ideally, in the future we'd like type comparability in switch statements to be the same as type comparability for equality (e.g. |
|
Cool beans—thanks for the follow up @gabritto! Maybe I can look into doing that later 😊 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This commit makes it so we don’t use the base type of literals when checking comparability in switch. The comparability checks handle that case already, is my understanding, so we don’t need to clobber the type before actually doing the check, causing missed errors.
When comparing the types in switch, if a union with a literal and a non-literal was used, the compiler in
checker.tswould automatically get the base type of all parts of the union, resulting in missed errors. For example, if the union of the non-literalnumberand literal"hello"was compared to the literal"world"in a switch case, the compiler would miss that they’re actually not comparable.Maybe someone can tell me why we were getting the base type before checking comparability, rather than relying on the logic within the comparability checks to handle literal/base type comparability?
Fixes #38686