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]
}

(

flow/lib/core.js

Line 58 in 172d28f

static keys(o: any): Array<string>;

)

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]

flow/src/typing/statement.ml

Lines 6061 to 6076 in 60fdca2

	| ("getOwnPropertyNames" | "keys"), None, [Expression e] ->
	let arr_reason = mk_reason RArrayType loc in
	let (_, o), _ as e_ast = expression cx e in
	DefT (arr_reason, bogus_trust (), ArrT (
	ArrayAT (
	Tvar.mk_where cx arr_reason (fun tvar ->
	let keys_reason = replace_reason (fun desc ->
	RCustom (spf "element of %s" (string_of_desc desc))
	) reason in
	Flow.flow cx (o, GetKeysT (keys_reason, UseT (unknown_use, tvar)));
	),
	None
	)
	)),
	None,
	[Expression e_ast]

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?