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

Bug with mapped types and indexing #17238

Closed
SimonMeskens opened this issue Jul 17, 2017 · 25 comments
Closed

Bug with mapped types and indexing #17238

SimonMeskens opened this issue Jul 17, 2017 · 25 comments
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@SimonMeskens
Copy link

SimonMeskens commented Jul 17, 2017

Minimal repro:

type AB = {
    a: 'a'
    b: 'a'
}

type T1<K extends keyof AB> = { [key in AB[K]]: true }
type T2<K extends keyof AB> = T1<K>[K] // BUG: should be an error for K = 'b'

I came across this in production code and made a contrived example to show the issue. Might be related to #15756.

Note that there might be two bugs here in play, one with mapped types and one with lookup types on mapped types. Once I start pushing the types a little, I come across a wild range of unexpected behavior.

Pay attention to the last type 'G', where the compiler is suddenly correct again, which is probably the oddest one of all.

// Let's make a simple type

type StringA = "a"

type A = {
  a: StringA
  b: StringA
}

// so type A[S] should always resolve to "a" for any valid keys
// note that it doesn't collapse T, even though it could

type T<S extends "a" | "b"> = A[S] // expands A, but doesn't collapse the indexer

let t1: T<"a"> // t1: "a"
let t2: T<"b"> // t1: "a"

// Let's make a silly generic type that only keeps key "a"
// The compiler correctly infers that this new type is just { a: true }

type B<S extends "a" | "b"> = {[key in "a"]: true} // B = { a: true }

// Let's make the type more generic, remember that A[S] is actually just "a"
// The compiler now infers that the type of C is {}, this is plain wrong

type C<S extends "a" | "b"> = {[key in A[S]]: true} // C = {}

// Obviously, we couldn't do a lookup on such a type
// Compiler correctly complains that we can't index an empty object

type D<S extends "a" | "b"> = {}[S] // Type 'S' cannot be used to index type '{}'.

// Now let's add a lookup to the generic version
// Curiously, it doesn't fail this time, but infers 'true', how odd

type E<S extends "a" | "b"> = {[key in A[S]]: true}[S] // = true

// It even happens when we index the above type C, which the compiler claims is {}

type F<S extends "a" | "b"> = C<S>[S] // = true

// Let's also try to use this type, it will become important later
// This does exactly what's expected

let a1: F<"a"> // a: true
let b1: F<"b"> // b: true

// Now let's make it even weirder and add a general indexer
// This correctly doesn't fail as any key is now valid
// The type it infers though, is just plain weird:
// G = ({} & { [key: string]: false; })[S]

type G<S extends "a" | "b"> = (
  C<S> &
  {[key: string]: false}
)[S]

// And now the weirdest thing of all, let's use this type
// It correctly infers the result! Both of these are correct!

let a2: G<"a"> // a: true
let b2: G<"b"> // b: false

This has to be the weirdest bug I've run across

@gcnew
Copy link
Contributor

gcnew commented Jul 17, 2017

It looks like you are experiencing the effects of #12351. The strange behaviour is happening because some of the usecases are expanded, while others are handled on the logical level, without carrying out the heavy lifting.

@olegdunkan
Copy link

olegdunkan commented Jul 19, 2017

type C<S extends "a" | "b"> = {[key in A[S]]: true} - maybe in this line of code the compiler doesn't evaluate A[S] because of some of sort of lazy evaluation, why does it have to go deeper and analyze A[S] ? But when you type E<S extends "a" | "b"> = {[key in A[S]]: true}[S] you force compiler to find out exactly what are inside A<T>. It is my assumption.

@gcnew
Copy link
Contributor

gcnew commented Jul 19, 2017

Because of #12351

The operation { [P in K]: T}[X] is equivalent to an instantiation of T where X is substituted for every occurrence of P. For example, { [P in K]: Box<T[P]> }[X] is equivalent to Box<T[X]>.

type E<S extends "a" | "b"> = {[key in A[S]]: true}[S]

is converted to just

