Control flow analysis for destructured rests#62350
Open
Andarist wants to merge 7 commits intomicrosoft:mainfrom
Open
Control flow analysis for destructured rests#62350Andarist wants to merge 7 commits intomicrosoft:mainfrom
Andarist wants to merge 7 commits intomicrosoft:mainfrom
Conversation
typescript-bot
added
the
For Uncommitted Bug
Collaborator
|
The TypeScript team hasn't accepted the linked issue #46680. If you can get it accepted, this PR will have a better chance of being reviewed. |
1 similar comment
Collaborator
|
The TypeScript team hasn't accepted the linked issue #46680. If you can get it accepted, this PR will have a better chance of being reviewed. |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR implements control flow analysis for destructured rest variables, addressing two specific TypeScript issues (#46680 and #53947). The changes enable the TypeScript compiler to properly narrow types in destructuring assignments that include rest elements, improving type inference when discriminated unions are involved.
Key Changes
- Enhanced control flow analysis to handle destructured rest variables by tracking narrowed types through rest patterns
- Modified the constraint substitution logic to be more selective, improving type narrowing for binding elements
- Extended support for both object and array destructuring with rest elements
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/cases/conformance/controlFlow/dependentDestructuredRestVariables1.tsx | New test cases demonstrating control flow analysis for destructured rest variables in various scenarios |
| tests/baselines/reference/dependentDestructuredRestVariables1.symbols | Generated symbol baseline for the new test cases |
| tests/baselines/reference/arrayDestructuringInSwitch1.types | Updated type baseline showing improved type inference for array destructuring |
| tests/baselines/reference/arrayDestructuringInSwitch1.symbols | Updated symbol baseline with refined symbol references |
| src/compiler/checker.ts | Core implementation changes to enable control flow analysis for rest variables |
Comments suppressed due to low confidence (1)
src/compiler/checker.ts:1
- [nitpick] The long conditional expression on line 30851 reduces readability. Consider extracting the check mode determination into a variable:
const checkMode = shouldSubstituteConstraints(type, location) ? CheckMode.Normal : CheckMode.SkipConstraintsSubstitution;
import {
Andarist
commented
Aug 28, 2025
src/compiler/checker.ts
Outdated
| links.flags |= NodeCheckFlags.InCheckIdentifier; | ||
| const parentType = getTypeForBindingElementParent(parent, CheckMode.Normal); | ||
| const parentTypeConstraint = parentType && mapType(parentType, getBaseConstraintOrType); | ||
| const parentType = getTypeForBindingElementParent(parent, shouldSubstituteConstraints(type, location) ? CheckMode.Normal : CheckMode.SkipConstraintsSubstitution); |
Contributor
Author
There was a problem hiding this comment.
When obtaining the parent type we don't want to get its constraints substituted based on its contextual type or it being in a constraint position or something. This would lead to returning its constraint and then using that for getBindingElementTypeFromParentType even when the current location isn't at the constraint position.
Contributor
Author
|
Huh, I'll have to investigate what's happening with the self-check |
|
wowwww ❤️ |
Andarist
commented
Aug 30, 2025
Comment on lines
3
to
7
| === Performance Stats === | ||
| Assignability cache: 2,500 | ||
| Type Count: 10,000 | ||
| Instantiation count: 100,000 | ||
| Symbol count: 50,000 |
Contributor
Author
There was a problem hiding this comment.
fwiw, those numbers are the same on main
Member
|
@typescript-bot test it |
Collaborator
Collaborator
|
Hey @jakebailey, 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 |
Collaborator
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
Collaborator
|
@jakebailey Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
Collaborator
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Collaborator
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Member
|
I feel fairly good about this PR, though I'm a bit shocked it didn't net anything in the extended tests. |
Member
|
@ahejlsberg Curious what you think of this. |
Andarist
force-pushed
the
cfa/destructured-rest
branch
from
06d27e3 to
77a6fb0
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 #46680
fixes #53947