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

Lookup across a mapped type gives different result when using a generic alias #15756

Closed
MatthewPopat opened this issue May 11, 2017 · 17 comments
Closed
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@MatthewPopat
Copy link

TypeScript Version: 2.3

I have been trying to create a generic type that maps the property names and types from one type to a discriminated union of key value pair types. I have found that using a lookup across a mapped type works fine as long as you use a concrete type before doing the lookup (see pair2 below). But if you try to create a generic type alias to do the lookup you end up with a different result (see pair1 below).

Code

type Pairs<T> = {
    [TKey in keyof T]: {
        key: TKey;
        value: T[TKey];
    };
};

type Pair<T> = Pairs<T>[keyof T];

type FooBar = {
    foo: string;
    bar: number;
};

let pair1: Pair<FooBar> = {
    key: "foo",
    value: 3
}; // no compile error here

let pair2: Pairs<FooBar>[keyof FooBar] = {
    key: "foo",
    value: 3
}; // this does cause a compile error

Expected behavior:

Both pair1 and pair2 above should cause a compile error.

Actual behavior:

pair1 seems to be assigned the type:

{
    key: "foo" | "bar";
    value: string | number;
}

whereas pair2 is assigned the type I would expect:

{
    key: "foo";
    value: string;
} | {
    key: "bar";
    value: number
}
@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label May 24, 2017
@jaen
Copy link

jaen commented Jul 15, 2017

I've just been bitten by that issue trying to type redux reducers generically.
A simplified version of my problem is as follows:

// a type which maps action kind string literal to payload type
type Simple = {
  setField: { field: string, value: string },
  submit:   { param: string }
};

// transform the above to a mapping from action kind to what I would emit in redux
type ActionTypesDictionary<Actions> =
  ({ [Key in keyof Actions]: { kind: Key, payload: Actions[Key]} });

// turns a mapping into a union of its values
type DictionaryToUnion<Dictionary> =
  Dictionary[keyof Dictionary];

// using the type explicitly works, but is boilerplate
type Test = ActionTypesDictionary<Simple>[keyof Simple];
//   { kind: "setField"; payload: { field: string; value: string; }; }
// | { kind: "submit"; payload: { param: string; }; }

// this is where it goes wrong - parameterisng over type to hide the boilerplate
type ActionTypes<Actions> = ActionTypesDictionary<Actions>[keyof Actions];

// results in incorrect type being inferred
type Test2 = ActionTypes<Simple>;
// type Test2 = { 
//   kind: "setField" | "submit";
//   payload: { field: string; value: string; } | {};
// }

It seems the mapping step (ActionTypesDictionary) works well on its own, as can be observed by the type Test when I want to abstract over the boilerplate of having to index the resulting dictionary manually by introducing parametricity the inference breaks. For some reason the disjoint union operator | get pushed down into the type in an unsound way, breaking the relation between kind and payload which enables incorrect mixing of payloads for a given kind of action.

Hopefully, this isn't hard to pin down why it happens, because it blocks typing a whole class of useful code.

@NaridaL
Copy link
Contributor

NaridaL commented Jul 15, 2017

Hovering over ActionTypes:

type ActionTypes<Actions> = {
    kind: keyof Actions;
    payload: Actions[keyof Actions];
}

which isn't a valid transformation.

@jaen
Copy link

jaen commented Jul 15, 2017

Right, maybe it's unclear where this exactly breaks down from my description.

To wit, the type generated by ActionTypesDictionary<Type> wen applied is correct:

type ActionTypesDictionary<Actions> = {
  [Key in keyof Actions]: {
    kind: Key;
    payload: Actions[Key];
  };
}

and mapping it by hand with Type[Keys] yields an intuitively correct type (an disjoint union of objects):

type ActionTypesDictionary<Simple>[keyof Simple] =
    { kind: "setField"; payload: { field: string; value: string; }; }
  | { kind: "submit"; payload: { param: string; }; }

while when the mapping is astracted over with a parametric type ActionTypes<Type> it produces a different, intuitively incorrect type (an object of disjoint unions) when applied:

