Fixed an issue with contextual type for intersection properties #48668
Conversation
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
src/compiler/checker.ts
Outdated
| if (t.flags & TypeFlags.Intersection) { | ||
| const intersection = t as IntersectionType; | ||
| const newTypes = intersection.types.map(propertyOfContextualTypeMapper).filter((t): t is Type => !!t); | ||
| if (newTypes.length === 0) { | ||
| return undefined; | ||
| } | ||
| return getIntersectionType(newTypes); | ||
| } |
There was a problem hiding this comment.
This logic breaks the test case from this issue.
I think I understand why and I think that what I have wanted to implement wouldn't actually cause this regression. However, I wasn't able to implement the original idea and used this simplified one as a conversation starter.
The problem is that this logic here resolves each member of the intersection separately - and the result is an array of property types which are later combined back into an intersection. So basically this is what happens:
intersection.types: [
"TestObject",
"{ [k: string]: any; }"
]
newTypes: [
"{ [k: string]: TestGeneric; }",
"any"
]
getIntersectionType(newTypes): any
And this makes sense - the problem is that we should create an instantiated intersection~ that could be resolved "together" and thus the index signature from the second member of the intersection wouldn't change the behavior in such a way.
Expected pseudocode:
instantiateIntersection(intersection)[prop]Actual pseudocode:
toIntersection(intersection.types.map(type => instantiate(type)[prop]))I initially tried to just use instantiateTypes but then it has been failing down the road on resolving type parameters to type arguments. I think this has been happening because the type parameter was passed to my propertyOfContextualTypeMapper and it has been returning undefined for that and crashed on the next operation when the cache key for the instantiated type was being created.
I couldn't figure out how to properly pass my mapper from here, in a way that it wouldn't even be used for that mentioned operation (in there my input mapper was combined with type.mapper, and that composite mapper was then returning undefined)
There was a problem hiding this comment.
I could manually "reduce" this intersection and compute the final intersection type on my own - but I wonder if maybe there is already a logic like this somewhere that could be reused. I've tried instantiating the type (couldn't make it work, perhaps it was not designed to be used within mapType) and also calling getReducedType (IIRC this didn't really "reduce" an intersection).
Let me know if there is a better way or if I should just compute this manually here.
There was a problem hiding this comment.
Right, so, contextual typing doesn't use instantiation straight up usually because instantiation would lose information via simplification (eg, union reduction) that still has value as a contextual type. Similarly, we want to be able to return type parameters without fixing them yet, as executing an inference mapper on them would do. (We want to save that for when the contextual breakdown is done and we've selected the final type.) Lastly, contextual typing usually wants to filter out types that delete information from its traversal, like any, never in contravariant positions, or unknown in covariant ones like this. In this case, I'd just recommend filtering any and unknown out of your processed type list - they don't provide useful contextual type information if our goal is to lookup a specific property on the result.
There was a problem hiding this comment.
Although in this case, we're looking at the property itself and not the parent type we're indexing into - to emulate index signature priority, I'd consider doing two passes over the list of intersection types - one where you look for concrete properties in the intersection members and, if that fails, a second where you look for indexers if no properties exist.
There was a problem hiding this comment.
@weswigham Ok, so I've refactored this slightly, and now CI passes 🎉
I didn't do anything special about any/unknown/never. I've rechecked and they work fine here (when a generic mapped type is intersected with them), without any special filtering.
Perhaps there is some more complex situation in which this could matter - but I'm not sure what those are. I've checked and simple cases are simply reduced earlier and the intersection is never observed by this function here.
So I would have to somehow defer creating those types and I'm not sure how to best prepare such a test case.
|
@Andarist to help our organisation, can you file a bug for this PR? |
|
@sandersn sure thing - I was actually planning to do that today |
…s comming from a property with intersection type
…hen intersections with indexers are used
ce4998f to
a7422db
Compare
|
@andrewbranch do you think we want this in for the rc? |
|
The bug is not a regression and has a known workaround, so I would lean toward waiting. @DanielRosenwasser what do you think? |
|
I would lean on waiting for the 4.8 period. |
Component commits: eff4406 Revert "Fixed an issue with contextual type for intersection properties (microsoft#48668)" This reverts commit 9236e39.
…es (microsoft#48668)" (microsoft#50279) This reverts commit 9236e39.
…properties (microsoft#48668)" (microsoft#50279)" This reverts commit adf26ff.
Component commits: eff4406 Revert "Fixed an issue with contextual type for intersection properties (microsoft#48668)" This reverts commit 9236e39. Co-authored-by: Ryan Cavanaugh <RyanCavanaugh@users.noreply.github.com>
…properties (microsoft#48668)" (microsoft#50279)" This reverts commit adf26ff.
fixes #48812
The fixed issue can be check on this TS playground and the behavior change can be observed verified in the diff here
I've diagnosed the issue to be caused by the fact that the intersection type was never instantiated~ (when it was instantiated within
isGenericMappedType(t) && !t.declaration.nameTypebranch) and thus a type for its property couldn't be reliably retrieved. It just has been falling back to check the structure of the uninstantiated type and, well, this yielded suboptimal results.