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

Incorrect iteration order of deleted and re-assigned properties. #3505

Open
jdalton opened this Issue Aug 10, 2017 · 6 comments

Comments

Projects
None yet
8 participants
@jdalton
Member

jdalton commented Aug 10, 2017

Using chakracore 1.7.1.0 of chakranode I noticed and interop issue when a unit test in my project failed.

o = {}
o.b = 2
o.a = 1
delete o.b
o.b = 2
Object.keys(o) // produces [ 'b', 'a' ] while v8 produces ['a', 'b']

The example uses Object.keys but for-in and friends will produce a similar result.

@MikeHolman

This comment has been minimized.

Show comment
Hide comment
@MikeHolman

MikeHolman Aug 10, 2017

Member

The spec doesn't mandate any particular ordering here, so either way is correct. Might be worth changing this for interop if it is convenient, but I don't think it's worth going out of our way to mimic V8's ordering, as there are likely a lot of corner cases where they differ.

Member

MikeHolman commented Aug 10, 2017

The spec doesn't mandate any particular ordering here, so either way is correct. Might be worth changing this for interop if it is convenient, but I don't think it's worth going out of our way to mimic V8's ordering, as there are likely a lot of corner cases where they differ.

@MikeHolman MikeHolman added this to the vNext milestone Aug 10, 2017

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Aug 10, 2017

Member

Key order is defined here.

My take on it is that if a key is deleted then added, it's being created again, so should be in creation order.

Member

jdalton commented Aug 10, 2017

Key order is defined here.

My take on it is that if a key is deleted then added, it's being created again, so should be in creation order.

@MikeHolman

This comment has been minimized.

Show comment
Hide comment
@MikeHolman

MikeHolman Aug 11, 2017

Member

My mistake, you're absolutely right! I was looking at the wrong part of the spec.

Member

MikeHolman commented Aug 11, 2017

My mistake, you're absolutely right! I was looking at the wrong part of the spec.

@dilijev

This comment has been minimized.

Show comment
Hide comment
@dilijev

dilijev May 31, 2018

Member
## Source
o = {};o.b = 2;o.a = 1;delete o.b;o.b = 2;print(Object.keys(o))

┌──────────┬─────┐
│ jsvu-ch  │ b,a │
├──────────┼─────┤
│ jsvu-jsc │ a,b │
│ jsvu-sm  │     │
│ jsvu-v8  │     │
└──────────┴─────┘
Member

dilijev commented May 31, 2018

## Source
o = {};o.b = 2;o.a = 1;delete o.b;o.b = 2;print(Object.keys(o))

┌──────────┬─────┐
│ jsvu-ch  │ b,a │
├──────────┼─────┤
│ jsvu-jsc │ a,b │
│ jsvu-sm  │     │
│ jsvu-v8  │     │
└──────────┴─────┘

@pleath pleath assigned pleath and aneeshdk and unassigned aneeshdk May 31, 2018

@pleath

This comment has been minimized.

Show comment
Hide comment
@pleath

pleath May 31, 2018

Member

This could/should be done along with sharing of types with deleted properties. It will require changing the way we handle deletion in dynamic type handlers.

Member

pleath commented May 31, 2018

This could/should be done along with sharing of types with deleted properties. It will require changing the way we handle deletion in dynamic type handlers.

@curtisman curtisman modified the milestones: 1.10, vNext Jul 2, 2018

@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Sep 11, 2018

Pedantry:

@jdalton, @MikeHolman was right the first time. Object.keys defers to EnumerableOwnPropertyNames, which says

Order the elements of properties so they are in the same relative order as would be produced by the Iterator that would be returned if the EnumerateObjectProperties internal method were invoked with O.

That is, Object.keys is only required to match for-in, the enumeration order for which is explicitly unspecified. (As such, this is a dupe of #4486.)

Anyway, though, the enumeration order of Reflect.ownKeys and friends is explicitly specified and is wrong in this way. And since all other engines are consistent for this case, I think it's right to regard it as a bug, and maybe try to get the spec to be more strict here.

bakkot commented Sep 11, 2018

Pedantry:

@jdalton, @MikeHolman was right the first time. Object.keys defers to EnumerableOwnPropertyNames, which says

Order the elements of properties so they are in the same relative order as would be produced by the Iterator that would be returned if the EnumerateObjectProperties internal method were invoked with O.

That is, Object.keys is only required to match for-in, the enumeration order for which is explicitly unspecified. (As such, this is a dupe of #4486.)

Anyway, though, the enumeration order of Reflect.ownKeys and friends is explicitly specified and is wrong in this way. And since all other engines are consistent for this case, I think it's right to regard it as a bug, and maybe try to get the spec to be more strict here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment