Flow: What’s going on with `Object.keys`?

[wchargin]

The core libdef for Object.keys is clear as can be:

declare class Object {
    // [snip]
    static keys(o: any): Array<string>;
    // [snip]
}

(https://github.com/facebook/flow/blob/172d28f542f49bbc1e765131c9dfb9e31780f3a2/lib/core.js#L58)

This suggests that the following code should have a type error:

// @flow
type K = "A" | "B";
const x: {[K]: number} = {};
(Object.keys(x): Array<K>);  // `string` should be incompatible with `K`

And yet the above code typechecks. Why?

This was not always the case. The above code did not typecheck in Flow v0.59.0, but has typechecked since Flow v0.60.0. But the release notes for v0.60.0 don’t mention such a change.

Also, this does _not_ work if the key type is opaque (_possibly_ related to #6569 and friends):

// @flow
declare opaque type O: string;
const y: {[O]: number} = {};
(Object.keys(y): Array<O>);  // fails (sad)

Having the refined type is quite useful, though the grievances in #5210 (from before the change) are legitimate. What is going on? Whence the inconsistency between the libdef, the working example with a transparent subtype of string, and the failing example with an opaque subtype of string?

(Full Flow-Try here.)

[dsainati1]

Object.keys is actually implemented internally using a special type constructor that basically ignores the libdef definitions. Why it's done this way, I don't know, but I'd bet the definition can be removed.

[jcready]

Object.keys is actually implemented internally using a special type constructor that basically ignores the libdef definitions.

Why is it implemented using an internal type constructor? If there's something special about Object.keys why not change the libdef syntax to support it? If I create a third-party flow-typed libdef for a module which just re-implements the Object.key functionality how should I type it?

[wchargin]

Object.keys is actually implemented internally using a special type
constructor that basically ignores the libdef definitions. Why it's
done this way, I don't know, but I'd bet the definition can be
removed.

Lovely. Can we provide a typing that approximates the behavior (probably
the current one is fine), and include a clear comment indicating (a)
that the typing is actually ignored, (b) where in the OCaml code the
actual implementation is, and (c) roughly what the semantics are
(because obviously they are unclear)? Simply removing the definition is
probably better than including an incorrect definition, yes, but the
libdef is where I turn when things don’t work how I’m expecting them to,
and it would be just as confusing if Object.keys were to appear absent
entirely.

If I create a third-party flow-typed libdef for a module which just
re-implements the Object.key functionality how should I type it?

Indeed, even using declare var ok: typeof Object.keys does not produce
the same behavior as using the actual Object.keys.

[goodmind]

https://github.com/facebook/flow/blob/60fdca2ef30004edfe4b9cb5ac0af78d2e0425b1/src/typing/statement.ml#L6061-L6076

Looks like $Keys call, but isn't it unsound?

[dsainati1]

Is it unsound?

[jcready]

Yes:

function test(val: { foo: string }) {
  Object.keys(val).forEach(key => {
    val[key].toLowerCase();
  });
}

test({ foo: 'x', bar: 123 });

[dsainati1]

This is not an issue with Object.keys, but rather with computed property lookups. Consider:

const x : {foo : string} = {foo : "A", bar : 3};
const y : string = "bar";
x[y].toLowerCase();

[jcready]

Without any property lookups:

type Foo = {foo: string};

function test(val: Foo): $Keys<Foo>[] {
  return Object.keys(val);
}

test({ foo: 'x', b: 123 });

[goodmind]

@dsainati1 what about this

// @flow
type K = "foo" | "f";
class X {
  foo: string
  f() {}
}
function f(o: {foo: string, +f: () => void}) {
  return Object.keys(o) // ["foo"]
}
(f(new X): Array<K>);  // `string` should be incompatible with `K`

[dsainati1]

Both of your examples make use of inexact objects, for which our contract is that they are sound up to undefined, the same contract we make with arrays. For convenience, we allow you to access array indices, for example, without requiring that you check that the result is undefined (as do most languages).

The case is similar here, in that using the result of Object.keys on a larger object on a smaller one may cause you to access an undefined property, but this is no different that what is already the situation for inexact objects. The fix for both of these examples is to use exact objects, and this is indeed what we recommend to fix all of these sorts of undefined property issues.

[jcready]

So if this concession is made for Object.keys(), why not also for Object.values() or Object.entries()?

[wchargin]

@dsainati1: Is this really considered wontfix given that it’s unsound
even if you never actually dereference a property on the object?

// @flow
type T = { good: number };
function getKeys(val: T): $Keys<T>[] {
  return Object.keys(val);
}
const keys: "good"[] = getKeys({ good: 1, bad: 2 });
for (const key of keys) {
  if (key !== "good") {
    // Unreachable according to Flow...
    throw new Error((key: empty));
  }
}

(This typechecks, but of course errors at runtime.)

Comments on #4527 from @vkurchatkin and @samwgoldman suggest that at
least previously this was not the position of the team. Likewise #2221.

[dsainati1]

@samwgoldman is working on a more comprehensive change to the object model that might subsume this issue, so I wouldn't say that this is a wontfix because as far as I am aware fixing these unsoundness issues around inexact object should by proxy fix the issue you are seeing with keys. Sam can probably give a more accurate description of what his work is about though.

[goodmind]

@samwgoldman can you explain what changes on object model would be made?

[goodmind]

These types would make sense

declare function keys<T: $Exact<any>>(object: T): Array<$Keys<T>>;
declare function keys(object: mixed): Array<string>;

const x: {|
  a: string
|} = {a: ''}
const xs = keys(x); // Array<$Keys<T>>

(xs[0]: 'b') // error: 'a' ~> 'b'

const y: {a: string} = {a: ''}
const ys = keys(y); // Array<string>

(ys[0]: 'b') // error: string ~> 'b'

[goodmind]

Some updates to my types for keys, values, entries

declare function keys<O: {| +[key: mixed]: mixed |}>(o: O): Array<string>;
declare function keys<T, O: $Exact<T>>(o: O): Array<$Keys<O>>;
declare function keys(o: mixed): Array<string>;

declare function values<O: {| +[key: mixed]: mixed |}>(o: O): Array<[string, mixed]>;
declare function values<T, O: $Exact<T>>(o: O): Array<$Values<O>>;
declare function values(o: mixed): Array<mixed>;

declare function entries<O: {| +[key: mixed]: mixed |}>(o: O): Array<[string, mixed]>;
declare function entries<T, O: $Exact<T>>(o: O): Array<[$Keys<O>, $Values<O>]>;
declare function entries(o: mixed): Array<[string, mixed]>;

[jcready]

@goodmind so should I make a PR for these or will you?

[goodmind]

@jcready @dsainati1 doesn't like idea of exact constraint

[jcready]

@goodmind @jcready doesn't like the idea of mixed types for Object.values() and Object.entries() especially when an exact constraint provides these types in a sound manner. @samwgoldman has been talking about this magical rework of the object type system which will make all these problems go away for years. Please consider a temporary solution like the one you just provided until the full solution is ready.

[goodmind]

@jcready I agree with you, this was from Discord months ago when I proposed this libdef change

[kevinbarabash]

Even if this doesn't get accepted as PR, people could still opt into this locally by redefining the Object type with the suggested change. I know it's kind of hacky. Looking forward to #7919.