-
Notifications
You must be signed in to change notification settings - Fork 13.1k
Fixed an issue with string mappings over generic intersections not being deferred in conditional types #55856
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
Fixed an issue with string mappings over generic intersections not being deferred in conditional types #55856
Conversation
…ing deferred in conditional types
This looks good to me (having touched this recently), but: @typescript-bot test top200 |
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 9f56c7f. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 9f56c7f. You can monitor the build here. |
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 9f56c7f. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the regular perf test suite on this PR at 9f56c7f. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 9f56c7f. You can monitor the build here. Update: The results are in! |
Hey @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 |
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
@jakebailey Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Hey @jakebailey, the results of running the DT tests are ready. |
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
Thank you for creating PR! It seems still fails in some (very artificial) examples...
|
Ah, ye - I see why it might fail. I'll try to improve the fix in a moment to handle more crazy scenarios like this recursively. |
function isPatternLiteralPlaceholderType(type: Type): boolean { | ||
function isPatternLiteralPlaceholderType(type: Type, ignoreGenericIntersections = false): boolean { | ||
if (type.flags & TypeFlags.Intersection) { | ||
return some((type as IntersectionType).types, t => !!(t.flags & (TypeFlags.Literal | TypeFlags.Null | TypeFlags.Undefined)) || isPatternLiteralPlaceholderType(t)); |
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 think this entire fix can be simplified to simply adding a check that the type isn't generic:
return !isGenericType(type) && some(...);
Generic types should never be classified as placeholders since upon instantiation they may become something completely different.
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.
Oh, that's cool! Thanks for the tip. I was worried that addSpans
would fail if I did something like this. It seems that (somewhat confusingly) isGenericIndexType
already returns true
for an intersection like this so addSpans
is covered.
LGTM but just going to double check: @typescript-bot test top200 |
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 5d29c23. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the tsc-only perf test suite on this PR at 5d29c23. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 5d29c23. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 5d29c23. You can monitor the build here. Update: The results are in! |
@jakebailey Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
Hey @jakebailey, the results of running the DT tests are ready. |
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
@jakebailey @ahejlsberg is this a fix intended for 5.3, or should we wait until 5.4? |
Probably meant for 5.3, not sure why we didn't merge it as we both approved |
fixes #55847