Repository navigation
Consider all union types matching discriminator for excess property checks - #51884
Conversation
|
The TypeScript team hasn't accepted the linked issue #51873. If you can get it accepted, this PR will have a better chance of being reviewed. |
|
@microsoft-github-policy-service agree |
|
TypeScript Bot (@typescript-bot) test this |
|
Heya Jake Bailey (@jakebailey), I've started to run the diff-based top-repos suite (tsserver) on this PR at ca945a2. You can monitor the build here. Update: The results are in! |
|
Heya Jake Bailey (@jakebailey), I've started to run the perf test suite on this PR at ca945a2. You can monitor the build here. Update: The results are in! |
|
Heya Jake Bailey (@jakebailey), I've started to run the extended test suite on this PR at ca945a2. You can monitor the build here. |
|
Heya Jake Bailey (@jakebailey), I've started to run the diff-based user code test suite on this PR at ca945a2. You can monitor the build here. Update: The results are in! |
|
Heya Jake Bailey (@jakebailey), I've started to run the diff-based top-repos suite on this PR at ca945a2. You can monitor the build here. Update: The results are in! |
|
Heya Jake Bailey (@jakebailey), I've started to run the diff-based user code test suite (tsserver) on this PR at ca945a2. You can monitor the build here. Update: The results are in! |
|
Heya Jake Bailey (@jakebailey), I've started to run the tarball bundle task on this PR at ca945a2. You can monitor the build here. |
|
Heya Jake Bailey (@jakebailey), I've started to run the parallelized Definitely Typed test suite on this PR at ca945a2. You can monitor the build here. |
|
Hey Jake Bailey (@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 |
|
Jake Bailey (@jakebailey) Here are the results of running the user test suite comparing Everything looks good! |
1 similar comment
|
Jake Bailey (@jakebailey) Here are the results of running the user test suite comparing Everything looks good! |
|
Jake Bailey (@jakebailey) Here they are:
CompilerComparison Report - main..51884
System
Hosts
Scenarios
TSServerComparison Report - main..51884
System
Hosts
Scenarios
StartupComparison Report - main..51884
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Jake Bailey (@jakebailey) Here are the results of running the top-repos suite comparing Everything looks good! |
|
Jake Bailey (@jakebailey) Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Detailsfelixrieseberg/windows95
|
Wesley Wigham (weswigham)
left a comment
There was a problem hiding this comment.
I think this approach is pretty reasonable, and, moreover, removing the isTypeIdenticalTo check is very performance positive. I need to run DT on this, but we should take it provided there's no unforseen breaks.
|
Heya Wesley Wigham (@weswigham), I've started to run the parallelized Definitely Typed test suite on this PR at ca945a2. You can monitor the build here. Update: The results are in! Update: The results are in! |
|
TypeScript Bot (@typescript-bot) perf test this |
|
Heya Wesley Wigham (@weswigham), I've started to run the perf test suite on this PR at ca945a2. You can monitor the build here. Update: The results are in! |
|
Wesley Wigham (@weswigham) Here they are:
CompilerComparison Report - main..51884
System
Hosts
Scenarios
TSServerComparison Report - main..51884
System
Hosts
Scenarios
StartupComparison Report - main..51884
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey Wesley Wigham (@weswigham), it looks like the DT test run failed. Please check the log for more details. |
|
I hit "rerun" on the above; want to see if that works to fix the problem temporarily rather than rerunning from scratch. |
|
Hey Wesley Wigham (@weswigham), it looks like the DT test run failed. Please check the log for more details. |
|
Heya Jake Bailey (@jakebailey), I've started to run the parallelized Definitely Typed test suite on this PR at ca945a2. You can monitor the build here. Update: The results are in! |
|
Hey Jake Bailey (@jakebailey), the results of running the DT tests are ready. Branch only errors:Package: waterline |
|
That DT error looks correct, but, it's a little weird: Playground Link Comment out |
Jake Bailey (jakebailey)
left a comment
There was a problem hiding this comment.
LGTM but maybe we should add the above as a test case?
|
Jake Bailey (@jakebailey) do we just want to make a followup PR that adds the test case? |
|
Sure, if the author isn't available to do it. I can send it or you can if you have the time. DT has to be fixed too. |
|
We can also push to this PR too. |
|
I just pushed the test onto the PR, so we should be good to merge this. I'll send the PR to DT. |
|
Solid, I just figured since it was 4 months since the author opened the PR, and we didn't see any engagement while we were probing the PR, we maybe shouldn't expect prompt implementation of a nice-to-have 😆 |
|
Well, good thing we didn't merge, baseline differs from main. |
Discriminating unions provide a means for type narrowing based on a discriminator property. The first property common to all union constituents is taken as the discriminator property.
If object literals provide a discriminator value, their expected shape gets narrowed to the corresponding union constituent. Excess properties from other constituents (or completely unrelated properties) raise an excess property check (EPC) error.
Previously this only worked if there was a single matching candidate constituent for the object literal being tested. If two or more types within the union matched the provided discriminator value, EPC accepted properties from the entire union - even though only a subset actually matched.
This PR improves on this behavior to correctly consider only the matching subset.
Fixes #51873