Don't error if binding pattern type is never in control flow analysis for destructured discriminated union #47927
Conversation
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
typescript-bot
added
Author: Team
For Milestone Bug
RyanCavanaugh
approved these changes
Feb 17, 2022
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at c4aa31b. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at c4aa31b. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at c4aa31b. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at c4aa31b. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at c4aa31b. You can monitor the build here. |
|
@typescript-bot cherry-pick this to release-4.6 and LKG |
|
Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into |
|
Hey @DanielRosenwasser, I've opened #48005 for you. |
typescript-bot
pushed a commit
to typescript-bot/TypeScript
that referenced
this pull request
Feb 22, 2022
Component commits: c4aa31b early return if pattern type is never
|
Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running |
|
@DanielRosenwasser Here they are:Comparison Report - main..47927
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
DanielRosenwasser
approved these changes
Feb 23, 2022
There was a problem hiding this comment.
I think the scary thing out of this is that there may be other fragile cases like this where narrowing can create a less-capable type - but perhaps you can't get to them via property narrowing. I think fixing the never case is probably worth it for now until we can see if anything else triggers something like this.
DanielRosenwasser
pushed a commit
that referenced
this pull request
Feb 23, 2022
…e-4.6 (#48005) * Cherry-pick PR #47927 into release-4.6 Component commits: c4aa31b early return if pattern type is never * Update LKG Co-authored-by: Gabriela Araujo Britto <[email protected]> Co-authored-by: typescript-bot <[email protected]>
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.
Fixes #47857.
Error explanation
The checker code introduced by #46266, more precisely
getNarrowedTypeOfSymbol, is the one that causes the error.In the bug example:
when we inside the default branch and are getting the narrowed type for
operator's symbol, we narrowexpression's type tonever(becauseoperatoris a discriminant and it has typenever). And then we try to get the type ofoperatorknowing thatexpressionhas typenever, by callinggetBindingElementTypeFromParentType. Eventually, insidegetBindingElementTypeFromParentType, we issue the error, because we can't figure out array/tuple element type for something of typenever.Fix
This PR fixes that by avoiding calling
getBindingElementTypeFromParentTypewhen the narrowed parent type isnever.I think the intuition is that this prevents us from issuing errors in destructuring expressions that are "outside" the part of the code where things are narrowed to
never, but I might be wrong.There's a test case (
arrayDestructuringInSwitch2.ts) that shows this change doesn't prevent us from issuingneverdestructuring errors inside the branch where something has typenever.My only concern would be the possibility that we are missing out on erroring in some programs, but this whole narrowing code was added for 4.6 and didn't exist before anyways.