[strong] Fix scoping related errors for methods.

[strong] Fix scoping related errors for methods.

Methods can refer to the class name.

BUG=v8:3927
LOG=N

Committed: https://crrev.com/4a709dd65837edd10c60c68e47ae8dc9727ecc33
Cr-Commit-Position: refs/heads/master@{#27075}

[marja, 2015-03-03 15:12:36]

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

[marja, 2015-03-03 15:12:37]

ptal.

I have no idea whether adding CLASS_SCOPE makes sense to begin with or whether
it'll break something unexpected (at least try jobs are happy). But we'd need
some way to reason about variables which are a class name (their initializer
position is only after the class scope, but methods can refer to them). So this
is a bit of a I-have-no-idea-what-I'm-doing CL. Pls advice. :)

[arv (Not doing code reviews), 2015-03-03 15:36:29]

https://codereview.chromium.org/968263002/diff/120001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/968263002/diff/120001/src/parser.cc#newcode4166
src/parser.cc:4166: // Unnamed classes should not have scopes (the class scope
will be empty).
Unnamed classed do not need to have a scope.

https://codereview.chromium.org/968263002/diff/120001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/968263002/diff/120001/src/scopes.cc#newcode1144
src/scopes.cc:1144: found_method_scope = true;
It is still not clear why this is only matching the class name?

https://codereview.chromium.org/968263002/diff/120001/test/mjsunit/strong/dec...
File test/mjsunit/strong/declaration-after-use.js (right):

https://codereview.chromium.org/968263002/diff/120001/test/mjsunit/strong/dec...
test/mjsunit/strong/declaration-after-use.js:184: class C4 { method() { C4;
method; } }; new C4();
This should be an error since method is an unresolved identifier.

Concise methods do not add a binding.

[marja, 2015-03-03 15:52:14]

Routing offline discussion here:

https://codereview.chromium.org/968263002/diff/120001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/968263002/diff/120001/src/parser.cc#newcode4166
src/parser.cc:4166: // Unnamed classes should not have scopes (the class scope
will be empty).
On 2015/03/03 15:36:28, arv wrote:
> Unnamed classed do not need to have a scope.

Clarification: this comment was b/c some places rely on unnamed classes not
having a scope. The places which rely on that are:

AstGraphBuilder::VisitClassLiteral
AstGraphBuilder::VisitClassLiteralContents
FullCodeGenerator::VisitClassLiteral calling EnterBlockScopeIfNeeded
FullCodeGenerator::VisitClassLiteral deciding whether to EmitVariableAssignment

https://codereview.chromium.org/968263002/diff/120001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/968263002/diff/120001/src/scopes.cc#newcode1144
src/scopes.cc:1144: found_method_scope = true;
On 2015/03/03 15:36:28, arv wrote:
> It is still not clear why this is only matching the class name?

This check only fires when we have resolved the VariableProxy against
*something* and then we reason that the *something* is a class name if it's
declared in the class scope. (The only declaration in that scope seems to be the
class name. (Empirically.))

https://codereview.chromium.org/968263002/diff/120001/test/mjsunit/strong/dec...
File test/mjsunit/strong/declaration-after-use.js (right):

https://codereview.chromium.org/968263002/diff/120001/test/mjsunit/strong/dec...
test/mjsunit/strong/declaration-after-use.js:184: class C4 { method() { C4;
method; } }; new C4();
On 2015/03/03 15:36:28, arv wrote:
> This should be an error since method is an unresolved identifier.
> 
> Concise methods do not add a binding.

This was actually not testing what I thought it was testing: "method" was
unresolved (we didn't return an error b/c we don't do check against the global
object yet) -> removed nonsensemaking tests.

[rossberg, 2015-03-04 10:39:27]

I think class_scope makes sense. Might be useful for other purposes as well
(e.g. devtools).

https://codereview.chromium.org/968263002/diff/140001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/968263002/diff/140001/src/scopes.cc#newcode802
src/scopes.cc:802: return "class";
Nit: format like others.

https://codereview.chromium.org/968263002/diff/140001/src/scopes.cc#newcode1144
src/scopes.cc:1144: found_method_scope = true;
Can't we simplify the logic by returning true here right away?

https://codereview.chromium.org/968263002/diff/140001/test/mjsunit/strong/dec...
File test/mjsunit/strong/declaration-after-use.js (right):

https://codereview.chromium.org/968263002/diff/140001/test/mjsunit/strong/dec...
test/mjsunit/strong/declaration-after-use.js:184: class C4 { method() { C4; } };
new C4();
PErhaps add a third test using a static method.

