[ignition] Fix hole check for dynamic local variables

src/interpreter/bytecode-generator.cc

Index: src/interpreter/bytecode-generator.cc
diff --git a/src/interpreter/bytecode-generator.cc b/src/interpreter/bytecode-generator.cc
index 5565e01f547fc2db137c61a1c82d8594b4cdef41..5efa2e60c283954b9bbe0d201dc42eadb7e19d7d 100644
--- a/src/interpreter/bytecode-generator.cc
+++ b/src/interpreter/bytecode-generator.cc
@@ -1860,7 +1860,7 @@ void BytecodeGenerator::BuildVariableLoad(Variable* variable,
               execution_context()->ContextChainDepth(local_variable->scope());
           builder()->LoadLookupContextSlot(variable->name(), typeof_mode,
                                            local_variable->index(), depth);
-          if (hole_check_mode == HoleCheckMode::kRequired) {
+          if (local_variable->binding_needs_init()) {

[rmcilroy, 2016/12/05 19:18:07]

This seems a bit hacky, I would rather fix the hole_check_mode in scope.cc
AccessNeedsHoleCheck (assuming this is where the error is introduced).

[Leszek Swirski, 2016/12/05 19:42:37]

That's what I thought originally, but non-local variables were explicitly
defined as not needing init in https://codereview.chromium.org/2232343002/
(which is why I added neis and adamk as reviewers).

I'm not sure what a good representation of the semantics is -- the actual lookup
variable itself does not need a hole check (because it's done in the runtime,
and has to be done in the runtime unconditionally because it's runtime anything
can happen). On the fast path, it's the local (non-shadowed) variable that we
care about, and for this one we *do* want the hole check to be conditional, but
it's conditional on some property of the local variable. So, the "needs hole
check" for the lookup variable is correct, it's just not the same as "needs hole
check" for the local variable.

At least, that's how I understand it, I may be way off.

[adamk, 2016/12/06 19:16:42]

I think it's possible for us to do this determination earlier, in
AccessNeedsHoleCheck, since we have all three pieces of data necessary: the
VariableProxy, the DYNAMIC_LOCAL variable, and the local_if_not_shadowed
variable.

It might be as simple as the following at the top of AccessNeedsHoleCheck():

if (var->mode() == DYNAMIC_LOCAL) return
AccessNeedsHoleCheck(var->local_if_not_shadowed(), proxy, scope);

[Leszek Swirski, 2016/12/07 16:09:52]

Fair enough, I was worried that changing it here would be a violation of the
semantics (or too tight a coupling with the execution), but if you're both fine
with it then sure. Done.

             BuildThrowIfHole(variable->name());
           }
           break;