type ActionTypes<Simple> = { 
  kind: "setField" | "submit";
  payload: { field: string; value: string; } | {};
}

so somehow wrapping the type indexing in a generic yields an incorrect transformation, because as @NeridaL shown, the type of unapplied ActionTypes<Type> incorrectly propagates the indexing into the type, effectively turning:

({
  [Key in keyof Actions]: {
    kind: Key;
    payload: Actions[Key];
  };
})[keyof SomeType]

into:

{
    kind: keyof SomeType;
    payload: Actions[keyof SomeType];
}

as if keyof SomeType was some property name in keyof Actions.

Hope it's clearer now.

@jaen
Copy link

jaen commented Jul 16, 2017

I've traced this down to this commit and it's associated pull request.

Introduction of

if (isGenericMappedType(objectType)) {
    return getIndexedAccessForMappedType(<MappedType>objectType, indexType, accessNode);
}

to enable the features in the pull request caused the Alias<Type> to propagate the | down in an unsound way. While of course, the features enabled by the PR are beneficial, maybe this logic could be adjusted not to propagate the '|' into the type?

Or maybe I am missing something about how this should work?

EDIT: after looking at this some more I understand what's the motivation of this substitution better (it does add soundness in other cases such as f13 in mappedTypeRelationships).
It also seems that at this stage of inference the typechecker doesn't seem to have enough information to see that the indexType can be constrained somehow (the fields for constraints are empty). And indeed, if TypeScript typechecks generics at declaration site and not use site (I think it does, correct me if I'm wrong) then there's no way to provide additional constraints at this point.
That is unless unions and string literals would be raised to first-class citizens and could be referred to as for eg. type Generic<Param extends Union<StringLiteral>.
And even then, it would need some expansion and simplification step at use-site to perform a correct substitution at the point the type parameter is of known type.

@jaen
Copy link

jaen commented Jul 22, 2017

Thanks to @tycho01's comment here #16018 (comment) I was able to work around this issue using a generic default, as evidenced here:

// some type mapping action kind string literal to payload type
type Simple = {
  setField: { field: string, value: string },
  submit:   { param: string }
};

// transform the above to a mapping from action kind to what I would emit in redux
type ActionTypesDictionary<Actions> =
  ({ [Key in keyof Actions]: { kind: Key, payload: Actions[Key]} });

// turns a mapping into a union of its values
type DictionaryToUnion<Dictionary> =
  Dictionary[keyof Dictionary];

// using the type explicitly works, but is boilerplate
type Test = ActionTypesDictionary<Simple>[keyof Simple];
//   { kind: "setField"; payload: { field: string; value: string; }; }
// | { kind: "submit"; payload: { param: string; }; }

// introducing abstraction to hide the boilerplate
type ActionTypes<Actions> = ActionTypesDictionary<Actions>[keyof Actions];

// changes the inferred type
type Test2 = ActionTypes<Simple>;
// type Test2 = { 
//   kind: "setField" | "submit";
//   payload: { field: string; value: string; } | {};
// }

// using the generic default hack
type WorkingActionTypes<Actions, Temp extends ActionTypesDictionary<Actions> = ActionTypesDictionary<Actions>> = Temp[keyof Actions];

// makes it work again!
type Test3 = ActionTypes<Simple>;
//   { kind: "setField"; payload: { field: string; value: string; }; }
// | { kind: "submit"; payload: { param: string; }; }

I might be far off base, but if I understand what TypeScript compiler does correctly is that the generic default hack forces the indexed access typecheck to, instead of doing the type parameter substitution via a mapper, defer the typechecking and propagating the generic application as a constraint, which when WorkingActionTypes finally gets applied to an actual parameter, goes down the usual mapped type application in instantiateMappedType using this constraint, which has proper distributive union behaviour.

If fixing the substitution behaviour is hard/undesirable due to errors it prevents in other cases, then maybe some annotation to defer typechecking of the generic to the point of application that's not as hacky as generic default could be introduced? Something like (to be bikeshedded of course):

type ActionTypes<Actions> = DeferredGenericApplication<ActionTypesDictionary<Actions>>[keyof Actions];

@KiaraGrouwstra
Copy link
Contributor

