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

src/compiler/ast-graph-builder.cc

 // Copyright 2014 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/compiler/ast-graph-builder.h"
 
 #include "src/compiler.h"
 #include "src/compiler/ast-loop-assignment-analyzer.h"
 #include "src/compiler/control-builders.h"
 #include "src/compiler/machine-operator.h"
@@ skipping 1995 matching lines @@
       Node* global = BuildLoadGlobalObject();
       Unique<Name> name = MakeUnique(variable->name());
       const Operator* op = javascript()->StoreNamed(strict_mode(), name);
       Node* store = NewNode(op, global, value);
       PrepareFrameState(store, bailout_id, combine);
       return store;
     }
     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()) {
           value = BuildHoleCheckSilent(current, value, current);
         }
       } else if (mode == CONST_LEGACY && op != Token::INIT_CONST_LEGACY) {
         // Non-initializing assignments to legacy const is ignored.
         return value;
       } else if (mode == LET && op != Token::INIT_LET) {
         // Perform an initialization check for let declared variables.
         // Also note that the dynamic hole-check is only done to ensure that
         // this does not break in the presence of do-expressions within the
         // temporal dead zone of a let declared variable.
         Node* current = environment()->Lookup(variable);
         if (current->op() == the_hole->op()) {
           value = BuildThrowReferenceError(variable, bailout_id);
         } 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);
         Node* current = NewNode(op, current_context());
         value = BuildHoleCheckSilent(current, value, current);
       } else if (mode == CONST_LEGACY && op != Token::INIT_CONST_LEGACY) {
         // Non-initializing assignments to legacy const is ignored.
         return value;
       } else if (mode == LET && op != Token::INIT_LET) {
         // Perform an initialization check for let declared variables.
         const Operator* op =
             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);
     }
     case Variable::LOOKUP: {
       // Dynamic lookup of context variable (anywhere in the chain).
       Node* name = jsgraph()->Constant(variable->name());
       Node* strict = jsgraph()->Constant(strict_mode());
       // TODO(mstarzinger): Use Runtime::kInitializeLegacyConstLookupSlot for
       // initializations of const declarations.
@@ skipping 73 matching lines @@
   // TODO(mstarzinger): Should be unified with the VisitThrow implementation.
   Node* variable_name = jsgraph()->Constant(variable->name());
   const Operator* op =
       javascript()->CallRuntime(Runtime::kThrowReferenceError, 1);
   Node* call = NewNode(op, variable_name);
   PrepareFrameState(call, bailout_id);
   return call;
 }
 
 
+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) {
     case Token::BIT_OR:
       js_op = javascript()->BitwiseOr();
       break;
     case Token::BIT_AND:
       js_op = javascript()->BitwiseAnd();
       break;
     case Token::BIT_XOR:
@@ skipping 62 matching lines @@
 
 BitVector* AstGraphBuilder::GetVariablesAssignedInLoop(
     IterationStatement* stmt) {
   if (loop_assignment_analysis_ == NULL) return NULL;
   return loop_assignment_analysis_->GetVariablesAssignedInLoop(stmt);
 }
 
 }  // namespace compiler
 }  // namespace internal
 }  // namespace v8