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

keyof any incorrectly maps (only) to type string #21983

Closed
ogwh opened this issue Feb 15, 2018 · 25 comments · Fixed by #23592

Comments

@ogwh
Copy link

commented Feb 15, 2018

TypeScript Version: 2.7.1 and 2.8.0-dev.20180215

Search Terms: "keyof any"

Code

function foo<T>(bar: T, baz: keyof T) {
        console.log(bar[baz]);
}

const sym = Symbol();
const quirk = { [sym]: "thing" };

foo<any>(quirk, sym);

Expected behavior:
keyof any should be equivalent to PropertyKey (that is string | number | symbol)

The example code should compile without error.

Actual behavior:
keyof any is equivalent to string

The example code does not compile and produces error:
error TS2345: Argument of type 'unique symbol' is not assignable to parameter of type 'string'.

Playground Link:
https://www.typescriptlang.org/play/#src=function%20foo%3CT%3E(bar%3A%20T%2C%20baz%3A%20keyof%20T)%20%7B%0D%0A%20%20%20%20%20%20%20%20console.log(bar%5Bbaz%5D)%3B%0D%0A%7D%0D%0A%0D%0Aconst%20sym%20%3D%20Symbol()%3B%0D%0Aconst%20quirk%20%3D%20%7B%20%5Bsym%5D%3A%20%22thing%22%20%7D%3B%0D%0A%0D%0Afoo%3Cany%3E(quirk%2C%20sym)%3B

@yortus

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2018