KiaraGrouwstra commented Jul 23, 2017

extends Union<StringLiteral>

Yeah, best we can do now is write like extends string /*union*/ or something... wouldn't that do though? I mean, I get why unions can't be expressed that way -- it's expected they would implicitly work everywhere. In practice that might not hold universally (could think of at least one (#17361) but really need to do more testing there) but yeah.

@jaen
Copy link

jaen commented Jul 23, 2017

Yeah, I've learned of extends string after that comment, but as far as I can tell that doesn't distinguish between an arbitrary string value string, a single literal "something" or union of those "something" | "else". I'm not quite sure how that looks in the compiler internals but on the surface, this seems to limit how I can constrain the generic.

@KiaraGrouwstra
Copy link
Contributor

KiaraGrouwstra commented Jul 23, 2017

@jaen:
Yeah, you're right, we can't technically distinguish them. It appears just kind of implied that it'd be useless to use a generic when you're always expecting string instead of literals.

I agree we don't have a great way to communicate to users that you're expecting a union, so I'd just kinda try to reflect that in both the code and the naming, e.g. Keys extends string /*union*/.

I guess you're not really supposed to constrain input types in the sense that everything is supposed to be able to scale to applying en masse to a union of types.
In that line of thinking, I suppose anything where that distributivity law breaks would currently be considered a bug in union behavior (see e.g. the thread I edited into my post above).
I'm not really sure yet how easy this standard would be to live up to (should mostly just work?), but I do think I like the idea.

@mhegazy mhegazy added Bug A bug in TypeScript and removed Needs Investigation This issue needs a team member to investigate its status. labels Aug 23, 2017
@KiaraGrouwstra
Copy link
Contributor

@jaen: Thank again for figuring out what was going on over here; I just caught up.

If I may summarize my understanding of this issue now, the relation between 'generic mapped type' relation between { [P in K]: Box<T[P]> }[X] and Box<T[X]> was presumed to be an equality relation, while in reality it is a sub-type relationship (the latter sub-typing the former), which holds as a legitimate equality relationship iff the distributive property holds over unions of the key for the simplified form, i.e. when type Inner<A, B> defined as Box<T[A | B]> extends type Outer<A, B> defined as Box<T[A]> | Box<T[B]>, I guess with A and B constrained as required by the index constraint as needed.
In reality the latter here sub-types the former again, so that check only holds when they turn out equivalent.

If getIndexedAccessForMappedType can make Box<T[X]> by whatever magic, then I presume it could also make those variations. The stricter check I suppose should be e.g. compareTypesAssignable(innerType, outerType).

@KiaraGrouwstra
Copy link
Contributor

So my hunch is that instead of this:

if (isGenericMappedType(objectType)) {
    return getIndexedAccessForMappedType(<MappedType>objectType, indexType, accessNode);
}

We should do something like this, using pseudo-code to create those generic types as I'm not sure how to do it.

if (isGenericMappedType(objectType)) {
    const innerType = `type Inner<A, B> = ${getIndexedAccessForMappedType(<MappedType>objectType, A | B)}`;
    const outerType = `type Outer<A, B> = ${getIndexedAccessForMappedType(<MappedType>objectType, A)} | ${getIndexedAccessForMappedType(<MappedType>objectType, B)}`;
    if (compareTypesAssignable(innerType, outerType)) {
        return getIndexedAccessForMappedType(<MappedType>objectType, indexType, accessNode);
    }
}

Would anyone know how to do that part of how to declare the types? If so, I'd be curious if this might do the trick. I'm not confident the compiler can pull off that assignability check there, and Murphy's law isn't in favor of my untested pseudo-code, but yeah.

KiaraGrouwstra added a commit to KiaraGrouwstra/TypeScript that referenced this issue Aug 25, 2017
This incidentally also uncovered one other case where generic mapped types had been applied in an unsound way w.r.t. union keys.
@KiaraGrouwstra
Copy link
Contributor

Opened a PR at #18036.

@ahejlsberg
Copy link
Member

@tycho01 Thanks for your contribution. I was separately working on a fix which I just put up. I think it is a more correct way of solving the issue. If you have a chance, could you verify that this fixes the problems you've been seeing?

