Ensure hole checks take place in switch statement scopes

src/full-codegen/full-codegen.cc

 // Copyright 2012 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/ast.h"
 #include "src/ast-numbering.h"
 #include "src/code-factory.h"
 #include "src/codegen.h"
 #include "src/compiler.h"
 #include "src/debug/debug.h"
@@ skipping 1567 matching lines @@
                                Context::PREVIOUS_INDEX);
     // Update local stack frame context field.
     codegen_->StoreToFrameField(StandardFrameConstants::kContextOffset,
                                 codegen_->context_register());
   }
   codegen_->PrepareForBailoutForId(exit_id_, NO_REGISTERS);
   codegen_->scope_ = saved_scope_;
 }
 
 
+bool FullCodeGenerator::NeedsHoleCheck(VariableProxy* proxy) {
+  Variable* var = proxy->var();
+
+  if (!var->binding_needs_init()) {
+    return false;
+  }
+
+  // var->scope() may be NULL when the proxy is located in eval code and
+  // refers to a potential outside binding. Currently those bindings are
+  // always looked up dynamically, i.e. in that case
+  //     var->location() == LOOKUP.
+  // always holds.
+  DCHECK(var->scope() != NULL);

[adamk, 2015/08/24 22:15:01]

Can you also add a DCHECK about the location? Now that this code doesn't live
directly in a switch statement, that'd help further-rationalize this comment:

DCHECK(var->location() == VariableLocation::PARAMETER ||
       var->location() == VariableLocation::LOCAL ||
       var->location() == VariableLocation::CONTEXT);

[Dan Ehrenberg, 2015/08/25 01:48:13]

Good idea, done

+
+  // Check if the binding really needs an initialization check. The check
+  // can be skipped in the following situation: we have a LET or CONST
+  // binding in harmony mode, both the Variable and the VariableProxy have
+  // the same declaration scope (i.e. they are both in global code, in the
+  // same function or in the same eval code) and the VariableProxy is in
+  // the source physically located after the initializer of the variable.
+  //
+  // We cannot skip any initialization checks for CONST in non-harmony
+  // mode because const variables may be declared but never initialized:
+  //   if (false) { const x; }; var y = x;
+  //
+  // The condition on the declaration scopes is a conservative check for
+  // nested functions that access a binding and are called before the
+  // binding is initialized:
+  //   function() { f(); let x = 1; function f() { x = 2; } }
+  //
+  // The check cannot be skipped on non-linear scopes, namely switch

[adamk, 2015/08/24 22:15:01]

I think the bit about non-linear scopes should be incorporated into the first
paragraph of this comment, as one of the requirements, just after the same-scope
requirement. Leaving this example here is fine, though.

[Dan Ehrenberg, 2015/08/25 01:48:14]

Done

+  // scopes, to ensure tests are done in cases like the following:
+  //   switch (1) { case 0: let x = 2; case 1: f(x); }
+  if (var->scope()->DeclarationScope() != scope()->DeclarationScope()) {
+    return true;
+  } else if (var->is_this()) {

[adamk, 2015/08/24 22:15:01]

I'd just make this a separate if statement, no need for an 'else' after a
return.

[Dan Ehrenberg, 2015/08/25 01:48:14]

Done

+    CHECK(literal() != nullptr &&

[adamk, 2015/08/24 22:15:01]

This CHECK looks scary, any idea why it's here?

[Dan Ehrenberg, 2015/08/25 01:48:14]

Not exactly. I guess when 'this' occurs globally, it's resolved more
dynamically, and when it's in a function which isn't a subclass constructor,
it's resolved more dynamically, but I'm having trouble finding that code. I made
this into a DCHECK.

+          (literal()->kind() & kSubclassConstructor) != 0);
+    // TODO(littledan): implement 'this' hole check elimination.
+    return true;
+  } else {

[adamk, 2015/08/24 22:15:01]

Same thing here, I'd remove the else.

[Dan Ehrenberg, 2015/08/25 01:48:14]

Done

+    // Check that we always have valid source position.
+    DCHECK(var->initializer_position() != RelocInfo::kNoPosition);
+    DCHECK(proxy->position() != RelocInfo::kNoPosition);
+    return var->mode() == CONST_LEGACY || scope()->is_nonlinear() ||

[adamk, 2015/08/24 22:15:01]

I think you need to be checking var->scope()->is_nonlinear(). Otherwise the
following code would incorrectly pass this check (please add a test):

switch (1) {
  case 0:
    let x = 2;
  case 1:
  { // this block, plus the let below, adds another lexical scope, this one
linear
    let y = 3;
    x = 2;
  }
}

[Dan Ehrenberg, 2015/08/25 01:48:13]

Oh, good catch! I fixed the bug and added a test based on this case which failed
with the previous version.

+           var->initializer_position() >= proxy->position();
+  }
+}
+
+
 #undef __
 
 
 }  // namespace internal
 }  // namespace v8