-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Revise discriminateTypeByDiscriminableItems
function
#53709
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@MariaSolOs Feel free to grab the changes in this PR and move them to yours. Alternatively, we can add the tests to this PR. |
@MariaSolOs Note that the new approach made it possible to eliminate overloads and several parameters from |
discriminateTypeByDiscriminableItems
function
I think it's easier to just move the tests over. I can do that. |
tests/baselines/reference/assignmentCompatWithDiscriminatedUnion.errors.txt
Show resolved
Hide resolved
tests/baselines/reference/excessPropertyCheckWithMultipleDiscriminants.errors.txt
Show resolved
Hide resolved
@typescript-bot test this |
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 65de9c2. You can monitor the build here. |
Heya @ahejlsberg, I've started to run the diff-based top-repos suite (tsserver) on this PR at 65de9c2. 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 65de9c2. You can monitor the build here. Update: The results are in! |
Heya @ahejlsberg, I've started to run the diff-based user code test suite (tsserver) on this PR at 65de9c2. You can monitor the build here. Update: The results are in! |
Heya @ahejlsberg, I've started to run the perf test suite on this PR at 65de9c2. You can monitor the build here. Update: The results are in! |
Heya @ahejlsberg, I've started to run the diff-based top-repos suite on this PR at 65de9c2. You can monitor the build here. Update: The results are in! |
Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at 65de9c2. You can monitor the build here. Update: The results are in! |
@ahejlsberg Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
@ahejlsberg Here are the results of running the user test suite comparing Everything looks good! |
Heya @ahejlsberg, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
@ahejlsberg Here they are:
CompilerComparison Report - main..53709
System
Hosts
Scenarios
TSServerComparison Report - main..53709
System
Hosts
Scenarios
StartupComparison Report - main..53709
System
Hosts
Scenarios
Developer Information: |
@ahejlsberg Here are the results of running the top-repos suite comparing Everything looks good! |
Hey @ahejlsberg, the results of running the DT tests are ready. Branch only errors:Package: pdfmake
Package: gensync
|
@ahejlsberg I think that the description should link #53165, not the previous PR. |
@ahejlsberg Here are the results of running the top-repos suite comparing Everything looks good! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
One direction I was thinking about this before was to have include
be a list of Ternary
s while tracking whether true matches and a non-match occurred.
With your scheme, you could start out with Ternary.True
and move to Ternary.Maybe
. If a match occurs, you set all of those Maybe
s to Ternary.False
. This would never allocate an exclude
array, but you could still keep track of whether any Maybe
s were ever set to avoid walking the list of types.
I still think the change is good as-is, though I might take a swing at that in another pass.
// If the remaining target types include at least one with a matching discriminant, eliminate those that | ||
// have non-matching discriminants. This ensures that we ignore erroneous discriminators and gradually | ||
// refine the target set without eliminating every constituent (which would lead to `never`). | ||
let exclude: number[] | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might call this excludeIndexes
Hm, check time does seem to go higher on a few repos. Let's try this again. @typescript-bot perf test this |
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 65de9c2. You can monitor the build here. Update: The results are in! |
@DanielRosenwasser Here they are:
CompilerComparison Report - main..53709
System
Hosts
Scenarios
TSServerComparison Report - main..53709
System
Hosts
Scenarios
StartupComparison Report - main..53709
System
Hosts
Scenarios
Developer Information: |
@DanielRosenwasser Once again, just run the perf tests until you get the desired outcome! |
Latest commit implements @DanielRosenwasser's suggestion of using |
There are a few baseline changes in DT and RWC, all related to picking better matching candidates for error reporting. Otherwise, tests and performance are unchanged, so I think this one is ready to merge. |
Implements what I suggest here.
Fixes #53165.