Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed an issue with contextual type for intersection properties #48668

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Apr 13, 2022

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.nameType branch) 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.

⚠️ At the moment, this contains a single test-case regression - please check my inline review comment about it

@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Apr 13, 2022
Comment on lines 26802 to 26809
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);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@sandersn
Copy link
Member

@Andarist to help our organisation, can you file a bug for this PR?

@sandersn sandersn requested a review from weswigham April 22, 2022 16:12
@sandersn sandersn moved this from Not started to Waiting on reviewers in PR Backlog Apr 22, 2022
@Andarist
Copy link
Contributor Author

@sandersn sure thing - I was actually planning to do that today

@Andarist Andarist force-pushed the contextual-type-function-obj-prop-intersection branch from ce4998f to a7422db Compare April 26, 2022 14:17
@typescript-bot typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Apr 26, 2022
PR Backlog automation moved this from Waiting on reviewers to Needs merge Apr 26, 2022
@weswigham
Copy link
Member

@andrewbranch do you think we want this in for the rc?

@andrewbranch
Copy link
Member

The bug is not a regression and has a known workaround, so I would lean toward waiting. @DanielRosenwasser what do you think?

@DanielRosenwasser
Copy link
Member

I would lean on waiting for the 4.8 period.

@andrewbranch andrewbranch added this to the TypeScript 4.8.0 milestone Apr 26, 2022
@sandersn sandersn merged commit 9236e39 into microsoft:main May 9, 2022
PR Backlog automation moved this from Needs merge to Done May 9, 2022
@Andarist Andarist deleted the contextual-type-function-obj-prop-intersection branch May 9, 2022 23:12
typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Aug 11, 2022
Component commits:
eff4406 Revert "Fixed an issue with contextual type for intersection properties (microsoft#48668)"
This reverts commit 9236e39.
GulajavaMinistudio pushed a commit to angular-indonesia/TypeScript that referenced this pull request Aug 12, 2022
amcasey added a commit to amcasey/TypeScript that referenced this pull request Aug 15, 2022
typescript-bot added a commit to typescript-bot/TypeScript that referenced this pull request Aug 16, 2022
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>
Andarist added a commit to Andarist/TypeScript that referenced this pull request Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Contextual type can't be provided to a mapped type intersected with an object type
7 participants