Closed Bug 924040 Opened 6 years ago Closed 6 years ago

Update yield* to use @@iterator protocol

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: anba, Assigned: wingo)

Details

Attachments

(1 file, 2 obsolete files)

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.
Is this as simple as I think it is - just updating BytecodeEmitter::EmitYieldStar() to perform the same steps as in BytecodeEmitter::EmitForOf()?
Assignee: nobody → wingo
Attachment #814325 - Attachment is obsolete: true
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 }.
Attachment #814333 - Attachment description: Update yield* to use @@iterator protocol → Update yield* to use @@iterator protocol v2
Attachment #814333 - Flags: review?(jwalden+bmo)
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.
Attachment #814333 - Flags: review?(jwalden+bmo) → review+
(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.
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.
Those cases can use a different assertion, perhaps assertDeepEq().
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.)
Filed bug 927611 which maybe explains what I meant a little better.
Attachment #814333 - Attachment is obsolete: true
Comment on attachment 818381 [details] [diff] [review]
Update yield* to use @@iterator protocol r=Waldo

Thanks for the review, updated patch addresses nits.
Attachment #818381 - Attachment description: Update yield* to use @@iterator protocol → Update yield* to use @@iterator protocol r=Waldo
Attachment #818381 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/60f90ee1eb33
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.