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.
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.
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