Skip to content

Add instantiation rules for reverse mapped types #42449

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

Merged
merged 4 commits into from
Apr 27, 2021

Conversation

weswigham
Copy link
Member

Fixes #42385

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Jan 21, 2021
@weswigham weswigham force-pushed the reverse-mapped-instantiation branch from 51ba26e to 1cdbfee Compare January 21, 2021 23:34
function instantiateReverseMappedType(type: ReverseMappedType, mapper: TypeMapper) {
const innerMappedType = instantiateType(type.mappedType, mapper);
if (!(getObjectFlags(innerMappedType) & ObjectFlags.Mapped)) {
return type;
Copy link
Member Author

Choose a reason for hiding this comment

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

It feels a bit odd to return the uninstantiated type, but when we can't perform the mapping post-instantiation, it's our best option, and preserves our current behaviors more (since we never instantiated the type prior to this). The most relevant time this occurs is with the permissiveMapper - it guarantees all homomorphic mapped types (that are still generic) map to any, which is great and all, but prevents us from forming a reverse mapped type (which, in turn, causes us to give paradoxical conditional type extends check results). So we kinda just have to skip that instantiation.

Copy link
Member

Choose a reason for hiding this comment

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

Do you have an example where this happens, i.e. where instantiation no longer produces a mapped type?

Copy link
Member

Choose a reason for hiding this comment

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

I mean, other that with permissiveMapper.

Copy link
Member Author

Choose a reason for hiding this comment

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

The wildcardType and the errorType are the only two types that make a mapped type outright collapse; so the permissiveMapper and scenarios involving errors are likely the only cases.

Copy link
Member Author

@weswigham weswigham Jan 28, 2021

Choose a reason for hiding this comment

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

Well, technically also any type that isn't a union and doesn't meet the mask

t.flags & (TypeFlags.AnyOrUnknown | TypeFlags.InstantiableNonPrimitive | TypeFlags.Object | TypeFlags.Intersection)

too. But that mask covers pretty much everything objecty. I think never and other primitives aren't included in it, so never will also collapse the mapped type (to never), as will primitives.

@RyanCavanaugh
Copy link
Member

Now that we have a root cause, can we add a test that minimally demonstrates what's being fixed? The OP repro was a bit of a garden path

@weswigham
Copy link
Member Author

weswigham commented Jan 22, 2021

It's actually pretty close to minimal (the "this works" bits are extra, and you could remove some bits from class bodies into global functions) - it is not trivial to get a reverse mapped type in a generic parameter position and later instantiate it.

@RyanCavanaugh
Copy link
Member

That's horrifying 😅

@RyanCavanaugh
Copy link
Member

@ahejlsberg I don't feel qualified to ✅ this; can you take a look?

@ahejlsberg
Copy link
Member

Here's a simpler repro:

type Boxified<T> = { [P in keyof T]: { value: T[P]} };

declare function unboxify<T>(obj: Boxified<T>): T;

function foo<U, V>(obj: { u: { value: U }, v: { value: V } }) {
    return unboxify(obj);
}

let qq = foo({ u: { value: 10 }, v: { value: 'hello'} });  // { u: U, v: V } but should be { u: number, v: string }

@andrewbranch
Copy link
Member

Fixes #43277

@andrewbranch andrewbranch requested a review from ahejlsberg April 20, 2021 15:46
Comment on lines +15973 to +15977
const instantiated = inferTypeForHomomorphicMappedType(
instantiateType(type.source, mapper),
innerMappedType as MappedType,
innerIndexType as IndexType
);
Copy link
Member

Choose a reason for hiding this comment

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

So, the instantiation for a reverse mapped type inferred from some generic source is just another reverse mapped type for the instantiation of that source. Is that the right way to read this?

So, to walk through a slight simplification of Anders’ unboxify example:

type Boxified<T> = { [P in keyof T]: { value: T[P] } };

declare function unboxify<T>(obj: Boxified<T>): T;

function foo<U>(obj: { prop: { value: U }  }) {
    return unboxify(obj);
}

foo({ prop: { value: 10 } });

At the unboxify(obj) call, we infer a reverse mapped type from source { prop: { value: U } } to target T. Later, at the foo({ prop: { value: U } }) when we have a mapper from U to number, we instantiate the aforementioned reverse mapped type by making another reverse mapped type as if we were performing inference from source { prop: { value: number } } to target T, which, by the actual reverse mapping mechanism (unchanged in this PR, and I don’t know where it lives or when it’s triggered) to resolve that reverse mapped type into { prop: number }. Prior to this PR, basically the same process was happening, but we were trying to resolve the original reverse mapped type with the uninstantiated source, so we would have just gotten { prop: U }.

I still don’t really understand the instantiations of type.mappedType and type.constraintType—in the examples I’ve walked through, those instantiateType calls end up just returning the original type passed to them. E.g., in the unboxify example, the mapped type and constraint type only have references to T, and the mapper only maps U (which is only relevant to the source), and it’s hard to imagine how the inner mapped type could contain any type variables you’d ever find in the source. Is this only relevant to the special mappers you mentioned to Anders earlier?

Copy link
Member

Choose a reason for hiding this comment

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

Or, let me ask a more specific question about permissiveMapper and the error scenarios you mentioned—what would be the consequences of, rather than early returning the input type, just proceeding to return the reverse mapped type with an instantiated source, but with mappedType and constraintType simply copied:

function instantiateReverseMappedType(type: ReverseMappedType, mapper: TypeMapper) {
     return inferTypeForHomomorphicMappedType(
         instantiateType(type.source, mapper),
         type.mappedType,
         type.constraintType
     ) || type;
}

I think this would have been my instinct, and I don’t understand what would go wrong, in what cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, function type mappers that eliminate all type variables are pretty much the only ones that would affect those inner instantiations. The effect would be those type parameters would go unmapped, so we'd get inaccurate results when we use them. The means that, eg, we'd measure conditional type extends checks incorrectly when they involved type parameters at these positions within a reverse mapped type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
5 participants