[ESnext] Implement Object Rest

[ESnext] Implement Object Rest

This rewrites the rest property into a runtime call which sets up the
correct properties in the newly created object.

- Changes flag to --harmony-object-rest-spread
- Changes pattern rewriter to desugar rest property
- Adds new runtime function CopyDataPropertiesWithExcludedProperties

BUG=v8:5549
Review-Url: https://codereview.chromium.org/2620943002
Cr-Commit-Position: refs/heads/master@{#42430}
Committed: https://chromium.googlesource.com/v8/v8/+/54b5c4b853e32d3a976072ead8ea880e55f45d9f

[gsathya, 2017-01-10 22:53:32]

Description was changed from

==========
[ESnext] Implement Object Rest

This rewrites the rest property into a runtime call which sets up the
correct properties in the newly created object.

BUG=v8:5549
==========

to

==========
[ESnext] Implement Object Rest

This rewrites the rest property into a runtime call which sets up the
correct properties in the newly created object.

- Changes flag to --harmony-object-rest-spread flag
- Changes pattern rewriter to desugar rest property
- Adds new runtime function CopyDataPropertiesWithExcludedProperties

BUG=v8:5549
==========

[gsathya, 2017-01-10 22:53:32]

gsathya@chromium.org changed reviewers:
+ adamk@chromium.org, littledan@chromium.org

[gsathya, 2017-01-10 22:54:08]

Will add more mjsunit and parsing tests. PTAL.

[adamk, 2017-01-10 23:22:56]

You can probably ignore comments in here that relate to JSArray...

https://codereview.chromium.org/2620943002/diff/40001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2620943002/diff/40001/src/ast/ast.h#newcode1480
src/ast/ast.h:1480: bool has_rest_property_;
This should be stored in the bit_field_, like the other bools below.

https://codereview.chromium.org/2620943002/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2620943002/diff/40001/src/objects.cc#newcode2054
src/objects.cc:2054: excluded_properties =
maybe_excluded_properties.ToHandleChecked();
Pattern for this is:

const bool has_excluded_properties =
maybe_excluded_properties.ToHandle(&excluded_properties);

https://codereview.chromium.org/2620943002/diff/40001/src/objects.cc#newcode2099
src/objects.cc:2099: if (!maybe_excluded_properties.is_null() &&
and then this would be

if (has_excluded_properties && ...)

https://codereview.chromium.org/2620943002/diff/40001/src/objects.cc#newcode2139
src/objects.cc:2139: excluded_properties =
maybe_excluded_properties.ToHandleChecked();
Same here as above

https://codereview.chromium.org/2620943002/diff/40001/src/objects.cc#newcode2166
src/objects.cc:2166: if (!maybe_excluded_properties.is_null() &&
And here.

https://codereview.chromium.org/2620943002/diff/40001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2620943002/diff/40001/src/parsing/parser-base...
src/parsing/parser-base.h:2136: if (peek() != Token::RBRACE) {
I don't think this is correct, since there could be a comma (unless the grammar
actually forbids that?)

https://codereview.chromium.org/2620943002/diff/40001/src/parsing/pattern-rew...
File src/parsing/pattern-rewriter.cc (right):

https://codereview.chromium.org/2620943002/diff/40001/src/parsing/pattern-rew...
src/parsing/pattern-rewriter.cc:354: // c = %CopyDataProperties(temp, [a, b]);
Can you add something about the desugaring of computed property names?

https://codereview.chromium.org/2620943002/diff/40001/src/parsing/pattern-rew...
src/parsing/pattern-rewriter.cc:376: key =
factory()->NewVariableProxy(CreateTempVar(key));
You need to assign CreateTempVar to something so that you can create multiple
VariableProxies referencing it.

And I think you only want to create this temp var if we have a rest property,
right?

https://codereview.chromium.org/2620943002/diff/40001/src/parsing/pattern-rew...
src/parsing/pattern-rewriter.cc:380: excluded_properties->Add(key, zone());
As noted above, this needs to be a NewVariableProxy(temp)

https://codereview.chromium.org/2620943002/diff/40001/src/runtime/runtime-obj...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/2620943002/diff/40001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:798: CONVERT_ARG_HANDLE_CHECKED(JSArray,
excluded_properties, 1);
As discussed, I think it'd be simpler to just pass the excluded properties
directly to this function instead of an array.

[gsathya, 2017-01-11 01:17:43]

Description was changed from

==========
[ESnext] Implement Object Rest

This rewrites the rest property into a runtime call which sets up the
correct properties in the newly created object.

- Changes flag to --harmony-object-rest-spread flag
- Changes pattern rewriter to desugar rest property
- Adds new runtime function CopyDataPropertiesWithExcludedProperties

BUG=v8:5549
==========

to

==========
[ESnext] Implement Object Rest

This rewrites the rest property into a runtime call which sets up the
correct properties in the newly created object.

- Changes flag to --harmony-object-rest-spread
- Changes pattern rewriter to desugar rest property
- Adds new runtime function CopyDataPropertiesWithExcludedProperties

BUG=v8:5549
==========

[gsathya, 2017-01-12 20:59:12]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 20:59:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 21:20:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 21:20:13]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[gsathya, 2017-01-12 21:27:29]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 21:27:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2017-01-12 21:27:41]

