[parser] Change how the preparser tracks parameters

This will make it easier to separate out parameter declaration from other other
parameter scope information tracking.

Change-Id: I8712dd7fc589c84bc1e1a1eab9038af6047b21cd
Reviewed-on: https://chromium-review.googlesource.com/c/1403118
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58698}

src/ast/prettyprinter.cc

@@ -1032,10 +1032,10 @@ void AstPrinter::VisitFunctionLiteral(FunctionLiteral* node) {
   IndentedScope indent(this, "FUNC LITERAL", node->position());
   PrintLiteralIndented("NAME", node->raw_name(), false);
   PrintLiteralIndented("INFERRED NAME", node->raw_inferred_name(), false);
-  PrintParameters(node->scope());
   // We don't want to see the function literal in this case: it
   // will be printed via PrintProgram when the code for it is
   // generated.
+  // PrintParameters(node->scope());
   // PrintStatements(node->body());
 }
 

src/ast/scopes.cc

@@ -147,13 +147,6 @@ DeclarationScope::DeclarationScope(Zone* zone, Scope* outer_scope,
   SetDefaults();
 }
 
-bool DeclarationScope::IsDeclaredParameter(const AstRawString* name) {
-  // If IsSimpleParameterList is false, duplicate parameters are not allowed,
-  // however `arguments` may be allowed if function is not strict code. Thus,
-  // the assumptions explained above do not hold.
-  return params_.Contains(variables_.Lookup(name));
-}
-
 ModuleScope::ModuleScope(DeclarationScope* script_scope,
                          AstValueFactory* ast_value_factory)
     : DeclarationScope(ast_value_factory->zone(), script_scope, MODULE_SCOPE,
@@ -484,7 +477,11 @@ void DeclarationScope::HoistSloppyBlockFunctions(AstNodeFactory* factory) {
   SloppyBlockFunctionMap* map = sloppy_block_function_map();
   if (map == nullptr) return;
 
-  const bool has_simple_parameters = HasSimpleParameters();
+  // In case of complex parameters the current scope is the body scope and the
+  // parameters are stored in the outer scope.
+  Scope* parameter_scope = HasSimpleParameters() ? this : outer_scope_;
+  DCHECK(parameter_scope->is_function_scope() || is_eval_scope() ||
+         is_script_scope());
 
   // The declarations need to be added in the order they were seen,
   // so accumulate declared names sorted by index.
@@ -499,22 +496,10 @@ void DeclarationScope::HoistSloppyBlockFunctions(AstNodeFactory* factory) {
     // or parameter,
 
     // Check if there's a conflict with a parameter.
-    // This depends on the fact that functions always have a scope solely to
-    // hold complex parameters, and the names local to that scope are
-    // precisely the names of the parameters. IsDeclaredParameter(name) does
-    // not hold for names declared by complex parameters, nor are those
-    // bindings necessarily declared lexically, so we have to check for them
-    // explicitly. On the other hand, if there are not complex parameters,
-    // it is sufficient to just check IsDeclaredParameter.
-    if (!has_simple_parameters) {
-      if (outer_scope_->LookupLocal(name) != nullptr) {
+    Variable* maybe_parameter = parameter_scope->LookupLocal(name);
+    if (maybe_parameter != nullptr && maybe_parameter->is_parameter()) {
       continue;
     }
-    } else {
-      if (IsDeclaredParameter(name)) {
-        continue;
-      }
-    }
 
     bool declaration_queued = false;
 
@@ -580,7 +565,7 @@ void DeclarationScope::HoistSloppyBlockFunctions(AstNodeFactory* factory) {
       // Based on the preceding checks, it doesn't matter what we pass as
       // sloppy_mode_block_scope_function_redefinition.
       bool ok = true;
-      DeclareVariable(declaration, VariableMode::kVar,
+      DeclareVariable(declaration, VariableMode::kVar, NORMAL_VARIABLE,
                       Variable::DefaultInitializationFlag(VariableMode::kVar),
                       nullptr, &ok);
       DCHECK(ok);
@@ -937,11 +922,12 @@ Variable* DeclarationScope::DeclareParameter(const AstRawString* name,
     var = NewTemporary(name);
   } else {
     DCHECK_EQ(mode, VariableMode::kVar);
-    var = Declare(zone(), name, mode);
+    var = Declare(zone(), name, mode, PARAMETER_VARIABLE);
   }
   has_rest_ = is_rest;
   var->set_initializer_position(position);
   params_.Add(var, zone());
+  if (!is_rest) ++num_parameters_;
   if (name == ast_value_factory->arguments_string()) {
     has_arguments_parameter_ = true;
   }
@@ -953,37 +939,31 @@ Variable* DeclarationScope::DeclareParameter(const AstRawString* name,
   return var;
 }
 
-Variable* DeclarationScope::DeclareParameterName(
-    const AstRawString* name, bool is_rest, AstValueFactory* ast_value_factory,
-    bool declare_as_local, bool add_parameter) {
+void DeclarationScope::RecordParameter(bool is_rest) {
   DCHECK(!already_resolved_);
   DCHECK(is_function_scope() || is_module_scope());
-  DCHECK(!has_rest_ || is_rest);
   DCHECK(is_being_lazily_parsed_);
+  DCHECK(!has_rest_);
   has_rest_ = is_rest;
-  if (name == ast_value_factory->arguments_string()) {
-    has_arguments_parameter_ = true;
-  }
-  Variable* var;
-  if (declare_as_local) {
-    var = Declare(zone(), name, VariableMode::kVar);
-  } else {
-    var = new (zone()) Variable(this, name, VariableMode::kTemporary,
-                                NORMAL_VARIABLE, kCreatedInitialized);
-  }
-  if (add_parameter) {
-    params_.Add(var, zone());
-  }
+  if (!is_rest) ++num_parameters_;
+}
+
+void DeclarationScope::DeclareParameterName(const AstRawString* name) {
+  DCHECK(!already_resolved_);
+  DCHECK(is_function_scope() || is_module_scope());
+  DCHECK(is_being_lazily_parsed_);
+  // The resulting variable isn't added to params. In the case of non-simple
+  // params, a dummy temp variable is added in AddNonSimpleParameterTemp.
+  Variable* var = Declare(zone(), name, VariableMode::kVar, PARAMETER_VARIABLE);
   // Params are automatically marked as used to make sure that the debugger and
   // function.arguments sees them.
   // TODO(verwaest): Reevaluate whether we always need to do this, since
   // strict-mode function.arguments does not make the arguments available.
   var->set_is_used();
-  return var;
 }
 
 Variable* Scope::DeclareLocal(const AstRawString* name, VariableMode mode,
-                              InitializationFlag init_flag) {
+                              VariableKind kind, InitializationFlag init_flag) {
   DCHECK(!already_resolved_);
   // This function handles VariableMode::kVar, VariableMode::kLet, and
   // VariableMode::kConst modes.  VariableMode::kDynamic variables are
@@ -994,11 +974,11 @@ Variable* Scope::DeclareLocal(const AstRawString* name, VariableMode mode,
                  mode == VariableMode::kVar || mode == VariableMode::kLet ||
                      mode == VariableMode::kConst);
   DCHECK(!GetDeclarationScope()->was_lazily_parsed());
-  return Declare(zone(), name, mode, NORMAL_VARIABLE, init_flag);
+  return Declare(zone(), name, mode, kind, init_flag);
 }
 
 void Scope::DeclareVariable(Declaration* declaration, VariableMode mode,
-                            InitializationFlag init,
+                            VariableKind kind, InitializationFlag init,
                             bool* sloppy_mode_block_scope_function_redefinition,
                             bool* ok) {
   DCHECK(IsDeclaredVariableMode(mode));
@@ -1008,8 +988,8 @@ void Scope::DeclareVariable(Declaration* declaration, VariableMode mode,
 
   if (mode == VariableMode::kVar && !is_declaration_scope()) {
     return GetDeclarationScope()->DeclareVariable(
-        declaration, mode, init, sloppy_mode_block_scope_function_redefinition,
-        ok);
+        declaration, mode, kind, init,
+        sloppy_mode_block_scope_function_redefinition, ok);
   }
   DCHECK(!is_catch_scope());
   DCHECK(!is_with_scope());
@@ -1040,12 +1020,12 @@ void Scope::DeclareVariable(Declaration* declaration, VariableMode mode,
       // 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.
-      var = new (zone())
-          Variable(this, name, mode, NORMAL_VARIABLE, init, kMaybeAssigned);
+      DCHECK_EQ(NORMAL_VARIABLE, kind);
+      var = new (zone()) Variable(this, name, mode, kind, init, kMaybeAssigned);
       var->AllocateTo(VariableLocation::LOOKUP, -1);
     } else {
       // Declare the name.
-      var = DeclareLocal(name, mode, init);
+      var = DeclareLocal(name, mode, kind, init);
     }
   } else {
     var->set_maybe_assigned();
@@ -1600,11 +1580,12 @@ void DeclarationScope::PrintParameters() {
   for (int i = 0; i < params_.length(); i++) {
     if (i > 0) PrintF(", ");
     const AstRawString* name = params_[i]->raw_name();
-    if (name->IsEmpty())
+    if (name->IsEmpty()) {
       PrintF(".%p", reinterpret_cast<void*>(params_[i]));
-    else
+    } else {
       PrintName(name);
     }
+  }
   PrintF(")");
 }
 
@@ -2122,6 +2103,7 @@ void DeclarationScope::AllocateParameterLocals() {
   // order is relevant!
   for (int i = num_parameters() - 1; i >= 0; --i) {
     Variable* var = params_[i];
+    DCHECK_NOT_NULL(var);
     DCHECK(!has_rest_ || var != rest_parameter());
     DCHECK_EQ(this, var->scope());
     if (has_mapped_arguments) {

src/ast/scopes.h

@@ -221,10 +221,11 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   // Declare a local variable in this scope. If the variable has been
   // declared before, the previously declared variable is returned.
   Variable* DeclareLocal(const AstRawString* name, VariableMode mode,
+                         VariableKind kind = NORMAL_VARIABLE,
                          InitializationFlag init_flag = kCreatedInitialized);
 
   void DeclareVariable(Declaration* declaration, VariableMode mode,
-                       InitializationFlag init,
+                       VariableKind kind, InitializationFlag init,
                        bool* sloppy_mode_block_scope_function_redefinition,
                        bool* ok);
 
@@ -723,8 +724,6 @@ class V8_EXPORT_PRIVATE DeclarationScope : public Scope {
   // Creates a script scope.
   DeclarationScope(Zone* zone, AstValueFactory* ast_value_factory);
 
-  bool IsDeclaredParameter(const AstRawString* name);
-
   FunctionKind function_kind() const { return function_kind_; }
 
   bool is_arrow_scope() const {
@@ -815,11 +814,10 @@ class V8_EXPORT_PRIVATE DeclarationScope : public Scope {
                              bool is_optional, bool is_rest,
                              AstValueFactory* ast_value_factory, int position);
 
-  // Declares that a parameter with the name exists. Creates a Variable and
-  // returns it if FLAG_preparser_scope_analysis is on.
-  Variable* DeclareParameterName(const AstRawString* name, bool is_rest,
-                                 AstValueFactory* ast_value_factory,
-                                 bool declare_local, bool add_parameter);
+  // Declares that a parameter with the name exists. Creates a Variable.
+  void DeclareParameterName(const AstRawString* name);
+  // Makes sure that num_parameters_ and has_rest is correct for the preparser.
+  void RecordParameter(bool is_rest);
 
   // Declare an implicit global variable in this scope which must be a
   // script scope.  The variable was introduced (possibly from an inner
@@ -861,6 +859,7 @@ class V8_EXPORT_PRIVATE DeclarationScope : public Scope {
   // Only valid for function and module scopes.
   Variable* parameter(int index) const {
     DCHECK(is_function_scope() || is_module_scope());
+    DCHECK(!is_being_lazily_parsed_);
     return params_[index];
   }
 
@@ -869,9 +868,7 @@ class V8_EXPORT_PRIVATE DeclarationScope : public Scope {
   //   function foo(a, b) {}         ==> 2
   //   function foo(a, b, ...c) {}   ==> 2
   //   function foo(a, b, c = 1) {}  ==> 3
-  int num_parameters() const {
-    return has_rest_ ? params_.length() - 1 : params_.length();
-  }
+  int num_parameters() const { return num_parameters_; }
 
   // The function's rest parameter (nullptr if there is none).
   Variable* rest_parameter() const {
@@ -1049,6 +1046,8 @@ class V8_EXPORT_PRIVATE DeclarationScope : public Scope {
   bool is_skipped_function_ : 1;
   bool has_inferred_function_name_ : 1;
 
+  int num_parameters_ = 0;
+
   // If the scope is a function scope, this is the function kind.
   const FunctionKind function_kind_;
 

src/ast/variables.h

@@ -136,6 +136,8 @@ class Variable final : public ZoneObject {
     return kind() == SLOPPY_FUNCTION_NAME_VARIABLE;
   }
 
+  bool is_parameter() const { return kind() == PARAMETER_VARIABLE; }
+
   Variable* local_if_not_shadowed() const {
     DCHECK(mode() == VariableMode::kDynamicLocal &&
            local_if_not_shadowed_ != nullptr);
@@ -198,7 +200,7 @@ class Variable final : public ZoneObject {
 
   class VariableModeField : public BitField16<VariableMode, 0, 3> {};
   class VariableKindField
-      : public BitField16<VariableKind, VariableModeField::kNext, 3> {};
+      : public BitField16<VariableKind, VariableModeField::kNext, 2> {};
   class LocationField
       : public BitField16<VariableLocation, VariableKindField::kNext, 3> {};
   class ForceContextAllocationField

src/globals.h

@@ -1091,6 +1091,7 @@ inline const char* VariableMode2String(VariableMode mode) {
 
 enum VariableKind : uint8_t {
   NORMAL_VARIABLE,
+  PARAMETER_VARIABLE,
   THIS_VARIABLE,
   SLOPPY_FUNCTION_NAME_VARIABLE
 };

src/parsing/parser-base.h

@@ -3314,8 +3314,6 @@ template <typename Impl>
 void ParserBase<Impl>::ParseFormalParameter(FormalParametersT* parameters) {
   // FormalParameter[Yield,GeneratorParameter] :
   //   BindingElement[?Yield, ?GeneratorParameter]
-  bool is_rest = parameters->has_rest;
-
   FuncNameInferrerState fni_state(&fni_);
   int pos = peek_position();
   ExpressionT pattern = ParseBindingPattern();
@@ -3329,7 +3327,7 @@ void ParserBase<Impl>::ParseFormalParameter(FormalParametersT* parameters) {
   if (Check(Token::ASSIGN)) {
     parameters->is_simple = false;
 
-    if (is_rest) {
+    if (parameters->has_rest) {
       ReportMessage(MessageTemplate::kRestDefaultInitializer);
       return;
     }
@@ -3340,7 +3338,7 @@ void ParserBase<Impl>::ParseFormalParameter(FormalParametersT* parameters) {
   }
 
   impl()->AddFormalParameter(parameters, pattern, initializer, end_position(),
-                             is_rest);
+                             parameters->has_rest);
 }
 
 template <typename Impl>

src/parsing/parser.cc

@@ -1372,9 +1372,11 @@ void Parser::Declare(Declaration* declaration,
                      int var_end_pos) {
   bool local_ok = true;
   bool sloppy_mode_block_scope_function_redefinition = false;
-  scope->DeclareVariable(declaration, mode, init,
-                         &sloppy_mode_block_scope_function_redefinition,
-                         &local_ok);
+  scope->DeclareVariable(
+      declaration, mode,
+      declaration_kind == DeclarationDescriptor::PARAMETER ? PARAMETER_VARIABLE
+                                                           : NORMAL_VARIABLE,
+      init, &sloppy_mode_block_scope_function_redefinition, &local_ok);
   if (!local_ok) {
     // If we only have the start position of a proxy, we can't highlight the
     // whole variable name.  Pretend its length is 1 so that we highlight at

src/parsing/preparser.h

@@ -1636,26 +1636,12 @@ class PreParser : public ParserBase<PreParser> {
                                     int initializer_end_position,
                                     bool is_rest) {
     DeclarationScope* scope = parameters->scope;
-    if (pattern.variables_ == nullptr) {
-      scope->DeclareParameterName(ast_value_factory()->empty_string(), is_rest,
-                                  ast_value_factory(), false, true);
-    } else {
-      // We declare the parameter name for all names, but only create a
-      // parameter entry for the first one.
-      auto it = pattern.variables_->begin();
-      if (!parameters->has_duplicate() &&
-          scope->LookupLocal(it->raw_name()) != nullptr) {
-        parameters->set_has_duplicate();
-      }
-      scope->DeclareParameterName(it->raw_name(), is_rest, ast_value_factory(),
-                                  true, true);
-      for (++it; it != pattern.variables_->end(); ++it) {
-        if (!parameters->has_duplicate() &&
-            scope->LookupLocal(it->raw_name()) != nullptr) {
-          parameters->set_has_duplicate();
-        }
-        scope->DeclareParameterName(it->raw_name(), is_rest,
-                                    ast_value_factory(), true, false);
+    scope->RecordParameter(is_rest);
+    if (pattern.variables_) {
+      for (VariableProxy* param : *pattern.variables_) {
+        const AstRawString* name = param->raw_name();
+        if (scope->LookupLocal(name)) parameters->set_has_duplicate();
+        scope->DeclareParameterName(name);
       }
     }
     parameters->UpdateArityAndFunctionLength(!initializer.IsNull(), is_rest);
@@ -1670,13 +1656,11 @@ class PreParser : public ParserBase<PreParser> {
       PreParserFormalParameters* parameters, const PreParserExpression& params,
       const Scanner::Location& params_loc) {
     if (params.variables_ != nullptr) {
-      Scope* scope = parameters->scope;
-      for (auto variable : *params.variables_) {
-        if (!parameters->has_duplicate() &&
-            scope->LookupLocal(variable->raw_name())) {
-          parameters->set_has_duplicate();
-        }
-        scope->DeclareVariableName(variable->raw_name(), VariableMode::kVar);
+      DeclarationScope* scope = parameters->scope;
+      for (auto param : *params.variables_) {
+        const AstRawString* name = param->raw_name();
+        if (scope->LookupLocal(name)) parameters->set_has_duplicate();
+        scope->DeclareParameterName(name);
       }
     }
   }