Uh oh!
There was an error while loading. Please reload this page.
fix(50858): parameters in function signatures inferred from default generic types sometimes not assigned instantiated types - #50960
Closed
Max Stoumen (mxsdev) wants to merge 3 commits into
Closed
Conversation
Max Stoumen (mxsdev)
commented
Sep 27, 2022
Author
@microsoft-github-policy-service agree |
Mateusz Burzyński (Andarist)
commented
Sep 28, 2022
Contributor
As a bystander - I applaud the description of this PR 👏 |
Uh oh!
There was an error while loading. Please reload this page.
Ryan Cavanaugh (RyanCavanaugh)
commented
Sep 28, 2022
Member
TypeScript Bot (@typescript-bot) perf test this |
Contributor
Heya Ryan Cavanaugh (@RyanCavanaugh), I've started to run the perf test suite on this PR at 92631fe. You can monitor the build here. Update: The results are in! |
Contributor
Heya Ryan Cavanaugh (@RyanCavanaugh), I've started to run the diff-based top-repos suite on this PR at 92631fe. You can monitor the build here. Update: The results are in! |
Ryan Cavanaugh (RyanCavanaugh)
requested review from
Anders Hejlsberg (ahejlsberg) and Wesley Wigham (weswigham)September 28, 2022 16:44
TypeScript Bot (typescript-bot)
commented
Sep 28, 2022
Contributor
Ryan Cavanaugh (@RyanCavanaugh) Here they are:CompilerComparison Report - main..50960
System
Hosts
Scenarios
TSServerComparison Report - main..50960
System
Hosts
Scenarios
Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
TypeScript Bot (typescript-bot)
commented
Sep 28, 2022
Contributor
Ryan Cavanaugh (@RyanCavanaugh) Here are the results of running the top-repos suite comparing Everything looks good! |
Max Stoumen (mxsdev)
commented
Sep 29, 2022
Author
Ha, thanks! I hope it helps the reviewers more than it confuses them 😅 |
Mateusz Burzyński (Andarist)
commented
Sep 29, 2022
Contributor
This PR would also fix #46310 and thus it would supersed my PR attempting to fix that issue: #48823 Max Stoumen (@mxsdev) could you add a test case from my PR here? |
Comment on lines
+27546
to
+27550
| // If the apparent type is any, we cannot lose any information by instantiating, | ||
| // so we might as well try and possibly get a useful type | ||
| if (apparentType === anyType) { | ||
| return instantiateInstantiableTypes(contextualType, inferenceContext.nonFixingMapper); | ||
| } |
Contributor
There was a problem hiding this comment.
In what cases this is helpful? It's not totally clear to me what exactly the apparent type is but if I take into account the comment above the getApparentType then it should return the base constraint for type parameters.
Given that this PR introduces only tests that have a base constraint that is not any - I wonder, what existing test cases does this branch satisfy?
| if (apparentType === anyType) { | ||
| return instantiateInstantiableTypes(contextualType, inferenceContext.nonFixingMapper); | ||
| } | ||
| else if((apparentType.flags & TypeFlags.Union)) { |
Contributor
There was a problem hiding this comment.
Suggested change
| elseif((apparentType.flags&TypeFlags.Union)){ | |
| elseif((apparentType.flags&TypeFlags.Union)){ | |
| if (!signatureList) { | ||
| // At this point, the signature type is guaranteed to eventually collapse into any | ||
| // because it is a union of incompatible function signatures |
Contributor
There was a problem hiding this comment.
Suggested change
| // because it is a union of incompatible function signatures | |
| // because it is a union of incompatible function signatures or there is no signature at all | |
Anders Hejlsberg (ahejlsberg)
commented
Sep 29, 2022
Member
Max Stoumen (@mxsdev) I think it is a good idea to fix this issue, but I think there's a much simpler way to implement it. I will put up a PR shortly. |
This file contains hidden or 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
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
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#50858
This issue is specifically about functions within the arguments of generic-inferring call expressions. Early in the execution flow, the internal function's signature will only be instantiated with types when some inferences have already been made about them, but this misses the case where those generics have default values. Instead of taking default values, the signature falls back to a base type. If that base type is a union of incompatible signatures or
any, then the signature's parameters will be set toany.Later in the execution flow, the signature is properly instantiated; however by that time diagnostics have already been created and the signature's symbol type may already have been set to
any.See below for a more in-depth look at the call stack.
Example
Consider the following test:
The function call
f1should take a default type parameter of"a"for type parameterT, and so we expecteventto take its contextual type ofstring. However, due to this issue, it is of typeanyand throws anoImplicitAnydiagnostic.Call Stack
Call Stack Analysis of Example (in checker.ts)
resolveCallExpressionon call expressionf1((event) => { })->resolveCall->chooseOverloadchooseOverload: NotypeParametersare specified, soinferTypeArgumentsis called twice. The first call skips context sensitive parameters (such as(event) => {}), so we are interested in the second callinferTypeArguments: callscheckExpressionWithContextualTypeto get the type of each argument before making any type inferencescheckExpressionWithContextualType-> ... ->contextuallyCheckFunctionExpressionOrObjectLiteralMethodcontextuallyCheckFunctionExpressionOrObjectLiteralMethod: Gets contextual signature of argument by callinggetContextualSignaturegetContextualSignature: Gets type fromgetApparentTypeOfContextualTypegetApparentTypeOfContextualType:contextualTypeis the typeType[T]of kindIndexedAccess. We then try and instantiate that type by callinginstantiateContextualTypeinstantiateContextualType: Since no inferences have been made, the type is not instantiated, and thecontextualTypeis returnedinstantiateContextualTypedoes nothing, so we find the apparent type ofType[T], which in this case is the base type, which is(e: string) => void|(e: number) => voidundefinedis returnedundefined, the parameter type is assigned withassignNonContextualParameterTypesassignNonContextualParameterTypes: CallsassignParameterTypewithundefinedtype on each parameterundefined, another attempt is made to infer it contextually, which ultimately hitsgetContextualSignatureagain which like before returnsundefinedwidenTypeForVariableLikeDeclaration: Type isundefinedsoanyTypeis returned and a diagnostic is throwninferTypesis now called, which makes type inferencesgetSignatureInstantiation, and the resulting signature is correct (has(event: string) => voidas a parameter)getSignatureApplicabilityErroris called, which flows through the same call stack as before with the instantiated signature, untilcontextuallyCheckFunctionExpressionOrObjectLiteralMethodbecause the previous execution stack already "checked the context" and setNodeCheckFlags.ContextChecked.This issue is caused by
instantiateContextualType, which is in the call stack ofinferTypeArgumentsinchooseOverload. There is a cyclic dependency: the result ofinferTypeArgumentsis necessary to fully instantiate the signature withgetSignatureInstantiation, but (it would seem) a fully instantiated signature is expected by the return result ofinstantiateContextualTypewithingetApparentTypeOfContextualType, which is in the call stack ofinferTypeArguments.Solution
The solution presented is to instantiate from within
instantiateContextualTypewhenever the apparent type ofcontextualTypeis "degenerate" - that is, when it is eitheranyor a union of incompatible signatures - since, in this case, essentially no further information on the parameters of the signature can be gleamed if we return it. So, in the worst case, we get back another degenerate type from instantiation, but in the best case, we get more type information.In cases like the previous example, the call stack off of
inferTypeArgumentswill generate the correct signature, and the correct parameter type will be assigned.Further thoughts
I doubt this fixes every single possible instance of this bug, but it fixes the linked issue and many others. But I presume the types of the function parameters should be set after or during the instantiation of the signature, not during
inferTypeArguments. It is indeed rather strange as well that a function named "assignNonContextualParameterTypes" ends up trying to infer a lot of contextual information. But my familiarity with the codebase is not excellent, so if somebody can clear that up, please do!One more potential concern I had was that
instantiateContextualTypemay now end up inferring "too much" - perhaps returning a non-any type where an any type is required. All of the tests pass, however, so this doesn't seem likely.My original assumption was that
inferTypeArgumentsshould have a flag to never result in a call ofassignParameterType, however I wasn't able to get this to work without destroying generic inference; it seems that many instances of type inference require types to be set from the call toinferTypeArguments, and not from the call togetSignatureApplicabilityErrorwith the instantiated signature.Another thought was to allow
getSignatureApplicabilityErrorto set the type by resettingNodeCheckFlags.ContextCheckedon the arguments. This does not work, however, sinceassignParameterTypeprevents setting the parameter type to one different than was cached; moreover, the diagnostic errors have already been thrown.Note that removing the check in
instantiateContextualTypethat "no inferences have been made" understandably breaks the codebase as well, since this will (I assume) instantiate preemptively in many cases.If anyone has any ideas on how to make the previous approaches work, please respond, since I think they're significantly more elegant solutions to the problem than the one presented.