Support rest element pattern with user-defined iterable

Support rest element pattern with user-defined iterable

The original implementation incorrectly supplied an iterator (not an
iterable) to a `for-of` statement. This functioned as expected for
built-in iterators because (by inheriting the InteratorPrototype
intrinsic's `Symbol.iterator` method) they are technically iterables as
well. This is not part of the formal Iterator interface requirements,
however, and user-created iterators that did not define a
`Symbol.iterator` property would produce an error when used as the
value for an AssignmentRestElement or BindingRestElement.

Update the internals to interpret the value as an iterator.

BUG=v8:4759
LOG=N
R=adamk@chromium.org

[mike3, 2016-02-16 16:30:21]

An alternative would be to avoid using `for-of` altogether and manually step
through the iterator by calling its `next` method. I initially took this
approach, but it required re-implementing a lot of `for-of`'s semantics, which
is just begging for rot. It wasn't until I saw `Array.of`'s implemetation that
this approach occurred to me.

I'm on the fence about it because of the reliance on an intermediate Object
value, but I honestly have no sense of the practical implications of this. If
we wanted to optimize a bit, we could close around a couple values:

    var iterVal;
    var iterable = { [iteratorSymbol]() { return iterVal; } };
    function ConcatIteratorToArray(target, iterator) {
       var index = target.length;
       iterVal = iterator;
       for (var element of iterable) {
         AddIndexedProperty(target, index++, element);
       }
       return target;
    }

Is that a sound optimization? Or just noise?

[caitp (gmail), 2016-02-16 17:24:38]

On 2016/02/16 16:30:21, mike3 wrote:
> An alternative would be to avoid using `for-of` altogether and manually step
> through the iterator by calling its `next` method. I initially took this
> approach, but it required re-implementing a lot of `for-of`'s semantics, which
> is just begging for rot. It wasn't until I saw `Array.of`'s implemetation that
> this approach occurred to me.
> 
> I'm on the fence about it because of the reliance on an intermediate Object
> value, but I honestly have no sense of the practical implications of this. If
> we wanted to optimize a bit, we could close around a couple values:
> 
>     var iterVal;
>     var iterable = { [iteratorSymbol]() { return iterVal; } };
>     function ConcatIteratorToArray(target, iterator) {
>        var index = target.length;
>        iterVal = iterator;
>        for (var element of iterable) {
>          AddIndexedProperty(target, index++, element);
>        }
>        return target;
>     }
> 
> Is that a sound optimization? Or just noise?

There was talk of inlining this using do expressions, I thought that work had
already been done (but I suppose it didn't touch RestElements). And note that,
the rest of the IteratorDestructuringAssignmentEvaluation implementation
desugars this manually, as well. Probably is still missing the IteratorClose
bits, but it wouldn't be a huge refactoring to fix that

[adamk, 2016-02-16 19:59:05]

As caitp says, the plan of record here is to desugar this fully in the parser,
without the concat_iterable_to_array helper function.

[mike3, 2016-02-16 20:06:24]

On 2016/02/16 19:59:05, adamk wrote:
> As caitp says, the plan of record here is to desugar this fully in the parser,
> without the concat_iterable_to_array helper function.

I think I understand. Are you saying that this patch should use the same
approach (an iterator wrapper object and a for-of statement) but that instead of
expressing it in literal JavaScript, it should build the AST in C++?

[adamk, 2016-02-16 22:46:36]

On 2016/02/16 20:06:24, mike3 wrote:
> On 2016/02/16 19:59:05, adamk wrote:
> > As caitp says, the plan of record here is to desugar this fully in the
parser,
> > without the concat_iterable_to_array helper function.
> 
> I think I understand. Are you saying that this patch should use the same
> approach (an iterator wrapper object and a for-of statement) but that instead
of
> expressing it in literal JavaScript, it should build the AST in C++?

Well, at the moment that desugaring is blocked on unifying some of the existing
rewriting code in the parser (specifically, array spread rewriting and
destructuring assignment rewriting).

[mike3, 2016-02-16 23:06:58]

On 2016/02/16 22:46:36, adamk wrote:
> On 2016/02/16 20:06:24, mike3 wrote:
> > On 2016/02/16 19:59:05, adamk wrote:
> > > As caitp says, the plan of record here is to desugar this fully in the
> parser,
> > > without the concat_iterable_to_array helper function.
> > 
> > I think I understand. Are you saying that this patch should use the same
> > approach (an iterator wrapper object and a for-of statement) but that
instead
> of
> > expressing it in literal JavaScript, it should build the AST in C++?
> 
> Well, at the moment that desugaring is blocked on unifying some of the
existing
> rewriting code in the parser (specifically, array spread rewriting and
> destructuring assignment rewriting).

Oh, okay. So I guess I should just sit tight until the re-write lands then? If
so, is there a ticket I can use to track the progress there?

[mike3, 2016-04-05 13:50:44]

Description was changed from

==========
Support rest element pattern with user-defined iterable

The original implementation incorrectly supplied an iterator (not an
iterable) to a `for-of` statement. This functioned as expected for
built-in iterators because (by inheriting the InteratorPrototype
intrinsic's `Symbol.iterator` method) they are technically iterables as
well. This is not part of the formal Iterator interface requirements,
however, and user-created iterators that did not define a
`Symbol.iterator` property would produce an error when used as the
value for an AssignmentRestElement or BindingRestElement.

Update the internals to interpret the value as an iterator.

BUG=v8:4759
LOG=N
R=adamk@chromium.org
==========

to

==========
Support rest element pattern with user-defined iterable

The original implementation incorrectly supplied an iterator (not an
iterable) to a `for-of` statement. This functioned as expected for
built-in iterators because (by inheriting the InteratorPrototype
intrinsic's `Symbol.iterator` method) they are technically iterables as
well. This is not part of the formal Iterator interface requirements,
however, and user-created iterators that did not define a
`Symbol.iterator` property would produce an error when used as the
value for an AssignmentRestElement or BindingRestElement.

Update the internals to interpret the value as an iterator.

BUG=v8:4759
LOG=N
R=adamk@chromium.org
==========

[mike3, 2016-04-05 14:00:16]

It looks like a project maintainer has stepped in to re-implement this patch in
the style suggested by Adam:

https://codereview.chromium.org/1852703002

I regret not having the understanding to do this on my own, but I hope that in
the future, I could work more closely with the maintainers to learn and to
avoid duplicating effort. In any case, I'm glad the bug is fixed!