Make sure type parameters stay fixed throughout the inference process #2356
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,8 +79,7 @@ module ts { | |
| let emptyObjectType = createAnonymousType(undefined, emptySymbols, emptyArray, emptyArray, undefined, undefined); | ||
| let anyFunctionType = createAnonymousType(undefined, emptySymbols, emptyArray, emptyArray, undefined, undefined); | ||
| let noConstraintType = createAnonymousType(undefined, emptySymbols, emptyArray, emptyArray, undefined, undefined); | ||
|
|
||
| let anySignature = createSignature(undefined, undefined, emptyArray, anyType, 0, false, false); | ||
| let unknownSignature = createSignature(undefined, undefined, emptyArray, unknownType, 0, false, false); | ||
|
|
||
|
|
@@ -3514,6 +3513,7 @@ module ts { | |
| return t => { | ||
| for (let i = 0; i < context.typeParameters.length; i++) { | ||
| if (t === context.typeParameters[i]) { | ||
| context.inferences[i].isFixed = true; | ||
| return getInferredType(context, i); | ||
| } | ||
| } | ||
|
|
@@ -4388,6 +4388,8 @@ module ts { | |
| } | ||
| } | ||
|
|
||
| Debug.assert(!!downfallType, "If there is no common supertype, each type should have a downfallType"); | ||
|
|
||
| if (score > bestSupertypeScore) { | ||
| bestSupertype = types[i]; | ||
| bestSupertypeDownfallType = downfallType; | ||
|
|
@@ -4570,12 +4572,11 @@ module ts { | |
| function createInferenceContext(typeParameters: TypeParameter[], inferUnionTypes: boolean): InferenceContext { | ||
| let inferences: TypeInferences[] = []; | ||
| for (let unused of typeParameters) { | ||
| inferences.push({ primary: undefined, secondary: undefined, isFixed: false }); | ||
| } | ||
| return { | ||
| typeParameters: typeParameters, | ||
| inferUnionTypes: inferUnionTypes, | ||
| inferences: inferences, | ||
| inferredTypes: new Array(typeParameters.length), | ||
|
There was a problem hiding this comment. Should this have There was a problem hiding this comment. I think I'm going to leave it out. The reason it's optional is because it is always initially undefined, and undefined means that there have been no failures (yet). |
||
| }; | ||
|
|
@@ -4622,11 +4623,21 @@ module ts { | |
| for (let i = 0; i < typeParameters.length; i++) { | ||
| if (target === typeParameters[i]) { | ||
| let inferences = context.inferences[i]; | ||
| if (!inferences.isFixed) { | ||
| // Any inferences that are made to a type parameter in a union type are inferior | ||
| // to inferences made to a flat (non-union) type. This is because if we infer to | ||
| // T | string[], we really don't know if we should be inferring to T or not (because | ||
| // the correct constituent on the target side could be string[]). Therefore, we put | ||
| // such inferior inferences into a secondary bucket, and only use them if the primary | ||
| // bucket is empty. | ||
| let candidates = inferiority ? | ||
|
There was a problem hiding this comment. There's no documentation for There was a problem hiding this comment. Also, the documentation for There was a problem hiding this comment. // Any inferences that are made to a type parameter in a union type are inferior
// to inferences made to a flat (non-union) type. This is because if we infer to
// T | string[], we really don't know if we should be inferring to T or not (because
// the correct constituent on the target side could be string[]). Therefore, we put
// such inferior inferences into a secondary bucket, and only use them if the primary
// bucket is empty. |
||
| inferences.secondary || (inferences.secondary = []) : | ||
| inferences.primary || (inferences.primary = []); | ||
| if (!contains(candidates, source)) { | ||
| candidates.push(source); | ||
| } | ||
| } | ||
| return; | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -4732,21 +4743,35 @@ module ts { | |
|
|
||
| function getInferredType(context: InferenceContext, index: number): Type { | ||
| let inferredType = context.inferredTypes[index]; | ||
| let inferenceSucceeded: boolean; | ||
| if (!inferredType) { | ||
| let inferences = getInferenceCandidates(context, index); | ||
| if (inferences.length) { | ||
| // Infer widened union or supertype, or the unknown type for no common supertype | ||
| let unionOrSuperType = context.inferUnionTypes ? getUnionType(inferences) : getCommonSupertype(inferences); | ||
| inferredType = unionOrSuperType ? getWidenedType(unionOrSuperType) : unknownType; | ||
| inferenceSucceeded = !!unionOrSuperType; | ||
| } | ||
| else { | ||
| // Infer the empty object type when no inferences were made. It is important to remember that | ||
| // in this case, inference still succeeds, meaning there is no error for not having inference | ||
| // candidates. An inference error only occurs when there are *conflicting* candidates, i.e. | ||
| // candidates with no common supertype. | ||
| inferredType = emptyObjectType; | ||
| inferenceSucceeded = true; | ||
|
There was a problem hiding this comment. This seems odd. No inferences were made, but inference succeeded? Can you provide some some explnatories comments on this? Thanks! There was a problem hiding this comment. Sure, but this is just a rule of the language. If there were no inferences, inference still succeeds. I don't love it. But @DanielRosenwasser has a PR where he issues an error for this case under a compiler flag. There was a problem hiding this comment. It's not yet a PR; I'd like to make it a PR if we could solve some of the original concerns in #360. To clarify, when inference for a type argument fails, the process falls back to using There was a problem hiding this comment. Ok, I'll add a comment. |
||
| } | ||
|
|
||
| // Only do the constraint check if inference succeeded (to prevent cascading errors) | ||
|
There was a problem hiding this comment. Great comment, thanks. |
||
| if (inferenceSucceeded) { | ||
| let constraint = getConstraintOfTypeParameter(context.typeParameters[index]); | ||
| inferredType = constraint && !isTypeAssignableTo(inferredType, constraint) ? constraint : inferredType; | ||
| } | ||
| else if (context.failedTypeParameterIndex === undefined || context.failedTypeParameterIndex > index) { | ||
| // If inference failed, it is necessary to record the index of the failed type parameter (the one we are on). | ||
| // It might be that inference has already failed on a later type parameter on a previous call to inferTypeArguments. | ||
| // So if this failure is on preceding type parameter, this type parameter is the new failure index. | ||
| context.failedTypeParameterIndex = index; | ||
| } | ||
| context.inferredTypes[index] = inferredType; | ||
| } | ||
| return inferredType; | ||
|
|
@@ -6343,11 +6368,32 @@ module ts { | |
| return getSignatureInstantiation(signature, getInferredTypes(context)); | ||
| } | ||
|
|
||
| function inferTypeArguments(signature: Signature, args: Expression[], excludeArgument: boolean[], context: InferenceContext): void { | ||
| let typeParameters = signature.typeParameters; | ||
| let inferenceMapper = createInferenceMapper(context); | ||
|
|
||
| // Clear out all the inference results from the last time inferTypeArguments was called on this context | ||
| for (let i = 0; i < typeParameters.length; i++) { | ||
| // As an optimization, we don't have to clear (and later recompute) inferred types | ||
|
There was a problem hiding this comment. Is this actually an optimization? Or is this necessary to get correct results? I thought it was the latter. There was a problem hiding this comment. It's not an optimization that we reset it. It's an optimization that we skip resetting it if the type parameter was already fixed. It would be just as correct to reset all of them. But then we'd be repeating the same work for the type parameters that were fixed. Namely the work done by getInferredType. There was a problem hiding this comment. Good to explain in the code. There was a problem hiding this comment. Ok, I'm basically adding my comment from above as a comment in the code |
||
| // for type parameters that have already been fixed on the previous call to inferTypeArguments. | ||
| // It would be just as correct to reset all of them. But then we'd be repeating the same work | ||
| // for the type parameters that were fixed, namely the work done by getInferredType. | ||
| if (!context.inferences[i].isFixed) { | ||
| context.inferredTypes[i] = undefined; | ||
| } | ||
| } | ||
|
|
||
| // On this call to inferTypeArguments, we may get more inferences for certain type parameters that were not | ||
| // fixed last time. This means that a type parameter that failed inference last time may succeed this time, | ||
| // or vice versa. Therefore, the failedTypeParameterIndex is useless if it points to an unfixed type parameter, | ||
| // because it may change. So here we reset it. However, getInferredType will not revisit any type parameters | ||
| // that were previously fixed. So if a fixed type parameter failed previously, it will fail again because | ||
| // it will contain the exact same set of inferences. So if we reset the index from a fixed type parameter, | ||
| // we will lose information that we won't recover this time around. | ||
| if (context.failedTypeParameterIndex !== undefined && !context.inferences[context.failedTypeParameterIndex].isFixed) { | ||
| context.failedTypeParameterIndex = undefined; | ||
| } | ||
|
|
||
| // We perform two passes over the arguments. In the first pass we infer from all arguments, but use | ||
| // wildcards for all context sensitive function expressions. | ||
| for (let i = 0; i < args.length; i++) { | ||
|
|
@@ -6382,18 +6428,7 @@ module ts { | |
| } | ||
| } | ||
|
|
||
| getInferredTypes(context); | ||
| } | ||
|
|
||
| function checkTypeArguments(signature: Signature, typeArguments: TypeNode[], typeArgumentResultTypes: Type[], reportErrors: boolean): boolean { | ||
|
|
@@ -6627,15 +6662,17 @@ module ts { | |
| return resolveErrorCall(node); | ||
|
|
||
| function chooseOverload(candidates: Signature[], relation: Map<RelationComparisonResult>) { | ||
| for (let originalCandidate of candidates) { | ||
| if (!hasCorrectArity(node, args, originalCandidate)) { | ||
| continue; | ||
| } | ||
|
|
||
| let candidate: Signature; | ||
| let typeArgumentsAreValid: boolean; | ||
| let inferenceContext = originalCandidate.typeParameters | ||
| ? createInferenceContext(originalCandidate.typeParameters, /*inferUnionTypes*/ false) | ||
| : undefined; | ||
|
|
||
| while (true) { | ||
| candidate = originalCandidate; | ||
| if (candidate.typeParameters) { | ||
|
|
@@ -6645,9 +6682,9 @@ module ts { | |
| typeArgumentsAreValid = checkTypeArguments(candidate, typeArguments, typeArgumentTypes, /*reportErrors*/ false) | ||
| } | ||
| else { | ||
| inferTypeArguments(candidate, args, excludeArgument, inferenceContext); | ||
| typeArgumentsAreValid = inferenceContext.failedTypeParameterIndex === undefined; | ||
| typeArgumentTypes = inferenceContext.inferredTypes; | ||
| } | ||
| if (!typeArgumentsAreValid) { | ||
| break; | ||
|
|
@@ -6677,7 +6714,7 @@ module ts { | |
| else { | ||
| candidateForTypeArgumentError = originalCandidate; | ||
| if (!typeArguments) { | ||
| resultOfFailedInference = inferenceContext; | ||
| } | ||
| } | ||
| } | ||
|
|
||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1487,11 +1487,15 @@ module ts { | |
| (t: Type): Type; | ||
| } | ||
|
|
||
| // @internal | ||
| export interface TypeInferences { | ||
|
There was a problem hiding this comment. Might want to consider marking this as @internal so it doesn't affect the API tests. There was a problem hiding this comment. We should just add a keyword for this instead of having this comment annotation thing. But sure. |
||
| primary: Type[]; // Inferences made directly to a type parameter | ||
| secondary: Type[]; // Inferences made to a type parameter in a union type | ||
| isFixed: boolean; // Whether the type parameter is fixed, as defined in section 4.12.2 of the TypeScript spec | ||
| // If a type parameter is fixed, no more inferences can be made for the type parameter | ||
| } | ||
|
|
||
| // @internal | ||
| export interface InferenceContext { | ||
| typeParameters: TypeParameter[]; // Type parameters for which inferences are made | ||
| inferUnionTypes: boolean; // Infer union types for disjoint candidates (otherwise undefinedType) | ||
|
|
||
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
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
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
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
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
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
Oops, something went wrong.
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.
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.
downfallType? Is this supposed to be like a fallbackType? If so, rename it. downfallType sounds like it describes revolutions, lost wars, etc.
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.
It is supposed to be downfallType. It is supposed to sound like it describes revolutions and lost wars, but in particular tragic heroes.
The downfallType is the type that caused a particular candidate to not be the common supertype. So if it weren't for this one downfallType, the type in question could have been the common supertype.
Note that downfallType is the first type that is not a subtype of type. It's not necessarily the only one.
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.
Can we add this explanation in the code