https://codereview.chromium.org/2620943002/diff/40001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2620943002/diff/40001/src/ast/ast.h#newcode1480
src/ast/ast.h:1480: bool has_rest_property_;
On 2017/01/10 23:22:55, adamk wrote:
> This should be stored in the bit_field_, like the other bools below.

Done.

https://codereview.chromium.org/2620943002/diff/40001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2620943002/diff/40001/src/parsing/parser-base...
src/parsing/parser-base.h:2136: if (peek() != Token::RBRACE) {
On 2017/01/10 23:22:56, adamk wrote:
> I don't think this is correct, since there could be a comma (unless the
grammar
> actually forbids that?)

No trailing comma allowed after destructuring property.

https://codereview.chromium.org/2620943002/diff/40001/src/parsing/pattern-rew...
File src/parsing/pattern-rewriter.cc (right):

https://codereview.chromium.org/2620943002/diff/40001/src/parsing/pattern-rew...
src/parsing/pattern-rewriter.cc:354: // c = %CopyDataProperties(temp, [a, b]);
On 2017/01/10 23:22:56, adamk wrote:
> Can you add something about the desugaring of computed property names?

There's a comment below where I handle the computed property name. Do you want
the desugaring to show the temporary variable creation?

https://codereview.chromium.org/2620943002/diff/40001/src/parsing/pattern-rew...
src/parsing/pattern-rewriter.cc:376: key =
factory()->NewVariableProxy(CreateTempVar(key));
On 2017/01/10 23:22:56, adamk wrote:
> You need to assign CreateTempVar to something so that you can create multiple
> VariableProxies referencing it.
> 
> And I think you only want to create this temp var if we have a rest property,
> right?

Done.

https://codereview.chromium.org/2620943002/diff/40001/src/parsing/pattern-rew...
src/parsing/pattern-rewriter.cc:380: excluded_properties->Add(key, zone());
On 2017/01/10 23:22:56, adamk wrote:
> As noted above, this needs to be a NewVariableProxy(temp)

Done.

[commit-bot: I haz the power, 2017-01-12 21:48:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 21:48:27]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[gsathya, 2017-01-12 22:04:24]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 22:04:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 22:27:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 22:27:39]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/20666)
  v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)

[adamk, 2017-01-12 22:43:07]

adamk@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[adamk, 2017-01-12 22:43:08]

The biggest issue now is this ToPropertyKey thing; adding rmcilroy for his
thoughts on whether this would make more sense in
BytecodeGenerator::VisitObjectLiteral.

https://codereview.chromium.org/2620943002/diff/40001/src/parsing/pattern-rew...
File src/parsing/pattern-rewriter.cc (right):

