[fix] ES5 Object.keys only accepts an object

[ljharb]

ES5 Object.keys throws on non-objects: http://www.ecma-international.org/ecma-262/5.1/#sec-15.2.3.14 - which is type object.

ES2015 Object.keys allows any "object coercible", ie any non-null non-undefined value - which is {}: http://www.ecma-international.org/ecma-262/6.0/#sec-object.keys

This attempts to fix the types.

• There is an associated issue that is labeled
  'Bug' or 'help wanted' or is in the Community milestone
• Code is up-to-date with the master branch
• You've successfully run jake runtests locally
• You've signed the CLA
• There are new or updated unit tests validating the change

This is my first PR to TypeScript; if you can point me to where I should add unit tests, I'll be happy to do so.

[msftclas]

All CLA requirements met.

[DanielRosenwasser]

Heya @ljharb! Looks like our baselines might need to be updated - it's likely the symbol baselines are printing out a different type for uses of Object.keys. Try running gulp baseline-accept or jake baseline-accept and git add tests. You can think of these as snapshot tests.

Additionally, is there a reason why Object.keys needed to be added to es2015.core.d.ts?

[ljharb]

@DanielRosenwasser yes, because the spec for Object.keys changed in ES2015 to no longer throw on non-nullish primitives (numbers, strings, booleans, symbols) - so while a type of object is correct for ES5, it's not correct for ES6+.

Thanks, I'll update the tests shortly.

[ljharb]

Tests updated.

[RyanCavanaugh]

Is there an associated bug for this?

[ljharb]

@RyanCavanaugh no, i didn't file one - it seemed like this would be faster. If you need me to file one that copy-pastes the first two lines in the OP, i can certainly do that.

[ljharb]

ping - does this really need an issue, or is it ready to go as-is?

[ljharb]

@DanielRosenwasser @RyanCavanaugh any update?

[ljharb]

hm, test failures seem unrelated

[ljharb]

@RyanCavanaugh yay, how soon is "soon"? :-D

[ljharb]

@RyanCavanaugh any update?

[RyanCavanaugh]

@ljharb the reason we ask for an associated issue is that the issue is how we track whether or not we decide we want a PR for something in the first place. I think it's a broadly open question whether calling e.g. Object.keys(42) is a sensical operation, or belongs in the category of Math.max({}, () => {}) and we would like to have a discussion about that first before jumping straight to a code fix.

[ljharb]

@RyanCavanaugh sure, that's fine; there's no harm in closing the PR if you don't decide you want the change. However, I'd argue it's not about "sensical" - it's allowed by the spec, and it was explicitly changed in ES6 because people pass primitives into Object.keys and want to not get an exception.

The primary issue is that the ES5 types are incorrect - they currently allow passing non-object non-nullish primitives, but ES5 disallowed this, and only ES2015 enabled that.

[RyanCavanaugh]

I'd argue it's not about "sensical" - it's allowed by the spec

We've had this discussion on Twitter already 😉

I think this is fine to merge once the failing tests gets fixed.

[ljharb]

Tests should now be fixed.

[RyanCavanaugh]

After a longer discussion than expected on the merits of Object.keys(number) vs Object.keys(any[]) vs Object.keys(number | object) we eventually settled on the current overload (object only) being the best fit for the API - basically that Object.keys("hello world") is closer to Math.atan("Valid JS!") than to a "likely intentional" call. We can re-evaluate if we get bug reports from people trying to call it that way on purpose.

[ljharb]

I opened this because it’s a bug, because i do it this way on purpose in qs, a very well-used library. I’m disappointed in this outcome.

[ljharb]

@RyanCavanaugh it’s also worth noting - what’s in master does not agree with your decision, because you have the ES5 types as the empty interface. The correction is to make es5 object and es6+ the empty interface. In other words, what you want is what I’m asking for es5 to do, and what you don’t want is what typescript already does.

[DanielRosenwasser]

I think that when this was discussed at the backlog slog, we misunderstood the topic. I'm sorry about that.

The root of the problem is that any sort of change sort of needs to build up its worth, especially when there's some risk of breakage or new conceptual overhead. Regardless of the eventual change, creating the distinction between ES5/ES2015 seems like it would be confusing (i.e. having an overload for object and {}). With the current approach of eventually changing the es5 lib to use object, it would potentially break working existing code in a surprising way.

You already saw the opposite approach - we erroneously assumed the type already was object, since it "felt" like the most reasonable behavior, and making it more permissive seemed questionable.

[ljharb]

My intention here was for those targeting es5 to be able to get warning of a runtime exception before it happens. Without this PR, the type system will allow it, but an ES5 runtime will not.

[RyanCavanaugh]

the correction is to make es5 object and es6+ the empty interface.

Without this PR, the type system will allow it, but an ES5 runtime will not.

@ljharb This is what I see in the PR and what we were basing our discussion off of:

I don't see any part of the PR that adds an object-accepting overload or changes the ES5 signature to only accept object. Maybe git didn't merge something properly? I think this is the source of confusion - we were assuming the diff here was a meaningful divergence from the current behavior, but what's in this PR is just the same as what's in the current lib file.

[RyanCavanaugh]

I'm OK taking the PR in its intended form 😉

[ljharb]

Thanks, sorry for all the back-and-forth.

PR updated.

[RyanCavanaugh]

@ljharb looks like you need to run gulp baseline-accept after gulp runtests-parallel and commit the result

[ljharb]

@RyanCavanaugh oops, thanks. Done!

[RyanCavanaugh]

This broke VS Code

[ljharb]

@RyanCavanaugh can you elaborate? #31380 (comment) implies it caught a bug in vscode, not broke it.

[RyanCavanaugh]

@ljharb see #31380 (comment)

[ljharb]

@RyanCavanaugh ah, gotcha. that suggests a bug in type narrowing then :-) thanks for the followup.

[RyanCavanaugh]

Let's say you have code like this

declare function keys(obj: object): string[];
function fn<T extends null | object>(arg: T) {
    // Point 1
    if (!arg) return;
    // Point 2
    if (typeof arg !== "object") return;
    // Point 3
    keys(arg);
}

There's an error here because arg is still of type T, not object.

To get the "correct" answer for the type of arg at point 3, you have to combine two separate facts about arg - that it isn't falsy (thus excluding null) and that its typeof is object. The inference system doesn't have a way to track these things right now - simply saying if (foo) { on some foo: T extends ... doesn't change anything, because we don't have a way representation of "the values of T which are truthy`.

We'd need #22348 or its equivalent to track this.

[ljharb]

That seems excessively valuable given that truthiness and falsiness are both core and idiomatic mechanisms used for runtime guards. Hopefully that could land soon, and type narrowing improved?

[RyanCavanaugh]

It's not that we don't recognize the value of it, believe me 😉