If symbolof (#20721) was supported, you could write it like this:

function foo<T>(bar: T, baz: keyof T | symbolof T) {
        console.log(bar[baz]);
}

const sym = Symbol();
const quirk = { [sym]: "thing" };

foo(quirk, sym);
@Jessidhia

This comment has been minimized.

Copy link

commented Feb 16, 2018

(There are no number property keys. All non-symbol property keys are either strings or numeric strings.)

@ogwh

This comment has been minimized.

Copy link
Author

commented Feb 16, 2018

Whilst there are technically no number keys in reality they are often used and it would be considered inappropriate to call toString every time you wanted to index an array. In addition we already have PropertyKey defined as the three types.

It makes sense to introduce some consistency and use PropertyKey everywhere a key could reasonably be used.

That would include in interface definitions and in keyof any.

Right now keyof any is string, interface index keys are string|number, PropertyKey is string|number|symbol and in actual usage of an object we use PropertyKey.

This isn’t a duplicate because I’m proposing that the inconsistency is the bug and symbolsof would do nothing useful but increase the chaos that we’re already faced with!

@Jessidhia

This comment has been minimized.

Copy link

commented Feb 16, 2018

I just think it's not appropriate as a widened type for the result of keyof.

@ogwh

This comment has been minimized.

Copy link
Author

commented Feb 16, 2018

How come?

@ogwh

This comment has been minimized.

Copy link
Author

commented Feb 16, 2018

I've just come up with a better example that demonstrates the inconsistency more clearly:

const foo: any = {};

// Fine
const symKey: symbol = Symbol();
foo[symKey] = null;

// Error
const keyOfKey: keyof any = symKey;

// Also error
const otherKeyOfKey: keyof typeof foo = symKey;

// Compiler output
index.ts(8,7): error TS2322: Type 'symbol' is not assignable to type 'string'.
index.ts(11,7): error TS2322: Type 'symbol' is not assignable to type 'string'
@ogwh

This comment has been minimized.

Copy link
Author

commented Feb 16, 2018

Another options would be to make keyof any equivalent to any. Because any is essentially a way to turn off type checking.

@yortus

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2018

As for making keyof return symbols.... Object.keys only returns string keys at runtime, and many other JS constructs only operate over string keys. I think we may do symbolof operator to maintain compatibility and keep a distinction between the two namespaces.

@ogwh that was in @weswigham's comment for why keyof should be just strings.

As for keyof any, since this is not referring to the keys of any particular object, why not just use the PropertyKey type in your annotation instead?

@ogwh

This comment has been minimized.

Copy link
Author

commented Feb 16, 2018

I was about to agree but thought it through a bit more. :-P

To answer your question @yortus I can't use PropertyKey because in the vast majority of cases the type T is known and so to use anything but keyof T would introduce the possibility of typos.

There is no reason for keyof to maintain consistency with Object.keys() because it's only used to ensure type safety. There is an argument that the ES spec is flawed and 7.3.21 point 4.a. should be changed to include symbols but sure that might break existing code.

In effect the type checking provided by keyof is plain and simply wrong if it doesn't include symbol keys. The only times it will be used is to ensure nobody tries to improperly index an object. Right now it's preventing people from properly indexing an object.

Also the ECMAScript spec (current draft) is inconsistent itself on this:

6.1.7.3 Invariants of the Essential Internal Methods

In this section we have a declaration that keys are either strings or symbols.

[[OwnPropertyKeys]] ( )
The return value must be a List.
The returned list must not contain any duplicate entries.
The Type of each element of the returned List is either String or Symbol.
The returned List must contain at least the keys of all non-configurable own properties that have previously been observed.
If the object is non-extensible, the returned List must contain only the keys of all own properties of the object that are observable using [[GetOwnProperty]].

Other sections that assert this include (I didn't do an exhaustive check):
https://tc39.github.io/ecma262/#sec-topropertykey
https://tc39.github.io/ecma262/#sec-ispropertykey

So in effect we have a standard that accepts property keys are strings and symbols, a backwards compatibility (presumably) to ensure legacy JS code doesn't break at runtime and a type checker that is observing the legacy runtime characteristics rather than the actual type characteristics of properties. The type checker itself also conceding (by the definition of PropertyKey) that a generic property of any old object should conform to compilation-time type checking expectations and not runtime expectations of legacy code.

Legacy JS doesn't benefit from Typescript enforcing what is already enforced by the runtime, yet Typescript code suffers because of the inconsistency outlined in the examples already given.

With the element type of the array returned by Object.keys always being a subset of the types of PropertyKey there's no danger there. But by keyof T being a subset of the possible actual property key types we end up with broken type checking that excludes possible values.

Finally symbolof has no practical purpose once keyof is fixed. If it were to be introduced then it would be equally valid to introduce stringof to complement it if it's accepted that keyof is currently broken.

Phew

Edit: I apparently wrote keysof in some places instead of keyof.

@ogwh

This comment has been minimized.

Copy link
Author

commented Feb 16, 2018

Ruminating on this I don't think Object.keys is ever supposed to return symbols. I think the word key is being used in reference to keys in the sense of a map and not in the sense of an actual JS object property.

That being a consequence of objects being used traditionally as maps. So we have two different usages of the word "key".

If we look at section 19.1.2.8 we see the proper definition of key used in reference to the getOwnPropertyDescriptors implementation:

19.1.2.8 Object.getOwnPropertyDescriptors ( O )

When the getOwnPropertyDescriptors function is called, the following steps are taken:

Let obj be ? ToObject(O).
Let ownKeys be ? obj.[OwnPropertyKeys].
Let descriptors be ! ObjectCreate(%ObjectPrototype%).
For each element key of ownKeys in List order, do
Let desc be ? obj.[GetOwnProperty].
Let descriptor be ! FromPropertyDescriptor(desc).
If descriptor is not undefined, perform ! CreateDataProperty(descriptors, key, descriptor).
Return descriptors.

Edit: OwnPropertyKeys being a reference to section 6.1.7.3 from my previous comment.

@dinoboff

This comment has been minimized.

Copy link

commented Feb 16, 2018

I don't mind having keyOf and symbolOf for backward compatibility but can we also have anyKeyOf which would return both type of keys because I can't think of a use case where I would want to use symbolOf on its own.

@robbiespeed

This comment has been minimized.

Copy link

commented Feb 16, 2018

Having keyof only refer to string keys can be useful, introducing symbolof seems to be the right way to go. My guess is most of the time people are going to need keyof more than symbolof anyway. If someone consistently needs both they can just create a type alias type SymbolKeyOf<T> = keyof T | symbolof T.

@ogwh

This comment has been minimized.

Copy link
Author

commented Feb 17, 2018

What's your reasoning behind:

  1. Most of the time people are going to need keyof more than symbolof; and
  2. Having keyof only refer to string keys can be useful

If that is the case then it seems to me to be more explicit/clear to have keyof, stringof and symbolof?

Thanks :-)

@robbiespeed

This comment has been minimized.

Copy link

commented Feb 18, 2018

  1. Symbols are usually used to store hidden values on an object, and are used far less than string keys.
  2. keyof as is can be useful when using Object.keys or Object.entries which do not return symbols as keys.

keyof has already been declared as string keys, and is being used as such, why break it? It also makes sense seeing as Object.keys refers only to strings.

Can you provide a use case where you would need to use keyof T | symbolof T many times throughout? Is there a instance where an alias wouldn't be enough?

@ogwh

This comment has been minimized.

Copy link
Author

commented Feb 18, 2018

  1. That's kind of my point! :-P Being able to pass a symbol to a function as a key to an object to access a hidden (symbol) property
  2. Object.keys and Object.entries always return collections where object keys are strings, what's the use case where keyof would come in to play? I haven't been able to come up with one.

keyof being extended to include all of the keys of an object wouldn't break anything. You can't use any of the info from it at runtime, it's only used for type checking (which it's currently getting wrong). The current behaviour kind of is broken because it's giving compiler errors erroneously.

