Class syntax parsing

Class syntax parsing

This implements parsing for ClassExpression and ClassDeclaration.
The runtime is not yet implemented and the value is currently
hard coded to undefined.

BUG=v8:3330
LOG=Y
R=dslomov@chromium.org, marja@chromium.org, rossberg@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=23988

[arv (Not doing code reviews), 2014-09-10 22:47:13]

Add early error checks for static prototype and get/set constructor

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

arv@chromium.org changed reviewers:
+ adamk@chromium.org, dslomov@chromium.org, marja@chromium.org,
rossberg@chromium.org

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

The codegen/runtime part is mostly just placeholders at this point.

I would like to get some early feedback on the ast/parsing changes though.

Thanks.

[marja, 2014-09-11 09:40:58]

parsing changes lg % comments

https://codereview.chromium.org/561913002/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/561913002/diff/20001/src/preparser.h#newcode635
src/preparser.h:635: bool IsEvalOrArguments() const { return type_ >=
kEvalIdentifier; }
This looks incorrect now, because you're adding more values to the type enum.

https://codereview.chromium.org/561913002/diff/20001/src/preparser.h#newcode637
src/preparser.h:637: bool IsPrototype() const { return type_ >=
kPrototypeIdentifier; }
Why >= ? And in case somebody adds new enum values in the future, isn't it
better to check against the exact values to avoid the same problem...

[arv (Not doing code reviews), 2014-09-11 15:39:32]

Fix IsEval...

[arv (Not doing code reviews), 2014-09-11 18:24:43]

Seems like I have to duplicate the class top level parsing to be able to set up
the correct scopes.

https://codereview.chromium.org/561913002/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/561913002/diff/20001/src/preparser.h#newcode635
src/preparser.h:635: bool IsEvalOrArguments() const { return type_ >=
kEvalIdentifier; }
On 2014/09/11 09:40:58, marja wrote:
> This looks incorrect now, because you're adding more values to the type enum.

Oops.

https://codereview.chromium.org/561913002/diff/20001/src/preparser.h#newcode637
src/preparser.h:637: bool IsPrototype() const { return type_ >=
kPrototypeIdentifier; }
On 2014/09/11 09:40:58, marja wrote:
> Why >= ? And in case somebody adds new enum values in the future, isn't it
> better to check against the exact values to avoid the same problem...

Ooops here too.

[arv (Not doing code reviews), 2014-09-11 19:25:37]

Classes are strict

[arv (Not doing code reviews), 2014-09-11 21:57:18]

Extends expression is also strict

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

Remove ClassDeclaration

[arv (Not doing code reviews), 2014-09-12 21:36:35]

This is now ready for real review.

The runtime behavior just returns undefined at this point.

Parsing is almost complete. There are some more things that we need to support
before parsing is consider complete:

 - Duplicate properties are currently not allowed.
 - Duplicate constructor check needs to be implemented when we start allowing
duplicate properties. I added a test for that so it does not get lost.
 - I want to store the constructor on the ClassLiteral AST node. Right now it
just uses NULL/EmptyFunctionLiteral.
 - Add support for concise generator methods.
 - Add support for computed properties.

Your feedback is highly desired.

https://codereview.chromium.org/561913002/diff/140001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/561913002/diff/140001/src/ast.h#newcode1492
src/ast.h:1492: bool is_static_;
What was the conclusion on C++ bit field members? I could fold kind_,
emit_store_ and is_static_ into 8 bits.

https://codereview.chromium.org/561913002/diff/140001/src/prettyprinter.cc
File src/prettyprinter.cc (right):

https://codereview.chromium.org/561913002/diff/140001/src/prettyprinter.cc#ne...
src/prettyprinter.cc:292: void PrettyPrinter::VisitClassLiteral(ClassLiteral*
node) {
When is the PrettyPrinter used? I don't see any tests for this.

[marja, 2014-09-15 09:24:29]

marja@chromium.org changed reviewers:
+ svenpanne@chromium.org

[marja, 2014-09-15 09:24:30]

parsing parts lgtm, pls wait for rossberg@ to ack this too, svenpanne@, what's
up with bitfields nowadays (see question above)?

https://codereview.chromium.org/561913002/diff/140001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/561913002/diff/140001/src/preparser.h#newcode638
src/preparser.h:638: bool IsEvalOrArguments() const { return type_ >=
kEvalIdentifier; }
Nit: I'd just get rid of the >= trickery altogether (I don't see any real reason
for it and it's just error prone), and add Prototype and Constructor at the end
of the enum instead of in the middle.

[Sven Panne, 2014-09-15 11:13:06]

https://codereview.chromium.org/561913002/diff/140001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/561913002/diff/140001/src/ast.h#newcode1492
src/ast.h:1492: bool is_static_;
On 2014/09/12 21:36:34, arv wrote:
> What was the conclusion on C++ bit field members? I could fold kind_,
> emit_store_ and is_static_ into 8 bits.

I would leave it as it is, to avoid premature optimization: Stuffing a byte
between 2 pointers won't save any memory, anyway.

[rossberg, 2014-09-15 12:32:49]

Looks mostly good, but I think we should store static methods in a separate list
(see comments).

https://codereview.chromium.org/561913002/diff/140001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/561913002/diff/140001/src/ast.h#newcode1464
src/ast.h:1464: bool is_static);
Nit: formatting

