[parser] Introduce FailureExpression to return instead of nullptr

That allows us to keep on running further without explicit RETURN_IF

Bug: v8:8363, v8:7926
Change-Id: If1424a1dae656ac725a8443b09ea1b8cc25dfcb1
Reviewed-on: https://chromium-review.googlesource.com/c/1322953Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57319}

src/ast/ast.h

@@ -109,6 +109,8 @@ namespace internal {
   V(Yield)                      \
   V(YieldStar)
 
+#define FAILURE_NODE_LIST(V) V(FailureExpression)
+
 #define AST_NODE_LIST(V)                        \
   DECLARATION_NODE_LIST(V)                      \
   STATEMENT_NODE_LIST(V)                        \
@@ -128,12 +130,16 @@ class Statement;
 
 #define DEF_FORWARD_DECLARATION(type) class type;
 AST_NODE_LIST(DEF_FORWARD_DECLARATION)
+FAILURE_NODE_LIST(DEF_FORWARD_DECLARATION)
 #undef DEF_FORWARD_DECLARATION
 
 class AstNode: public ZoneObject {
  public:
 #define DECLARE_TYPE_ENUM(type) k##type,
-  enum NodeType : uint8_t { AST_NODE_LIST(DECLARE_TYPE_ENUM) };
+  enum NodeType : uint8_t {
+    AST_NODE_LIST(DECLARE_TYPE_ENUM) /* , */
+    FAILURE_NODE_LIST(DECLARE_TYPE_ENUM)
+  };
 #undef DECLARE_TYPE_ENUM
 
   void* operator new(size_t size, Zone* zone) { return zone->New(size); }
@@ -152,6 +158,7 @@ class AstNode: public ZoneObject {
   V8_INLINE type* As##type();        \
   V8_INLINE const type* As##type() const;
   AST_NODE_LIST(DECLARE_NODE_FUNCTIONS)
+  FAILURE_NODE_LIST(DECLARE_NODE_FUNCTIONS)
 #undef DECLARE_NODE_FUNCTIONS
 
   BreakableStatement* AsBreakableStatement();
@@ -249,6 +256,12 @@ class Expression : public AstNode {
   static const uint8_t kNextBitFieldIndex = AstNode::kNextBitFieldIndex;
 };
 
+class FailureExpression : public Expression {
+ private:
+  friend class AstNodeFactory;
+  FailureExpression() : Expression(kNoSourcePosition, kFailureExpression) {}
+};
+
 // V8's notion of BreakableStatement does not correspond to the notion of
 // BreakableStatement in ECMAScript. In V8, the idea is that a
 // BreakableStatement is a statement that can be the target of a break
@@ -2837,9 +2850,14 @@ class AstVisitor {
   case AstNode::k##NodeType:                                            \
     return this->impl()->Visit##NodeType(static_cast<NodeType*>(node));
 
-#define GENERATE_AST_VISITOR_SWITCH()  \
-  switch (node->node_type()) {         \
-    AST_NODE_LIST(GENERATE_VISIT_CASE) \
+#define GENERATE_FAILURE_CASE(NodeType) \
+  case AstNode::k##NodeType:            \
+    UNREACHABLE();
+
+#define GENERATE_AST_VISITOR_SWITCH()        \
+  switch (node->node_type()) {               \
+    AST_NODE_LIST(GENERATE_VISIT_CASE)       \
+    FAILURE_NODE_LIST(GENERATE_FAILURE_CASE) \
   }
 
 #define DEFINE_AST_VISITOR_SUBCLASS_MEMBERS()               \
@@ -2894,7 +2912,8 @@ class AstNodeFactory final {
   AstNodeFactory(AstValueFactory* ast_value_factory, Zone* zone)
       : zone_(zone),
         ast_value_factory_(ast_value_factory),
-        empty_statement_(new (zone) class EmptyStatement()) {}
+        empty_statement_(new (zone) class EmptyStatement()),
+        failure_expression_(new (zone) class FailureExpression()) {}
 
   AstValueFactory* ast_value_factory() const { return ast_value_factory_; }
 
@@ -3051,6 +3070,10 @@ class AstNodeFactory final {
     return empty_statement_;
   }
 
+  class FailureExpression* FailureExpression() {
+    return failure_expression_;
+  }
+
   SloppyBlockFunctionStatement* NewSloppyBlockFunctionStatement() {
     return new (zone_) SloppyBlockFunctionStatement(EmptyStatement());
   }
@@ -3422,6 +3445,7 @@ class AstNodeFactory final {
   Zone* zone_;
   AstValueFactory* ast_value_factory_;
   class EmptyStatement* empty_statement_;
+  class FailureExpression* failure_expression_;
 };
 
 
@@ -3463,6 +3487,7 @@ class AstNodeFactory final {
                                     : nullptr;                               \
   }
 AST_NODE_LIST(DECLARE_NODE_FUNCTIONS)
+FAILURE_NODE_LIST(DECLARE_NODE_FUNCTIONS)
 #undef DECLARE_NODE_FUNCTIONS
 
 }  // namespace internal

src/ast/scopes.cc

@@ -1446,6 +1446,7 @@ void DeclarationScope::ResetAfterPreparsing(AstValueFactory* ast_value_factory,
     zone_ = ast_value_factory->zone();
     variables_.Reset(ZoneAllocationPolicy(zone_));
     if (!IsArrowFunction(function_kind_)) {
+      has_simple_parameters_ = true;
       DeclareDefaultFunctionVariables(ast_value_factory);
     }
   } else {

src/parsing/parser.cc

@@ -138,24 +138,6 @@ void Parser::GetUnexpectedTokenMessage(Token::Value token,
   }
 }
 
-// ----------------------------------------------------------------------------
-// The RETURN_IF_PARSE_ERROR macro is a convenient macro to enforce error
-// handling for functions that may fail (by returning if there was an parser
-// error).
-//
-// Usage:
-//     foo = ParseFoo(); // may fail
-//     RETURN_IF_PARSE_ERROR
-//
-//     SAFE_USE(foo);
-
-#define RETURN_IF_PARSE_ERROR_VALUE(x) \
-  if (has_error()) return x;
-
-#define RETURN_IF_PARSE_ERROR RETURN_IF_PARSE_ERROR_VALUE(nullptr)
-#define RETURN_IF_PARSE_ERROR_VOID \
-  if (has_error()) return;
-
 // ----------------------------------------------------------------------------
 // Implementation of Parser
 
@@ -327,7 +309,7 @@ Expression* Parser::ImportMetaExpression(int pos) {
                                    pos);
 }
 
-Literal* Parser::ExpressionFromLiteral(Token::Value token, int pos) {
+Expression* Parser::ExpressionFromLiteral(Token::Value token, int pos) {
   switch (token) {
     case Token::NULL_LITERAL:
       return factory()->NewNullLiteral(pos);
@@ -354,7 +336,7 @@ Literal* Parser::ExpressionFromLiteral(Token::Value token, int pos) {
     default:
       DCHECK(false);
   }
-  return nullptr;
+  return FailureExpression();
 }
 
 Expression* Parser::NewV8Intrinsic(const AstRawString* name,
@@ -384,14 +366,14 @@ Expression* Parser::NewV8Intrinsic(const AstRawString* name,
         return args.at(0);
       } else {
         ReportMessage(MessageTemplate::kNotIsvar);
-        return nullptr;
+        return FailureExpression();
       }
     }
 
     // Check that the expected number of arguments are being passed.
     if (function->nargs != -1 && function->nargs != args.length()) {
       ReportMessage(MessageTemplate::kRuntimeWrongNumArgs);
-      return nullptr;
+      return FailureExpression();
     }
 
     return factory()->NewCallRuntime(function, args, pos);
@@ -403,7 +385,7 @@ Expression* Parser::NewV8Intrinsic(const AstRawString* name,
   // Check that the function is defined.
   if (context_index == Context::kNotFound) {
     ReportMessage(MessageTemplate::kNotDefined, name);
-    return nullptr;
+    return FailureExpression();
   }
 
   return factory()->NewCallRuntime(context_index, args, pos);
@@ -614,7 +596,6 @@ FunctionLiteral* Parser::DoParseProgram(Isolate* isolate, ParseInfo* info) {
       // unchanged if the property already exists.
       InsertSloppyBlockFunctionVarBindings(scope);
     }
-    RETURN_IF_PARSE_ERROR;
     CheckConflictingVarDeclarations(scope);
 
     if (info->parse_restriction() == ONLY_SINGLE_FUNCTION_LITERAL) {
@@ -639,7 +620,7 @@ FunctionLiteral* Parser::DoParseProgram(Isolate* isolate, ParseInfo* info) {
   // Make sure the target stack is empty.
   DCHECK_NULL(target_stack_);
 
-  RETURN_IF_PARSE_ERROR;
+  if (has_error()) return nullptr;
   return result;
 }
 
@@ -867,7 +848,7 @@ FunctionLiteral* Parser::DoParseFunction(Isolate* isolate, ParseInfo* info,
           arguments_for_wrapped_function);
     }
 
-    RETURN_IF_PARSE_ERROR;
+    if (has_error()) return nullptr;
     result->set_requires_instance_members_initializer(
         info->requires_instance_members_initializer());
   }
@@ -918,7 +899,7 @@ void Parser::ParseModuleItemList(ScopedPtrList<Statement>* body) {
   DCHECK(scope()->is_module_scope());
   while (peek() != Token::EOS) {
     Statement* stat = ParseModuleItem();
-    RETURN_IF_PARSE_ERROR_VOID;
+    if (has_error()) return;
     if (stat->IsEmptyStatement()) continue;
     body->Add(stat);
   }
@@ -975,7 +956,7 @@ ZoneChunkList<Parser::ExportClauseData>* Parser::ParseExportClause(
     export_data->push_back({export_name, local_name, location});
     if (peek() == Token::RBRACE) break;
     Expect(Token::COMMA);
-    RETURN_IF_PARSE_ERROR;
+    if (has_error()) break;
   }
 
   Expect(Token::RBRACE);
@@ -1018,7 +999,6 @@ ZonePtrList<const Parser::NamedImport>* Parser::ParseNamedImports(int pos) {
       return nullptr;
     }
 
-    RETURN_IF_PARSE_ERROR;
     DeclareVariable(local_name, VariableMode::kConst, kNeedsInitialization,
                     position());
 
@@ -1069,7 +1049,6 @@ void Parser::ParseImportDeclaration() {
   if (tok != Token::MUL && tok != Token::LBRACE) {
     import_default_binding = ParseIdentifier(kDontAllowRestrictedIdentifiers);
     import_default_binding_loc = scanner()->location();
-    RETURN_IF_PARSE_ERROR_VOID;
     DeclareVariable(import_default_binding, VariableMode::kConst,
                     kNeedsInitialization, pos);
   }
@@ -1086,7 +1065,6 @@ void Parser::ParseImportDeclaration() {
         module_namespace_binding =
             ParseIdentifier(kDontAllowRestrictedIdentifiers);
         module_namespace_binding_loc = scanner()->location();
-        RETURN_IF_PARSE_ERROR_VOID;
         DeclareVariable(module_namespace_binding, VariableMode::kConst,
                         kCreatedInitialized, pos);
         break;
@@ -1176,7 +1154,6 @@ Statement* Parser::ParseExportDefault() {
       ExpressionClassifier classifier(this);
       Expression* value = ParseAssignmentExpression(true);
       ValidateExpression();
-      RETURN_IF_PARSE_ERROR;
       SetFunctionName(value, ast_value_factory()->default_string());
 
       const AstRawString* local_name =
@@ -1199,7 +1176,7 @@ Statement* Parser::ParseExportDefault() {
     }
   }
 
-  RETURN_IF_PARSE_ERROR;
+  if (has_error()) return nullptr;
   DCHECK_EQ(local_names.length(), 1);
   module()->AddExport(local_names.first(),
                       ast_value_factory()->default_string(), default_loc,
@@ -1244,7 +1221,6 @@ void Parser::ParseExportStar() {
   Scanner::Location export_name_loc = scanner()->location();
   const AstRawString* local_name = NextInternalNamespaceExportName();
   Scanner::Location local_name_loc = Scanner::Location::invalid();
-  RETURN_IF_PARSE_ERROR_VOID;
   DeclareVariable(local_name, VariableMode::kConst, kCreatedInitialized, pos);
 
   ExpectContextualKeyword(Token::FROM);
@@ -1304,7 +1280,6 @@ Statement* Parser::ParseExportDeclaration() {
         return nullptr;
       }
       ExpectSemicolon();
-      RETURN_IF_PARSE_ERROR;
       if (module_specifier == nullptr) {
         for (const ExportClauseData& data : *export_data) {
           module()->AddExport(data.local_name, data.export_name, data.location,
@@ -1573,10 +1548,11 @@ Expression* Parser::RewriteReturn(Expression* return_value, int pos) {
 }
 
 Expression* Parser::RewriteDoExpression(Block* body, int pos) {
+  if (has_error()) return FailureExpression();
   Variable* result = NewTemporary(ast_value_factory()->dot_result_string());
   DoExpression* expr = factory()->NewDoExpression(body, result, pos);
   if (!Rewriter::Rewrite(this, GetClosureScope(), expr, ast_value_factory())) {
-    return nullptr;
+    return FailureExpression();
   }
   return expr;
 }
@@ -1977,7 +1953,6 @@ void Parser::DesugarBindingInForEachStatement(ForInfo* for_info,
               for_info->bound_names.Contains(name)) {
             ReportMessageAt(for_info->parsing_result.bindings_loc,
                             MessageTemplate::kVarRedeclaration, name);
-            return;
           }
         }
         catch_scope = catch_scope->outer_scope();
@@ -2614,7 +2589,7 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
 
   if (!is_wrapped) {
     Expect(Token::LPAREN);
-    RETURN_IF_PARSE_ERROR;
+    if (has_error()) return nullptr;
   }
   scope->set_start_position(position());
 
@@ -2658,8 +2633,6 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
     }
   }
 
-  RETURN_IF_PARSE_ERROR;
-
   // Validate function name. We can do this only after parsing the function,
   // since the function can declare itself strict.
   language_mode = scope->language_mode();
@@ -3219,6 +3192,7 @@ Expression* Parser::RewriteClassLiteral(Scope* block_scope,
 }
 
 void Parser::CheckConflictingVarDeclarations(Scope* scope) {
+  if (has_error()) return;
   Declaration* decl = scope->CheckConflictingVarDeclarations();
   if (decl != nullptr) {
     // In ES6, conflicting variable bindings are early errors.
@@ -3631,7 +3605,7 @@ void Parser::SetFunctionNameFromPropertyName(ObjectLiteralProperty* property,
 
 void Parser::SetFunctionNameFromIdentifierRef(Expression* value,
                                               Expression* identifier) {
-  if (has_error() || !identifier->IsVariableProxy()) return;
+  if (!identifier->IsVariableProxy()) return;
   SetFunctionName(value, identifier->AsVariableProxy()->raw_name());
 }
 
@@ -4102,8 +4076,5 @@ Statement* Parser::FinalizeForOfStatement(ForOfStatement* loop,
   return final_loop;
 }
 
-#undef RETURN_IF_PARSE_ERROR_VOID
-#undef RETURN_IF_PARSE_ERROR
-#undef RETURN_IF_PARSE_ERROR_VALUE
 }  // namespace internal
 }  // namespace v8

src/parsing/parser.h

@@ -812,6 +812,7 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
     return nullptr;
   }
   V8_INLINE static std::nullptr_t NullStatement() { return nullptr; }
+  Expression* FailureExpression() { return factory()->FailureExpression(); }
 
   template <typename T>
   V8_INLINE static bool IsNull(T subject) {
@@ -851,7 +852,7 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
   Expression* NewTargetExpression(int pos);
   Expression* ImportMetaExpression(int pos);
 
-  Literal* ExpressionFromLiteral(Token::Value token, int pos);
+  Expression* ExpressionFromLiteral(Token::Value token, int pos);
 
   V8_INLINE VariableProxy* ExpressionFromIdentifier(
       const AstRawString* name, int start_position,

src/parsing/pattern-rewriter.cc

@@ -74,6 +74,7 @@ class PatternRewriter final : public AstVisitor<PatternRewriter> {
   }
 
   Expression* Rewrite(Assignment* assign) {
+    if (parser_->has_error()) return parser_->FailureExpression();
     DCHECK_EQ(Token::ASSIGN, assign->op());
 
     int pos = assign->position();

src/parsing/preparser.h

@@ -95,6 +95,9 @@ class PreParserExpression {
       : code_(TypeField::encode(kNull)), variables_(nullptr) {}
 
   static PreParserExpression Null() { return PreParserExpression(); }
+  static PreParserExpression Failure() {
+    return PreParserExpression(TypeField::encode(kFailure));
+  }
 
   static PreParserExpression Default(
       VariableZoneThreadedListType* variables = nullptr) {
@@ -320,6 +323,7 @@ class PreParserExpression {
  private:
   enum Type {
     kNull,
+    kFailure,
     kExpression,
     kIdentifierExpression,
     kStringLiteralExpression,
@@ -1503,6 +1507,9 @@ class PreParser : public ParserBase<PreParser> {
   V8_INLINE static PreParserExpression NullExpression() {
     return PreParserExpression::Null();
   }
+  V8_INLINE static PreParserExpression FailureExpression() {
+    return PreParserExpression::Failure();
+  }
   V8_INLINE static PreParserExpression NullLiteralProperty() {
     return PreParserExpression::Null();
   }

test/cctest/test-parsing.cc

@@ -1838,6 +1838,7 @@ void RunParserSyncTest(
                                context_data[i][0],
                                statement_data[j],
                                context_data[i][1]);
+      PrintF("%s\n", program.start());
       CHECK_EQ(length, kProgramSize);
       TestParserSync(program.start(), flags, flags_len, result,
                      always_true_flags, always_true_len, always_false_flags,