https://codereview.chromium.org/2620943002/diff/40001/src/parsing/pattern-rew...
src/parsing/pattern-rewriter.cc:354: // c = %CopyDataProperties(temp, [a, b]);
On 2017/01/12 21:27:41, gsathya wrote:
> On 2017/01/10 23:22:56, adamk wrote:
> > Can you add something about the desugaring of computed property names?
> 
> There's a comment below where I handle the computed property name. Do you want
> the desugaring to show the temporary variable creation?

Yeah, I was thinking that this should basically read like an example from your
design doc, with the desugaring including the computed property case.

https://codereview.chromium.org/2620943002/diff/80001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2620943002/diff/80001/src/objects.cc#newcode2010
src/objects.cc:2010: if (*search_element == (*arguments)[i]) {
Pointer equality isn't sufficient for strings. Use Object::SameValue() instead.

https://codereview.chromium.org/2620943002/diff/80001/src/parsing/pattern-rew...
File src/parsing/pattern-rewriter.cc (right):

https://codereview.chromium.org/2620943002/diff/80001/src/parsing/pattern-rew...
src/parsing/pattern-rewriter.cc:373: // so that we don't evaluate it twice when
we include it in
This isn't sufficient as it turns out: we need to store not only the output of
the expression, but also the ToPropertyKey() of that output. Otherwise a case
like:

var x = 0;
var {[{valueOf() { return x++ }}]: y, ...z} = {};

will call valueOf() twice!

I begin to wonder if that means we should move more of this logic to the
BytecodeGenerator, since it already has access to that ToPropertyKey'd value.
The thing I don't know enough to answer is the best way to accumulate a list in
BytecodeGenerator.

https://codereview.chromium.org/2620943002/diff/80001/src/runtime/runtime-obj...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/2620943002/diff/80001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:791: Arguments excluded_properties(1,
args.arguments() - 1);
Passing 1 for length here seems wrong, how does this work for tests that pass in
multiple excluded properties?

Elsewhere I see this pattern of allocating a ScopedVector<Handle> and filling it
with the arguments. Perhaps we could do that here. It does call new, which is a
little strange, but it's still probably lower overhead than allocating a heap
object like a FixedArray. One advantage of going that route is it would be easy
to check that all the arguments are Names.

https://codereview.chromium.org/2620943002/diff/80001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/2620943002/diff/80001/src/runtime/runtime.h#n...
src/runtime/runtime.h:419: F(CopyDataPropertiesWithExcludedProperties, -1 /* =>
1 */, 1) \
s/=>/>=/

https://codereview.chromium.org/2620943002/diff/80001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/2620943002/diff/80001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6580: "{ x : y, ...z }",
Rather than interleaving the new tests and always turning the RestSpread flag
on, you should create a separate array of data holding just those patterns
including rest syntax. Then you can have four calls to RunTests:

RunTests(data, noflags, kSuccess)
RunTests(rest_data, noflags, kFailure)
RunTests(data, RestSpreadFlag, kSuccess)
RunTests(rest_data, RestSpreadFlag, kSuccess)

Once the feature is shipped, you can then merge data and rest_data and have a
single call.

Side note: this would be a good thing for us to fix with more test
infrastructure.

[adamk, 2017-01-12 22:50:06]

On 2017/01/12 22:43:08, adamk wrote:
> The biggest issue now is this ToPropertyKey thing; adding rmcilroy for his
> thoughts on whether this would make more sense in
> BytecodeGenerator::VisitObjectLiteral.

Apologies, I was confusing this with the Object spread stuff. This code doesn't
affect VisitObjectLiteral, since the pattern-rewriter already desugars the
destructuring into:

var y = value[{valueOf() { return x++ }}];

and then we let the normal keyed load code do the ToPropertyKey for us. Not sure
whether this eliminates the question of whether this code should do more work in
BytecodeGenerator, but it definitely makes it less of a slam dunk (since the
generator doesn't see the ToPropertyKey()'d value right now at all).

