Instantiate conditional types within contextual type instantiation when inference candidates are available #48838
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Conflicts: # src/compiler/checker.ts
Andarist
commented
Sep 30, 2022
|
@ahejlsberg this fix tries to fix the weird inconsistency where the position of the conditional type in the second parameter to a function call changes what is inferred for types without callable signatures (see the seemingly harmless position change in the diff presented at the top of this thread here, or play with this TS playground). The problem is that the |
# Conflicts: # src/compiler/checker.ts
Andarist
commented
Sep 30, 2022
Andarist
changed the title
Andarist
changed the title
Simplifiable types within contextual type instantiation when inference candidates are available
weswigham
approved these changes
Mar 15, 2023
There was a problem hiding this comment.
Honestly, every time I come across contextual type instantiation I get more and more wishy-washy about when and why we do it. I'm pretty sure, in theory, we should always do it. But only if it'll be useful. But also not when it'll only produce "useless" constraint values. Except we also try to not do it when we don't think it'll be useful to avoid instantiation work. It's just so... eck.
In any case, this looks fine to me, actually, though in this case I somewhat want to defer to @ahejlsberg who may have stronger opinions on why/why not we should be instantiating contextual types with the active inference context. So I'm going to mark this as approved, but refrain from merging it until either @RyanCavanaugh steps in and says "sure, we'll take this for this release" or @ahejlsberg weighs in one way or another.
|
@RyanCavanaugh @ahejlsberg any thoughts on this approved PR? |
|
@jakebailey would you be so kind to create a playground for this PR? 😉 |
|
@typescript-bot pack this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at be61797. You can monitor the build here. |
|
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 they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here they are:
tscComparison 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. Branch only errors:Package: lodash Package: oojs Package: underscore |
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
@jakebailey Here are some more interesting changes from running the top-repos suite Details
|
|
@jakebailey Here are some more interesting changes from running the top-repos suite Details
|
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. DetailsServer exited prematurely with code unknown and signal SIGABRTAffected reposcalcom/cal.comRaw error text:RepoResults7/calcom.cal.com.rawError.txt in the artifact folder
Last few requests{"seq":856,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":[],"openFiles":[{"file":"@PROJECT_ROOT@/apps/swagger/next-env.d.ts","projectRootPath":"@PROJECT_ROOT@"}]}}
{"seq":857,"type":"request","command":"getOutliningSpans","arguments":{"file":"@PROJECT_ROOT@/apps/swagger/next-env.d.ts"}}
{"seq":858,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":["@PROJECT_ROOT@/apps/api/test/lib/middleware/verifyApiKey.test.ts"],"openFiles":[]}}
{"seq":859,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":[],"openFiles":[{"file":"@PROJECT_ROOT@/apps/web/app/_trpc/client.ts","projectRootPath":"@PROJECT_ROOT@"}]}}
Repro steps
|
|
Seems like it wasn't a fluke, and this is very breaky? |
Andarist
changed the title
Simplifiable types within contextual type instantiation when inference candidates are available
Yeah, it seems like it. I reviewed a couple of those and started work on extracting new test cases. I think that all of those might be related to instantiating indexed access types. This is something that wasn't originally in this PR - I only added it because it felt like maybe they would fall into the same category as conditional types. I removed this now and it would be cool to rerun all of those tests. I will also take another look at the core issue. I created this PR almost 2 years ago and I learned a lot about the codebase in the meantime 😉 Maybe I will be able to figure out some alternative solution to the original issue (but maybe not). |
|
@typescript-bot test top200 @typescript-bot perf test this @typescript-bot user test tsserver |
|
Heya @jakebailey, I've started to run the diff-based user code test suite (tsserver) on this PR at e3f11a8. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite (tsserver) on this PR at e3f11a8. 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 e3f11a8. 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 e3f11a8. 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 e3f11a8. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the public perf test suite on this PR at e3f11a8. You can monitor the build here. Update: The results are in! |
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
|
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here they are:
tscComparison 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 Something interesting changed - please have a look. DetailsServer exited prematurely with code unknown and signal SIGABRTAffected reposcalcom/cal.comRaw error text:RepoResults7/calcom.cal.com.rawError.txt in the artifact folder
Last few requests{"seq":887,"type":"request","command":"organizeImports","arguments":{"scope":{"type":"file","args":{"file":"@PROJECT_ROOT@/apps/swagger/next-env.d.ts"}},"skipDestructiveCodeActions":false}}
{"seq":888,"type":"request","command":"getOutliningSpans","arguments":{"file":"@PROJECT_ROOT@/apps/swagger/next-env.d.ts"}}
{"seq":889,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":["@PROJECT_ROOT@/apps/api/test/lib/middleware/verifyApiKey.test.ts"],"openFiles":[]}}
{"seq":890,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":[],"openFiles":[{"file":"@PROJECT_ROOT@/apps/web/abTest/utils.ts","projectRootPath":"@PROJECT_ROOT@"}]}}
Repro steps
|
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
@jakebailey Here are some more interesting changes from running the top-repos suite Details
|
|
@jakebailey Here are some more interesting changes from running the top-repos suite Details
|
|
Oh, that's not great for me 😅 I'll have to extract some test cases out of this to add as tests to the repo, and... gonna get back to the drawing board to figure out a different fix for this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #46201
Highly related to #48823
The problem was that in this case, even though there were some inference candidates here, the conditional type was not instantiated "soon enough". It was especially surprising to me, as a user, as changing the position of this conditional type was fixing the issue:
You can checkout the playground with both variants here.
So how both of those cases were different?
The working variant was able to retrieve the
contextualTypewithininferTypeArgumentsas:Where within
getContextualTypeForObjectLiteralElementthegetApparentTypeOfContextualTypewas returning:and from that type a type for the
actionproperty was unpackages bygetTypeOfPropertyOfContextualType. So a conditional type itself, as a type for this property, was returned toinferTypeArgumentsand it was simply instantiated usinginstantiateType(contextualType, outerMapper), so the correct type was computed just fine here.The problem with the broken variant was that the conditional type wasn't instantiated in
getApparentTypeOfContextualType(while thecontextualTypethere was this conditional type) and it was passed tomapType(instantiatedType, getApparentType, true). This converted the conditional type to a union of its branches:This has lost the relation between the condition and those union members.
So my idea is that we could just instantiate conditional types early to avoid such situations because if we can evaluate the condition it should always be better to focus on the resulting branch than to unionize both of them.