Optimize narrowTypeByEquality and narrowTypeBySwitchOnDiscriminant - #4781
Open
ahejlsberg wants to merge 6 commits into
Open
Optimize narrowTypeByEquality and narrowTypeBySwitchOnDiscriminant#4781ahejlsberg wants to merge 6 commits into
narrowTypeByEquality and narrowTypeBySwitchOnDiscriminant#4781ahejlsberg wants to merge 6 commits into
Conversation
Member
Author
|
@typescript-bot test it |
Contributor
There was a problem hiding this comment.
Pull request overview
Optimizes primitive-union narrowing during control-flow analysis.
Changes:
- Adds fast paths for equality and switch narrowing.
- Optimizes type removal and centralizes union membership checks.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
internal/checker/flow.go |
Adds narrowing fast paths. |
internal/checker/checker.go |
Optimizes union helpers and type removal. |
Comment on lines
+601
to
+603
| filteredType := c.removeType(t, c.getRegularTypeOfLiteralType(valueType)) | ||
| if filteredType != t { | ||
| return filteredType |
Comment on lines
+589
to
+594
| if !doubleEquals && t.objectFlags&ObjectFlagsPrimitiveUnion != 0 && valueType.flags&TypeFlagsPrimitive != 0 { | ||
| regularType := c.getRegularTypeOfLiteralType(valueType) | ||
| if c.unionContainsType(t, regularType) { | ||
| return regularType | ||
| } | ||
| } |
Comment on lines
+1123
to
+1128
| if t.objectFlags&ObjectFlagsPrimitiveUnion != 0 && discriminantType.flags&TypeFlagsPrimitive != 0 { | ||
| regularType := c.getRegularTypeOfLiteralType(discriminantType) | ||
| if c.unionContainsType(t, regularType) { | ||
| caseType = regularType | ||
| } | ||
| } |
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Member
Author
|
@typescript-bot perf test this faster |
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Member
Author
|
@typescript-bot perf test this faster |
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Member
Author
|
@typescript-bot perf test this faster |
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
The fast paths introduce incorrect narrowing for computed enums and non-primitive union constituents.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (1)
internal/checker/flow.go:1129
- This shortcut never applies to grouped case labels. Their
discriminantTypeis a union, whose flags do not includeTypeFlagsPrimitive, andunionContainsTypewould in any event look for that whole union as one direct constituent. Large switches with many empty fallthrough labels therefore still use the O(union constituents × grouped cases) relational fallback. Check membership for each regular discriminant constituent instead, and cover a grouped-case switch in the performance regression.
if t.objectFlags&ObjectFlagsPrimitiveUnion != 0 && discriminantType.flags&TypeFlagsPrimitive != 0 && c.isUniformUnionType(t) {
regularType := c.getRegularTypeOfLiteralType(discriminantType)
if c.unionContainsType(t, regularType, false /*matchSymbol*/) {
caseType = regularType
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Medium
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| return c.replacePrimitivesWithLiterals(filteredType, valueType) | ||
| } | ||
| if isUnitType(valueType) { | ||
| if c.isUniformUnionType(t) { |
| var enumSymbol *ast.Symbol | ||
| var hasStringOrNumberLiteral bool | ||
| for _, t := range types { | ||
| if t.flags&TypeFlagsEnumLiteral != 0 { |
Member
Author
There was a problem hiding this comment.
We all hate that rule, but yeah.
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
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.
Inspired by the performance gains in #4711, this PR further optimizes union type narrowing in control flow analysis.