[gsathya, 2017-01-13 00:32:57]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 00:32:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 01:09:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 01:09:22]

Dry run: This issue passed the CQ dry run.

[gsathya, 2017-01-13 06:25:36]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 06:25:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 06:56:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 06:56:16]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2017-01-13 11:24:24]

On 2017/01/12 22:50:06, adamk wrote:
> On 2017/01/12 22:43:08, adamk wrote:
> > The biggest issue now is this ToPropertyKey thing; adding rmcilroy for his
> > thoughts on whether this would make more sense in
> > BytecodeGenerator::VisitObjectLiteral.
> 
> Apologies, I was confusing this with the Object spread stuff. This code
doesn't
> affect VisitObjectLiteral, since the pattern-rewriter already desugars the
> destructuring into:
> 
> var y = value[{valueOf() { return x++ }}];
> 
> and then we let the normal keyed load code do the ToPropertyKey for us. Not
sure
> whether this eliminates the question of whether this code should do more work
in
> BytecodeGenerator, but it definitely makes it less of a slam dunk (since the
> generator doesn't see the ToPropertyKey()'d value right now at all).

I'm not sure I have the context to answer this. Where is the ToPropertyKey part
of the code, and what part would you propose moving to the bytecode generator?

[adamk, 2017-01-13 19:08:49]

On 2017/01/13 11:24:24, rmcilroy wrote:
> On 2017/01/12 22:50:06, adamk wrote:
> > On 2017/01/12 22:43:08, adamk wrote:
> > > The biggest issue now is this ToPropertyKey thing; adding rmcilroy for his
> > > thoughts on whether this would make more sense in
> > > BytecodeGenerator::VisitObjectLiteral.
> > 
> > Apologies, I was confusing this with the Object spread stuff. This code
> doesn't
> > affect VisitObjectLiteral, since the pattern-rewriter already desugars the
> > destructuring into:
> > 
> > var y = value[{valueOf() { return x++ }}];
> > 
> > and then we let the normal keyed load code do the ToPropertyKey for us. Not
> sure
> > whether this eliminates the question of whether this code should do more
work
> in
> > BytecodeGenerator, but it definitely makes it less of a slam dunk (since the
> > generator doesn't see the ToPropertyKey()'d value right now at all).
> 
> I'm not sure I have the context to answer this. Where is the ToPropertyKey
part
> of the code, and what part would you propose moving to the bytecode generator?

Apologies, I think you can safely ignore for now. We will return to you with a
more focused question once we've got a baseline version working.

[gsathya, 2017-01-17 19:18:29]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 19:18:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2017-01-17 19:28:59]

https://codereview.chromium.org/2620943002/diff/80001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2620943002/diff/80001/src/objects.cc#newcode2010
src/objects.cc:2010: if (*search_element == (*arguments)[i]) {
On 2017/01/12 22:43:08, adamk wrote:
> Pointer equality isn't sufficient for strings. Use Object::SameValue()
instead.

Done.

https://codereview.chromium.org/2620943002/diff/80001/src/parsing/pattern-rew...
File src/parsing/pattern-rewriter.cc (right):

https://codereview.chromium.org/2620943002/diff/80001/src/parsing/pattern-rew...
src/parsing/pattern-rewriter.cc:373: // so that we don't evaluate it twice when
we include it in
On 2017/01/12 22:43:08, adamk wrote:
> This isn't sufficient as it turns out: we need to store not only the output of
> the expression, but also the ToPropertyKey() of that output. Otherwise a case
> like:
> 
> var x = 0;
> var {[{valueOf() { return x++ }}]: y, ...z} = {};
> 
> will call valueOf() twice!
> 
> I begin to wonder if that means we should move more of this logic to the
> BytecodeGenerator, since it already has access to that ToPropertyKey'd value.
> The thing I don't know enough to answer is the best way to accumulate a list
in
> BytecodeGenerator.

Done.

https://codereview.chromium.org/2620943002/diff/80001/src/runtime/runtime-obj...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/2620943002/diff/80001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:791: Arguments excluded_properties(1,
args.arguments() - 1);
On 2017/01/12 22:43:08, adamk wrote:
> Passing 1 for length here seems wrong, how does this work for tests that pass
in
> multiple excluded properties?
> 
> Elsewhere I see this pattern of allocating a ScopedVector<Handle> and filling
it
> with the arguments. Perhaps we could do that here. It does call new, which is
a
> little strange, but it's still probably lower overhead than allocating a heap
> object like a FixedArray. One advantage of going that route is it would be
easy
> to check that all the arguments are Names.

