ES6 computed property names

ES6 computed property names

This adds support for computed property names, under the flag
--harmony-computed-property-names, for both object literals and
classes.

BUG=v8:3754
LOG=Y

Committed: https://crrev.com/6e38caf8d35463e29a2530b9d3df928c9cc231ba
Cr-Commit-Position: refs/heads/master@{#25821}

[arv (Not doing code reviews), 2014-12-10 23:25:24]

arv@chromium.org changed reviewers:
+ dslomov@chromium.org, wingo@igalia.com

[arv (Not doing code reviews), 2014-12-10 23:25:25]

This only does ia32 at the moment and I need a ton more tests.

I just want some early feedback.

[arv (Not doing code reviews), 2014-12-10 23:38:13]

Self review

https://codereview.chromium.org/795573005/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/795573005/diff/1/src/ast.h#newcode1520
src/ast.h:1520: bool is_computed_name_;
Another Kind would not work since computed can be applied to all but PROTOTYPE.

https://codereview.chromium.org/795573005/diff/1/src/compiler/ast-graph-build...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/795573005/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.cc:919: Literal* key =
property->key()->AsLiteral();
This is not going to work. I disabled the tests for now.

https://codereview.chromium.org/795573005/diff/1/src/func-name-inferrer.cc
File src/func-name-inferrer.cc (right):

https://codereview.chromium.org/795573005/diff/1/src/func-name-inferrer.cc#ne...
src/func-name-inferrer.cc:46: UNREACHABLE();
removing

https://codereview.chromium.org/795573005/diff/1/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/795573005/diff/1/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:1731: // TODO(arv): ToName for computed
properties.
todo is done ;-)

https://codereview.chromium.org/795573005/diff/1/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:1759: __ CallRuntime(Runtime::kDefineClassGetter,
3);
I'll rename these two and move them to runtime-object.cc

https://codereview.chromium.org/795573005/diff/1/src/parser.cc
File src/parser.cc (left):

https://codereview.chromium.org/795573005/diff/1/src/parser.cc#oldcode720
src/parser.cc:720: if (parser_->fni_ != NULL)
parser_->fni_->PushVariableName(name);
I can problably revert this now. I don't call these functions from
ParsePropertyName any more.

