Correct this in tuple type parameter constraints #10234
Conversation
Instantiate this in tuple types used as type parameter constraints
|
@ahejlsberg can you take a look? @weswigham I didn't add special tuple comparison code to |
| @@ -4000,6 +4000,10 @@ namespace ts { | |||
| return createTypeReference((<TypeReference>type).target, | |||
| concatenate((<TypeReference>type).typeArguments, [thisArgument || (<TypeReference>type).target.thisType])); | |||
| } | |||
| if (type.flags & TypeFlags.Tuple) { | |||
| resolveTupleTypeMembers(type as TupleType, thisArgument); | |||
There was a problem hiding this comment.
This isn't the right approach. You need to create a new tuple type that records its associated this-type, and then use that this-type when (and if) the members are resolved. Member resolution is lazy, but cannot change once it has been performed. The way you have it now, you might be stepping on information that was already recorded by a previous call to resolveTypeMembers.
There was a problem hiding this comment.
How do I create a new tuple type safely? Can I just say createNewTupleType(type.elementTypes)?
And once I have the tuple type, I think I'll need to store thisType in a separate property so that resolveTupleTypeMembers can use it later, right? (I guess I could still immediately call resultTupleTypeMembers like I do in the current commit.)
There was a problem hiding this comment.
You basically have to change createTupleType and the TupleType type to record the this-type information alongside the element types. Also, it would need to be part of the key used for the tuple type in the cache. Once you do that you can then call createTupleType from getTypeWithThisArgument passing along the appropriate this-type.
There was a problem hiding this comment.
Don't immediately resolve the members, that defeats the whole purpose of the implementation.
Create a new tuple that stores the this-type.
| return tupleTypes[id] || (tupleTypes[id] = createNewTupleType(elementTypes)); | ||
| function createTupleType(elementTypes: Type[], thisType?: Type) { | ||
| let id = getTypeListId(elementTypes); | ||
| if (thisType) { |
There was a problem hiding this comment.
You need to always append an extra id no matter whether you have a this-type or not. Otherwise you end up confusing tuples with a this-type with next-higher-arity tuples without a this-type. I suggest always using
let id = getTypeListId(elementTypes) + "," + (thisType ? thisType.id : 0);
Then also change createType to increment typeCount before assigning it to the new type's id property.
|
@ahejlsberg I think this is ready to go in after Travis finishes running. Can you take another look? |
Fixes #9707
The problem is that tuples are compared structurally, but are created differently from other object types. They aren't normally created with a this-argument, so they just use themselves as
this. Which is normally correct, except when the type inference process checks the assignability of the inferred type argument to the type constraint. (if (!isTypeAssignableTo(inferredType, getTypeWithThisArgument(instantiatedConstraint, inferredType))) {ingetInferredType).The fix is to make
getTypeWithThisArgumenteagerly resolve tuples in the type constraint case, using thethisArgumentprovided there.