Fixed inferred type widening with contextual types created by binding patterns #56875
Conversation
| // widening of the binding pattern type substitutes a regular any for the non-inferrable unknown. | ||
| return includePatternInType ? nonInferrableUnknownType : anyType; |
There was a problem hiding this comment.
I'm not sure when "widening of the binding pattern type substitutes a regular any for the non-inferrable any" is happening and if this part of the comment is even relevant today.
| @@ -20905,7 +20905,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| function isSimpleTypeRelatedTo(source: Type, target: Type, relation: Map<string, RelationComparisonResult>, errorReporter?: ErrorReporter) { | |||
| const s = source.flags; | |||
| const t = target.flags; | |||
| if (t & TypeFlags.Any || s & TypeFlags.Never || source === wildcardType) return true; | |||
| if (t & TypeFlags.Any || s & TypeFlags.Never || source === wildcardType || source === nonInferrableUnknownType) return true; | |||
There was a problem hiding this comment.
This likely isn't strictly needed - at least not for the test cases I have here now. While iterating on this, I planned to make the created pattern type pass the constraint check in getInferredType. This line would allow for that in some scenarios. Later I realized that it wouldn't allow for that in all scenarios though - and that's why I just avoid doing the constraint check in getInferredType. The idea is still that the nonInferrableUnknown should act like any for assignability purposes though - I just replaced its type with unknown to benefit from the T & unknown // T.
| @@ -25089,8 +25108,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| } | |||
|
|
|||
| function tupleTypesDefinitelyUnrelated(source: TupleTypeReference, target: TupleTypeReference) { | |||
| return !(target.target.combinedFlags & ElementFlags.Variadic) && target.target.minLength > source.target.minLength || | |||
| !target.target.hasRestElement && (source.target.hasRestElement || target.target.fixedLength < source.target.fixedLength); | |||
| return !source.pattern && (!(target.target.combinedFlags & ElementFlags.Variadic) && target.target.minLength > source.target.minLength || | |||
There was a problem hiding this comment.
This allows us to infer between tuples of different shapes in those scenarios - since the tuple created by an array binding pattern might be shorter than the target. So far, I haven't managed to break this but I would advise extra caution around this when reviewing.
| return undefined; | ||
| } | ||
| const type = getTypeOfSymbol(prop); | ||
| if (type !== nonInferrableUnknownType) { |
There was a problem hiding this comment.
Since nonInferrableUnknownType doesn't hold any useful information for contextual typing I let it fallthrough to index signatures here. Note that { x: nonInferrableUnknownType } & { x: string } shouldn't return nonInferrableUnknownType for 'x' property here. So if the constraint already defines a property then index signatures won't be consulted.
| if (t.flags & TypeFlags.Intersection) { | ||
| t = getIntersectionType((t as IntersectionType).types.filter(t => !t.pattern)); | ||
| } |
There was a problem hiding this comment.
I wish I knew how to do it in a better way... this is mainly to satisfy the isTupleType(t) below (when it should be satisfied)
| var intrinsicMarkerType = createIntrinsicType(TypeFlags.Any, "intrinsic"); | ||
| var unknownType = createIntrinsicType(TypeFlags.Unknown, "unknown"); | ||
| var nonNullUnknownType = createIntrinsicType(TypeFlags.Unknown, "unknown", /*objectFlags*/ undefined, "non-null"); | ||
| var nonInferrableUnknownType = createIntrinsicType(TypeFlags.Unknown, "unknown", ObjectFlags.ContainsWideningType, "non-inferrable"); |
There was a problem hiding this comment.
Likely getIntersectionType should reduce nonInferrableUnknown & unknown to unknown. Most of such reductions are performed based on includes coming from addTypesToIntersection. It would be cool to introduce TypeFlags.IncludesNonInferrableUnknown but those Includes* flags are mostly reusing (?) other existing flags and I'm not super comfortable with touching this on my own.
|
@typescript-bot test top200 @typescript-bot perf test this |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 8f0a2c9. 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 8f0a2c9. 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 8f0a2c9. 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 8f0a2c9. 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 8f0a2c9. 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 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:
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! |
@andrewbranch concluded here that:
While working on this PR here, I realized that even object binding patterns can contribute useful things to inferences. That relies on initializers' types though, such as here (a test case from this PR):
One extra thing that I noticed based on the issue here is that constraints are still useful. When inferring from a binding pattern we often end up with an inferred type like
{ a: nonInferrableAny }but that doesn't pass the constraint check ingetInferredTypein the situations like this:The constraint here requires 2 properties but the type inferred from the binding pattern has only one. This particular case isn't problematic because when the inferred type doesn't satisfy the constraint the algorithm (silently~) picks the constraint.
Let's take a look at what happens in the reported case:
The inferred type here (
{ A: nonInferrableAny; }) does satisfy the constraint and that leads to some lossy behaviors.getWidenedLiteralLikeTypeForContextualTypetostring{ A: string }fails the constraint check ingetInferredTypeThe problem is that now the inferred type is the constraint (and that's nullable). We could have infer much better type of
{ A: { a: "A" } }.This indicates that the constraint is still useful but our inferred type from the binding patterns might even be a supertype of the constraint (a binding pattern might only refer to some of the keys etc).
So the solution to this problem is to combine both pieces of information - the inferred type and the constraint type. Without introducing any new concepts to the compiler we can do that by intersecting them. That creates a problem without further changes since
nonInferrablyAnyis justanyandT & anyis justany. Using this is lossy for contextual typing in a similar way to the one described above.On the other hand,
unknownbehaves almost perfectly for this purpose. It's "erasable" in intersections:T & unknown // unknown. The only real problem I have found with it is that intersections between objects with concrete properties and objects with index signatures don't behave as I'd like them to here.A concrete property always shadows over the index signature - which is especially annoying~ here when it comes to tuple/array types:
In both of those situations we want to avoid those concrete properties to overshadow the more useful information contained in the index signature-based constraint. Since
nonInferrableUnknowndoesn't really hold any useful information for contextual typing it seems to be OK to ignore it and to look into the index signature in such cases. That has been implemented here.fixes #56771
fixes #42969