https://codereview.chromium.org/561913002/diff/140001/src/ast.h#newcode1492
src/ast.h:1492: bool is_static_;
On 2014/09/12 21:36:34, arv wrote:
> What was the conclusion on C++ bit field members? I could fold kind_,
> emit_store_ and is_static_ into 8 bits.

Opinions still differ, better to avoid them for the time being.

How about avoiding this flag by simply putting static and non-static properties
into two separate lists?

https://codereview.chromium.org/561913002/diff/140001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/561913002/diff/140001/src/parser.cc#newcode1961
src/parser.cc:1961: Interface* interface = Interface::NewValue();
Nit: inline into call

https://codereview.chromium.org/561913002/diff/140001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/561913002/diff/140001/src/preparser.h#newcode638
src/preparser.h:638: bool IsEvalOrArguments() const { return type_ >=
kEvalIdentifier; }
On 2014/09/15 09:24:29, marja wrote:
> Nit: I'd just get rid of the >= trickery altogether (I don't see any real
reason
> for it and it's just error prone), and add Prototype and Constructor at the
end
> of the enum instead of in the middle.

I agree.

https://codereview.chromium.org/561913002/diff/140001/src/preparser.h#newcode...
src/preparser.h:1980: } else if (in_class && name_is_static && !is_static) {
Hm, I'd prefer pulling recognising 'static' out of this method and avoid both
the funny recursion and computing the extra flag in ParsePropertyName. Let the
caller do it.

https://codereview.chromium.org/561913002/diff/140001/src/preparser.h#newcode...
src/preparser.h:2723: this->NewScope(scope_, BLOCK_SCOPE);
Why does the extends clause need its own scope?

https://codereview.chromium.org/561913002/diff/140001/src/preparser.h#newcode...
src/preparser.h:2738: typename Traits::Type::ScopePtr scope =
this->NewScope(scope_, BLOCK_SCOPE);
I'm not sure it will work out to reuse BLOCK_SCOPE here. We probably need a
separate CLASS_SCOPE. At least it would be cleaner. (I'm fine with making that a
TODO for now.)

https://codereview.chromium.org/561913002/diff/140001/src/preparser.h#newcode...
src/preparser.h:2745: const bool is_static = false;
Nit: If you find it worth naming these constants, then at least move them next
to the call using them.

https://codereview.chromium.org/561913002/diff/140001/src/preparser.h#newcode...
src/preparser.h:2755: ObjectLiteralPropertyT property =
this->ParsePropertyDefinition(
I'd prefer to check for 'static' here. That would also make it easy to store
static and non-static properties in separate lists, and avoid the extra flag on
their AST node.

https://codereview.chromium.org/561913002/diff/140001/src/prettyprinter.cc
File src/prettyprinter.cc (right):

https://codereview.chromium.org/561913002/diff/140001/src/prettyprinter.cc#ne...
src/prettyprinter.cc:292: void PrettyPrinter::VisitClassLiteral(ClassLiteral*
node) {
On 2014/09/12 21:36:34, arv wrote:
> When is the PrettyPrinter used? I don't see any tests for this.

Good question...

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

https://codereview.chromium.org/561913002/diff/140001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:3743: "class { set m() {) }",  // missing required
param
How about

  class { static static() {} }

https://codereview.chromium.org/561913002/diff/140001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:3876: "get constructor(_) {}",
"static constructor"? (Or is that actually valid? Then add a positive test.)

https://codereview.chromium.org/561913002/diff/140001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:3910: // We currently do not allow any duplicate
properties in class bodies. This
Bogus comment copy & paste?

[arv (Not doing code reviews), 2014-09-15 15:12:19]

Code review updates

[arv (Not doing code reviews), 2014-09-15 15:12:33]

Updated. PTAL.

https://codereview.chromium.org/561913002/diff/140001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/561913002/diff/140001/src/ast.h#newcode1464
src/ast.h:1464: bool is_static);
On 2014/09/15 12:32:48, rossberg wrote:
> Nit: formatting

Done.

https://codereview.chromium.org/561913002/diff/140001/src/ast.h#newcode1492
src/ast.h:1492: bool is_static_;
On 2014/09/15 12:32:48, rossberg wrote:
> How about avoiding this flag by simply putting static and non-static
properties
> into two separate lists?

See later comment.

https://codereview.chromium.org/561913002/diff/140001/src/full-codegen.cc
File src/full-codegen.cc (right):

https://codereview.chromium.org/561913002/diff/140001/src/full-codegen.cc#new...
src/full-codegen.cc:1551:
context()->Plug(isolate()->factory()->undefined_value());
Another option would be to throw `new Error('Not yet implemented')`. Would that
be better?

https://codereview.chromium.org/561913002/diff/140001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/561913002/diff/140001/src/parser.cc#newcode1961
src/parser.cc:1961: Interface* interface = Interface::NewValue();
On 2014/09/15 12:32:48, rossberg wrote:
> Nit: inline into call

Done.

https://codereview.chromium.org/561913002/diff/140001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/561913002/diff/140001/src/preparser.h#newcode638
src/preparser.h:638: bool IsEvalOrArguments() const { return type_ >=
kEvalIdentifier; }
On 2014/09/15 12:32:48, rossberg wrote:
> On 2014/09/15 09:24:29, marja wrote:
> > Nit: I'd just get rid of the >= trickery altogether (I don't see any real
> reason
> > for it and it's just error prone), and add Prototype and Constructor at the
> end
> > of the enum instead of in the middle.
> 
> I agree.

Done.

https://codereview.chromium.org/561913002/diff/140001/src/preparser.h#newcode...
src/preparser.h:1980: } else if (in_class && name_is_static && !is_static) {
On 2014/09/15 12:32:48, rossberg wrote:
> Hm, I'd prefer pulling recognising 'static' out of this method and avoid both
> the funny recursion and computing the extra flag in ParsePropertyName. Let the
> caller do it.

That gets a lot more complicated. The problem is that seeing 'static' does not
mean that it is a static property. You have to do a look ahead to determine that
before calling ParsePropertyDefinition. Consider the following valid cases, all
starting with static.

static() { }
static m() { }
static 'm'() { }
static "m"() { }
static 42() { }
static [expr]() { }
static *m() { }
static *'m'() { }
static *"m"() { }
static *42() { }
static *[expr]() { }

https://codereview.chromium.org/561913002/diff/140001/src/preparser.h#newcode...
src/preparser.h:2723: this->NewScope(scope_, BLOCK_SCOPE);
On 2014/09/15 12:32:48, rossberg wrote:
> Why does the extends clause need its own scope?

This is premature. My thinking was this.

1. The extends expression is supposed to be strict so we I need to wrap that in
a block.
2. The name of the class is not supposed to be in scope for the extends
expression but for the class body.

I could do one scope for the extends expression and then close that scope. And
then a second one for the class body. At least the scope depth is only 1 in that
case.

Either way. This needs more work when I implement the actual visibility of the
class name.

https://codereview.chromium.org/561913002/diff/140001/src/preparser.h#newcode...
src/preparser.h:2738: typename Traits::Type::ScopePtr scope =
this->NewScope(scope_, BLOCK_SCOPE);
On 2014/09/15 12:32:48, rossberg wrote:
> I'm not sure it will work out to reuse BLOCK_SCOPE here. We probably need a
> separate CLASS_SCOPE. At least it would be cleaner. (I'm fine with making that
a
> TODO for now.)

I agree that this is not obvious at this point. I'll add a todo. I need to
revisit this area anyway.

https://codereview.chromium.org/561913002/diff/140001/src/preparser.h#newcode...
src/preparser.h:2745: const bool is_static = false;
On 2014/09/15 12:32:48, rossberg wrote:
> Nit: If you find it worth naming these constants, then at least move them next
> to the call using them.

Done.

https://codereview.chromium.org/561913002/diff/140001/src/preparser.h#newcode...
src/preparser.h:2755: ObjectLiteralPropertyT property =
this->ParsePropertyDefinition(
On 2014/09/15 12:32:48, rossberg wrote:
> I'd prefer to check for 'static' here. That would also make it easy to store
> static and non-static properties in separate lists, and avoid the extra flag
on
> their AST node.

Storing two separate list is not going to work because computed properties makes
the ordering observable.

let i = 0;
class C {
  [(assert(i === 0), i++)]() {}
  static [(assert(i === 1), i++)]() {}
  [(assert(i === 2), i++)]() {}
}

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

https://codereview.chromium.org/561913002/diff/140001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:3743: "class { set m() {) }",  // missing required
param
On 2014/09/15 12:32:49, rossberg wrote:
> How about
> 
>   class { static static() {} }

That is not an error. This case is covered by NoErrorsClassPropertyName.

https://codereview.chromium.org/561913002/diff/140001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:3743: "class { set m() {) }",  // missing required
param
This has another syntax error though... fixing.

https://codereview.chromium.org/561913002/diff/140001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:3876: "get constructor(_) {}",
On 2014/09/15 12:32:49, rossberg wrote:
> "static constructor"? (Or is that actually valid? Then add a positive test.)

That is valid. Adding test.

The spec says that `static constructor()` is valid but `static get
constructor()` and `static set constructor(_)` is invalid. Filed bug:
https://bugs.ecmascript.org/show_bug.cgi?id=3223

https://codereview.chromium.org/561913002/diff/140001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:3910: // We currently do not allow any duplicate
properties in class bodies. This
On 2014/09/15 12:32:49, rossberg wrote:
> Bogus comment copy & paste?

Done.

[arv (Not doing code reviews), 2014-09-16 14:55:41]

Remove extra scope for now

[arv (Not doing code reviews), 2014-09-16 14:56:44]

Andreas, do you think you'll have time to take another look today?

Dmitry, I would also appreciate you taking a look at this.

[rossberg, 2014-09-16 15:25:40]

LGTM, just a few minor comments

https://codereview.chromium.org/561913002/diff/140001/src/full-codegen.cc
File src/full-codegen.cc (right):

https://codereview.chromium.org/561913002/diff/140001/src/full-codegen.cc#new...
src/full-codegen.cc:1551:
context()->Plug(isolate()->factory()->undefined_value());
On 2014/09/15 15:12:32, arv wrote:
> Another option would be to throw `new Error('Not yet implemented')`. Would
that
> be better?

No, I actually like this better. It allows you to already write JS tests, e.g.
to test parsing.

https://codereview.chromium.org/561913002/diff/140001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/561913002/diff/140001/src/preparser.h#newcode...
src/preparser.h:2723: this->NewScope(scope_, BLOCK_SCOPE);
On 2014/09/15 15:12:32, arv wrote:
> On 2014/09/15 12:32:48, rossberg wrote:
> > Why does the extends clause need its own scope?
> 
> This is premature. My thinking was this.
> 
> 1. The extends expression is supposed to be strict so we I need to wrap that
in
> a block.

I see, oh dear. Having a strict pseudo scope for that probably makes sense,
although it should perhaps not be a block scope.

> 2. The name of the class is not supposed to be in scope for the extends
> expression but for the class body.

I don't see how that one is related, though. The name is only added to the inner
scope anyway, isn't it?

> I could do one scope for the extends expression and then close that scope. And
> then a second one for the class body. At least the scope depth is only 1 in
that
> case.

Yes, I would prefer that, since it doesn't mess up the scope/context chain with
an empty scope.

> Either way. This needs more work when I implement the actual visibility of the
> class name.

https://codereview.chromium.org/561913002/diff/140001/src/preparser.h#newcode...
src/preparser.h:2755: ObjectLiteralPropertyT property =
this->ParsePropertyDefinition(
On 2014/09/15 15:12:32, arv wrote:
> On 2014/09/15 12:32:48, rossberg wrote:
> > I'd prefer to check for 'static' here. That would also make it easy to store
> > static and non-static properties in separate lists, and avoid the extra flag
> on
> > their AST node.
> 
> Storing two separate list is not going to work because computed properties
makes
> the ordering observable.
> 
> let i = 0;
> class C {
>   [(assert(i === 0), i++)]() {}
>   static [(assert(i === 1), i++)]() {}
>   [(assert(i === 2), i++)]() {}
> }

Oh hell. Make sure to add the above as a test case in time. :)

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

https://codereview.chromium.org/561913002/diff/180001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:3899: "static set constructor(_) {}",
I assume "constructor(){} static constructor(){}" and variations are legal as
well?

[arv (Not doing code reviews), 2014-09-16 15:46:41]

Add disabled test for duplicate property issue

[arv (Not doing code reviews), 2014-09-16 15:58:59]

git rebase

[Dmitry Lomov (no reviews), 2014-09-16 18:14:02]

lgtm

[arv (Not doing code reviews), 2014-09-16 22:15:58]

Committed patchset #12 (id:220001) manually as 23988 (presubmit successful).