[parser] Detect duplciate lexical declarations in preparser

This changes how rewind upon preparser abort works. It now rewinds to the start
of the parameter scope. In the case of "function X(" it is before the "(". In
the case of arrow functions it's before the start of the arrow function. This
allows us to reparse the arrow function from the start so all parameters are
declared properly.

Bug: v8:2728, v8:7390
Change-Id: I1c40056a49ec198560e63cd73949a59221ee0401
Reviewed-on: https://chromium-review.googlesource.com/c/1382736Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58332}

src/ast/scopes.cc

@@ -1147,8 +1147,8 @@ Variable* Scope::DeclareVariableName(const AstRawString* name,
              IsLexicalVariableMode(var->mode())) {
     // Duplicate functions are allowed in the sloppy mode, but if this is not
     // a function declaration, it's an error. This is an error PreParser
-    // hasn't previously detected. TODO(marja): Investigate whether we can now
-    // start returning this error.
+    // hasn't previously detected.
+    return nullptr;
   } else if (mode == VariableMode::kVar) {
     var->set_maybe_assigned();
   }
@@ -1463,11 +1463,7 @@ void DeclarationScope::ResetAfterPreparsing(AstValueFactory* ast_value_factory,
   DCHECK(is_function_scope());
 
   // Reset all non-trivial members.
-  if (!aborted || !IsArrowFunction(function_kind_)) {
-    // Do not remove parameters when lazy parsing an Arrow Function has failed,
-    // as the formal parameters are not re-parsed.
-    params_.Clear();
-  }
+  params_.Clear();
   decls_.Clear();
   locals_.Clear();
   inner_scope_ = nullptr;

src/parsing/parser-base.h

@@ -1414,6 +1414,15 @@ class ParserBase {
              kind == FunctionKind::kArrowFunction && has_simple_parameter_list;
     }
 
+    void Reset() {
+      rewritable_length = -1;
+      scope_snapshot.Clear();
+      kind = FunctionKind::kArrowFunction;
+      has_simple_parameter_list = true;
+      ClearStrictParameterError();
+      DCHECK(HasInitialState());
+    }
+
     // Tracks strict-mode parameter violations of sloppy-mode arrow heads in
     // case the function ends up becoming strict mode. Only one global place to
     // track this is necessary since arrow functions with none-simple parameters
@@ -2638,9 +2647,6 @@ ParserBase<Impl>::ParseAssignmentExpressionCoverGrammar() {
       return impl()->FailureExpression();
     }
 
-    // Reset to default.
-    next_arrow_function_info_.kind = FunctionKind::kArrowFunction;
-
     // Because the arrow's parameters were parsed in the outer scope,
     // we need to fix up the scope chain appropriately.
     next_arrow_function_info_.scope_snapshot.Reparent(scope);
@@ -2649,11 +2655,9 @@ ParserBase<Impl>::ParseAssignmentExpressionCoverGrammar() {
     parameters.set_strict_parameter_error(
         next_arrow_function_info_.strict_parameter_error_location,
         next_arrow_function_info_.strict_parameter_error_message);
-    next_arrow_function_info_.ClearStrictParameterError();
     if (!next_arrow_function_info_.has_simple_parameter_list) {
       scope->SetHasNonSimpleParameters();
       parameters.is_simple = false;
-      next_arrow_function_info_.has_simple_parameter_list = true;
     }
 
     scope->set_start_position(lhs_beg_pos);
@@ -4044,7 +4048,7 @@ ParserBase<Impl>::ParseArrowFunctionLiteral(
     // in this function's parameter list into its own function_state.
     function_state.AdoptDestructuringAssignmentsFromParentState(
         next_arrow_function_info_.rewritable_length);
-    next_arrow_function_info_.rewritable_length = -1;
+    next_arrow_function_info_.Reset();
 
     Consume(Token::ARROW);
 
@@ -4080,10 +4084,21 @@ ParserBase<Impl>::ParseArrowFunctionLiteral(
         } else {
           // In case we did not sucessfully preparse the function because of an
           // unidentified error we do a full reparse to return the error.
+          ExpressionT expression = ParseConditionalExpression();
+          Scanner::Location loc(scope()->start_position(), end_position());
+          FormalParametersT parameters(formal_parameters.scope);
+          parameters.is_simple =
+              next_arrow_function_info_.has_simple_parameter_list;
+          impl()->DeclareArrowFunctionFormalParameters(&parameters, expression,
+                                                       loc);
+          next_arrow_function_info_.Reset();
+
+          Consume(Token::ARROW);
           Consume(Token::LBRACE);
+
           AcceptINScope scope(this, true);
           ParseFunctionBody(&body, impl()->NullIdentifier(), kNoSourcePosition,
-                            formal_parameters, kind,
+                            parameters, kind,
                             FunctionLiteral::kAnonymousExpression,
                             FunctionBodyType::kBlock);
           CHECK(has_error());

src/parsing/parser.cc

@@ -2588,6 +2588,9 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
                    &produced_preparsed_scope_data, is_lazy_top_level_function,
                    &eager_compile_hint);
   if (!did_preparse_successfully) {
+    // If skipping aborted, it rewound the scanner until before the LPAREN.
+    // Consume it in that case.
+    if (should_preparse) Consume(Token::LPAREN);
     should_post_parallel_task = false;
     ParseFunction(&body, function_name, pos, kind, function_type, scope,
                   &num_parameters, &function_length, &has_duplicate_parameters,
@@ -2696,7 +2699,7 @@ bool Parser::SkipFunction(
   }
 
   Scanner::BookmarkScope bookmark(scanner());
-  bookmark.Set();
+  bookmark.Set(function_scope->start_position());
 
   // With no cached data, we partially parse the function, without building an
   // AST. This gathers the data needed to build a lazy function.

src/parsing/preparser.cc

@@ -434,6 +434,10 @@ void PreParser::DeclareAndInitializeVariables(
       declaration_descriptor->scope->DeleteUnresolved(variable);
       Variable* var = scope()->DeclareVariableName(
           variable->raw_name(), declaration_descriptor->mode);
+      if (var == nullptr) {
+        ReportUnidentifiableError();
+        return;
+      }
       MarkLoopVariableAsAssigned(declaration_descriptor->scope, var,
                                  declaration_descriptor->declaration_kind);
       // This is only necessary if there is an initializer, but we don't have

src/parsing/scanner.cc

@@ -122,26 +122,14 @@ void Scanner::LiteralBuffer::AddTwoByteChar(uc32 code_unit) {
 // ----------------------------------------------------------------------------
 // Scanner::BookmarkScope
 
-const size_t Scanner::BookmarkScope::kBookmarkAtFirstPos =
-    std::numeric_limits<size_t>::max() - 2;
 const size_t Scanner::BookmarkScope::kNoBookmark =
     std::numeric_limits<size_t>::max() - 1;
 const size_t Scanner::BookmarkScope::kBookmarkWasApplied =
     std::numeric_limits<size_t>::max();
 
-void Scanner::BookmarkScope::Set() {
+void Scanner::BookmarkScope::Set(size_t position) {
   DCHECK_EQ(bookmark_, kNoBookmark);
-
-  // The first token is a bit special, since current_ will still be
-  // uninitialized. In this case, store kBookmarkAtFirstPos and special-case it
-  // when
-  // applying the bookmark.
-  DCHECK_IMPLIES(scanner_->current().token == Token::UNINITIALIZED,
-                 scanner_->current().location.beg_pos ==
-                     scanner_->next().location.beg_pos);
-  bookmark_ = (scanner_->current().token == Token::UNINITIALIZED)
-                  ? kBookmarkAtFirstPos
-                  : scanner_->location().beg_pos;
+  bookmark_ = position;
 }
 
 void Scanner::BookmarkScope::Apply() {
@@ -150,13 +138,7 @@ void Scanner::BookmarkScope::Apply() {
     scanner_->set_parser_error();
   } else {
     scanner_->reset_parser_error_flag();
-    if (bookmark_ == kBookmarkAtFirstPos) {
-      scanner_->SeekNext(0);
-    } else {
-      scanner_->SeekNext(bookmark_);
-      scanner_->Next();
-      DCHECK_EQ(scanner_->location().beg_pos, static_cast<int>(bookmark_));
-    }
+    scanner_->SeekNext(bookmark_);
   }
   bookmark_ = kBookmarkWasApplied;
 }

src/parsing/scanner.h

@@ -217,7 +217,7 @@ class Scanner {
     }
     ~BookmarkScope() = default;
 
-    void Set();
+    void Set(size_t bookmark);
     void Apply();
     bool HasBeenSet() const;
     bool HasBeenApplied() const;
@@ -225,7 +225,6 @@ class Scanner {
    private:
     static const size_t kNoBookmark;
     static const size_t kBookmarkWasApplied;
-    static const size_t kBookmarkAtFirstPos;
 
     Scanner* scanner_;
     size_t bookmark_;

test/cctest/parsing/test-scanner.cc

@@ -70,7 +70,7 @@ TEST(Bookmarks) {
 
     for (size_t i = 0; i < std::min(bookmark_pos + 10, tokens.size()); i++) {
       if (i == bookmark_pos) {
-        bookmark.Set();
+        bookmark.Set(scanner->peek_location().beg_pos);
       }
       CHECK_TOK(tokens[i], scanner->Next());
     }

test/mjsunit/regress/regress-crbug-800032.js

@@ -6,10 +6,7 @@
 
 
 class Derived extends RegExp {
-  constructor(a) {
-    // Syntax Error
-    const a = 1;
-  }
+  constructor(a) { throw "error" }
 }
 
 let o = Reflect.construct(RegExp, [], Derived);

test/mjsunit/regress/regress-crbug-806388.js

@@ -5,10 +5,7 @@
 // Flags: --allow-natives-syntax --enable-slow-asserts --expose-gc
 
 class Derived extends Array {
-    constructor(a) {
-      // Syntax Error.
-      const a = 1;
-    }
+    constructor(a) { throw "error" }
 }
 
 // Derived is not a subclass of RegExp

test/test262/test262.status

@@ -499,14 +499,6 @@
   'annexB/language/eval-code/direct/func-switch-case-eval-func-no-skip-try': [FAIL],
   'annexB/language/eval-code/direct/func-switch-dflt-eval-func-no-skip-try': [FAIL],
 
-  # PreParser doesn't produce early errors
-  # https://bugs.chromium.org/p/v8/issues/detail?id=2728
-  'language/expressions/async-arrow-function/early-errors-arrow-formals-body-duplicate': [FAIL],
-  'language/expressions/object/method-definition/generator-param-redecl-const': [FAIL],
-  'language/expressions/object/method-definition/generator-param-redecl-let': [FAIL],
-  'language/expressions/object/method-definition/name-param-redecl': [FAIL],
-  'language/statements/async-function/early-errors-declaration-formals-body-duplicate': [FAIL],
-
   # SharedArrayBuffer tests that require flags
   'built-ins/SharedArrayBuffer/*': ['--harmony-sharedarraybuffer'],
   'built-ins/Atomics/*': ['--harmony-sharedarraybuffer'],