Clean up and simplify Parser::Declare

src/parsing/parser.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/parsing/parser.h"
 
 #include <memory>
 
 #include "src/api.h"
 #include "src/ast/ast.h"
@@ skipping 1671 matching lines @@
 
       const AstRawString* local_name =
           ast_value_factory()->star_default_star_string();
       local_names.Add(local_name, zone());
 
       // It's fine to declare this as CONST because the user has no way of
       // writing to it.
       VariableProxy* proxy = NewUnresolved(local_name, CONST);
       Declaration* declaration =
           factory()->NewVariableDeclaration(proxy, CONST, scope(), pos);
-      Declare(declaration, DeclarationDescriptor::NORMAL, true, CHECK_OK);
+      Declare(declaration, DeclarationDescriptor::NORMAL, CHECK_OK);
       proxy->var()->set_initializer_position(position());
 
       Assignment* assignment = factory()->NewAssignment(
           Token::INIT, proxy, value, kNoSourcePosition);
       result = factory()->NewExpressionStatement(assignment, kNoSourcePosition);
 
       ExpectSemicolon(CHECK_OK);
       break;
     }
   }
@@ skipping 269 matching lines @@
                               scanner()->location().beg_pos,
                               scanner()->location().end_pos);
 }
 
 
 void Parser::DeclareImport(const AstRawString* local_name, int pos, bool* ok) {
   DCHECK_NOT_NULL(local_name);
   VariableProxy* proxy = NewUnresolved(local_name, IMPORT);
   Declaration* declaration =
       factory()->NewVariableDeclaration(proxy, IMPORT, scope(), pos);
-  Declare(declaration, DeclarationDescriptor::NORMAL, true, CHECK_OK_VOID);
+  Declare(declaration, DeclarationDescriptor::NORMAL, CHECK_OK_VOID);
 }
 