Done.

https://codereview.chromium.org/2620943002/diff/80001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/2620943002/diff/80001/src/runtime/runtime.h#n...
src/runtime/runtime.h:419: F(CopyDataPropertiesWithExcludedProperties, -1 /* =>
1 */, 1) \
On 2017/01/12 22:43:08, adamk wrote:
> s/=>/>=/

Done.

https://codereview.chromium.org/2620943002/diff/80001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/2620943002/diff/80001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6580: "{ x : y, ...z }",
On 2017/01/12 22:43:08, adamk wrote:
> Rather than interleaving the new tests and always turning the RestSpread flag
> on, you should create a separate array of data holding just those patterns
> including rest syntax. Then you can have four calls to RunTests:
> 
> RunTests(data, noflags, kSuccess)
> RunTests(rest_data, noflags, kFailure)
> RunTests(data, RestSpreadFlag, kSuccess)
> RunTests(rest_data, RestSpreadFlag, kSuccess)
> 
> Once the feature is shipped, you can then merge data and rest_data and have a
> single call.
> 
> Side note: this would be a good thing for us to fix with more test
> infrastructure.

Done.

[commit-bot: I haz the power, 2017-01-17 19:49:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 19:49:29]

Dry run: This issue passed the CQ dry run.

[adamk, 2017-01-17 19:56:33]

Looking good, a few stylistic nits below and more test suggestions.

https://codereview.chromium.org/2620943002/diff/160001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2620943002/diff/160001/src/objects.cc#newcode...
src/objects.cc:2006: bool HasExcludedProperty(ScopedVector<Handle<Name>>*
excluded_properties,
const ScopedVector*

https://codereview.chromium.org/2620943002/diff/160001/src/objects.cc#newcode...
src/objects.cc:2020: ScopedVector<Handle<Name>>* excluded_properties, bool
use_set) {
const ScopedVector*

https://codereview.chromium.org/2620943002/diff/160001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2620943002/diff/160001/src/objects.h#newcode2006
src/objects.h:2006: ScopedVector<Handle<Name>>* excluded_properties = nullptr,
Can this argument be a "const ScopedVector<...>*"?

https://codereview.chromium.org/2620943002/diff/160001/src/parsing/pattern-re...
File src/parsing/pattern-rewriter.cc (right):

https://codereview.chromium.org/2620943002/diff/160001/src/parsing/pattern-re...
src/parsing/pattern-rewriter.cc:381: // TODO(gsathya): Skip %ToName runtime call
for string literals.
s/string //

https://codereview.chromium.org/2620943002/diff/160001/src/runtime/runtime-ob...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/2620943002/diff/160001/src/runtime/runtime-ob...
src/runtime/runtime-object.cc:771: excluded_properties[i - 1] =
Handle<Name>::cast(args.at(i));
Slightly shorter:

args.at<Name>(i);

https://codereview.chromium.org/2620943002/diff/160001/src/runtime/runtime-ob...
src/runtime/runtime-object.cc:774: // 2. If source is undefined or null, let
keys be an empty List.
Maybe move this before ScopedVector creation? It's not like we care about the
speed of this case, but it would be a little easier to read.

https://codereview.chromium.org/2620943002/diff/160001/test/cctest/test-parsi...
File test/cctest/test-parsing.cc (left):

https://codereview.chromium.org/2620943002/diff/160001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:6703: "{x}",
Why did this get deleted?

https://codereview.chromium.org/2620943002/diff/160001/test/mjsunit/harmony/o...
File test/mjsunit/harmony/object-rest-basic.js (right):

https://codereview.chromium.org/2620943002/diff/160001/test/mjsunit/harmony/o...
test/mjsunit/harmony/object-rest-basic.js:5: // Flags:
--harmony-object-rest-spread
Some more ideas for tests:

- Patterns where there are keys which are Smis (1, 2)
- Patterns where the keys are doubles (e.g., 2.5)
- Rest in function params
- Rest in arrow parameters
- Further destructuring (both object and array patterns) in the rest position

[gsathya, 2017-01-17 21:56:29]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 21:56:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2017-01-17 21:59:23]

