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

bugfix: homomorphic mapped types when T is non-generic, solves 27995 #48433

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

mikearnaldi
Copy link

@mikearnaldi mikearnaldi commented Mar 26, 2022

Took a stab at it, the way I've approached is detecting if T in a { [k in keyof T]: F<T> } is a tuple type and in case it is resolve the node to an instantiated tuple type.

I am not sure if this covers all the cases or if it ends up breaking anything else, please let me know if I should change something

Fixes #27995

cc @Andarist

--

This only deals with tuple, if the way to do it is correct I'd be happy to extend it to mapped arrays too.

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Mar 26, 2022
@ghost
Copy link

ghost commented Mar 26, 2022

CLA assistant check
All CLA requirements met.

@mikearnaldi mikearnaldi marked this pull request as ready for review March 26, 2022 01:06
@mikearnaldi mikearnaldi marked this pull request as draft March 26, 2022 12:17
@mikearnaldi mikearnaldi marked this pull request as ready for review March 26, 2022 19:08
src/compiler/checker.ts Outdated Show resolved Hide resolved
@@ -15848,6 +15859,19 @@ namespace ts {
// Eagerly resolve the constraint type which forces an error if the constraint type circularly
// references itself through one or more type aliases.
getConstraintTypeFromMappedType(type);
Copy link
Contributor

Choose a reason for hiding this comment

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

do you perhaps understand why this isn't sort of handled within getConstraintTypeFromMappedType? I recall we were trying to investigate that

Copy link
Author

Choose a reason for hiding this comment

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

So getConstraintTypeFromMappedType is delegating to getConstraintOfTypeParameter the job of getting the constraint type and additionaly sets it in the mapped type, here I have 2 choices either extend getConstraintTypeFromMappedType to detect the condition and eventually mutate the constraint of the type parameter or to extend the logic of getConstraintOfTypeParameter to check if the constraint is for a homomorphic mapped type. The function getConstraintOfTypeParameter is also directly invoked when checking index expressions like A[k] where getConstraintTypeFromMappedType isn't, the function returns the already resolved constraint if there so in theory it could still work but I felt more appropriate to have this logic in getConstraintOfTypeParameter as the lowest common

const constraintType = getConstraintTypeFromMappedType(type);
checkTypeAssignableTo(constraintType, keyofConstraintType, getEffectiveConstraintOfTypeParameter(node.typeParameter));
const type = getTypeFromMappedTypeNode(node);
// Continue to check if the type returned is a mapped type, that means it wasn't resolved to a homomorphic tuple type
Copy link
Contributor

Choose a reason for hiding this comment

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

where does this typechecking for a homomorphic mapped type happen now? in the added instantiateMappedTupleType call?

Copy link
Author

Choose a reason for hiding this comment

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

So in case the returned type is a tuple type (which I now realize it may be a simpler condition here) we don't have a mapped type to check, one condition is implicitly checked, as the type can only be homomorphic if it has no nameType (the AS rest component of the type parameter) the other on assignability of the constraint to the keyOfConstraintType should be once again implicit given the constraint is the keyof of a tuple BUT I may be wrong :)

@Andarist
Copy link
Contributor

Andarist commented Apr 7, 2022

Looking at the last comment here it also might fix the issue there

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

I think a maybe more consistent approach would be changing how getTypeFromMappedTypeNode constructs the type to align more with how instantiateMappedType behaves - ideally sharing some functionality between the two. The current approach feels a bit ad-hoc, and I don't particularly like trafficking around all this information as part of the constraint logic for the mapped type. If {[K in keyof MyTuple]: MyTuple[K] | string} is just supposed to do the mapping immediately and make a tuple type reference, then that's just what we should do when we get the type from the type node - we should skip out on making the mapped type object entirely.

@Andarist
Copy link
Contributor

@weswigham I pushed out the requested changes - to some extent at least. I'm not really sure if this was worth it though. The parameters list change of instantiateMappedTupleTypeand instantiateMappedArrayType isn't that great and reusing instantiateMappedType further would require similar changes since currently all of that works on the MappedType whereas you requested skipping that altogether. To skip it entirely we need functions that accept all of the information that was previously obtained from the mapped type itself.

Let me know what you think.

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
Status: Waiting on author
Development

Successfully merging this pull request may close these issues.

Mapped tuples types iterates over all properties
4 participants