-
 Variable* Parser::Declare(Declaration* declaration,
                           DeclarationDescriptor::Kind declaration_kind,
-                          bool resolve, bool* ok, Scope* scope) {
+                          bool* ok, Scope* scope) {
   VariableProxy* proxy = declaration->proxy();
   DCHECK(proxy->raw_name() != NULL);
   const AstRawString* name = proxy->raw_name();
   VariableMode mode = declaration->mode();
   DCHECK(IsDeclaredVariableMode(mode) && mode != CONST_LEGACY);
   bool is_function_declaration = declaration->IsFunctionDeclaration();
   if (scope == nullptr) scope = this->scope();
-  Scope* declaration_scope =
-      IsLexicalVariableMode(mode) ? scope : scope->GetDeclarationScope();
+  if (mode == VAR) scope = scope->GetDeclarationScope();
+
+  DCHECK(!scope->is_catch_scope());
+  DCHECK(!scope->is_with_scope());
+  DCHECK(scope->is_declaration_scope() ||
+         (IsLexicalVariableMode(mode) && scope->is_block_scope()));
+
   Variable* var = NULL;
-
-  // If a suitable scope exists, then we can statically declare this
-  // variable and also set its mode. In any case, a Declaration node
-  // will be added to the scope so that the declaration can be added
-  // to the corresponding activation frame at runtime if necessary.
-  // For instance, var declarations inside a sloppy eval scope need
-  // to be added to the calling function context. Similarly, strict
-  // mode eval scope and lexical eval bindings do not leak variable
-  // declarations to the caller's scope so we declare all locals, too.
-  if (declaration_scope->is_function_scope() ||
-      declaration_scope->is_block_scope() ||
-      declaration_scope->is_module_scope() ||
-      declaration_scope->is_script_scope() ||
-      (declaration_scope->is_eval_scope() &&
-       (is_strict(declaration_scope->language_mode()) ||
-        IsLexicalVariableMode(mode)))) {
+  if (scope->is_eval_scope() && is_sloppy(scope->language_mode()) &&
+      mode == VAR) {
+    // In a var binding in a sloppy direct eval, pollute the enclosing scope
+    // with this new binding by doing the following:
+    // The proxy is bound to a lookup variable to force a dynamic declaration
+    // using the DeclareEvalVar or DeclareEvalFunction runtime functions.
+    Variable::Kind kind = Variable::NORMAL;
+    // TODO(sigurds) figure out if kNotAssigned is OK here
+    var = new (zone()) Variable(scope, name, mode, kind,
+                                declaration->initialization(), kNotAssigned);
+    var->AllocateTo(VariableLocation::LOOKUP, -1);
+  } else {
     // Declare the variable in the declaration scope.
-    var = declaration_scope->LookupLocal(name);
+    var = scope->LookupLocal(name);
     if (var == NULL) {
       // Declare the name.
       Variable::Kind kind = Variable::NORMAL;
       if (is_function_declaration) {
         kind = Variable::FUNCTION;
       }
-      var = declaration_scope->DeclareLocal(
-          name, mode, declaration->initialization(), kind, kNotAssigned);
+      var = scope->DeclareLocal(name, mode, declaration->initialization(), kind,
+                                kNotAssigned);
     } else if (IsLexicalVariableMode(mode) ||
                IsLexicalVariableMode(var->mode())) {
       // Allow duplicate function decls for web compat, see bug 4693.
       bool duplicate_allowed = false;
-      if (is_sloppy(language_mode()) && is_function_declaration &&
+      if (is_sloppy(scope->language_mode()) && is_function_declaration &&

[neis, 2016/08/08 09:42:34]

Was this just wrong before?

[adamk, 2016/08/08 17:27:26]

Yes, but not in any observable way (due to what callers pass for "scope").

           var->is_function()) {
         DCHECK(IsLexicalVariableMode(mode) &&
                IsLexicalVariableMode(var->mode()));
         // If the duplication is allowed, then the var will show up
         // in the SloppyBlockFunctionMap and the new FunctionKind
         // will be a permitted duplicate.
         FunctionKind function_kind =
             declaration->AsFunctionDeclaration()->fun()->kind();
         duplicate_allowed =
             scope->GetDeclarationScope()->sloppy_block_function_map()->Lookup(
@@ skipping 25 matching lines @@
           ParserTraits::ReportMessage(MessageTemplate::kVarRedeclaration, name);
         } else {
           ParserTraits::ReportMessage(MessageTemplate::kParamDupe);
         }
         *ok = false;
         return nullptr;
       }
     } else if (mode == VAR) {
       var->set_maybe_assigned();
     }
-  } else if (declaration_scope->is_eval_scope() &&
-             is_sloppy(declaration_scope->language_mode()) &&
-             !IsLexicalVariableMode(mode)) {
-    // In a var binding in a sloppy direct eval, pollute the enclosing scope
-    // with this new binding by doing the following:
-    // The proxy is bound to a lookup variable to force a dynamic declaration
-    // using the DeclareEvalVar or DeclareEvalFunction runtime functions.
-    Variable::Kind kind = Variable::NORMAL;
-    // TODO(sigurds) figure out if kNotAssigned is OK here
-    var = new (zone()) Variable(declaration_scope, name, mode, kind,
-                                declaration->initialization(), kNotAssigned);
-    var->AllocateTo(VariableLocation::LOOKUP, -1);
-    resolve = true;
   }
-
+  DCHECK_NOT_NULL(var);
 
   // We add a declaration node for every declaration. The compiler
   // will only generate code if necessary. In particular, declarations
   // for inner local variables that do not represent functions won't
   // result in any generated code.
   //
-  // Note that we always add an unresolved proxy even if it's not
-  // used, simply because we don't know in this method (w/o extra
-  // parameters) if the proxy is needed or not. The proxy will be
-  // bound during variable resolution time unless it was pre-bound
-  // below.
-  //
-  // WARNING: This will lead to multiple declaration nodes for the
+  // This will lead to multiple declaration nodes for the
   // same variable if it is declared several times. This is not a
-  // semantic issue as long as we keep the source order, but it may be
-  // a performance issue since it may lead to repeated
-  // DeclareEvalVar or DeclareEvalFunction calls.
-  declaration_scope->AddDeclaration(declaration);
-
-  // If requested and we have a local variable, bind the proxy to the variable
-  // at parse-time. This is used for functions (and consts) declared inside
-  // statements: the corresponding function (or const) variable must be in the
-  // function scope and not a statement-local scope, e.g. as provided with a
-  // 'with' statement:
-  //
-  //   with (obj) {
-  //     function f() {}
-  //   }
-  //
-  // which is translated into:
-  //
-  //   with (obj) {
-  //     // in this case this is not: 'var f; f = function () {};'
-  //     var f = function () {};
-  //   }
-  //
-  // Note that if 'f' is accessed from inside the 'with' statement, it
-  // will be allocated in the context (because we must be able to look
-  // it up dynamically) but it will also be accessed statically, i.e.,
-  // with a context slot index and a context chain length for this
-  // initialization code. Thus, inside the 'with' statement, we need
-  // both access to the static and the dynamic context chain; the
-  // runtime needs to provide both.
-  if (resolve && var != NULL) {
-    proxy->BindTo(var);
-  }
+  // semantic issue, but it may be a performance issue since it may
+  // lead to repeated DeclareEvalVar or DeclareEvalFunction calls.
+  scope->AddDeclaration(declaration);
+  proxy->BindTo(var);
   return var;
 }
 
 
 // Language extension which is only enabled for source files loaded
 // through the API's extension mechanism.  A native function
 // declaration is resolved by looking up the function through a
 // callback provided by the extension.
 Statement* Parser::ParseNativeDeclaration(bool* ok) {
   int pos = peek_position();
@@ skipping 19 matching lines @@
   // because of lazy compilation.
   // TODO(adamk): Should this be GetClosureScope()?
   scope()->GetDeclarationScope()->ForceEagerCompilation();
 
   // TODO(1240846): It's weird that native function declarations are
   // introduced dynamically when we meet their declarations, whereas
   // other functions are set up when entering the surrounding scope.
   VariableProxy* proxy = NewUnresolved(name, VAR);
   Declaration* declaration =
       factory()->NewVariableDeclaration(proxy, VAR, scope(), pos);
-  Declare(declaration, DeclarationDescriptor::NORMAL, true, CHECK_OK);
+  Declare(declaration, DeclarationDescriptor::NORMAL, CHECK_OK);
   NativeFunctionLiteral* lit =
       factory()->NewNativeFunctionLiteral(name, extension_, kNoSourcePosition);
   return factory()->NewExpressionStatement(
       factory()->NewAssignment(Token::INIT, proxy, lit, kNoSourcePosition),
       pos);
 }
 
 Statement* Parser::ParseHoistableDeclaration(
     ZoneList<const AstRawString*>* names, bool default_export, bool* ok) {
   Expect(Token::FUNCTION, CHECK_OK);
@@ skipping 69 matching lines @@
       pos, FunctionLiteral::kDeclaration, language_mode(), CHECK_OK);
 
   // In ES6, a function behaves as a lexical binding, except in
   // a script scope, or the initial scope of eval or another function.
   VariableMode mode =
       (!scope()->is_declaration_scope() || scope()->is_module_scope()) ? LET
                                                                        : VAR;
   VariableProxy* proxy = NewUnresolved(variable_name, mode);
   Declaration* declaration =
       factory()->NewFunctionDeclaration(proxy, mode, fun, scope(), pos);
-  Declare(declaration, DeclarationDescriptor::NORMAL, true, CHECK_OK);
+  Declare(declaration, DeclarationDescriptor::NORMAL, CHECK_OK);
   if (names) names->Add(variable_name, zone());
   EmptyStatement* empty = factory()->NewEmptyStatement(kNoSourcePosition);
   // Async functions don't undergo sloppy mode block scoped hoisting, and don't
   // allow duplicates in a block. Both are represented by the
   // sloppy_block_function_map. Don't add them to the map for async functions.
   // Generators are also supposed to be prohibited; currently doing this behind
   // a flag and UseCounting violations to assess web compatibility.
   if (is_sloppy(language_mode()) && !scope()->is_declaration_scope() &&
       !is_async && !(allow_harmony_restrictive_generators() && is_generator)) {
     SloppyBlockFunctionStatement* delegate =
@@ skipping 38 matching lines @@
     name = ParseIdentifierOrStrictReservedWord(&is_strict_reserved, CHECK_OK);
     variable_name = name;
   }
 
   Expression* value = ParseClassLiteral(nullptr, name, scanner()->location(),
                                         is_strict_reserved, pos, CHECK_OK);
 
   VariableProxy* proxy = NewUnresolved(variable_name, LET);
   Declaration* declaration =
       factory()->NewVariableDeclaration(proxy, LET, scope(), pos);
-  Declare(declaration, DeclarationDescriptor::NORMAL, true, CHECK_OK);
+  Declare(declaration, DeclarationDescriptor::NORMAL, CHECK_OK);
   proxy->var()->set_initializer_position(position());
   Assignment* assignment =
       factory()->NewAssignment(Token::INIT, proxy, value, pos);
   Statement* assignment_statement =
       factory()->NewExpressionStatement(assignment, kNoSourcePosition);
   if (names) names->Add(variable_name, zone());
   return assignment_statement;
 }
 
 Block* Parser::ParseBlock(ZoneList<const AstRawString*>* labels, bool* ok) {
@@ skipping 1212 matching lines @@
 
     Block* ignore_completion_block =
         factory()->NewBlock(NULL, names->length() + 3, true, kNoSourcePosition);
     ZoneList<Variable*> inner_vars(names->length(), zone());
     // For each let variable x:
     //    make statement: let/const x = temp_x.
     for (int i = 0; i < names->length(); i++) {
       VariableProxy* proxy = NewUnresolved(names->at(i), mode);
       Declaration* declaration = factory()->NewVariableDeclaration(
           proxy, mode, scope(), kNoSourcePosition);
-      Declare(declaration, DeclarationDescriptor::NORMAL, true, CHECK_OK);
+      Declare(declaration, DeclarationDescriptor::NORMAL, CHECK_OK);
       inner_vars.Add(declaration->proxy()->var(), zone());
       VariableProxy* temp_proxy = factory()->NewVariableProxy(temps.at(i));
       Assignment* assignment = factory()->NewAssignment(
           Token::INIT, proxy, temp_proxy, kNoSourcePosition);
       Statement* assignment_statement =
           factory()->NewExpressionStatement(assignment, kNoSourcePosition);
       DCHECK(init->position() != kNoSourcePosition);
       proxy->var()->set_initializer_position(init->position());
       ignore_completion_block->statements()->Add(assignment_statement, zone());
     }
@@ skipping 330 matching lines @@
           init_block =
               factory()->NewBlock(nullptr, 1, false, kNoSourcePosition);
 
           for (int i = 0; i < bound_names.length(); ++i) {
             // TODO(adamk): This needs to be some sort of special
             // INTERNAL variable that's invisible to the debugger
             // but visible to everything else.
             VariableProxy* tdz_proxy = NewUnresolved(bound_names[i], LET);
             Declaration* tdz_decl = factory()->NewVariableDeclaration(
                 tdz_proxy, LET, scope(), kNoSourcePosition);
-            Variable* tdz_var = Declare(
-                tdz_decl, DeclarationDescriptor::NORMAL, true, CHECK_OK);
+            Variable* tdz_var =
+                Declare(tdz_decl, DeclarationDescriptor::NORMAL, CHECK_OK);
             tdz_var->set_initializer_position(position());
           }
         }
 
         for_state.set_end_position(scanner()->location().end_pos);
         Scope* for_scope = for_state.FinalizedBlockScope();
         // Parsed for-in loop w/ variable declarations.
         if (init_block != nullptr) {
           init_block->statements()->Add(final_loop, zone());
           init_block->set_scope(for_scope);
@@ skipping 1160 matching lines @@
 #ifdef DEBUG
   scope()->SetScopeName(name);
 #endif
 
   VariableProxy* proxy = nullptr;
   if (name != nullptr) {
     proxy = NewUnresolved(name, CONST);
     // TODO(verwaest): declare via block_state.
     Declaration* declaration = factory()->NewVariableDeclaration(
         proxy, CONST, block_state.scope(), pos);
-    Declare(declaration, DeclarationDescriptor::NORMAL, true, CHECK_OK);
+    Declare(declaration, DeclarationDescriptor::NORMAL, CHECK_OK);
   }
 
   Expression* extends = nullptr;
   if (Check(Token::EXTENDS)) {
     block_state.set_start_position(scanner()->location().end_pos);
     ExpressionClassifier extends_classifier(this);
     extends = ParseLeftHandSideExpression(&extends_classifier, CHECK_OK);
     CheckNoTailCallExpressions(&extends_classifier, CHECK_OK);
     RewriteNonPattern(&extends_classifier, CHECK_OK);
     if (classifier != nullptr) {
@@ skipping 257 matching lines @@
       } while (query_scope != outer_scope);
 
       if (!should_hoist) continue;
 
       // Declare a var-style binding for the function in the outer scope
       if (!var_created) {
         var_created = true;
         VariableProxy* proxy = scope->NewUnresolved(factory(), name);
         Declaration* declaration = factory()->NewVariableDeclaration(
             proxy, VAR, scope, kNoSourcePosition);
-        Declare(declaration, DeclarationDescriptor::NORMAL, true, ok, scope);
+        Declare(declaration, DeclarationDescriptor::NORMAL, ok, scope);
         DCHECK(ok);  // Based on the preceding check, this should not fail
         if (!ok) return;
       }
 
       // Read from the local lexical scope and write to the function scope
       VariableProxy* to = scope->NewUnresolved(factory(), name);
       VariableProxy* from = delegate->scope()->NewUnresolved(factory(), name);
       Expression* assignment =
           factory()->NewAssignment(Token::ASSIGN, to, from, kNoSourcePosition);
       Statement* statement =
@@ skipping 1785 matching lines @@
   node->Print(Isolate::Current());
 }
 #endif  // DEBUG
 
 #undef CHECK_OK
 #undef CHECK_OK_VOID
 #undef CHECK_FAILED
 
 }  // namespace internal
 }  // namespace v8