Eliminate redundant or meaningless elaborations in type relations #47738
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
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at a009ee1. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at a009ee1. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the diff-based community code test suite on this PR at a009ee1. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at a009ee1. You can monitor the build here. |
|
@ahejlsberg |
|
@ahejlsberg Here they are:Comparison Report - main..47738
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at f269f42. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at f269f42. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at f269f42. You can monitor the build here. |
|
@ahejlsberg Here they are:Comparison Report - main..47738
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test this faster |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at 9404e06. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at 9404e06. You can monitor the build here. Update: The results are in! |
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the diff-based community code test suite on this PR at 9404e06. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 9404e06. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 9404e06. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at 25a71c4. You can monitor the build here. Update: The results are in! |
|
@ahejlsberg Here they are:Comparison Report - main..47738
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I haven't looked over the checker code thoroughly, but all the baseline diffs are either positive or equivalently OK IMO |
DanielRosenwasser
requested changes
Feb 9, 2022
| }; | ||
| } | ||
|
|
||
| function reportIncompatibleError(message: DiagnosticMessage, arg0?: string | number, arg1?: string | number, arg2?: string | number, arg3?: string | number) { | ||
| overrideNextErrorInfo++; // Suppress the next relation error | ||
| lastSkippedInfo = undefined; // Reset skipped info cache | ||
| incompatibleStack.push([message, arg0, arg1, arg2, arg3]); | ||
| (incompatibleStack || (incompatibleStack = [])).push([message, arg0, arg1, arg2, arg3]); |
There was a problem hiding this comment.
Suggested change
| (incompatibleStack || (incompatibleStack = [])).push([message, arg0, arg1, arg2, arg3]); | |
| (incompatibleStack ||= []).push([message, arg0, arg1, arg2, arg3]); |
src/compiler/checker.ts
Outdated
| @@ -17998,21 +17997,21 @@ namespace ts { | |||
| return { | |||
| errorInfo, | |||
| lastSkippedInfo, | |||
| incompatibleStack: incompatibleStack.slice(), | |||
| incompatibleStack: incompatibleStack ? incompatibleStack.slice() : undefined, | |||
There was a problem hiding this comment.
Suggested change
| incompatibleStack: incompatibleStack ? incompatibleStack.slice() : undefined, | |
| incompatibleStack: incompatibleStack?.slice(), |
or
Suggested change
| incompatibleStack: incompatibleStack ? incompatibleStack.slice() : undefined, | |
| incompatibleStack: incompatibleStack && incompatibleStack.slice(), |
src/compiler/checker.ts
Outdated
| overrideNextErrorInfo, | ||
| relatedInfo: !relatedInfo ? undefined : relatedInfo.slice() as ([DiagnosticRelatedInformation, ...DiagnosticRelatedInformation[]] | undefined) | ||
| relatedInfo: relatedInfo ? relatedInfo.slice() as [DiagnosticRelatedInformation, ...DiagnosticRelatedInformation[]] : undefined, |
There was a problem hiding this comment.
Suggested change
| relatedInfo: relatedInfo ? relatedInfo.slice() as [DiagnosticRelatedInformation, ...DiagnosticRelatedInformation[]] : undefined, | |
| relatedInfo: relatedInfo?.slice() as [DiagnosticRelatedInformation, ...DiagnosticRelatedInformation[]] | undefined, | |
src/compiler/checker.ts
Outdated
Comment on lines
18584
to
18585
| // useful and leads to some confusing error messages. Instead it is better to let the below checks | ||
| // take care of this, or to not elaborate at all. For instance, |
There was a problem hiding this comment.
Instead it is better to let the below checks take care of this, or to not elaborate at all.
Those below checks no longer exist :D
Instead, I guess you have to comment that you're expecting callers to handle elaboration on intersection types.
src/compiler/checker.ts
Outdated
Comment on lines
18575
to
18576
| source = getIntersectionType(constraints); | ||
| if (!(source.flags & TypeFlags.Intersection)) { |
There was a problem hiding this comment.
Suggested change
| source = getIntersectionType(constraints); | |
| if (!(source.flags & TypeFlags.Intersection)) { | |
| // At this point we're only interested in constraints of the intersection, | |
| // either here or in further checks on the apparent type below. | |
| source = getIntersectionType(constraints); | |
| if (!(source.flags & TypeFlags.Intersection)) { |
There was a problem hiding this comment.
Actually, this is just handling the case where the intersection gets reduced down to a singleton type.
There was a problem hiding this comment.
This is the part that's handling the cases where the intersection of constraints could be a singleton (or technically a primitive or union), but you're changing source for the intersection case below as well, right?
There was a problem hiding this comment.
Yes, but the comment above the whole section already says that we're hoisting the constraints into a new intersection (with the implication that it replaces the old source). But perhaps that isn't clear enough.
src/compiler/checker.ts
Outdated
Comment on lines
18295
to
18296
| // See if we're relating a definitely non-nullable type to a union that includes null and/or undefined | ||
| // plus a single non-nullable type. If so, remove null and/or undefined from the target type. |
There was a problem hiding this comment.
I actually slightly prefer the old comment, but I'm not opposed to this.
Can you elaborate on that? I saw that you did hoist a bunch of |
In general, logic that sits in
We could further consider moving the excess and common property checking code to the cached side, though it isn't run all that often and would require some restructuring in |
This reverts commit 25a71c4.
|
@typescript-bot perf test this |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 1d70b01. You can monitor the build here. Update: The results are in! |
|
@DanielRosenwasser Here they are:Comparison Report - main..47738
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
DanielRosenwasser
approved these changes
Feb 10, 2022
This comment was marked as off-topic.
This comment was marked as off-topic.
# Conflicts: # src/compiler/diagnosticMessages.json
RyanCavanaugh
approved these changes
Feb 15, 2022
|
@typescript-bot cherry-pick this to release-4.6 |
|
Heya @RyanCavanaugh, I've started to run the task to cherry-pick this into |
|
Hey @RyanCavanaugh, I've opened #47907 for you. |
typescript-bot
pushed a commit
to typescript-bot/TypeScript
that referenced
this pull request
Feb 15, 2022
Component commits: 628da10 Eliminate redundant or meaningless elaborations in type relations a009ee1 Accept new baselines a65794c Add resetErrorInfo (though, oddly, shouldn't be necessary) f269f42 Less aggressive reduction in second pass union/intersection checks 1312e8a Accept new baselines b82966f Restructure and back off a little bit more f0b8742 Only cache union/intersection relations once 9404e06 Accept new baselines 4866ce5 Properly cache identity relations, clean up error reporting 2670b26 Move more logic to cached side of relation checks adb37a5 Optimize and remove more redundant elaborations 16b986b Accept new baselines 1c69acb Remove unnecessary error state capture 530c876 More optimizing 25a71c4 Cache isWeakType computation c427a46 Revert "Cache isWeakType computation" This reverts commit 25a71c4. 1d70b01 Address CR feedback 28439f7 Merge branch 'main' into fix47668 # Conflicts: # src/compiler/diagnosticMessages.json c472ba5 Accept new baselines
DanielRosenwasser
pushed a commit
that referenced
this pull request
Feb 18, 2022
Component commits: 628da10 Eliminate redundant or meaningless elaborations in type relations a009ee1 Accept new baselines a65794c Add resetErrorInfo (though, oddly, shouldn't be necessary) f269f42 Less aggressive reduction in second pass union/intersection checks 1312e8a Accept new baselines b82966f Restructure and back off a little bit more f0b8742 Only cache union/intersection relations once 9404e06 Accept new baselines 4866ce5 Properly cache identity relations, clean up error reporting 2670b26 Move more logic to cached side of relation checks adb37a5 Optimize and remove more redundant elaborations 16b986b Accept new baselines 1c69acb Remove unnecessary error state capture 530c876 More optimizing 25a71c4 Cache isWeakType computation c427a46 Revert "Cache isWeakType computation" This reverts commit 25a71c4. 1d70b01 Address CR feedback 28439f7 Merge branch 'main' into fix47668 # Conflicts: # src/compiler/diagnosticMessages.json c472ba5 Accept new baselines Co-authored-by: Anders Hejlsberg <andersh@microsoft.com>
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 PR removes a several pointless error elaborations, making diagnostics more concise. For example, when there is no relation between some source type and a target union type, we now only elaborate when we can identify a best match on the target side. Previously we'd default to elaborating on the last target constituent, which generally just adds noise and depends on a unstable sort order.
The PR also optimizies the
isRelatedTofunction by moving more work to the cached side on relationship checking. This improves checker performance by up to 3% and fixes an issue related to infinite constraints involving intersections.Fixes #46900.
Fixes #47668.