Bug 147677 – Ensure that Reflect.enumerate does not produce the deleted keys

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Jordan Harband]

Given this code:
```
var obj = {a: 1, b: 2};
var iterator = Reflect.enumerate(obj);
obj.c = 3;
assert.deepEqual(Array.from(iterator), ['a', 'b', 'c']);
```

Webkit will fail (specifically, it won't yield the "c" key, because it was added after the iterator was created). According to the spec, keys should be determined at each `next` call - and any unvisited key, that's added before the first `next` call, must therefore be visitable (assuming I'm reading the spec right)

[Yusuke Suzuki]

Hm, I'm not sure this is true in the spec.
http://www.ecma-international.org/ecma-262/6.0/#sec-ordinary-object-internal-methods-and-internal-slots-enumerate

Which part is corresponding to this issue?

[Jordan Harband]

I read "The iterator’s next method processes object properties to determine whether the property key should be returned as an iterator value" as, only "next" should look at properties.

The generator function example in fact will not process the function body until the first `next` is called - which means, `Reflect.ownKeys` is not called until that first `next` - which means that prior to the first `next` call, any and all key changes should be reflected in the upcoming `ownKeys` call.

[Darin Adler]

Is there a way to make changes to properties after the first time next is called but before the second time next is called? Do we handle that properly? Are there tests covering that?

[Yusuke Suzuki]

(In reply to comment #3)
> Is there a way to make changes to properties after the first time next is
> called but before the second time next is called? Do we handle that
> properly? Are there tests covering that?

If you delete the property name from the object, the deleted name is not produced by the iterator. This behavior is specified in the spec. "A property that is deleted before it is processed by the iterator’s next method is ignored."

And if you add the property name to the object after calling next(), this name may or may not be produced by the iterator. "If new properties are added to the target object during enumeration, the newly added properties are not guaranteed to be processed in the active enumeration."
In WebKit's implementation, we don't produce it. And actually, our implementation works correctly on them.

> Are there tests covering that?

Oops, maybe, I didn't add the test for them. It's my fault. I need to add tests anyway.

[Yusuke Suzuki]

(In reply to comment #2)
> I read "The iterator’s next method processes object properties to determine
> whether the property key should be returned as an iterator value" as, only
> "next" should look at properties.
> 
> The generator function example in fact will not process the function body
> until the first `next` is called - which means, `Reflect.ownKeys` is not
> called until that first `next` - which means that prior to the first `next`
> call, any and all key changes should be reflected in the upcoming `ownKeys`
> call.

Still I'm not confident that which behavior is correct because I think the spec is slightly ambiguous on this.
(Maybe, I've missed something. And Darin, what do you think of?)
I'd like to ask about this to the es-discuss to ensure the correct behavior, OK?

The spec says "If new properties are added to the target object during enumeration, the newly added properties are not guaranteed to be processed in the active enumeration.". In for-in, we first call [[Enumerate]] in http://www.ecma-international.org/ecma-262/6.0/#sec-runtime-semantics-forin-div-ofheadevaluation-tdznames-expr-iterationkind. So if [[Enumerate]] call is included in the "active enumeration" process, I think that we can say caching the all property names at first is the correct implementation.

In the current implementation, I think we can say that "The iterator’s next method processes object properties to determine whether the property key should be returned as an iterator value" is correct if the initialization is included in the "active enumeration".

And for the generator's implementation, we have notes.
"NOTE The following is an informative definition of an ECMAScript generator function that conforms to these rules:"
So I think this example implementation just follow the rules specified in the spec. But if the rule doesn't say anything about the iterator's initialization time, this generator's implementation is just the example. In that case, this is the implementation dependent, both (our implementation and generator example) are correct.

[Jordan Harband]

(In reply to comment #5)

Absolutely we should clarify with es-discuss.

However, enumeration of a generator does not start until the first time "next" is called - since the canonical example in the spec is itself a generator (one of the rare times code is supplied in the spec), it's pretty clear to be that up until the first "next" call, no keys should be cached.

You're absolutely right that after the first "next" is called, any keys deleted before being yielded, will not be yielded - and any keys added may or may not be yielded, up to the implementation.

[Darin Adler]

We should definitely have this discussion with the others in es-discuss.

It’s peculiar design to require behavior where added keys before the first call to "next" are guaranteed to be iterated, but where added keys before other calls to "next" might or might not be.

Doesn’t seem obviously better to make the first call to "next" special and different from the others, as opposed to making the key moment the time when the object is created. Also strange that the model specifies exactly this degree of laziness but leave behavior for added keys between calls to "next" undefined; would be better to have it specified one way or another rather than left to each implementer to do differently, unless there is a substantial advantage to leaving this undefined. This kind of undefined behavior often leads to real world incompatibilities, so we should avoid it if we can.

Any chance we could get this refined to be something more logical?

[Yusuke Suzuki]

https://esdiscuss.org/topic/question-about-enumerate-and-property-decision-timing

Now, let's just add the tests to ensure that deleted key before producing the key is not produced.

[Jordan Harband]

Glad we clarified it :-) I'll update the es6-shim's tests and implementation.

[Yusuke Suzuki]

(In reply to comment #9)
> Glad we clarified it :-) I'll update the es6-shim's tests and implementation.

Nice :D

[Yusuke Suzuki]

Created attachment 258702 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 258702 [details]
Patch

Thank you for your review!

[WebKit Commit Bot]

Comment on attachment 258702 [details]
Patch

Clearing flags on attachment: 258702

Committed r188253: <http://trac.webkit.org/changeset/188253>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.