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

T[keyof T] should be never for T={} #22042

Closed
yortus opened this issue Feb 20, 2018 · 10 comments
Closed

T[keyof T] should be never for T={} #22042

yortus opened this issue Feb 20, 2018 · 10 comments
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@yortus
Copy link
Contributor

yortus commented Feb 20, 2018

TypeScript Version: 2.8.0-dev.20180216

Search Terms:

  • indexed access type
  • keyof
  • never

Code

type A = {};
type B = A[keyof A];    // B = any
        // ^^^^^^^      // ERROR: 'never' cannot be used as an index type

Expected behavior:
A[keyof A] is a valid indexed access type that evaluated to never.

Actual behavior:
A[keyof A] is treated as an error, and evaluates to any (presumably due to compiler bailout).

Related Issues:

#11929 describes indexed access types as follows:

An indexed access type T[K] requires K to be a type that is assignable to keyof T [...] and yields the type of the property or properties in T selected by K.

In this case K=never and T={}, so K is assignable to keyof T, so T[K] appears to be a valid type.

The union of zero property keys is never, as tsc shows with keyof {} = never.

So the result should be the union of zero property types, i.e. also never.

Consequences:

In more complex generics this leads to meaningful problems, e.g. in #21988 where RequiredProps<{}> evalutes to {[x: string]: any} when it should be {}.

@yortus yortus changed the title T[keyof T] should be valid for T={} T[keyof T] should be never for T={} Feb 20, 2018
@sandersn
Copy link
Member

Note that adding

if (indexType.flags & TypeFlags.Never) {
  return neverType;
}

in getPropertyTypeForIndexType does not cause any tests to fail, but causes the example to pass. I think that means our test coverage is pretty bad for index types. I'll try compiling the RWC or user tests to see if the types change in some react projects.

@sandersn
Copy link
Member

sandersn commented Feb 20, 2018

There is one break in Large Microsoft Project 199 (the one with a blue logo. no, the other one):

// NOT THE ACTUAL CODE
import _ = require('lodash');
interface A {
  m(): void;
}
const xs = { [n: number]: A } = {};
// other code ...
function all() {
  _.each(xs, x => x.m());
}

x's inferred type is never now where it was previously any. Here's an altered example that doesn't depend on lodash:

type A = { p: number }
type It<T, U> = (value: T[keyof T]) => U;
declare function _each<T>(collection: T, iteratee: It<T, any>): void;
function allofem(xs: { [n: number]: A }) {
    _each(xs, x => x.p)
}

@yortus
Copy link
Contributor Author

yortus commented Feb 21, 2018

Thanks for looking at it @sandersn. Pity it's a breaking change, although that must be incredible rare.

So do you think this is a bug or a suggestion? I think it's a bug, since T[never] is implicitly covered by the definition in #11929 but doesn't work as specified there.

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Feb 21, 2018
@sandersn
Copy link
Member

@yortus I don't think it's clear whether it's a bug or a suggestion. I'm leaning toward bug, but either way we need to look at the pros and cons of fixing it.

Re:rarity. I'm not sure about incredibly rare — lodash had 2.5 million downloads yesterday and @types/lodash had over 100,000. Anybody who calls _.each with a number-indexed type will get this bad behaviour. (I think they probably wanted to call the T[] overload in the first place, but it may exhibit the same behaviour).

Of course we can probably fix lodash to work around any changes that we end up making. The question is whether other code will run into it.

@yortus
Copy link
Contributor Author

yortus commented Feb 22, 2018

@sandersn right - actually I see that T[keyof T] comes up 90 times across the lodash definitions.

But isn't this showing up a different problem with keyof? Why does keyof {[n: number]: any} eveluate to never? Shouldn't it be string? The numeric indexer is just telling TypeScript that the objects keys are constrained to numbers only, but they are still effectively all string-typed keys.

Since keyof {1: 1} is "1" and keyof {1:1, 2:2, ...99: 99} is "1"|"2"|..|"99", then by extension keyof {[n: number]: number} should be string right? At least it should not be never, since we are telling TypeScript there are string keys in there.

If keyof was fixed for numeric indexers, then the _.each example above would work properly regardless of this T[K] issue.

@yortus
Copy link
Contributor Author

yortus commented Feb 22, 2018

I opened a new issue at #22105 regarding keyof {[n: number]: any}.

@yortus
Copy link
Contributor Author

yortus commented Mar 8, 2018

@sandersn everything works if you preserve the special handling of types with string/numeric indexers in getPropertyTypeForIndexType (which are special cases according to #11929).

If you handle the T[never] case after the special handling for string/numberic indexers, then this change won't break things like lodash anymore, and things like RequiredProps<{}> (#21988) will get better typing.

Actually if done this way, wouldn't it be a strictly non-breaking change, since the only changed behaviour is for cases that are currently compile-time errors?

@sandersn
Copy link
Member

sandersn commented Mar 8, 2018

@yortus Thanks for the additional investigation. I'm heads down on Javascript work right now, but I'll come back to this when I have time, or if you want to submit the PR, that would work too.

@kpdonn
Copy link
Contributor

kpdonn commented Mar 22, 2018

Heads up I ran into this problem also, and based on everything you guys said it sounded pretty simple to fix so I created a PR at #22787.

@RyanCavanaugh
Copy link
Member

@ahejlsberg have a look at the associated PR?

@mhegazy mhegazy added this to the TypeScript 2.9 milestone Mar 27, 2018
@mhegazy mhegazy added Fixed A PR has been merged for this issue and removed In Discussion Not yet reached consensus labels Mar 27, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

5 participants