[marja, 2015-03-04 11:14:39]

Thanks for having a look.

dslomov@, class_scope, yay or nay?

https://codereview.chromium.org/968263002/diff/140001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/968263002/diff/140001/src/scopes.cc#newcode802
src/scopes.cc:802: return "class";
On 2015/03/04 10:39:27, rossberg wrote:
> Nit: format like others.

clang-format wants it this way :/ Instead, formatted the others like this.

https://codereview.chromium.org/968263002/diff/140001/src/scopes.cc#newcode1144
src/scopes.cc:1144: found_method_scope = true;
On 2015/03/04 10:39:27, rossberg wrote:
> Can't we simplify the logic by returning true here right away?

Done.

https://codereview.chromium.org/968263002/diff/140001/test/mjsunit/strong/dec...
File test/mjsunit/strong/declaration-after-use.js (right):

https://codereview.chromium.org/968263002/diff/140001/test/mjsunit/strong/dec...
test/mjsunit/strong/declaration-after-use.js:184: class C4 { method() { C4; } };
new C4();
On 2015/03/04 10:39:27, rossberg wrote:
> PErhaps add a third test using a static method.

Done.

[rossberg, 2015-03-04 12:39:33]

lgtm

https://codereview.chromium.org/968263002/diff/140001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/968263002/diff/140001/src/scopes.cc#newcode802
src/scopes.cc:802: return "class";
On 2015/03/04 11:14:38, marja wrote:
> On 2015/03/04 10:39:27, rossberg wrote:
> > Nit: format like others.
> 
> clang-format wants it this way :/ Instead, formatted the others like this.

I wish we would just fracking nuke this clang-format nonsense...

[Dmitry Lomov (no reviews), 2015-03-04 13:08:15]

Putting not lgtm marker just to prevent accidental landing.

The CLASS_SCOPE addition looks fine to me but we need debugger support for it as
well. In current state, this CL will break debugger.

You can just closely follow the support for BLOCK_SCOPE in your implementation.

[Dmitry Lomov (no reviews), 2015-03-06 14:48:01]

This approach looks good to me, and seems like CLASS_SCOPE is unnecessary
complication indeed. As discussed offline, this still does not report an error
in the following case:

class C {
  [ o = { m() { ... C ... } }; o.m();] { .... }
} 

but that be fixed in a later CL.

I have some test suggestions as well.

https://codereview.chromium.org/968263002/diff/200001/test/mjsunit/strong/dec...
File test/mjsunit/strong/declaration-after-use.js (right):

https://codereview.chromium.org/968263002/diff/200001/test/mjsunit/strong/dec...
test/mjsunit/strong/declaration-after-use.js:146: ReferenceError);
Add a test that tests the wrong behavior:
class C {
  [ { m() { ... C ... } }; "x"] { .... }
} 
with a bug reference

https://codereview.chromium.org/968263002/diff/200001/test/mjsunit/strong/dec...
test/mjsunit/strong/declaration-after-use.js:218: let C8 = class C9 { static
method() { C9; } }; new C8();
Add a test for inner class referencing outer class:

class COuter {
   m() {
       class CInner {
           n() {
                COuter();
           }
       }
   }
}

Also the same for object literal method

[marja, 2015-03-06 15:50:17]

Disclaimer, after patch set 10 this is a rewritten version:

1) No CLASS_SCOPE, we don't want to add "class context" which would basically be
the same as block context. Instead, allow tagging some block scopes as class
scopes.
2) Not all functions inside a class scope are methods, so we need to also store
the function kind in scope.
3) There was a bug; we would allow the class name reference inside *any* method,
we should only allow it inside *a method of that class*. The difference is
relevant if a computed property name has a method inside (see tests).


*But* the class name finding is not yet correct when we have methods inside
object literals. I need to fix that. At first I thought I could add errors for
methods-inside-object-literals later but no, this is too broken.

[marja, 2015-03-06 16:14:23]

No, I was confused. I was just failing w/ unnamed classes but I fixed that.

Now I still think that this makes sense as an step forward; we don't yet produce
errors for methods inside object literals inside computed property names (bug
filed) but the overall state is saner (stronger?) after this CL.

https://codereview.chromium.org/968263002/diff/200001/test/mjsunit/strong/dec...
File test/mjsunit/strong/declaration-after-use.js (right):

