-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Fix for relating covered discriminants in unions #39393
Conversation
@typescript-bot perf test |
Heya @rbuckton, I've started to run the perf test suite on this PR at 8eba362. You can monitor the build here. Update: The results are in! |
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
@rbuckton Here they are:Comparison Report - master..39393
System
Hosts
Scenarios
|
I noticed some failures in the DT test run. I'm trying to determine if these are existing errors or are due to this change. |
It looks like the DT failures are issues in DT:
|
In #30779 we added the ability to relate a source type to a target when the target is a discriminated union and the source is covered by the target. This did not work when the target was a union of tuple types as the relationship check would fail when comparing the synthesized index types for each tuple in
indexTypesRelatedTo
. This was further complicated by the fact that the new variadic tuple type logic added topropertiesRelatedTo
fails to exclude relationship checks for non-variable elements whose indices are included in theexcludedProperties
map.This PR does two things to address these issues:
propertiesRelatedTo
so that it aligns with the existing, non-tuple-specific logic.indexTypesRelatedTo
check when both the source type and the target type are tuples, as the index type check should be sufficiently covered by the tuple-specific branch inpropertiesRelatedTo
.Alternatively, rather than skipping the
indexTypesRelatedTo
check, we could synthesize a copy of the source and target tuple types with the excluded discriminants erased tonever
so that they are excluded from the index type. That would result in the allocation of types purely to accommodateindexTypesRelatedTo
, which would then be subsequently discarded. This seems like an unnecessary cost, aspropertiesRelatedTo
sufficiently covers the numeric index type case.Fixes #39357
Fixes #34967