924040 - Update yield* to use @@iterator protocol

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

yield* needs to call GetIterator() [25.4.3.5] on the input object. This is not yet implemented, see https://bugzilla.mozilla.org/show_bug.cgi?id=666396#c4. The plan was to make this happen as part of bug 907077, but it didn't make it.

Comment 1

[André Bargull [:anba]]

Is this as simple as I think it is - just updating BytecodeEmitter::EmitYieldStar() to perform the same steps as in BytecodeEmitter::EmitForOf()?

Comment 2

[Andy Wingo [:wingo]]

Attachment: Update yield* to use @@iterator protocol

Comment 3

[Andy Wingo [:wingo]]

Attachment: Update yield* to use @@iterator protocol v2

Comment 4

[Andy Wingo [:wingo]]

Comment on attachment 814333 [details] [diff] [review]
Update yield* to use @@iterator protocol v2

r? to Waldo, as I seem to have filled the jorendorff patch buffer :-)

Note that this patch changes the ecma_6/Generators assertIteratorResult, as results from array iteration have a null prototype and so aren't deepEq to { value: foo, done: bar }.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 814333 [details] [diff] [review]
Update yield* to use @@iterator protocol v2

Review of attachment 814333 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/tests/ecma_6/Generators/delegating-yield-1.js
@@ +11,5 @@
>          return results[i++];
>      }
> +    var iter = { next: next }
> +    var ret = {};
> +    ret[std_iterator] = function () { return iter; }

Could also do |var ret = { next: next }; ret[std_iterator] = function() { return this; }; return ret;|.  But nothing wrong with this sort of extra variety, if indeed that's what you're going for in having the different things use different ways of iterator-ing.

::: js/src/tests/ecma_6/Generators/delegating-yield-6.js
@@ +43,5 @@
>  outer.next();
>  outer.next();
>  outer.next();
>  
> +assertEq(log, "indndndndndndv");

Elegant.

::: js/src/tests/ecma_6/Generators/shell.js
@@ +4,5 @@
>  
>  
> +var std_iterator = (function() {
> +    try {
> +        for (var _ of Proxy.create({get: function(_, name) { throw name; } }))

new Proxy({}, { get: function(_, name) { throw name; } })

@@ +9,5 @@
> +            break;
> +    } catch (name) {
> +        return name;
> +    }
> +    throw 'wat';

Clever.

@@ +17,5 @@
>  function assertTrue(a) { assertEq(a, true) }
>  function assertNotEq(found, not_expected) { assertFalse(found === expected) }
>  function assertIteratorResult(result, value, done) {
> +    assertDeepEq(result.value, value);
> +    assertDeepEq(result.done, done);

assertEq seems better for .done to me, as .done better always be a boolean.

Comment 6

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> >  function assertIteratorResult(result, value, done) {
> > +    assertDeepEq(result.value, value);
> > +    assertDeepEq(result.done, done);
> 
> assertEq seems better for .done to me, as .done better always be a boolean.

Oh, interesting. Can we use assertEq for the value too? I just realized I reviewed a bunch of changes to existing tests that previously used assertEq on the values. I don't think the intent was to weaken the assertion.

Comment 7

[Jeff Walden [:Waldo]]

I've definitely reviewed patches that compared .value that was an array, to a different array containing the same values.  So, no.

But honestly I'd rather we did extra work to use the same objects in such cases, so that we could do same-value/identity-based comparisons instead of allowing sloppier checking.  I don't know how much work would be in doing this, tho, and it's hard to complain too loudly, so I haven't insisted on it yet.

Comment 8

[Jason Orendorff [:jorendorff]]

Those cases can use a different assertion, perhaps assertDeepEq().

Comment 9

[Jeff Walden [:Waldo]]

This is inside a utility method used everywhere, tho, so it's not quite so simple.  (Having assertIteratorNextDeep seems supremely undesirable, but maybe that's just me.)

Comment 10

[Jason Orendorff [:jorendorff]]

Filed bug 927611 which maybe explains what I meant a little better.

Comment 11

[Andy Wingo [:wingo]]

Attachment: Update yield* to use @@iterator protocol r=Waldo

Comment 12

[Andy Wingo [:wingo]]

Comment on attachment 818381 [details] [diff] [review]
Update yield* to use @@iterator protocol r=Waldo

Thanks for the review, updated patch addresses nits.

Comment 13

[Till Schneidereit [:till]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60f90ee1eb33

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/60f90ee1eb33