https://codereview.chromium.org/2620943002/diff/160001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2620943002/diff/160001/src/objects.cc#newcode...
src/objects.cc:2006: bool HasExcludedProperty(ScopedVector<Handle<Name>>*
excluded_properties,
On 2017/01/17 19:56:33, adamk wrote:
> const ScopedVector*

Done.

https://codereview.chromium.org/2620943002/diff/160001/src/objects.cc#newcode...
src/objects.cc:2020: ScopedVector<Handle<Name>>* excluded_properties, bool
use_set) {
On 2017/01/17 19:56:33, adamk wrote:
> const ScopedVector*

Done.

https://codereview.chromium.org/2620943002/diff/160001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2620943002/diff/160001/src/objects.h#newcode2006
src/objects.h:2006: ScopedVector<Handle<Name>>* excluded_properties = nullptr,
On 2017/01/17 19:56:33, adamk wrote:
> Can this argument be a "const ScopedVector<...>*"?

Done.

https://codereview.chromium.org/2620943002/diff/160001/src/parsing/pattern-re...
File src/parsing/pattern-rewriter.cc (right):

https://codereview.chromium.org/2620943002/diff/160001/src/parsing/pattern-re...
src/parsing/pattern-rewriter.cc:381: // TODO(gsathya): Skip %ToName runtime call
for string literals.
On 2017/01/17 19:56:33, adamk wrote:
> s/string //

Done.

https://codereview.chromium.org/2620943002/diff/160001/src/runtime/runtime-ob...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/2620943002/diff/160001/src/runtime/runtime-ob...
src/runtime/runtime-object.cc:771: excluded_properties[i - 1] =
Handle<Name>::cast(args.at(i));
On 2017/01/17 19:56:33, adamk wrote:
> Slightly shorter:
> 
> args.at<Name>(i);

Done.

https://codereview.chromium.org/2620943002/diff/160001/src/runtime/runtime-ob...
src/runtime/runtime-object.cc:774: // 2. If source is undefined or null, let
keys be an empty List.
On 2017/01/17 19:56:33, adamk wrote:
> Maybe move this before ScopedVector creation? It's not like we care about the
> speed of this case, but it would be a little easier to read.

Done.

https://codereview.chromium.org/2620943002/diff/160001/test/cctest/test-parsi...
File test/cctest/test-parsing.cc (left):

https://codereview.chromium.org/2620943002/diff/160001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:6703: "{x}",
On 2017/01/17 19:56:33, adamk wrote:
> Why did this get deleted?

Done.

https://codereview.chromium.org/2620943002/diff/160001/test/mjsunit/harmony/o...
File test/mjsunit/harmony/object-rest-basic.js (right):

https://codereview.chromium.org/2620943002/diff/160001/test/mjsunit/harmony/o...
test/mjsunit/harmony/object-rest-basic.js:5: // Flags:
--harmony-object-rest-spread
On 2017/01/17 19:56:33, adamk wrote:
> Some more ideas for tests:
> 
> - Patterns where there are keys which are Smis (1, 2)
> - Patterns where the keys are doubles (e.g., 2.5)
> - Rest in function params
> - Rest in arrow parameters
> - Further destructuring (both object and array patterns) in the rest position

Done.

[adamk, 2017-01-17 22:21:06]

lgtm with one more test

https://codereview.chromium.org/2620943002/diff/180001/test/mjsunit/harmony/o...
File test/mjsunit/harmony/object-rest-basic.js (right):

https://codereview.chromium.org/2620943002/diff/180001/test/mjsunit/harmony/o...
test/mjsunit/harmony/object-rest-basic.js:118: var [...{...z}] = [{ x: 1}];
Please also add

{...{z}} or something

[commit-bot: I haz the power, 2017-01-17 22:27:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 22:27:47]

Dry run: This issue passed the CQ dry run.

[gsathya, 2017-01-17 23:02:09]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[gsathya, 2017-01-17 23:02:13]

https://codereview.chromium.org/2620943002/diff/180001/test/mjsunit/harmony/o...
File test/mjsunit/harmony/object-rest-basic.js (right):

https://codereview.chromium.org/2620943002/diff/180001/test/mjsunit/harmony/o...
test/mjsunit/harmony/object-rest-basic.js:118: var [...{...z}] = [{ x: 1}];
On 2017/01/17 22:21:06, adamk wrote:
> Please also add
> 
> {...{z}} or something

Done.

[commit-bot: I haz the power, 2017-01-17 23:02:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[adamk, 2017-01-17 23:04:14]

Thanks, still lgtm

[commit-bot: I haz the power, 2017-01-17 23:16:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 23:16:03]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[Dan Ehrenberg, 2017-01-17 23:53:14]

lgtm

Sadly, I don't see any spread/rest test262 PRs. Any way I could tempt you into
developing some of these tests as test262 tests in test/test262/local-tests? I'm
fine with that as a follow-on test refactoring patch.

https://codereview.chromium.org/2620943002/diff/220001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2620943002/diff/220001/src/ast/ast.h#newcode1448
src/ast/ast.h:1448: kHasRestProperty = 1 << 3
Nit optional suggestion: Add a trailing comma so future patches don't need to
change the previous line.

https://codereview.chromium.org/2620943002/diff/220001/src/parsing/pattern-re...
File src/parsing/pattern-rewriter.cc (right):

https://codereview.chromium.org/2620943002/diff/220001/src/parsing/pattern-re...
src/parsing/pattern-rewriter.cc:363: // var temp1 = %ToName(y);
Nit: %ToName("y")

https://codereview.chromium.org/2620943002/diff/220001/src/parsing/pattern-re...
src/parsing/pattern-rewriter.cc:381: // TODO(gsathya): Skip %ToName runtime call
for literals.
Would this be as simple as putting most the below code in the above conditional
(except the last line)? Maybe you should just do it here.

https://codereview.chromium.org/2620943002/diff/220001/src/runtime/runtime-ob...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/2620943002/diff/220001/src/runtime/runtime-ob...
src/runtime/runtime-object.cc:764:
RUNTIME_FUNCTION(Runtime_CopyDataPropertiesWithExcludedProperties) {
Unimportant to answer before this lands: I've been seeing other code go in where
the parser outputs code to call a builtin function which is on the context,
rather than a runtime function. That could be done here too. What's the general
best practice?

[Dan Ehrenberg, 2017-01-17 23:53:18]

lgtm

lgtm

Sadly, I don't see any spread/rest test262 PRs. Any way I could tempt you into
developing some of these tests as test262 tests in test/test262/local-tests? I'm
fine with that as a follow-on test refactoring patch.

[gsathya, 2017-01-18 00:37:08]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 00:37:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2017-01-18 00:58:21]

I pinged sebastian markbage about tests and he said he will write some this
week. I can work on moving some of our tests to test262 once he has some
preliminary tests so theres more coverage

https://codereview.chromium.org/2620943002/diff/220001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2620943002/diff/220001/src/ast/ast.h#newcode1448
src/ast/ast.h:1448: kHasRestProperty = 1 << 3
On 2017/01/17 23:53:14, Dan Ehrenberg wrote:
> Nit optional suggestion: Add a trailing comma so future patches don't need to
> change the previous line.

Done.

https://codereview.chromium.org/2620943002/diff/220001/src/parsing/pattern-re...
File src/parsing/pattern-rewriter.cc (right):

https://codereview.chromium.org/2620943002/diff/220001/src/parsing/pattern-re...
src/parsing/pattern-rewriter.cc:363: // var temp1 = %ToName(y);
On 2017/01/17 23:53:14, Dan Ehrenberg wrote:
> Nit: %ToName("y")

Done.

https://codereview.chromium.org/2620943002/diff/220001/src/parsing/pattern-re...
src/parsing/pattern-rewriter.cc:381: // TODO(gsathya): Skip %ToName runtime call
for literals.
On 2017/01/17 23:53:14, Dan Ehrenberg wrote:
> Would this be as simple as putting most the below code in the above
conditional
> (except the last line)? Maybe you should just do it here.

Almost, also requires changes in SetOrCopyDataProperties to not convert all keys
to strings and ScopedVector can not have only Names. There's also the case where
a computed property returns a number (which we ToName) and there's mismatch with
the property on RHS (if we dont convert all keys to strings)

https://codereview.chromium.org/2620943002/diff/220001/src/runtime/runtime-ob...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/2620943002/diff/220001/src/runtime/runtime-ob...
src/runtime/runtime-object.cc:764:
RUNTIME_FUNCTION(Runtime_CopyDataPropertiesWithExcludedProperties) {
On 2017/01/17 23:53:14, Dan Ehrenberg wrote:
> Unimportant to answer before this lands: I've been seeing other code go in
where
> the parser outputs code to call a builtin function which is on the context,
> rather than a runtime function. That could be done here too. What's the
general
> best practice?

Not sure about what's more performant, tbh. I went with this approach because
there's no need to create a js function for this or expose it to userland js.
builtins *only* installed on the context are an exception and seem to be mostly
used when there's existing codestubs that can be reused from within the builtin

[Dan Ehrenberg, 2017-01-18 01:02:05]

lgtm

[commit-bot: I haz the power, 2017-01-18 01:02:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 01:02:40]

Dry run: This issue passed the CQ dry run.

[gsathya, 2017-01-18 01:03:09]

The CQ bit was checked by gsathya@chromium.org

[gsathya, 2017-01-18 01:03:09]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2620943002/#ps240001
(title: "fix nits")

[commit-bot: I haz the power, 2017-01-18 01:03:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 01:05:18]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1484701389175980,
"parent_rev": "c236fe1459db363b56b23d841a7af141aaf6f21c", "commit_rev":
"54b5c4b853e32d3a976072ead8ea880e55f45d9f"}

[commit-bot: I haz the power, 2017-01-18 01:05:25]

Description was changed from

==========
[ESnext] Implement Object Rest

This rewrites the rest property into a runtime call which sets up the
correct properties in the newly created object.

- Changes flag to --harmony-object-rest-spread
- Changes pattern rewriter to desugar rest property
- Adds new runtime function CopyDataPropertiesWithExcludedProperties

BUG=v8:5549
==========

to

==========
[ESnext] Implement Object Rest

This rewrites the rest property into a runtime call which sets up the
correct properties in the newly created object.

- Changes flag to --harmony-object-rest-spread
- Changes pattern rewriter to desugar rest property
- Adds new runtime function CopyDataPropertiesWithExcludedProperties

BUG=v8:5549

Review-Url: https://codereview.chromium.org/2620943002
Cr-Commit-Position: refs/heads/master@{#42430}
Committed:
https://chromium.googlesource.com/v8/v8/+/54b5c4b853e32d3a976072ead8ea880e55f...
==========

[commit-bot: I haz the power, 2017-01-18 01:05:26]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/v8/v8/+/54b5c4b853e32d3a976072ead8ea880e55f...