@ahejlsberg ahejlsberg added the Fixed A PR has been merged for this issue label Aug 25, 2017
@KiaraGrouwstra
Copy link
Contributor

@ahejlsberg: now there's a pleasant surprise! I hadn't even checked my own original use-case anymore, but turns out it was already working on master again anyway. 😅
It looks like your version has fixed a bunch more as well, so no objections here!

@jaen
Copy link

jaen commented Aug 26, 2017

Just letting you know this also fixes my original issue, thanks a lot!

Incidentally, when I try to autocomplete the payload parameter with kind already filled in, the autocompletion seems to suggest fields from either case of union (even though the kind field should limit that). Is that an expected behaviour, or should I open a bug for it?

@jcalz
Copy link
Contributor

jcalz commented Aug 28, 2017

EDIT: IGNORE THE FOLLOWING, issue is fixed in TS v2.6.0-dev.20170826


Now that the fix for this is in (#18042), I tried implementing (in TS v2.6.0-dev.20170824) the type function Transpose, which reverses a mapping of string literals. Example:

// which kinds of ice cream does each person like
type IceCreamPreferences = {
  'alice': 'vanilla' | 'chocolate' | 'strawberry';
  'bob': 'chocolate';
  'carol': 'strawberry' | 'rumRaisin';
  'dave': 'rumRaisin' | 'chocolate';
  'eve': 'tripleFudgeRipple';
}
 
// which people like each kind of ice cream
type TransposedIceCreamPreferences = {
  'vanilla': 'alice';
  'chocolate': 'alice' | 'bob' | 'dave';
  'strawberry': 'alice' | 'carol';
  'rumRaisin': 'carol' | 'dave';
  'tripleFudgeRipple': 'eve';
}

Here is a version of Transpose that still doesn't work:

// union of possible value types
type ValueOf<T> = T[keyof T];

// subtract unions of string literals
type Diff<T extends string, U extends string> = (
  {[K in T]: K} &
  {[K in U]: never} &
  { [K: string]: never }
)[T];

type Transpose<T extends Record<string, string>> = ValueOf<{
  [P in keyof T]: Record<Diff<ValueOf<T>, T[P]>, never> & Record<T[P], P>
}> // broken!  

type WhoLikes = Transpose<IceCreamPreferences>;
var chocolateLover: WhoLikes['chocolate'];
chocolateLover = 'alice'; // okay
chocolateLover = 'bob'; // okay
chocolateLover = 'carol'; // 🙁 should error, but doesn't! 
chocolateLover = 'dave'; // okay
chocolateLover = 'eve'; // 🙁 should error, but doesn't! 

Something is still doing an eager substitution where I don't expect it.

Luckily the default-generic workaround does work here:

type Transpose<T extends Record<string, string>, X = {
  [P in keyof T]: Record<Diff<ValueOf<T>, T[P]>, never> & Record<T[P], P>
}> = ValueOf<X> // works

type WhoLikes = Transpose<IceCreamPreferences>;
var chocolateLover: WhoLikes['chocolate'];
chocolateLover = 'alice'; // okay
chocolateLover = 'bob'; // okay
chocolateLover = 'carol'; // 🙂 error, carol doesn't like chocolate
chocolateLover = 'dave'; // okay
chocolateLover = 'eve'; // 🙂 error, eve doesn't like chocolate

So, the question is: is this the same issue or a new issue? If it's a new one, I'll open one for it. Also, are we dealing with a bug/limitation or is this how people want TS to behave?

@KiaraGrouwstra
Copy link
Contributor

@jcalz: still looks like a bug, rather than desired behavior.
I'd recommend opening a new issue, as I'm not sure they'd still recheck this one after closing it.
Please link to it from here though, then we know which one to follow for this.

@jcalz
Copy link
Contributor

jcalz commented Aug 29, 2017

Looks like I tested a version of TS without all the relevant commits. The above issue does not appear in 2.6.0-dev.20170826. Transpose is good to go!

@jcalz jcalz mentioned this issue Oct 11, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

8 participants