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

src/compiler/ast-graph-builder.cc

Index: src/compiler/ast-graph-builder.cc
diff --git a/src/compiler/ast-graph-builder.cc b/src/compiler/ast-graph-builder.cc
index e2d8aabc2b95c6f4d5205a110e37c6c5535ef06c..6e9c7622356d243263d62190322d3f79ec08382e 100644
--- a/src/compiler/ast-graph-builder.cc
+++ b/src/compiler/ast-graph-builder.cc
@@ -2013,7 +2013,9 @@ Node* AstGraphBuilder::BuildVariableAssignment(
     case Variable::PARAMETER:
     case Variable::LOCAL:
       // Local var, const, or let variable.
-      if (mode == CONST_LEGACY && op == Token::INIT_CONST_LEGACY) {
+      if (variable->IsSignallingAssignmentToConst(op, strict_mode())) {

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

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?

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

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:01]

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

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

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. ;)

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

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]

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]

Ok, if you honestly believe this is better.

+        return BuildThrowConstAssignError(bailout_id);
+      } else if (mode == CONST_LEGACY && op == Token::INIT_CONST_LEGACY) {
         // Perform an initialization check for legacy const variables.
         Node* current = environment()->Lookup(variable);
         if (current->op() != the_hole->op()) {
@@ -2033,16 +2035,15 @@ Node* AstGraphBuilder::BuildVariableAssignment(
         } else if (value->opcode() == IrOpcode::kPhi) {
           value = BuildHoleCheckThrow(current, variable, value, bailout_id);
         }
-      } else if (mode == CONST && op != Token::INIT_CONST) {
-        // All assignments to const variables are early errors.
-        UNREACHABLE();
       }
       environment()->Bind(variable, value);
       return value;
     case Variable::CONTEXT: {
       // Context variable (potentially up the context chain).
       int depth = current_scope()->ContextChainLength(variable->scope());
-      if (mode == CONST_LEGACY && op == Token::INIT_CONST_LEGACY) {
+      if (variable->IsSignallingAssignmentToConst(op, strict_mode())) {

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

Same here.

+        return BuildThrowConstAssignError(bailout_id);
+      } else if (mode == CONST_LEGACY && op == Token::INIT_CONST_LEGACY) {
         // Perform an initialization check for legacy const variables.
         const Operator* op =
             javascript()->LoadContext(depth, variable->index(), false);
@@ -2057,9 +2058,6 @@ Node* AstGraphBuilder::BuildVariableAssignment(
             javascript()->LoadContext(depth, variable->index(), false);
         Node* current = NewNode(op, current_context());
         value = BuildHoleCheckThrow(current, variable, value, bailout_id);
-      } else if (mode == CONST && op != Token::INIT_CONST) {
-        // All assignments to const variables are early errors.
-        UNREACHABLE();
       }
       const Operator* op = javascript()->StoreContext(depth, variable->index());
       return NewNode(op, current_context(), value);
@@ -2153,6 +2151,16 @@ Node* AstGraphBuilder::BuildThrowReferenceError(Variable* variable,
 }
 
 
+Node* AstGraphBuilder::BuildThrowConstAssignError(BailoutId bailout_id) {
+  // TODO(mstarzinger): Should be unified with the VisitThrow implementation.
+  const Operator* op =
+      javascript()->CallRuntime(Runtime::kThrowConstAssignError, 0);
+  Node* call = NewNode(op);
+  PrepareFrameState(call, bailout_id);
+  return call;
+}
+
+
 Node* AstGraphBuilder::BuildBinaryOp(Node* left, Node* right, Token::Value op) {
   const Operator* js_op;
   switch (op) {