type E<S extends "a" | "b"> = true

by applying the above rule, without doing any actual "mapping" or "selection".

PS: You can see the rule is in action, because "extra" keys are silently ignored:

type E<S extends "a" | "b" | "extra"> = {[key in A[S]]: true}[S] // no error

@gcnew
Copy link
Contributor

gcnew commented Jul 19, 2017

As strange as it may seem

type C<S extends "a" | "b"> = {[key in A[S]]: true} // C = {}

is actually correct. The reason is that S extends "a" | "b" means that S can be either

  • never
  • "a"
  • "b"
  • "a" | "b"

TypeScript plays it safe here and chooses never. The "expected" type is returned when C is instantiated with a specific value for S.

declare const x: C<"a"|"b">; // { a: true } 

Note that the same applies for S extends keyof A.

@SimonMeskens
Copy link
Author

I disagree that choosing never is ever the right choice. Every type has the option of becoming never. If we assume that, then every inferred type everywhere should become never.

Also, it should never collapse a parametrized type, as you can just wait until it's used and check what the correct value is. A type constraint is just that, a constraint. It should pick the widest type possible for constraints, so in this case, it should pick "a" | "b" as long as possible.

In any case, this behavior is clearly buggy and needs to be fixed.

@gcnew
Copy link
Contributor

gcnew commented Jul 19, 2017

I disagree. The compiler should (and does) choose the most neutral type. In this case, because at use site the S parameter might be any of the above mentioned options, the only safe one is the least restrictive one - i.e. never. If another option was chosen, it would have forced every use site to provide the pertinent properties. If all properties is the intention the right operator is keyof. When talking about sets of options, the only choice that makes no assumptions is the empty set.

@SimonMeskens
Copy link
Author

So how would you expect someone to write a parameter constrained by string keys? S extends keyof { a, b }?

Would that have different behavior from S extends "a" | "b"? Sounds like that would still cause buggy behavior, as keyof { a, b } infers to "a" | "b" when collapsed in other places.

@gcnew
Copy link
Contributor

gcnew commented Jul 19, 2017

I'm not sure I understand you. The parameter is still constrained to the said set of keys, it's just that intelli-sense chooses one of the valid options which is the empty set. In subsequent uses of the alias, both value and type level, it works correctly. I'm not convinced that choosing the fullest set in intelli-sense would be better.

@SimonMeskens
Copy link
Author

Ah, you're just talking about intellisense? I really don't agree. Intellisense should always display the widest possible type. Otherwise you run into cases where intellisense tells you a certain type is {} and then a line below it, suddenly it widens to a different type.

That doesn't change the fact that there's a clear bug going on here though, that needs to be fixed.

@gcnew
Copy link
Contributor

gcnew commented Jul 19, 2017

Which one do you feel is a bug?

From the comments I'm left with the impression that

type C<S extends "a" | "b"> = {[key in A[S]]: true} // C = {}

and

type E<S extends "a" | "b"> = {[key in A[S]]: true}[S] // = true

are the ones you feel are odd, but I think I've explained them in #17238 (comment) and #17238 (comment).

Edit: I've updated wording and gave more context.

@SimonMeskens
Copy link
Author

I think the example above provides ample amount of bugs, but the main one is this:

type B<S extends "a" | "b"> = {[key in "a"]: true} // B = { a: true }
type C<S extends "a" | "b"> = {[key in A[S]]: true}
type F<S extends "a" | "b"> = C<S>[S] // = true

let a1: F<"a"> // a: true
let b1: F<"b"> // b: true

How can b1 ever be true? That's just an outright bug. Type C is exactly the same as type B, because A[S] is always "a". So type C is { a: true }. b1 tries to index with an index that doesn't even exist on the type it's indexing.

@gcnew
Copy link
Contributor

gcnew commented Jul 19, 2017

b1 tries to index with an index that doesn't even exist on the type it's indexing.

You use double indexing. b resolves to an "a".

type StringA = "a"

type A = {
  a: StringA
  b: StringA
}

@gcnew
Copy link
Contributor

gcnew commented Jul 19, 2017

OK, I see. Yes, it is a bug. The problem is what happens here:

PS: You can see the rule is in action, because "extra" keys are silently ignored:

type E<S extends "a" | "b" | "extra"> = {[key in A[S]]: true}[S] // no error

A[S] should still be checked for compatibility before applying the "substitution" rules.

@SimonMeskens
Copy link
Author

SimonMeskens commented Jul 19, 2017

I'll add one more example to make it clear:

type F<S extends "a" | "b"> = C<S>[S] // = true

let a1: F<"a"> // a: true
let b1: F<"b"> // b: true

type G<S extends "a" | "b"> = (
  C<S> &
  {[key: string]: false}
)[S]

let a2: G<"a"> // a2: true
let b2: H<"b"> // b2: false

Adding a bogus indexer to F makes it give different results. I'll edit G above with this version to make it clearer.

@SimonMeskens
Copy link
Author

SimonMeskens commented Jul 19, 2017

I had a small typo in the above example, fixed it, should say C<S>, not F<S>

And you are correct that if E is correctly discarded as an error, the rest of the bugs resulting from that behavior wouldn't matter. I still wanted to document all the behavior as I suspect there is more than one bug at play here.

@gcnew
Copy link
Contributor

gcnew commented Jul 19, 2017

Minimal repro:

type AB = {
    a: 'a'
    b: 'a'
}

type T1<K extends keyof AB> = { [key in AB[K]]: true }
type T2<K extends 'a'|'b'> = T1<K>[K] // BUG 1: should be error for K = 'b'

type R = AB[keyof AB]; // "a"
type T3 = { [key in R]: true }
type T4<K extends 'a'|'b'> = T3[K] // error as expected

// BUG 2: 'extra' not checked in AB[S]
type T5<S extends 'a'|'b'|'extra'> = {[key in AB[S]]: true}[S]

@gcnew
Copy link
Contributor

gcnew commented Jul 19, 2017

BUG 2 might have the same cause as #17297.

@gcnew
Copy link
Contributor

gcnew commented Jul 19, 2017

@SimonMeskens E is definitely a bug. It's a bit hard to spot though.

@gcnew
Copy link
Contributor

gcnew commented Jul 19, 2017

@SimonMeskens Could you include

type AB = {
    a: 'a'
    b: 'a'
}

type T1<K extends keyof AB> = { [key in AB[K]]: true }
type T2<K extends keyof AB> = T1<K>[K] // BUG: should be an error for K = 'b'

in the main post as this is the minimal repro of the problem? I think it will help for clarity.

@SimonMeskens
Copy link
Author

Will do

I was going to add your previous one with the second bug too. Or should we open a new issue for that? Can you recommend a better title for the bug report too?

@gcnew
Copy link
Contributor

gcnew commented Jul 19, 2017

The second bug seems to be an incarnation of the same core bug as b is still an extra key. About the title, I think the current one is fine.

@SimonMeskens
Copy link
Author

Thanks for the help, I didn't know how to get it down to a minimal repro.

@jaen
Copy link

jaen commented Jul 20, 2017

Out of curiosity, does it indeed relate to #15756 as the OP assumes or is it s disparate issue? Seems related, because it seems like the cause is the simplistic substitution of the type variable (if I understand the thread correctly), but I'm not quite sure.

@gcnew
Copy link
Contributor

gcnew commented Jul 21, 2017

@SimonMeskens I've put a PR #17336 fixing the issues found in this thread. It turned out that BUG 2 had a different cause and was indeed a second problem.

@SimonMeskens
Copy link
Author

Oh, that's great man! Thanks!

@DanielRosenwasser DanielRosenwasser added the Bug A bug in TypeScript label Aug 2, 2017
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 2.5 milestone Aug 2, 2017
@ahejlsberg ahejlsberg added the Fixed A PR has been merged for this issue label Aug 2, 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

Successfully merging a pull request may close this issue.

6 participants