Classes: Implement correct name binding

Classes: Implement correct name binding

Named class declarations and class expression have a const binding for
the name that is in TDZ for the extends expression.

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

Committed: https://chromium.googlesource.com/v8/v8/+/4fe564d85f08346c1dca35e7e4eef210de604120

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

PTAL

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

https://codereview.chromium.org/722793005/diff/1/src/parser.cc#newcode3922
src/parser.cc:3922: ClassLiteral* Parser::ParseClassLiteral(const AstRawString*
name,
I had to split the code from ParserBase into Parser and PreParser specific
implementations :'(

The parser base cannot handle variables and bindings.

On the plus side, it means that the pre parser got simpler.

[adamk, 2014-11-13 01:45:44]

Just a style point, I don't yet feel comfortable enough with the parser to offer
much insight on this patch.

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

https://codereview.chromium.org/722793005/diff/1/src/parser.cc#newcode3932
src/parser.cc:3932: if (this->IsEvalOrArguments(name)) {
I don't think you need these explicit |this| references now that your code is in
{,pre}parser.cc

Same goes everywhere below where "this->" appears.

https://codereview.chromium.org/722793005/diff/1/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/722793005/diff/1/src/preparser.cc#newcode946
src/preparser.cc:946: return this->EmptyExpression();
"this->" unnecessary here and below, as well.

[Dmitry Lomov (no reviews), 2014-11-13 11:17:38]

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

https://codereview.chromium.org/722793005/diff/1/src/ast.h#newcode2675
src/ast.h:2675: VariableProxy* proxy() const { return proxy_; }
Nit: rename proxy => class_variable_proxy (also field)

"Proxy" is way overloaded.

https://codereview.chromium.org/722793005/diff/1/src/ast.h#newcode2684
src/ast.h:2684: VariableProxy* proxy, Expression* extends,
proxy => class_variable_proxy

https://codereview.chromium.org/722793005/diff/1/src/ast.h#newcode2699
src/ast.h:2699: VariableProxy* proxy_;
proxy_ => class_variable_proxy_

https://codereview.chromium.org/722793005/diff/1/src/ast.h#newcode3684
src/ast.h:3684: VariableProxy* proxy, Expression* extends,
proxy => class_variable_proxy

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

https://codereview.chromium.org/722793005/diff/1/src/full-codegen.cc#newcode1613
src/full-codegen.cc:1613: Comment cmnt(masm_, "[ Extend block context");
Please share code with VisitBlock here (add an EnterBlockScope method)

We will also need bailout ids here in the future, when/if we
hydrogenize/turbofanize this. 
I suggest extracting that part too (so both context pushing and visiting
declarations below are part of EnterBlockScope method), parameterizing
EnterBlockScope with BailoutIds and for now passing BailoutId::None here.

https://codereview.chromium.org/722793005/diff/1/src/full-codegen.cc#newcode1651
src/full-codegen.cc:1651: LoadContextField(context_register(),
Context::PREVIOUS_INDEX);
Please share code with VisitBlock here (add an ExitBlockScope method)

Same as above for BailoutIds.

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

https://codereview.chromium.org/722793005/diff/1/src/parser.cc#newcode3976
src/parser.cc:3976: if (has_seen_constructor != old_has_seen_constructor) {
Nit: replace this condition with 'constructor == NULL && has_seen_constructor'
and remove old_has_seen_constructor.

https://codereview.chromium.org/722793005/diff/1/src/parser.cc#newcode3991
src/parser.cc:3991: if (!has_seen_constructor) {
Nit: replace this condition with 'constructor == NULL'

[arv (Not doing code reviews), 2014-11-13 16:59:40]

git rebase

[arv (Not doing code reviews), 2014-11-13 20:45:04]

Round two

[arv (Not doing code reviews), 2014-11-13 20:46:27]

Round two

[arv (Not doing code reviews), 2014-11-13 20:46:39]

Patchset #3 (id:40001) has been deleted

[arv (Not doing code reviews), 2014-11-13 20:47:50]

Ready for round two. PTAL.

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

https://codereview.chromium.org/722793005/diff/1/src/ast.h#newcode2675
src/ast.h:2675: VariableProxy* proxy() const { return proxy_; }
On 2014/11/13 11:17:37, Dmitry Lomov (chromium) wrote:
> Nit: rename proxy => class_variable_proxy (also field)
> 
> "Proxy" is way overloaded.

Done.

https://codereview.chromium.org/722793005/diff/1/src/ast.h#newcode2684
src/ast.h:2684: VariableProxy* proxy, Expression* extends,
On 2014/11/13 11:17:37, Dmitry Lomov (chromium) wrote:
> proxy => class_variable_proxy

Done.

https://codereview.chromium.org/722793005/diff/1/src/ast.h#newcode2699
src/ast.h:2699: VariableProxy* proxy_;
On 2014/11/13 11:17:37, Dmitry Lomov (chromium) wrote:
> proxy_ => class_variable_proxy_

Done.

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

https://codereview.chromium.org/722793005/diff/1/src/full-codegen.cc#newcode1613
src/full-codegen.cc:1613: Comment cmnt(masm_, "[ Extend block context");
On 2014/11/13 11:17:37, Dmitry Lomov (chromium) wrote:
> Please share code with VisitBlock here (add an EnterBlockScope method)
> 
> We will also need bailout ids here in the future, when/if we
> hydrogenize/turbofanize this. 
> I suggest extracting that part too (so both context pushing and visiting
> declarations below are part of EnterBlockScope method), parameterizing
> EnterBlockScope with BailoutIds and for now passing BailoutId::None here.

Done.

https://codereview.chromium.org/722793005/diff/1/src/full-codegen.cc#newcode1651
src/full-codegen.cc:1651: LoadContextField(context_register(),
Context::PREVIOUS_INDEX);
On 2014/11/13 11:17:37, Dmitry Lomov (chromium) wrote:
> Please share code with VisitBlock here (add an ExitBlockScope method)
> 
> Same as above for BailoutIds.

Done.

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

https://codereview.chromium.org/722793005/diff/1/src/parser.cc#newcode3932
src/parser.cc:3932: if (this->IsEvalOrArguments(name)) {
On 2014/11/13 01:45:43, adamk wrote:
> I don't think you need these explicit |this| references now that your code is
in
> {,pre}parser.cc
> 
> Same goes everywhere below where "this->" appears.

Done.

https://codereview.chromium.org/722793005/diff/1/src/parser.cc#newcode3976
src/parser.cc:3976: if (has_seen_constructor != old_has_seen_constructor) {
On 2014/11/13 11:17:37, Dmitry Lomov (chromium) wrote:
> Nit: replace this condition with 'constructor == NULL && has_seen_constructor'
> and remove old_has_seen_constructor.

Done.

https://codereview.chromium.org/722793005/diff/1/src/parser.cc#newcode3991
src/parser.cc:3991: if (!has_seen_constructor) {
On 2014/11/13 11:17:37, Dmitry Lomov (chromium) wrote:
> Nit: replace this condition with 'constructor == NULL' 

Done.

https://codereview.chromium.org/722793005/diff/1/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/722793005/diff/1/src/preparser.cc#newcode946
src/preparser.cc:946: return this->EmptyExpression();
On 2014/11/13 01:45:44, adamk wrote:
> "this->" unnecessary here and below, as well.

Done.

[Dmitry Lomov (no reviews), 2014-11-13 21:10:37]

looking good, just a couple comments

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

https://codereview.chromium.org/722793005/diff/60001/src/full-codegen.cc#newc...
src/full-codegen.cc:1581: this, lit->scope(), BailoutId::None(),
BailoutId::None(),
Nice!

https://codereview.chromium.org/722793005/diff/60001/src/full-codegen.cc#newc...
src/full-codegen.cc:1785: codegen_->PrepareForBailoutForId(entry_id,
NO_REGISTERS);
This should crash under --always-opt when entry_id == BailoutId::kNone. You will
have to protect it here specifically.

While you are here, maybe add a DCHECK in PrepareForBailoutForId that the id it
not None.

https://codereview.chromium.org/722793005/diff/60001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/722793005/diff/60001/src/parser.cc#newcode3960
src/parser.cc:3960: ObjectLiteral::Property* property = ParsePropertyDefinition(
Nit: move has_seen_constructor variable here (it is not used outside the look
AFAICS)

https://codereview.chromium.org/722793005/diff/60001/src/parser.cc#newcode3980
src/parser.cc:3980: this->DefaultConstructor(extends != NULL, block_scope, pos,
end_pos);
Nit: remove 'this->'

[arv (Not doing code reviews), 2014-11-13 21:56:24]

Remove this->

[arv (Not doing code reviews), 2014-11-13 21:56:29]

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

https://codereview.chromium.org/722793005/diff/60001/src/full-codegen.cc#newc...
src/full-codegen.cc:1785: codegen_->PrepareForBailoutForId(entry_id,
NO_REGISTERS);
On 2014/11/13 21:10:37, Dmitry Lomov (chromium) wrote:
> This should crash under --always-opt when entry_id == BailoutId::kNone. You
will
> have to protect it here specifically.
> 
> While you are here, maybe add a DCHECK in PrepareForBailoutForId that the id
it
> not None.

Since ClassLiteral is not optimized this is fine.

https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/full-codege...

https://codereview.chromium.org/722793005/diff/60001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/722793005/diff/60001/src/parser.cc#newcode3960
src/parser.cc:3960: ObjectLiteral::Property* property = ParsePropertyDefinition(
On 2014/11/13 21:10:37, Dmitry Lomov (chromium) wrote:
> Nit: move has_seen_constructor variable here (it is not used outside the look
> AFAICS)

It needs to be done outside the loop since ParsePropertyDefinition returns a
parse error if it finds a second constructor.

https://codereview.chromium.org/722793005/diff/60001/src/parser.cc#newcode3980
src/parser.cc:3980: this->DefaultConstructor(extends != NULL, block_scope, pos,
end_pos);
On 2014/11/13 21:10:37, Dmitry Lomov (chromium) wrote:
> Nit: remove 'this->'

Done.

[Dmitry Lomov (no reviews), 2014-11-14 06:23:06]

lgtm if tests pass. I am still curious about BailoutIds though.

On 2014/11/13 21:56:29, arv wrote:
> https://codereview.chromium.org/722793005/diff/60001/src/full-codegen.cc
> File src/full-codegen.cc (right):
> 
>
https://codereview.chromium.org/722793005/diff/60001/src/full-codegen.cc#newc...
> src/full-codegen.cc:1785: codegen_->PrepareForBailoutForId(entry_id,
> NO_REGISTERS);
> On 2014/11/13 21:10:37, Dmitry Lomov (chromium) wrote:
> > This should crash under --always-opt when entry_id == BailoutId::kNone. You
> will
> > have to protect it here specifically.
> > 
> > While you are here, maybe add a DCHECK in PrepareForBailoutForId that the id
> it
> > not None.
> 
> Since ClassLiteral is not optimized this is fine.

Hmm, I though even though ClassLiteral is not optimized we can still compile
with deopt data support and this will break.
Maybe that has changed?

> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/full-codege...
> 
> https://codereview.chromium.org/722793005/diff/60001/src/parser.cc
> File src/parser.cc (right):
> 
> https://codereview.chromium.org/722793005/diff/60001/src/parser.cc#newcode3960
> src/parser.cc:3960: ObjectLiteral::Property* property =
ParsePropertyDefinition(
> On 2014/11/13 21:10:37, Dmitry Lomov (chromium) wrote:
> > Nit: move has_seen_constructor variable here (it is not used outside the
look
> > AFAICS)
> 
> It needs to be done outside the loop since ParsePropertyDefinition returns a
> parse error if it finds a second constructor.

Ah, got it. Sorry for the noise.
> 
> https://codereview.chromium.org/722793005/diff/60001/src/parser.cc#newcode3980
> src/parser.cc:3980: this->DefaultConstructor(extends != NULL, block_scope,
pos,
> end_pos);
> On 2014/11/13 21:10:37, Dmitry Lomov (chromium) wrote:
> > Nit: remove 'this->'
> 
> Done.

[arv (Not doing code reviews), 2014-11-14 14:22:06]

The CQ bit was checked by arv@chromium.org

[commit-bot: I haz the power, 2014-11-14 14:22:54]

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

[commit-bot: I haz the power, 2014-11-14 14:24:33]

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

[commit-bot: I haz the power, 2014-11-14 14:24:34]

Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/1321)
  v8_win64_compile_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_win64_compile_rel/builds...)
  v8_win_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel/builds/1359)

[arv (Not doing code reviews), 2014-11-14 14:25:46]

The CQ bit was unchecked by arv@chromium.org

[arv (Not doing code reviews), 2014-11-14 14:36:47]

git rebase

[arv (Not doing code reviews), 2014-11-14 14:36:55]

The CQ bit was checked by arv@chromium.org

[commit-bot: I haz the power, 2014-11-14 14:38:00]

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

[commit-bot: I haz the power, 2014-11-14 15:05:17]

Committed patchset #5 (id:100001)

[marja, 2015-02-10 11:31:48]

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

[marja, 2015-02-10 11:31:49]

post-commit comments:

https://codereview.chromium.org/722793005/diff/100001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/722793005/diff/100001/src/parser.cc#newcode3897
src/parser.cc:3897: ClassLiteral* Parser::ParseClassLiteral(const AstRawString*
name,
Is there a reason why ParseClassLiteral is not in ParserBase, but both in Parser
and PreParser (so that there are 2 versions of it)? Is something missing in the
PreParser? Is there some mess in the code that makes it hard?

All other Expression parsing is unified (so, implemented only once, in
ParserBase), so this is moving the situation backwards...

[arv (Not doing code reviews), 2015-02-10 14:32:11]

On 2015/02/10 11:31:49, marja wrote:
> post-commit comments:
> 
> https://codereview.chromium.org/722793005/diff/100001/src/parser.cc
> File src/parser.cc (right):
> 
>
https://codereview.chromium.org/722793005/diff/100001/src/parser.cc#newcode3897
> src/parser.cc:3897: ClassLiteral* Parser::ParseClassLiteral(const
AstRawString*
> name,
> Is there a reason why ParseClassLiteral is not in ParserBase, but both in
Parser
> and PreParser (so that there are 2 versions of it)? Is something missing in
the
> PreParser? Is there some mess in the code that makes it hard?
> 
> All other Expression parsing is unified (so, implemented only once, in
> ParserBase), so this is moving the situation backwards...

ClassExpression needs to set up an inner name binding, similar to how
FunctionExpression sets up a binding. The difference between the two is this
code. Another difference is that Parser creates a DefaultConstructor if needed.

It seems like the PreParser might be able to handle that after all. Let me look
into it a bit more.

[arv (Not doing code reviews), 2015-02-10 17:11:36]

On 2015/02/10 14:32:11, arv wrote:
> On 2015/02/10 11:31:49, marja wrote:
> > post-commit comments:
> > 
> > https://codereview.chromium.org/722793005/diff/100001/src/parser.cc
> > File src/parser.cc (right):
> > 
> >
>
https://codereview.chromium.org/722793005/diff/100001/src/parser.cc#newcode3897
> > src/parser.cc:3897: ClassLiteral* Parser::ParseClassLiteral(const
> AstRawString*
> > name,
> > Is there a reason why ParseClassLiteral is not in ParserBase, but both in
> Parser
> > and PreParser (so that there are 2 versions of it)? Is something missing in
> the
> > PreParser? Is there some mess in the code that makes it hard?
> > 
> > All other Expression parsing is unified (so, implemented only once, in
> > ParserBase), so this is moving the situation backwards...
> 
> ClassExpression needs to set up an inner name binding, similar to how
> FunctionExpression sets up a binding. The difference between the two is this
> code. Another difference is that Parser creates a DefaultConstructor if
needed.
> 
> It seems like the PreParser might be able to handle that after all. Let me
look
> into it a bit more.

One more thing that is making this less than optimal is that in Parser we use
Expression* like a Maybe type and use NULL as missin. For PreParser we don't
have a way to have these null expressions so we need to use an extra out
parameter.