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

Normative: make EnumerableOwnPropertyNames ordered #1793

Merged
merged 1 commit into from Dec 12, 2019
Merged

Normative: make EnumerableOwnPropertyNames ordered #1793

merged 1 commit into from Dec 12, 2019

Conversation

@bakkot
Copy link
Contributor

bakkot commented Nov 23, 2019

Related to proposal-for-in-order. Added as a separate PR because this change wasn't technically in scope during the process for that proposal. Thanks to @domenic for bringing this to my attention.

EnumerableOwnPropertyNames is used by Object.keys, Object.values, Object.entries, JSON.parse, and JSON.stringify.

Removing this line has the effect of requiring those to be ordered the same way as Reflect.ownKeys, which is fully specified, rather than for-in, which is only partially specified.

I have not been able to find cases where implementations order any of the APIs which use EnumerableOwnPropertyNames differently than they would Reflect.ownKeys, even in the presence of exotic objects and accessors which modify the object being iterated. So removing this line should not have any effects on the implementations I have on hand.

I want to make this change for two reasons:

  1. It removes a case where implementations are permitted to differ, and
  2. This line is incredibly confusing anyway, since it is impossible for an implementation to know how the "the Iterator that would be returned if the EnumerateObjectProperties internal method were invoked with O" would behave without actually invoking it, which can have arbitrary other side effects.
Copy link
Member

devsnek left a comment

begone evil prose

@ljharb ljharb requested review from syg, ljharb and tc39/ecma262-editors Nov 27, 2019
@bakkot bakkot added has consensus and removed needs consensus labels Dec 3, 2019
@ljharb
ljharb approved these changes Dec 3, 2019
@ljharb ljharb self-assigned this Dec 3, 2019
@ljharb ljharb force-pushed the other-orders branch from b4d8568 to ecb4178 Dec 12, 2019
@ljharb ljharb merged commit ecb4178 into master Dec 12, 2019
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
netlify/ecma262-snapshots/deploy-preview Deploy preview ready!
Details
@ljharb ljharb deleted the other-orders branch Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.