https://codereview.chromium.org/795573005/diff/1/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/795573005/diff/1/src/preparser.h#newcode2054
src/preparser.h:2054: if (!*is_computed_name && fni_ != NULL) {
if (fni_ != NULL) {

since name is empty string for computed properties.

https://codereview.chromium.org/795573005/diff/1/src/preparser.h#newcode2060
src/preparser.h:2060: if (!*is_computed_name && checker != NULL) {
maybe same here... need to check

https://codereview.chromium.org/795573005/diff/1/src/runtime/runtime-object.cc
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/795573005/diff/1/src/runtime/runtime-object.c...
src/runtime/runtime-object.cc:1447: CONVERT_ARG_HANDLE_CHECKED(Object, key, 1);
revert this. handle ToName in caller instead.

https://codereview.chromium.org/795573005/diff/1/src/typing.cc
File src/typing.cc (right):

https://codereview.chromium.org/795573005/diff/1/src/typing.cc#newcode410
src/typing.cc:410: if (prop->key()->AsLiteral()->value()->IsInternalizedString()
&&
check is_computed_name here

[Dmitry Lomov (no reviews), 2014-12-11 12:34:35]

shaping up nicely!

https://codereview.chromium.org/795573005/diff/1/src/ast.cc
File src/ast.cc (right):

https://codereview.chromium.org/795573005/diff/1/src/ast.cc#newcode658
src/ast.cc:658: TypeFeedbackId id = key()->AsLiteral()->LiteralFeedbackId();
What if key() is not Literal?

https://codereview.chromium.org/795573005/diff/1/src/compiler/ast-graph-build...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/795573005/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.cc:919: Literal* key =
property->key()->AsLiteral();
On 2014/12/10 23:38:13, arv wrote:
> This is not going to work. I disabled the tests for now.

Probably should be a DontOptimize reason for TurboFan

https://codereview.chromium.org/795573005/diff/1/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/795573005/diff/1/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:1733: __ InvokeBuiltin(Builtins::TO_NAME,
CALL_FUNCTION);
You can probably just call runtime function here, and not invoke the JS buitin

https://codereview.chromium.org/795573005/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/795573005/diff/1/src/parser.cc#newcode4039
src/parser.cc:4039: bool is_computed_name = false;
this should be has_seen_computed_name, right? Or why is it unused?

[arv (Not doing code reviews), 2014-12-11 15:47:25]

https://codereview.chromium.org/795573005/diff/1/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/795573005/diff/1/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:1733: __ InvokeBuiltin(Builtins::TO_NAME,
CALL_FUNCTION);
On 2014/12/11 12:34:35, Dmitry Lomov (chromium) wrote:
> You can probably just call runtime function here, and not invoke the JS buitin

It is not clear to me why that is preferred?

[Dmitry Lomov (no reviews), 2014-12-11 16:01:15]

https://codereview.chromium.org/795573005/diff/1/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/795573005/diff/1/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:1733: __ InvokeBuiltin(Builtins::TO_NAME,
CALL_FUNCTION);
On 2014/12/11 15:47:24, arv wrote:
> On 2014/12/11 12:34:35, Dmitry Lomov (chromium) wrote:
> > You can probably just call runtime function here, and not invoke the JS
buitin
> 
> It is not clear to me why that is preferred?

Saves you adding a builtin just for this case

[Dmitry Lomov (no reviews), 2014-12-11 16:50:26]

On 2014/12/11 16:01:15, Dmitry Lomov (chromium) wrote:
> https://codereview.chromium.org/795573005/diff/1/src/ia32/full-codegen-ia32.cc
> File src/ia32/full-codegen-ia32.cc (right):
> 
>
https://codereview.chromium.org/795573005/diff/1/src/ia32/full-codegen-ia32.c...
> src/ia32/full-codegen-ia32.cc:1733: __ InvokeBuiltin(Builtins::TO_NAME,
> CALL_FUNCTION);
> On 2014/12/11 15:47:24, arv wrote:
> > On 2014/12/11 12:34:35, Dmitry Lomov (chromium) wrote:
> > > You can probably just call runtime function here, and not invoke the JS
> buitin
> > 
> > It is not clear to me why that is preferred?
> 
> Saves you adding a builtin just for this case

Ah scratch that I somehow thought we have Runtime_ToName already. This is fine.

[arv (Not doing code reviews), 2014-12-11 20:54:16]

progress

[arv (Not doing code reviews), 2014-12-11 20:59:57]

Disable TF correctly

[arv (Not doing code reviews), 2014-12-11 22:47:31]

Moar tests

[arv (Not doing code reviews), 2014-12-11 23:09:46]

moar cleanup

[arv (Not doing code reviews), 2014-12-11 23:10:34]

PTAL

This is now ready for review. If it looks good to you I'll implement the codegen
for the other platforms.

https://codereview.chromium.org/795573005/diff/1/src/ast.cc
File src/ast.cc (right):

https://codereview.chromium.org/795573005/diff/1/src/ast.cc#newcode658
src/ast.cc:658: TypeFeedbackId id = key()->AsLiteral()->LiteralFeedbackId();
On 2014/12/11 12:34:35, Dmitry Lomov (chromium) wrote:
> What if key() is not Literal?

The caller needs to test that the property is not a computed property name. I've
added that check and added a DCHECK here.

https://codereview.chromium.org/795573005/diff/1/src/compiler/ast-graph-build...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/795573005/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.cc:919: Literal* key =
property->key()->AsLiteral();
On 2014/12/11 12:34:35, Dmitry Lomov (chromium) wrote:
> On 2014/12/10 23:38:13, arv wrote:
> > This is not going to work. I disabled the tests for now.
> 
> Probably should be a DontOptimize reason for TurboFan

Do you mean mark all ObjectLiteral as not optimizable by TF?

https://codereview.chromium.org/795573005/diff/1/src/func-name-inferrer.cc
File src/func-name-inferrer.cc (right):

https://codereview.chromium.org/795573005/diff/1/src/func-name-inferrer.cc#ne...
src/func-name-inferrer.cc:46: UNREACHABLE();
On 2014/12/10 23:38:13, arv wrote:
> removing

I removed all the changes to func-name-inferrer.

https://codereview.chromium.org/795573005/diff/1/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/795573005/diff/1/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:1759: __ CallRuntime(Runtime::kDefineClassGetter,
3);
On 2014/12/10 23:38:13, arv wrote:
> I'll rename these two and move them to runtime-object.cc

Done.

https://codereview.chromium.org/795573005/diff/1/src/parser.cc
File src/parser.cc (left):

https://codereview.chromium.org/795573005/diff/1/src/parser.cc#oldcode720
src/parser.cc:720: if (parser_->fni_ != NULL)
parser_->fni_->PushVariableName(name);
On 2014/12/10 23:38:13, arv wrote:
> I can problably revert this now. I don't call these functions from
> ParsePropertyName any more.

Done.

