harmony-scoping: make assignment to 'const' a late error.

harmony-scoping: make assignment to 'const' a late error.

Per TC39 Nov 2014 decision.

This patch also changes behavior for "legacy const": assignments to sloppy const in strict mode is now also a type error. This fixes v8:2243 and also brings us in compliance with other engines re assignment to function names (see updated webkit test), but might have bigger implications. 
That change can easily be reverted by changing Variable::IsSignallingAssignmentToConst.

BUG=v8:3713, v8:2243
LOG=N

[Dmitry Lomov (no reviews), 2014-11-21 19:26:49]

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

[Dmitry Lomov (no reviews), 2014-11-21 19:37:34]

PTAL

[adamk, 2014-11-24 18:59:39]

lgtm

[rossberg, 2014-11-25 15:22:29]

rossberg@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[rossberg, 2014-11-25 15:23:36]

mstarzinger should judge the change to the graph builder.

https://codereview.chromium.org/749633002/diff/80001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/749633002/diff/80001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:2016: if
(variable->IsSignallingAssignmentToConst(op, strict_mode())) {
I'm not sure I like this factorisation, since it breaks the symmetry with the
other cases below and thereby makes the code less readable. It also duplicates
some of the cases analysis. Better factor it out into the two relevant cases
below.

Michi, what do you think?

https://codereview.chromium.org/749633002/diff/80001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:2044: if
(variable->IsSignallingAssignmentToConst(op, strict_mode())) {
Same here.

https://codereview.chromium.org/749633002/diff/80001/src/variables.h
File src/variables.h (right):

https://codereview.chromium.org/749633002/diff/80001/src/variables.h#newcode142
src/variables.h:142: if (mode() == CONST) return op != Token::INIT_CONST;
Hm, wouldn't it be easier and more accurate to implement this as follows:

  if (strict_mode = SLOPPY) return false;
  if (op == Token::INIT_CONST || op == Token::INIT_CONST_LEGACY) return false;
  return mode() == CONST || mode() == CONST_LEGACY;

https://codereview.chromium.org/749633002/diff/80001/src/variables.h#newcode146
src/variables.h:146: op != Token::INIT_CONST;
Can this even occur when mode is CONST_LEGACY?

[Michael Starzinger, 2014-11-25 15:48:35]

https://codereview.chromium.org/749633002/diff/80001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/749633002/diff/80001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:2016: if
(variable->IsSignallingAssignmentToConst(op, strict_mode())) {
On 2014/11/25 15:23:36, rossberg wrote:
> I'm not sure I like this factorisation, since it breaks the symmetry with the
> other cases below and thereby makes the code less readable. It also duplicates
> some of the cases analysis. Better factor it out into the two relevant cases
> below.
> 
> Michi, what do you think?

Acknowledged. I would slightly prefer writing down the cases explicitly as
Andreas suggested, but I don't care enough be the deciding factor here. I am
fine either way (sorry for being evasive).

[Dmitry Lomov (no reviews), 2014-11-25 15:51:02]

PTAL (comments addressed; sorry for rebase)

https://codereview.chromium.org/749633002/diff/80001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/749633002/diff/80001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:2016: if
(variable->IsSignallingAssignmentToConst(op, strict_mode())) {
On 2014/11/25 15:23:36, rossberg wrote:
> I'm not sure I like this factorisation, since it breaks the symmetry with the
> other cases below and thereby makes the code less readable. It also duplicates
> some of the cases analysis. Better factor it out into the two relevant cases
> below.
> 
> Michi, what do you think?

I'd prefer to share logic with the backends. Michi?

https://codereview.chromium.org/749633002/diff/80001/src/variables.h
File src/variables.h (right):

https://codereview.chromium.org/749633002/diff/80001/src/variables.h#newcode142
src/variables.h:142: if (mode() == CONST) return op != Token::INIT_CONST;
On 2014/11/25 15:23:36, rossberg wrote:
> Hm, wouldn't it be easier and more accurate to implement this as follows:
> 
>   if (strict_mode = SLOPPY) return false;
>   if (op == Token::INIT_CONST || op == Token::INIT_CONST_LEGACY) return false;
>   return mode() == CONST || mode() == CONST_LEGACY;
Hmm, I like my version better since it puts the variable mode first.
I did a hybrid of the two which I think is more readable

https://codereview.chromium.org/749633002/diff/80001/src/variables.h#newcode146
src/variables.h:146: op != Token::INIT_CONST;
On 2014/11/25 15:23:36, rossberg wrote:
> Can this even occur when mode is CONST_LEGACY?

No, done

[rossberg, 2014-11-25 16:25:02]

https://codereview.chromium.org/749633002/diff/80001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/749633002/diff/80001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:2016: if
(variable->IsSignallingAssignmentToConst(op, strict_mode())) {
On 2014/11/25 15:51:01, Dmitry Lomov (chromium) wrote:
> On 2014/11/25 15:23:36, rossberg wrote:
> > I'm not sure I like this factorisation, since it breaks the symmetry with
the
> > other cases below and thereby makes the code less readable. It also
duplicates
> > some of the cases analysis. Better factor it out into the two relevant cases
> > below.
> > 
> > Michi, what do you think?
> 
> I'd prefer to share logic with the backends. Michi?

Well, sharing the logic would make sense if we did so consistently for the rest
of this branching logic as well. But arbitrarily picking one case for a remote
helper function is "spurious abstraction" -- which is harmful because it
pretends there is something special going on when there isn't.

So unless you think there is benefit in also introducing
IsSignallingTheHoleAssignment, IsSilentlyIgnoredAssignment and
IsSilentlyIgnoredButSignallingTheHoleAssignment, you don't want to introduce
this one. ;)

https://codereview.chromium.org/749633002/diff/80001/src/variables.h
File src/variables.h (right):

https://codereview.chromium.org/749633002/diff/80001/src/variables.h#newcode142
src/variables.h:142: if (mode() == CONST) return op != Token::INIT_CONST;
On 2014/11/25 15:51:01, Dmitry Lomov (chromium) wrote:
> On 2014/11/25 15:23:36, rossberg wrote:
> > Hm, wouldn't it be easier and more accurate to implement this as follows:
> > 
> >   if (strict_mode = SLOPPY) return false;
> >   if (op == Token::INIT_CONST || op == Token::INIT_CONST_LEGACY) return
false;
> >   return mode() == CONST || mode() == CONST_LEGACY;
> Hmm, I like my version better since it puts the variable mode first.
> I did a hybrid of the two which I think is more readable

Looks good.

[Dmitry Lomov (no reviews), 2014-11-25 17:22:31]

https://codereview.chromium.org/749633002/diff/80001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/749633002/diff/80001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:2016: if
(variable->IsSignallingAssignmentToConst(op, strict_mode())) {
On 2014/11/25 16:25:02, rossberg wrote:
> On 2014/11/25 15:51:01, Dmitry Lomov (chromium) wrote:
> > On 2014/11/25 15:23:36, rossberg wrote:
> > > I'm not sure I like this factorisation, since it breaks the symmetry with
> the
> > > other cases below and thereby makes the code less readable. It also
> duplicates
> > > some of the cases analysis. Better factor it out into the two relevant
cases
> > > below.
> > > 
> > > Michi, what do you think?
> > 
> > I'd prefer to share logic with the backends. Michi?
> 
> Well, sharing the logic would make sense if we did so consistently for the
rest
> of this branching logic as well. But arbitrarily picking one case for a remote
> helper function is "spurious abstraction" -- which is harmful because it
> pretends there is something special going on when there isn't.
> 
> So unless you think there is benefit in also introducing
> IsSignallingTheHoleAssignment, IsSilentlyIgnoredAssignment and
> IsSilentlyIgnoredButSignallingTheHoleAssignment, you don't want to introduce
> this one. ;)
Yes I am quite sure there is a benefit in introducing all these. Otherwise we
repeat the same logic in more than 5 places!
I can do this in this CL or in subsequent CL.

[rossberg, 2014-11-25 18:02:47]

https://codereview.chromium.org/749633002/diff/80001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/749633002/diff/80001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:2016: if
(variable->IsSignallingAssignmentToConst(op, strict_mode())) {
On 2014/11/25 17:22:31, Dmitry Lomov (chromium) wrote:
> On 2014/11/25 16:25:02, rossberg wrote:
> > On 2014/11/25 15:51:01, Dmitry Lomov (chromium) wrote:
> > > On 2014/11/25 15:23:36, rossberg wrote:
> > > > I'm not sure I like this factorisation, since it breaks the symmetry
with
> > the
> > > > other cases below and thereby makes the code less readable. It also
> > duplicates
> > > > some of the cases analysis. Better factor it out into the two relevant
> cases
> > > > below.
> > > > 
> > > > Michi, what do you think?
> > > 
> > > I'd prefer to share logic with the backends. Michi?
> > 
> > Well, sharing the logic would make sense if we did so consistently for the
> rest
> > of this branching logic as well. But arbitrarily picking one case for a
remote
> > helper function is "spurious abstraction" -- which is harmful because it
> > pretends there is something special going on when there isn't.
> > 
> > So unless you think there is benefit in also introducing
> > IsSignallingTheHoleAssignment, IsSilentlyIgnoredAssignment and
> > IsSilentlyIgnoredButSignallingTheHoleAssignment, you don't want to introduce
> > this one. ;)
> Yes I am quite sure there is a benefit in introducing all these. Otherwise we
> repeat the same logic in more than 5 places!
> I can do this in this CL or in subsequent CL. 

My point was: you could argue the same for a gazillion of other pieces of logic
in the compilers.

The other backends are just one gigantic, obsolete mess of wholesale code
duplication anyway, and have always been. Let's not engage in futile attempts to
micro-fight that by regressing the clarity of the new and shiny TF code base.
Our decided route to getting rid of the duplication is a clean slate with
Turbofan. That makes distributing random tidbits of logic to random helpers in
random places counterproductive. Because ultimately, the logic should all be in
here and only here. ;)

[Dmitry Lomov (no reviews), 2014-11-25 18:37:26]

https://codereview.chromium.org/749633002/diff/80001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/749633002/diff/80001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:2016: if
(variable->IsSignallingAssignmentToConst(op, strict_mode())) {
On 2014/11/25 18:02:47, rossberg wrote:
> On 2014/11/25 17:22:31, Dmitry Lomov (chromium) wrote:
> > On 2014/11/25 16:25:02, rossberg wrote:
> > > On 2014/11/25 15:51:01, Dmitry Lomov (chromium) wrote:
> > > > On 2014/11/25 15:23:36, rossberg wrote:
> > > > > I'm not sure I like this factorisation, since it breaks the symmetry
> with
> > > the
> > > > > other cases below and thereby makes the code less readable. It also
> > > duplicates
> > > > > some of the cases analysis. Better factor it out into the two relevant
> > cases
> > > > > below.
> > > > > 
> > > > > Michi, what do you think?
> > > > 
> > > > I'd prefer to share logic with the backends. Michi?
> > > 
> > > Well, sharing the logic would make sense if we did so consistently for the
> > rest
> > > of this branching logic as well. But arbitrarily picking one case for a
> remote
> > > helper function is "spurious abstraction" -- which is harmful because it
> > > pretends there is something special going on when there isn't.
> > > 
> > > So unless you think there is benefit in also introducing
> > > IsSignallingTheHoleAssignment, IsSilentlyIgnoredAssignment and
> > > IsSilentlyIgnoredButSignallingTheHoleAssignment, you don't want to
introduce
> > > this one. ;)
> > Yes I am quite sure there is a benefit in introducing all these. Otherwise
we
> > repeat the same logic in more than 5 places!
> > I can do this in this CL or in subsequent CL. 
> 
> My point was: you could argue the same for a gazillion of other pieces of
logic
> in the compilers.
> 
> The other backends are just one gigantic, obsolete mess of wholesale code
> duplication anyway, and have always been. Let's not engage in futile attempts
to
> micro-fight that by regressing the clarity of the new and shiny TF code base.
> Our decided route to getting rid of the duplication is a clean slate with
> Turbofan. That makes distributing random tidbits of logic to random helpers in
> random places counterproductive. Because ultimately, the logic should all be
in
> here and only here. ;)

Ok, if you honestly believe this is better.

[rossberg, 2014-11-26 08:31:46]

Thanks. :) LGTM up to the comments below.

https://codereview.chromium.org/749633002/diff/140001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/749633002/diff/140001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:2041: } else if (mode == CONST && op !=
Token::INIT_CONST) {
Why not merge this case with the above?

https://codereview.chromium.org/749633002/diff/140001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:2075: } else if (mode == CONST && op !=
Token::INIT_CONST) {
Same here.

https://codereview.chromium.org/749633002/diff/140001/src/variables.h
File src/variables.h (right):

https://codereview.chromium.org/749633002/diff/140001/src/variables.h#newcode140
src/variables.h:140: bool IsSignallingAssignmentToConst(Token::Value op,
Can you also move this to full-codegen.h? This is compilation logic for
assignments, and should not pollute the variable interface.

[Dmitry Lomov (no reviews), 2014-11-26 08:52:33]

https://codereview.chromium.org/749633002/diff/140001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/749633002/diff/140001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:2041: } else if (mode == CONST && op !=
Token::INIT_CONST) {
On 2014/11/26 08:31:46, rossberg wrote:
> Why not merge this case with the above?

And extract the condition into IsNonInitializingAssignementToConst because it is
so unwieldy? Riiight.
There is way too much ado around a bunch of conditionals

[rossberg, 2014-11-26 09:28:35]

On 2014/11/26 08:52:33, Dmitry Lomov (chromium) wrote:
>
https://codereview.chromium.org/749633002/diff/140001/src/compiler/ast-graph-...
> File src/compiler/ast-graph-builder.cc (right):
> 
>
https://codereview.chromium.org/749633002/diff/140001/src/compiler/ast-graph-...
> src/compiler/ast-graph-builder.cc:2041: } else if (mode == CONST && op !=
> Token::INIT_CONST) {
> On 2014/11/26 08:31:46, rossberg wrote:
> > Why not merge this case with the above?
> 
> And extract the condition into IsNonInitializingAssignementToConst because it
is
> so unwieldy? Riiight.

No, leave the condition in clear text, not obfuscate it with a spurious
abstraction. ;)

> There is way too much ado around a bunch of conditionals

Agreed. Sorry about that, but I'm allergic to this kind of thing, because that's
what slowly turns clean and readable code into Crankshaft over time.

[Dmitry Lomov (no reviews), 2014-11-26 10:30:57]

Comments addressed, landing

https://codereview.chromium.org/749633002/diff/140001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/749633002/diff/140001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:2041: } else if (mode == CONST && op !=
Token::INIT_CONST) {
On 2014/11/26 08:52:32, Dmitry Lomov (chromium) wrote:
> On 2014/11/26 08:31:46, rossberg wrote:
> > Why not merge this case with the above?
> 
> And extract the condition into IsNonInitializingAssignementToConst because it
is
> so unwieldy? Riiight.
> There is way too much ado around a bunch of conditionals

Actually, I realized that we also decided that non-initializing assignments to
(non-legacy) const should throw in all modes, so these cases are not unifiable.
Fixed here and in full codegen

https://codereview.chromium.org/749633002/diff/140001/src/variables.h
File src/variables.h (right):

https://codereview.chromium.org/749633002/diff/140001/src/variables.h#newcode140
src/variables.h:140: bool IsSignallingAssignmentToConst(Token::Value op,
On 2014/11/26 08:31:46, rossberg wrote:
> Can you also move this to full-codegen.h? This is compilation logic for
> assignments, and should not pollute the variable interface.

Done.

[Dmitry Lomov (no reviews), 2014-11-26 10:30:58]

Comments addressed, landing

[Dmitry Lomov (no reviews), 2014-11-26 10:31:12]

The CQ bit was checked by dslomov@chromium.org

[commit-bot: I haz the power, 2014-11-26 10:32:02]

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

[commit-bot: I haz the power, 2014-11-26 11:21:16]

Committed patchset #10 (id:180001)