[parser] Track duplicate formals through FormalParametersT

This simplifies the ExpressionClassifier a bit again, making it a little more
understandable.

Change-Id: I57bdd871b10409ea04b33748609160f2b40a498a
Reviewed-on: https://chromium-review.googlesource.com/c/1348431Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57753}

src/parsing/expression-classifier.h

@@ -22,10 +22,9 @@ class ZoneList;
   T(FormalParameterInitializerProduction, 1) \
   T(BindingPatternProduction, 2)             \
   T(AssignmentPatternProduction, 3)          \
-  T(DistinctFormalParametersProduction, 4)   \
-  T(StrictModeFormalParametersProduction, 5) \
-  T(LetPatternProduction, 6)                 \
-  T(AsyncArrowFormalParametersProduction, 7)
+  T(StrictModeFormalParametersProduction, 4) \
+  T(LetPatternProduction, 5)                 \
+  T(AsyncArrowFormalParametersProduction, 6)
 
 // Expression classifiers serve two purposes:
 //
@@ -145,10 +144,6 @@ class ExpressionClassifierBase {
     return is_valid(AssignmentPatternProduction);
   }
 
-  V8_INLINE bool is_valid_formal_parameter_list_without_duplicates() const {
-    return is_valid(DistinctFormalParametersProduction);
-  }
-
   // Note: callers should also check
   // is_valid_formal_parameter_list_without_duplicates().
   V8_INLINE bool is_valid_strict_mode_formal_parameters() const {
@@ -387,10 +382,6 @@ class ExpressionClassifier
     return this->reported_error(ErrorKind::kAssignmentPatternProduction);
   }
 
-  V8_INLINE const Error& duplicate_formal_parameter_error() const {
-    return this->reported_error(ErrorKind::kDistinctFormalParametersProduction);
-  }
-
   V8_INLINE const Error& strict_mode_formal_parameter_error() const {
     return this->reported_error(
         ErrorKind::kStrictModeFormalParametersProduction);
@@ -451,12 +442,6 @@ class ExpressionClassifier
                     ErrorKind::kAsyncArrowFormalParametersProduction, arg));
   }
 
-  void RecordDuplicateFormalParameterError(const Scanner::Location& loc) {
-    this->Add(TP::DistinctFormalParametersProduction,
-              Error(loc, MessageTemplate::kParamDupe,
-                    ErrorKind::kDistinctFormalParametersProduction));
-  }
-
   // Record a binding that would be invalid in strict mode.  Confusingly this
   // is not the same as StrictFormalParameterList, which simply forbids
   // duplicate bindings.

src/parsing/parser-base.h