https://codereview.chromium.org/795573005/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/795573005/diff/1/src/parser.cc#newcode4039
src/parser.cc:4039: bool is_computed_name = false;
On 2014/12/11 12:34:35, Dmitry Lomov (chromium) wrote:
> this should be has_seen_computed_name, right? Or why is it unused?

Classes do not care about computed property name here. I added a comment.

https://codereview.chromium.org/795573005/diff/1/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/795573005/diff/1/src/preparser.h#newcode2054
src/preparser.h:2054: if (!*is_computed_name && fni_ != NULL) {
On 2014/12/10 23:38:13, arv wrote:
> if (fni_ != NULL) {
> 
> since name is empty string for computed properties.

This one is needed.

https://codereview.chromium.org/795573005/diff/1/src/preparser.h#newcode2060
src/preparser.h:2060: if (!*is_computed_name && checker != NULL) {
On 2014/12/10 23:38:13, arv wrote:
> maybe same here... need to check

This one is needed too.

https://codereview.chromium.org/795573005/diff/1/src/preparser.h#newcode2126
src/preparser.h:2126: if (is_static && !*is_computed_name &&
this->IsPrototype(name)) {
this one is not needed because IsPrototype will be false for the empty literal.

https://codereview.chromium.org/795573005/diff/1/src/runtime/runtime-object.cc
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/795573005/diff/1/src/runtime/runtime-object.c...
src/runtime/runtime-object.cc:1447: CONVERT_ARG_HANDLE_CHECKED(Object, key, 1);
On 2014/12/10 23:38:13, arv wrote:
> revert this. handle ToName in caller instead.

Done.

https://codereview.chromium.org/795573005/diff/1/src/typing.cc
File src/typing.cc (right):

https://codereview.chromium.org/795573005/diff/1/src/typing.cc#newcode410
src/typing.cc:410: if (prop->key()->AsLiteral()->value()->IsInternalizedString()
&&
On 2014/12/10 23:38:13, arv wrote:
> check is_computed_name here

Done.

https://codereview.chromium.org/795573005/diff/60001/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/795573005/diff/60001/src/arm/full-codegen-arm...
src/arm/full-codegen-arm.cc:2491: EmitPropertyKey(property);
Ignore

[arv (Not doing code reviews), 2014-12-12 14:17:48]

Ping

[Dmitry Lomov (no reviews), 2014-12-12 15:11:58]

looking good, I have a couple of comments, but proceed to ports.

I haven't looked at tests in detail

https://codereview.chromium.org/795573005/diff/80001/src/ia32/full-codegen-ia...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/795573005/diff/80001/src/ia32/full-codegen-ia...
src/ia32/full-codegen-ia32.cc:1725: switch (property->kind()) {
Suggestion about this switch - all cases except PROTOTYPE are similar, so
structure code like this:

duplicate receiver
if(PROTOTYPE) {
  ...
} else {
  VisitForStackValue(key);
  VisitForStackValue(value);
  conversion to name (optional; as I noted elsewhere, let's unify args to
DefineXXXPropertyUnchecked
  call different functions depending
  ..
}
  
}

https://codereview.chromium.org/795573005/diff/80001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/795573005/diff/80001/src/preparser.h#newcode1987
src/preparser.h:1987: // table when we computed constant properties.
typo: 'computed -> compute'

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

https://codereview.chromium.org/795573005/diff/80001/src/runtime/runtime-obje...
src/runtime/runtime-object.cc:1599: CONVERT_ARG_HANDLE_CHECKED(Object, key, 1);
Feels weird that DefineDataPropertyUnchecked takes Name and
Define{Getter,Setter}PropertyUnchecked take Object. Let's use the same pattern
in all of them.

[arv (Not doing code reviews), 2014-12-12 16:48:17]

Clean up Name and codegen

[arv (Not doing code reviews), 2014-12-12 16:49:34]

Feedback resolved. Starting porting now.

https://codereview.chromium.org/795573005/diff/80001/src/ia32/full-codegen-ia...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/795573005/diff/80001/src/ia32/full-codegen-ia...
src/ia32/full-codegen-ia32.cc:1725: switch (property->kind()) {
On 2014/12/12 15:11:58, Dmitry Lomov (chromium) wrote:
> Suggestion about this switch - all cases except PROTOTYPE are similar, so
> structure code like this:
> 
> duplicate receiver
> if(PROTOTYPE) {
>   ...
> } else {
>   VisitForStackValue(key);
>   VisitForStackValue(value);
>   conversion to name (optional; as I noted elsewhere, let's unify args to
> DefineXXXPropertyUnchecked
>   call different functions depending
>   ..
> }
>   
> }

Done.

https://codereview.chromium.org/795573005/diff/80001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/795573005/diff/80001/src/preparser.h#newcode1987
src/preparser.h:1987: // table when we computed constant properties.
On 2014/12/12 15:11:58, Dmitry Lomov (chromium) wrote:
> typo: 'computed -> compute'

Done.

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

https://codereview.chromium.org/795573005/diff/80001/src/runtime/runtime-obje...
src/runtime/runtime-object.cc:1599: CONVERT_ARG_HANDLE_CHECKED(Object, key, 1);
On 2014/12/12 15:11:58, Dmitry Lomov (chromium) wrote:
> Feels weird that DefineDataPropertyUnchecked takes Name and
> Define{Getter,Setter}PropertyUnchecked take Object. Let's use the same pattern
> in all of them.

I agree.

I ended up changing all the runtime functions to take a Name instead.

[arv (Not doing code reviews), 2014-12-12 17:19:22]

Ports

[arv (Not doing code reviews), 2014-12-12 17:37:22]

git rebase

[Dmitry Lomov (no reviews), 2014-12-15 10:43:47]

lgtm % comments

https://codereview.chromium.org/795573005/diff/140001/test/mjsunit/harmony/co...
File test/mjsunit/harmony/computed-property-names.js (right):

https://codereview.chromium.org/795573005/diff/140001/test/mjsunit/harmony/co...
test/mjsunit/harmony/computed-property-names.js:8: (function TestBasicsString()
{
Could you also add (more) tests where computed name values are not immediate
compile-time constant? We should really implement an easy optimization that
reduces `[<literal>] : ..` to `<literal> : ..`.

https://codereview.chromium.org/795573005/diff/140001/test/mjsunit/harmony/co...
test/mjsunit/harmony/computed-property-names.js:192: object = {
Nit: here and below, set calls to 0 before executing each test case, and
assertEquals(1, calls). This would make test cases independent of each other.

[arv (Not doing code reviews), 2014-12-15 14:58:15]

Cleanup tests a bit

[arv (Not doing code reviews), 2014-12-15 14:59:00]

The CQ bit was checked by arv@chromium.org

[arv (Not doing code reviews), 2014-12-15 14:59:01]

https://codereview.chromium.org/795573005/diff/140001/test/mjsunit/harmony/co...
File test/mjsunit/harmony/computed-property-names.js (right):

https://codereview.chromium.org/795573005/diff/140001/test/mjsunit/harmony/co...
test/mjsunit/harmony/computed-property-names.js:8: (function TestBasicsString()
{
On 2014/12/15 10:43:47, Dmitry Lomov (chromium) wrote:
> Could you also add (more) tests where computed name values are not immediate
> compile-time constant? We should really implement an easy optimization that
> reduces `[<literal>] : ..` to `<literal> : ..`.

I'm not sure it is worth optimizing [literal] since that does seems unlikely to
be used.

I will update the test to make sure these are at least not a PrimaryExpression.

https://codereview.chromium.org/795573005/diff/140001/test/mjsunit/harmony/co...
test/mjsunit/harmony/computed-property-names.js:192: object = {
On 2014/12/15 10:43:47, Dmitry Lomov (chromium) wrote:
> Nit: here and below, set calls to 0 before executing each test case, and
> assertEquals(1, calls). This would make test cases independent of each other.

Done.

[commit-bot: I haz the power, 2014-12-15 14:59:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/795573005/160001

[commit-bot: I haz the power, 2014-12-15 15:27:14]

Committed patchset #9 (id:160001)

[arv (Not doing code reviews), 2014-12-15 16:37:08]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/809433002/ by arv@chromium.org.

The reason for reverting is: Crashes on Win32

http://build.chromium.org/p/client.v8/builders/V8%20Win32%20-%201/builds/1357

Test: mjsunit/harmony/computed-property-names
Flags: --stress-opt --always-opt
Command: build\Release\d8.exe --test --random-seed=-233815021 --stress-opt
--always-opt --nohard-abort --nodead-code-elimination --nofold-constants
--harmony-computed-property-names test\mjsunit\mjsunit.js
test\mjsunit\harmony\computed-property-names.js

Run #1
Exit code: -1073741819
Result: CRASH
Expected outcomes: PASS

Run #2
Exit code: -1073741819
Result: CRASH
Expected outcomes: PASS

Run #3
Exit code: -1073741819
Result: CRASH
Expected outcomes: PASS.

[commit-bot: I haz the power, 2014-12-17 10:13:30]

Patchset 9 (id:??) landed as
https://crrev.com/6e38caf8d35463e29a2530b9d3df928c9cc231ba
Cr-Commit-Position: refs/heads/master@{#25821}