https://codereview.chromium.org/968263002/diff/200001/test/mjsunit/strong/dec...
test/mjsunit/strong/declaration-after-use.js:146: ReferenceError);
On 2015/03/06 14:48:01, Dmitry Lomov (chromium) wrote:
> Add a test that tests the wrong behavior:
> class C {
>   [ { m() { ... C ... } }; "x"] { .... }
> } 
> with a bug reference

Done.

https://codereview.chromium.org/968263002/diff/200001/test/mjsunit/strong/dec...
test/mjsunit/strong/declaration-after-use.js:218: let C8 = class C9 { static
method() { C9; } }; new C8();
On 2015/03/06 14:48:01, Dmitry Lomov (chromium) wrote:
> Add a test for inner class referencing outer class:
> 
> class COuter {
>    m() {
>        class CInner {
>            n() {
>                 COuter();
>            }
>        }
>    }
> }
> 
> Also the same for object literal method

Done (not sure what you meant w/ object literal method here, did you mean object
literal method inside a computed property name? ptal if the test I added matches
w/ what you meant.

[arv (Not doing code reviews), 2015-03-06 16:37:24]

I like this better.

https://codereview.chromium.org/968263002/diff/220001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/968263002/diff/220001/src/scopes.cc#newcode1455
src/scopes.cc:1455: if (!IsConciseMethod(function_kind_) &&
!IsConstructor(function_kind_)) {
This should include IsAccessorFunction too.

https://codereview.chromium.org/968263002/diff/220001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/968263002/diff/220001/src/scopes.h#newcode285
src/scopes.h:285: void tag_as_class_scope() { block_scope_is_class_scope_ =
true; }
DCHECK(is_block_scope());

[Dmitry Lomov (no reviews), 2015-03-07 18:17:11]

lgtm from my side % arv@'s and mine comments

https://codereview.chromium.org/968263002/diff/200001/test/mjsunit/strong/dec...
File test/mjsunit/strong/declaration-after-use.js (right):

https://codereview.chromium.org/968263002/diff/200001/test/mjsunit/strong/dec...
test/mjsunit/strong/declaration-after-use.js:218: let C8 = class C9 { static
method() { C9; } }; new C8();
On 2015/03/06 16:14:23, marja wrote:
> On 2015/03/06 14:48:01, Dmitry Lomov (chromium) wrote:
> > Add a test for inner class referencing outer class:
> > 
> > class COuter {
> >    m() {
> >        class CInner {
> >            n() {
> >                 COuter();
> >            }
> >        }
> >    }
> > }
> > 
> > Also the same for object literal method
> 
> Done (not sure what you meant w/ object literal method here, did you mean
object
> literal method inside a computed property name? ptal if the test I added
matches
> w/ what you meant.

Yes, it does

https://codereview.chromium.org/968263002/diff/220001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/968263002/diff/220001/src/scopes.cc#newcode1455
src/scopes.cc:1455: if (!IsConciseMethod(function_kind_) &&
!IsConstructor(function_kind_)) {
On 2015/03/06 16:37:24, arv wrote:
> This should include IsAccessorFunction too.

+ test

https://codereview.chromium.org/968263002/diff/220001/test/mjsunit/strong/dec...
File test/mjsunit/strong/declaration-after-use.js (right):

https://codereview.chromium.org/968263002/diff/220001/test/mjsunit/strong/dec...
test/mjsunit/strong/declaration-after-use.js:169: //         "        [({ m() {
CInner; return 'A'; } }).m()]() { return 'B'; }\n" +
line length

https://codereview.chromium.org/968263002/diff/220001/test/mjsunit/strong/dec...
test/mjsunit/strong/declaration-after-use.js:254: // Here we can refer to COuter
but not to CInner (see corresponding assertion test):
line length

[marja, 2015-03-09 10:01:38]

Thanks for comments! arv@, lgty?

