harmony-classes: Implement 'super(...)' call syntactic restriction.

harmony-classes: Implement 'super(...)' call syntactic restriction.

R=rossberg@chromium.org,arv@chromium.org
BUG=v8:3330
LOG=N

[Dmitry Lomov (no reviews), 2014-11-27 17:53:37]

PTAL

[rossberg, 2014-11-27 19:04:40]

LGTM with comments

https://codereview.chromium.org/766663003/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/766663003/diff/1/src/compiler.cc#newcode792
src/compiler.cc:792: Statement* stmt = body->at(super_call_index);
Perhaps add

CHECK(super_call_index < body->length())  // We know there is a super call.

before this line, for extra clarity.

https://codereview.chromium.org/766663003/diff/1/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/766663003/diff/1/src/messages.js#newcode185
src/messages.js:185: super_constructor_call:        ["'super(...)' constructor
call is currently only supported if it is the first statement of a constructor
and its arguments do not access 'this'"],
Suggestion:

"A 'super' constructor call may only appear as the first statement of a
function, and its arguments may not access 'this'. Other forms are not yet
supported."

https://codereview.chromium.org/766663003/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/766663003/diff/1/src/objects.h#newcode6834
src/objects.h:6834: // Indicates that this function uses super property. This is
needed to set up
Nit: ...uses a super property.

https://codereview.chromium.org/766663003/diff/1/src/objects.h#newcode6839
src/objects.h:6839: // Indicates that this function uses super constructor.
Nit: ...uses the super constructor.

https://codereview.chromium.org/766663003/diff/1/test/mjsunit/harmony/classes.js
File test/mjsunit/harmony/classes.js (right):

https://codereview.chromium.org/766663003/diff/1/test/mjsunit/harmony/classes...
test/mjsunit/harmony/classes.js:782: (function
TestSuperCallSyntacticRestriction() {
Add tests for nested calls, e.g.:

  constructor() { { super() } }
  constructor() { if (1) { super() } }

Also add some positive tests here, e.g.:

  constructor() {}
  constructor() { super() }
  constructor() { 0 }
  constructor() { "" }
  constructor() { ""; super() }

[Dmitry Lomov (no reviews), 2014-11-27 19:41:57]

Comments addressed, landing

https://codereview.chromium.org/766663003/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/766663003/diff/1/src/compiler.cc#newcode792
src/compiler.cc:792: Statement* stmt = body->at(super_call_index);
On 2014/11/27 19:04:40, rossberg wrote:
> Perhaps add
> 
> CHECK(super_call_index < body->length())  // We know there is a super call.
> 
> before this line, for extra clarity.

Done.

https://codereview.chromium.org/766663003/diff/1/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/766663003/diff/1/src/messages.js#newcode185
src/messages.js:185: super_constructor_call:        ["'super(...)' constructor
call is currently only supported if it is the first statement of a constructor
and its arguments do not access 'this'"],
On 2014/11/27 19:04:40, rossberg wrote:
> Suggestion:
> 
> "A 'super' constructor call may only appear as the first statement of a
> function, and its arguments may not access 'this'. Other forms are not yet
> supported."

Done.

https://codereview.chromium.org/766663003/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/766663003/diff/1/src/objects.h#newcode6834
src/objects.h:6834: // Indicates that this function uses super property. This is
needed to set up
On 2014/11/27 19:04:40, rossberg wrote:
> Nit: ...uses a super property.

Done.
Russian has no articles, so I keep messing them up :)

https://codereview.chromium.org/766663003/diff/1/src/objects.h#newcode6839
src/objects.h:6839: // Indicates that this function uses super constructor.
On 2014/11/27 19:04:40, rossberg wrote:
> Nit: ...uses the super constructor.

Done.

https://codereview.chromium.org/766663003/diff/1/test/mjsunit/harmony/classes.js
File test/mjsunit/harmony/classes.js (right):

https://codereview.chromium.org/766663003/diff/1/test/mjsunit/harmony/classes...
test/mjsunit/harmony/classes.js:782: (function
TestSuperCallSyntacticRestriction() {
On 2014/11/27 19:04:40, rossberg wrote:
> Add tests for nested calls, e.g.:
> 
>   constructor() { { super() } }
>   constructor() { if (1) { super() } }
> 


> Also add some positive tests here, e.g.:
> 
>   constructor() {}
>   constructor() { super() }
>   constructor() { 0 }
>   constructor() { "" }
>   constructor() { ""; super() }


Done.

[Dmitry Lomov (no reviews), 2014-11-27 19:42:01]

The CQ bit was checked by dslomov@chromium.org

[commit-bot: I haz the power, 2014-11-27 19:42:48]

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

[commit-bot: I haz the power, 2014-11-28 04:08:56]

Committed patchset #2 (id:20001)

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

I thought the design we agreed upon was to allow statements and expressions that
did not reference `this`?

I can change this to work in that fashion now that I am back.

https://codereview.chromium.org/766663003/diff/20001/src/ast-this-access-visi...
File src/ast-this-access-visitor.cc (right):

https://codereview.chromium.org/766663003/diff/20001/src/ast-this-access-visi...
src/ast-this-access-visitor.cc:55: void ATAV::VisitBlock(Block* stmt) {
VisitStatements(stmt->statements()); }
I'm surprised that we don't have a base class that does all of this already?

https://codereview.chromium.org/766663003/diff/20001/src/ast-this-access-visi...
src/ast-this-access-visitor.cc:98: VisitIfNotNull(e->constructor());
constructor cannot be null. If there isn't a user defined constructor we create
the default constructor.

https://codereview.chromium.org/766663003/diff/20001/src/ast-this-access-visi...
File src/ast-this-access-visitor.h (right):

https://codereview.chromium.org/766663003/diff/20001/src/ast-this-access-visi...
src/ast-this-access-visitor.h:33: }  // namespace v8::intrenal
typo

https://codereview.chromium.org/766663003/diff/20001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/766663003/diff/20001/src/compiler.cc#newcode772
src/compiler.cc:772: MessageLocation location(info->script(),
lit->start_position(),
This position is not all that good.

https://codereview.chromium.org/766663003/diff/20001/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/766663003/diff/20001/src/messages.js#newcode186
src/messages.js:186: super_constructor_call:        ["A 'super' constructor call
may only appear as the first statement of a function, and its arguments may not
access 'this'. Other forms are not yet supported."]
I think you fixed the duplicate property in a follow up CL?

https://codereview.chromium.org/766663003/diff/20001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/766663003/diff/20001/src/scopes.cc#newcode905
src/scopes.cc:905: if (scope_uses_super_constructor_call_) {
Inconsistent use of {}

https://codereview.chromium.org/766663003/diff/20001/test/mjsunit/harmony/cla...
File test/mjsunit/harmony/classes.js (right):

https://codereview.chromium.org/766663003/diff/20001/test/mjsunit/harmony/cla...
test/mjsunit/harmony/classes.js:786: var y;
Why should this throw? I thought the design we agreed upon was to allow
statements and expressions that did not reference `this`?

https://codereview.chromium.org/766663003/diff/20001/test/mjsunit/harmony/cla...
test/mjsunit/harmony/classes.js:851: }());
Can you add a test for `super.method()` too?

class C extends B {
  constructor() {
    super.method();
    super();
  }
}

[Dmitry Lomov (no reviews), 2014-12-01 15:37:03]

I'll address other comments in a separate CL.

On 2014/12/01 15:14:07, arv wrote:
> I thought the design we agreed upon was to allow statements and expressions
that
> did not reference `this`?
> 
> I can change this to work in that fashion now that I am back.

The proper design would be to use a TDZ for this. I do not think we should
expand purely syntactic restriction to more than one statement
(e.g. what about
   if (1) { super(); }
or 
  { try { super(); } }
?


> 
>
https://codereview.chromium.org/766663003/diff/20001/src/ast-this-access-visi...
> File src/ast-this-access-visitor.cc (right):
> 
>
https://codereview.chromium.org/766663003/diff/20001/src/ast-this-access-visi...
> src/ast-this-access-visitor.cc:55: void ATAV::VisitBlock(Block* stmt) {
> VisitStatements(stmt->statements()); }
> I'm surprised that we don't have a base class that does all of this already?

Base recursive visitors are considered harmful. If you have to spell all cases
in all visitors, then you have to think about every visitor when you add an AST
node.
If there is a base recursive visitor, it is easy to forget to update some of
them.
> 
>
https://codereview.chromium.org/766663003/diff/20001/src/ast-this-access-visi...
> src/ast-this-access-visitor.cc:98: VisitIfNotNull(e->constructor());
> constructor cannot be null. If there isn't a user defined constructor we
create
> the default constructor.
> 
>
https://codereview.chromium.org/766663003/diff/20001/src/ast-this-access-visi...
> File src/ast-this-access-visitor.h (right):
> 
>
https://codereview.chromium.org/766663003/diff/20001/src/ast-this-access-visi...
> src/ast-this-access-visitor.h:33: }  // namespace v8::intrenal
> typo
> 
> https://codereview.chromium.org/766663003/diff/20001/src/compiler.cc
> File src/compiler.cc (right):
> 
>
https://codereview.chromium.org/766663003/diff/20001/src/compiler.cc#newcode772
> src/compiler.cc:772: MessageLocation location(info->script(),
> lit->start_position(),
> This position is not all that good.
> 
> https://codereview.chromium.org/766663003/diff/20001/src/messages.js
> File src/messages.js (right):
> 
>
https://codereview.chromium.org/766663003/diff/20001/src/messages.js#newcode186
> src/messages.js:186: super_constructor_call:        ["A 'super' constructor
call
> may only appear as the first statement of a function, and its arguments may
not
> access 'this'. Other forms are not yet supported."]
> I think you fixed the duplicate property in a follow up CL?

Yes. 
> 
> https://codereview.chromium.org/766663003/diff/20001/src/scopes.cc
> File src/scopes.cc (right):
> 
> https://codereview.chromium.org/766663003/diff/20001/src/scopes.cc#newcode905
> src/scopes.cc:905: if (scope_uses_super_constructor_call_) {
> Inconsistent use of {}
> 
>
https://codereview.chromium.org/766663003/diff/20001/test/mjsunit/harmony/cla...
> File test/mjsunit/harmony/classes.js (right):
> 
>
https://codereview.chromium.org/766663003/diff/20001/test/mjsunit/harmony/cla...
> test/mjsunit/harmony/classes.js:786: var y;
> Why should this throw? I thought the design we agreed upon was to allow
> statements and expressions that did not reference `this`?
> 
>
https://codereview.chromium.org/766663003/diff/20001/test/mjsunit/harmony/cla...
> test/mjsunit/harmony/classes.js:851: }());
> Can you add a test for `super.method()` too?
> 
> class C extends B {
>   constructor() {
>     super.method();
>     super();
>   }
> }