@@ -900,10 +900,15 @@ class ParserBase {
   }
 
   void ValidateFormalParameters(LanguageMode language_mode,
+                                const FormalParametersT& parameters,
                                 bool allow_duplicates) {
-    if (!allow_duplicates &&
-        !classifier()->is_valid_formal_parameter_list_without_duplicates()) {
-      ReportClassifierError(classifier()->duplicate_formal_parameter_error());
+    if (!allow_duplicates && parameters.has_duplicate()) {
+      if (classifier()->does_error_reporting()) {
+        impl()->ReportMessageAt(parameters.duplicate_location(),
+                                MessageTemplate::kParamDupe);
+      } else {
+        impl()->ReportUnidentifiableError();
+      }
     } else if (is_strict(language_mode) &&
                !classifier()->is_valid_strict_mode_formal_parameters()) {
       ReportClassifierError(classifier()->strict_mode_formal_parameter_error());
@@ -3969,7 +3974,8 @@ void ParserBase<Impl>::ParseFunctionBody(
     body->Add(inner_block);
   }
 
-  ValidateFormalParameters(language_mode(), allow_duplicate_parameters);
+  ValidateFormalParameters(language_mode(), parameters,
+                           allow_duplicate_parameters);
 
   if (!IsArrowFunction(kind)) {
     // Declare arguments after parsing the function since lexical 'arguments'
@@ -4103,7 +4109,7 @@ ParserBase<Impl>::ParseArrowFunctionLiteral(
 
         // Validate parameter names. We can do this only after preparsing the
         // function, since the function can declare itself strict.
-        ValidateFormalParameters(language_mode(), false);
+        ValidateFormalParameters(language_mode(), formal_parameters, false);
 
         DCHECK_NULL(produced_preparsed_scope_data);
 

src/parsing/parser.cc

@@ -3007,8 +3007,7 @@ void Parser::ParseFunction(
 
   RewriteDestructuringAssignments();
 
-  *has_duplicate_parameters =
-      !classifier()->is_valid_formal_parameter_list_without_duplicates();
+  *has_duplicate_parameters = formals.has_duplicate();
 
   *expected_property_count = function_state.expected_property_count();
   *suspend_count = function_state.suspend_count();

src/parsing/parser.h

@@ -113,9 +113,13 @@ struct ParserFormalParameters : FormalParametersBase {
     Parameter* const* next() const { return &next_parameter; }
   };
 
+  Scanner::Location duplicate_location() const { return duplicate_loc; }
+  bool has_duplicate() const { return duplicate_loc.IsValid(); }
+
   explicit ParserFormalParameters(DeclarationScope* scope)
       : FormalParametersBase(scope) {}
   base::ThreadedList<Parameter> params;
+  Scanner::Location duplicate_loc = Scanner::Location::invalid();
 };
 
 template <>
@@ -905,8 +909,7 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
     parameters->params.Add(parameter);
   }
 
-  V8_INLINE void DeclareFormalParameters(
-      const ParserFormalParameters* parameters) {
+  V8_INLINE void DeclareFormalParameters(ParserFormalParameters* parameters) {
     bool is_simple = parameters->is_simple;
     DeclarationScope* scope = parameters->scope;
     if (!is_simple) scope->SetHasNonSimpleParameters();
@@ -916,10 +919,11 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
       // their names. If the parameter list is not simple, declare a temporary
       // for each parameter - the corresponding named variable is declared by
       // BuildParamerterInitializationBlock.
-      if (is_simple && scope->LookupLocal(parameter->name)) {
-        classifier()->RecordDuplicateFormalParameterError(
+      if (is_simple && !parameters->has_duplicate() &&
+          scope->LookupLocal(parameter->name)) {
+        parameters->duplicate_loc =
             Scanner::Location(parameter->position,
-                              parameter->position + parameter->name->length()));
+                              parameter->position + parameter->name->length());
       }
       scope->DeclareParameter(
           is_simple ? parameter->name : ast_value_factory()->empty_string(),

src/parsing/preparser.cc

@@ -197,7 +197,8 @@ PreParser::PreParseResult PreParser::PreParseFunction(
     if (!IsArrowFunction(kind)) {
       // Validate parameter names. We can do this only after parsing the
       // function, since the function can declare itself strict.
-      ValidateFormalParameters(language_mode(), allow_duplicate_parameters);
+      ValidateFormalParameters(language_mode(), formals,
+                               allow_duplicate_parameters);
       if (has_error()) {
         if (pending_error_handler()->has_error_unidentifiable_by_preparser()) {
           return kPreParseNotIdentifiableError;

src/parsing/preparser.h

@@ -817,12 +817,18 @@ class PreParserFactory {
   Zone* zone_;
 };
 
-
-struct PreParserFormalParameters : FormalParametersBase {
+class PreParserFormalParameters : public FormalParametersBase {
+ public:
   explicit PreParserFormalParameters(DeclarationScope* scope)
       : FormalParametersBase(scope) {}
-};
 
+  Scanner::Location duplicate_location() const { UNREACHABLE(); }
+  bool has_duplicate() const { return has_duplicate_; }
+  void set_has_duplicate() { has_duplicate_ = true; }
+
+ private:
+  bool has_duplicate_ = false;
+};
 
 class PreParser;
 
@@ -1630,16 +1636,16 @@ class PreParser : public ParserBase<PreParser> {
       // We declare the parameter name for all names, but only create a
       // parameter entry for the first one.
       auto it = pattern.variables_->begin();
-      if (scope->LookupLocal(it->raw_name()) != nullptr) {
-        classifier()->RecordDuplicateFormalParameterError(
-            Scanner::Location::invalid());
+      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 (scope->LookupLocal(it->raw_name()) != nullptr) {
-          classifier()->RecordDuplicateFormalParameterError(
-              Scanner::Location::invalid());
+        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);
@@ -1659,9 +1665,9 @@ class PreParser : public ParserBase<PreParser> {
     if (params.variables_ != nullptr) {
       Scope* scope = parameters->scope;
       for (auto variable : *params.variables_) {
-        if (scope->LookupLocal(variable->raw_name())) {
-          classifier()->RecordDuplicateFormalParameterError(
-              Scanner::Location::invalid());
+        if (!parameters->has_duplicate() &&
+            scope->LookupLocal(variable->raw_name())) {
+          parameters->set_has_duplicate();
         }
         scope->DeclareVariableName(variable->raw_name(), VariableMode::kVar);
       }