https://codereview.chromium.org/968263002/diff/220001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/968263002/diff/220001/src/scopes.cc#newcode1455
src/scopes.cc:1455: if (!IsConciseMethod(function_kind_) &&
!IsConstructor(function_kind_)) {
On 2015/03/07 18:17:10, Dmitry Lomov (chromium) wrote:
> On 2015/03/06 16:37:24, arv wrote:
> > This should include IsAccessorFunction too.
> 
> + test

Done.

https://codereview.chromium.org/968263002/diff/220001/src/scopes.cc#newcode1455
src/scopes.cc:1455: if (!IsConciseMethod(function_kind_) &&
!IsConstructor(function_kind_)) {
On 2015/03/06 16:37:24, arv wrote:
> This should include IsAccessorFunction too.

Done.

https://codereview.chromium.org/968263002/diff/220001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/968263002/diff/220001/src/scopes.h#newcode285
src/scopes.h:285: void tag_as_class_scope() { block_scope_is_class_scope_ =
true; }
On 2015/03/06 16:37:24, arv wrote:
> DCHECK(is_block_scope());

Done.

https://codereview.chromium.org/968263002/diff/220001/test/mjsunit/strong/dec...
File test/mjsunit/strong/declaration-after-use.js (right):

https://codereview.chromium.org/968263002/diff/220001/test/mjsunit/strong/dec...
test/mjsunit/strong/declaration-after-use.js:169: //         "        [({ m() {
CInner; return 'A'; } }).m()]() { return 'B'; }\n" +
On 2015/03/07 18:17:10, Dmitry Lomov (chromium) wrote:
> line length

Done.

https://codereview.chromium.org/968263002/diff/220001/test/mjsunit/strong/dec...
test/mjsunit/strong/declaration-after-use.js:254: // Here we can refer to COuter
but not to CInner (see corresponding assertion test):
On 2015/03/07 18:17:10, Dmitry Lomov (chromium) wrote:
> line length

Done.

[rossberg, 2015-03-09 10:41:47]

lgtm

https://codereview.chromium.org/968263002/diff/240001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/968263002/diff/240001/src/scopes.cc#newcode1453
src/scopes.cc:1453: Variable* Scope::ClassVariableForMethod() const {
Nit: move this next to CheckStrongModeDeclaration

https://codereview.chromium.org/968263002/diff/240001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/968263002/diff/240001/src/scopes.h#newcode514
src/scopes.h:514: // Some block scopes are tagged as class scopes.
Yeah, I think this is fine for the time being, but it strikes me as somewhat
hacky. Classes aren't blocks. Long term, we should probably rethink the way we
distinguish various scope types. It might make sense to have subtypes there, but
then the naming should be properly adjusted. I'm not yet convinced it's worth
it, though.

[arv (Not doing code reviews), 2015-03-09 13:39:05]

LGTM

https://codereview.chromium.org/968263002/diff/240001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/968263002/diff/240001/src/parser.cc#newcode4177
src/parser.cc:4177: DCHECK(block_scope->num_var_or_const() == 0);
Prefer DCHECK_EQ etc in general

[marja, 2015-03-09 13:44:07]

thanks for review!

https://codereview.chromium.org/968263002/diff/240001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/968263002/diff/240001/src/parser.cc#newcode4177
src/parser.cc:4177: DCHECK(block_scope->num_var_or_const() == 0);
On 2015/03/09 13:39:05, arv wrote:
> Prefer DCHECK_EQ etc in general

Done.

https://codereview.chromium.org/968263002/diff/240001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/968263002/diff/240001/src/scopes.cc#newcode1453
src/scopes.cc:1453: Variable* Scope::ClassVariableForMethod() const {
On 2015/03/09 10:41:46, rossberg wrote:
> Nit: move this next to CheckStrongModeDeclaration

Done.

https://codereview.chromium.org/968263002/diff/240001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/968263002/diff/240001/src/scopes.h#newcode514
src/scopes.h:514: // Some block scopes are tagged as class scopes.
On 2015/03/09 10:41:46, rossberg wrote:
> Yeah, I think this is fine for the time being, but it strikes me as somewhat
> hacky. Classes aren't blocks. Long term, we should probably rethink the way we
> distinguish various scope types. It might make sense to have subtypes there,
but
> then the naming should be properly adjusted. I'm not yet convinced it's worth
> it, though.

It'll also depend on the object literal solution... at some point I thought
object literals would be another subtype of block scopes, but meh, I don't think
that's the right solution. I'm experimenting with some alternative solutions...

[marja, 2015-03-09 13:44:56]

The CQ bit was checked by marja@chromium.org

[marja, 2015-03-09 13:44:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
dslomov@chromium.org, arv@chromium.org, rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/968263002/#ps260001
(title: "code review (arv, rossberg)")

[commit-bot: I haz the power, 2015-03-09 13:45:38]

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

[commit-bot: I haz the power, 2015-03-09 14:30:35]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-03-09 14:30:56]

Patchset 14 (id:??) landed as
https://crrev.com/4a709dd65837edd10c60c68e47ae8dc9727ecc33
Cr-Commit-Position: refs/heads/master@{#27075}