Yeah I could use an alias. But why use an alias when keyof is broken and fixing it won't (can't) break anything? My argument isn't one of convenience, it's that keyof is fundamentally broken as it stands at the moment.

The hard question is: how should symbols be identified in the string representation of the type generated by keyof? (This also applies to symbolof so perhaps someone has an answer over there.)

Edit: I realise I've called it both "kind of" and "fundamentally" broken. I'm trying not to come across as a knuckhead which is difficult without body language! It is fundamentally broken (imho). :-P

@ogwh

This comment has been minimized.

Copy link
Author

commented Feb 20, 2018

I've found another bug that emerges as a side effect to this. Because symbols aren't included in keyof a Readonly<T> where T implements the iterable protocol can't be iterated over:

class MyIterable {
    [Symbol.iterator]() {
        const it = (function*() {
            for (let s of ["foo", "bar"]) yield s;
        })();
        return it;
    }
}

const collection = new MyIterable();
const readonlyCollection: Readonly<MyIterable> = MyIterable;

for (let value of collection) {
    console.log(value);
}

for (let value of readonlyCollection) {
    console.log(value);
}

tsc output:

index.ts(17,19): error TS2488: Type must have a '[Symbol.iterator]()' method that returns an iterator.
@yortus

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2018

@ogwh that could be fixed either way: either by (a) adding symbols to keyof, or by (b) adding symbolof and redefining Readonly<T> to use keyof T | symbolof T.

So yes, that is a problem with the current definitions, but its not necessary an argument for one approach over the other.

@ogwh

This comment has been minimized.

Copy link
Author

commented Feb 20, 2018

Perhaps not, but I think I've already made a strong case! That addition is just supplementary! ;-)

My biggest fear is that it becomes a case of tl;dr! >_<

@ogwh

This comment has been minimized.

Copy link
Author

commented Feb 20, 2018

For the busy people I'll summarise:

  1. The spec defines a property key as string or symbol
  2. Typescript defines PropertyKey as string | number | symbol even though number isn't a valid indexing type
  3. From 2 we can infer that Typescript favours type sanity, which is good (no obj[num.toString()])
  4. The spec defines Object.keys as returning string[]
  5. Some claim that parity with Object.keys is important, this seems to be based on the word key
  6. The "keys" in Object.keys refers to the specific usage of "key" when an object is used as a hash table; as in HashTableKey (made up) vs PropertyKey (which it isn't)
  7. Object.keys already differs from the type of a property key (it's a single-item subset)
  8. Through 2, 3, 4, 6 and 7 the argument that keyof matches Object.keys is disproven (as is point 5)
  9. The type information from keyof is only available at compile time currently
  10. Because symbols are guaranteed to be unique there's no possibility of it resolving to too large a set of values that may be used accidentally
  11. Through 4, 5 and 6 we have backwards compatibility if keyof is changed now
  12. symbolof has no use case except as a hack around the broken keyof
  13. keyof without symbols is already a source of bugs in Typescript itself (Readonly<t> from my previous example)
  14. Typescript is a superset of Javascript, refer to 1 :-P

I think that covers everything.

@robbiespeed

This comment has been minimized.

Copy link

commented Feb 20, 2018

@ogwh Here's a simple example of how expanding keyof to include symbols could break existing code

const sym = Symbol();

function values <T, K extends keyof T>(obj: T) {
  return Object.keys(obj).map((key: K) => {
    return obj[key];
  });
}

const exampleObj = {
  a: 1,
  b: 2,
  [sym]: '',
};

const numbers: number[] = values(exampleObj);

Currently works perfectly, but if symbols were included you'd get the resulting type error

Type '(string | number)[]' is not assignable to type 'number[]'.

When in reality that would not be the correct type to return.

In response to some of your summary:

  1. I think I've shown an example that proves this is not something based merely on wording. For a long time there were no symbols in JS, hence why when talking about objects, a key generally refers to a string property (even if technically the spec defines it as either string or symbol).
  2. (+ 7, 8) I'm afraid I don't get what your point is here. Though you are correct that Object.keys does not directly correlate to keyof, in most cases though usage of keyof as the return type of Object.keys does work (and is useful).

  1. My example above shows your proposal would be a breaking change.
  2. symbolof would not break anything, and allow us to do everything we would need to

One last note about expanding keyof
If one were to try and separate keys into 2 types of symbol and string:

interface O {
  a; b; c;
  [sA]; [sB];
}
type OStringKeys = keyof O & string;
type OSymbolKeys = keyof O & symbol;

it would result in this mess:

type OStringKeys = ("a" & string) | ("b" & string) | ("c" & string) | (unique symbol & string) | (unique symbol & string)
type OSymbolKeys = ("a" & symbol) | ("b" & symbol) | ("c" & symbol) | (unique symbol & symbol) | (unique symbol & symbol)

if we left keyof alone and added symbolof, we'd get:

type OStringKeys = keyof O = "a" | "b" | "c"
type OSymbolKeys = symbolof O = unique symbol | unique symbol
@ogwh

This comment has been minimized.

Copy link
Author

commented Feb 20, 2018

The example you gave currently results in a compilation error because string can't be assigned to the more restricted type K. This goes back to 5, 6, 7 & 8 which still hold and invalidate 11.

It seems that tsc still emits output when there's an error in the file (version 2.7.2), and it also omits the error message when there are errors in other files. It's very bizarre behaviour and seems like a separate bug. But I'm guessing that tricked you! Try the example you gave in a clean environment and you'll get the same error I've included at the end of this comment.

12 is true but it's still a hack around the broken keyof, I still don't see any need for symbolof once keyof is brought in line with the JS spec definition of a property key.

Moreover anywhere a keyof type is used currently it will be a subset of string, so it can always be assigned to string but never vice-versa without a cast. For that reason expanding keyof can't break existing code, the worst it can do is make it easier to introduce the following bug:

const foo = Symbol();
const bar = {
  "foo": "baz",
  [foo]: "quirk"
};

function doThing<T, K extends keyof T>(obj: T, key: K) {
  // Do the thing
}

doThing(bar, foo); // Bug here
// The coder meant "foo" but left out the quotes, currently this results in a type mismatch.
// However with the change the foo symbol is perfectly valid.
// But, this bug is still possible currently and just as likely as demonstrated by:
const value = bar[foo];

Copied from your example:

function values<T, K extends keyof T>(obj: T) {
    return Object.keys(obj).map((key: K) => {
        return obj[key];
    });
}

We currently get the error:

test.ts(2,33): error TS2345: Argument of type '(key: K) => T[K]' is not assignable to parameter of type '(value: string, index: number, array: string[]) => T[K]'.
  Types of parameters 'key' and 'value' are incompatible.
    Type 'string' is not assignable to type 'K'.

I kid you not that's the actual error code too btw!

@ogwh

This comment has been minimized.

Copy link
Author

commented Feb 20, 2018

There is a breakage when we go in the other direction:

function foo(bar: string) {}
const sym = Symbol();
class Thing { [sym] = 42 };
const key: keyof Thing = sym;
foo(sym); // Error here

Damn! It would be correct, and would only affect types that already have symbol properties so it's probably very rare but it is no less a breaking change.

That's so disappointing. Can we change it anyway?! :'(

@robbiespeed

This comment has been minimized.

Copy link

commented Feb 20, 2018

@ogwh it is producing an error because you've enabled strict function types, and is due to the fact that Object.keys does not return the type (keyof T)[], there was a proposal to have it do so, but that wouldn't be correct in all scenarios (discussion on that).
Here's a updated version with strict function types enabled (Also tested locally with 2.7.2)

@ogwh

This comment has been minimized.

Copy link
Author

commented Feb 20, 2018

Thanks for the clarification, that makes sense. I've filed a new bug in relation to the JS spec which I believe is what has lead to the current situation. There's a reference notification for that just before this comment.

I'm fairly sure we can't change this in TS 2, but perhaps once the spec is clear it could be something to change in TS 3?

@typescript-bot

This comment has been minimized.

Copy link
Collaborator

commented Mar 7, 2018

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@mhegazy mhegazy added this to the TypeScript 2.9 milestone Apr 23, 2018

@mhegazy mhegazy added Fixed and removed Duplicate labels Apr 23, 2018

@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
8 participants
You can’t perform that action at this time.