[parser] Refactor of ParseClass* and ParseNativeDeclaration

src/parsing/parser-base.h

Index: src/parsing/parser-base.h
diff --git a/src/parsing/parser-base.h b/src/parsing/parser-base.h
index 47d96d2ddbd1af14486c3cfee884e016f244c06d..cc9d9e6d928eb02abca43385af91e298b83201fb 100644
--- a/src/parsing/parser-base.h
+++ b/src/parsing/parser-base.h
@@ -133,7 +133,8 @@ struct FormalParametersBase {
 //   typedef ObjectLiteralProperty;
 //   typedef ClassLiteralProperty;
 //   typedef ExpressionList;
-//   typedef PropertyList;
+//   typedef ObjectPropertyList;
+//   typedef ClassPropertyList;
 //   typedef FormalParameters;
 //   typedef Statement;
 //   typedef StatementList;
@@ -161,7 +162,6 @@ class ParserBase {
   typedef typename Types::ObjectLiteralProperty ObjectLiteralPropertyT;
   typedef typename Types::ClassLiteralProperty ClassLiteralPropertyT;
   typedef typename Types::ExpressionList ExpressionListT;
-  typedef typename Types::PropertyList PropertyListT;
   typedef typename Types::FormalParameters FormalParametersT;
   typedef typename Types::Statement StatementT;
   typedef typename Types::StatementList StatementListT;
@@ -680,6 +680,25 @@ class ParserBase {
     DeclarationParsingResult parsing_result;
   };
 
+  struct ClassInfo {
+   public:
+    explicit ClassInfo(ParserBase* parser)
+        : proxy(nullptr),

[marja, 2016/09/26 09:35:26]

The creation & modification (set_end_pos) & usage of proxy are now somehow far
apart and difficult to follow... I don't have immediate ideas on how to make it
simpler though...

[nickie, 2016/09/26 17:28:12]

I'm afraid not much can be done about 'proxy' here.

In principle, the ClassInfo type (and all the *Info classes that I introduced in
the refactoring) should be something different for every parser implementation. 
The object's construction does some implementation-specific initialization
(therefore 'proxy', which is not even visible in parser base, is initialized if
needed).  It is then modified and/or consumed by other implementation methods. 
I think this design is quite reasonable --- only this class should be moved to
the parser.  The preparser could have fewer fields or be entirely dummy.

+          extends(parser->impl()->EmptyExpression()),
+          properties(parser->impl()->NewClassPropertyList(4)),
+          instance_field_initializers(parser->impl()->NewExpressionList(0)),
+          constructor(parser->impl()->EmptyFunctionLiteral()),
+          has_seen_constructor(false),
+          static_initializer_var(nullptr) {}
+    VariableProxy* proxy;
+    ExpressionT extends;
+    typename Types::ClassPropertyList properties;
+    ExpressionListT instance_field_initializers;
+    FunctionLiteralT constructor;
+    bool has_seen_constructor;
+    Variable* static_initializer_var;
+  };
+
   DeclarationScope* NewScriptScope() const {
     return new (zone()) DeclarationScope(zone(), ast_value_factory());
   }
@@ -1189,6 +1208,10 @@ class ParserBase {
                                         const FormalParametersT& parameters,
                                         bool is_async,
                                         bool* ok);
+  ExpressionT ParseClassLiteral(IdentifierT name,
+                                Scanner::Location class_name_location,
+                                bool name_is_strict_reserved, int pos,
+                                bool* ok);
   ExpressionT ParseTemplateLiteral(ExpressionT tag, int start, bool* ok);
   ExpressionT ParseSuperExpression(bool is_new, bool* ok);
   ExpressionT ParseNewTargetExpression(bool* ok);
@@ -1208,6 +1231,9 @@ class ParserBase {
   StatementT ParseHoistableDeclaration(int pos, ParseFunctionFlags flags,
                                        ZoneList<const AstRawString*>* names,
                                        bool default_export, bool* ok);
+  StatementT ParseClassDeclaration(ZoneList<const AstRawString*>* names,
+                                   bool default_export, bool* ok);
+  StatementT ParseNativeDeclaration(bool* ok);
 
   // Under some circumstances, we allow preparsing to abort if the preparsed
   // function is "long and trivial", and fully parse instead. Our current
@@ -1806,9 +1832,9 @@ typename ParserBase<Impl>::ExpressionT ParserBase<Impl>::ParsePrimaryExpression(
                                                    CHECK_OK);
         class_name_location = scanner()->location();
       }
-      return impl()->ParseClassLiteral(name, class_name_location,
-                                       is_strict_reserved_name,
-                                       class_token_position, ok);
+      return ParseClassLiteral(name, class_name_location,
+                               is_strict_reserved_name, class_token_position,
+                               ok);
     }
 
     case Token::TEMPLATE_SPAN:
@@ -2474,7 +2500,8 @@ typename ParserBase<Impl>::ExpressionT ParserBase<Impl>::ParseObjectLiteral(
   // '{' (PropertyDefinition (',' PropertyDefinition)* ','? )? '}'
 
   int pos = peek_position();
-  PropertyListT properties = impl()->NewPropertyList(4);
+  typename Types::ObjectPropertyList properties =
+      impl()->NewObjectPropertyList(4);
   int number_of_boilerplate_properties = 0;
   bool has_computed_names = false;
   ObjectLiteralChecker checker(this);
@@ -3806,6 +3833,70 @@ ParserBase<Impl>::ParseHoistableDeclaration(
 }
 
 template <typename Impl>
+typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseClassDeclaration(
+    ZoneList<const AstRawString*>* names, bool default_export, bool* ok) {
+  // ClassDeclaration ::
+  //   'class' Identifier ('extends' LeftHandExpression)? '{' ClassBody '}'
+  //   'class' ('extends' LeftHandExpression)? '{' ClassBody '}'
+  //
+  // The anonymous form is allowed iff [default_export] is true.
+  //
+  // 'class' is expected to be consumed by the caller.
+  //
+  // A ClassDeclaration
+  //
+  //   class C { ... }
+  //
+  // has the same semantics as:
+  //
+  //   let C = class C { ... };
+  //
+  // so rewrite it as such.
+
+  int pos = position();
+  IdentifierT name = impl()->EmptyIdentifier();
+  bool is_strict_reserved = false;
+  IdentifierT variable_name = impl()->EmptyIdentifier();
+  if (default_export && (peek() == Token::EXTENDS || peek() == Token::LBRACE)) {
+    impl()->GetDefaultStrings(&name, &variable_name);
+  } else {
+    name = ParseIdentifierOrStrictReservedWord(&is_strict_reserved,
+                                               CHECK_OK_CUSTOM(NullStatement));
+    variable_name = name;
+  }
+
+  ExpressionClassifier no_classifier(this);
+  ExpressionT value =
+      ParseClassLiteral(name, scanner()->location(), is_strict_reserved, pos,
+                        CHECK_OK_CUSTOM(NullStatement));
+  return impl()->DeclareClass(variable_name, value, names, pos, ok);
+}
+
+// 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.
+template <typename Impl>
+typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseNativeDeclaration(
+    bool* ok) {
+  int pos = peek_position();
+  Expect(Token::FUNCTION, CHECK_OK_CUSTOM(NullStatement));
+  // Allow "eval" or "arguments" for backward compatibility.
+  IdentifierT name = ParseIdentifier(kAllowRestrictedIdentifiers,
+                                     CHECK_OK_CUSTOM(NullStatement));
+  Expect(Token::LPAREN, CHECK_OK_CUSTOM(NullStatement));
+  if (peek() != Token::RPAREN) {
+    do {
+      ParseIdentifier(kAllowRestrictedIdentifiers,
+                      CHECK_OK_CUSTOM(NullStatement));
+    } while (Check(Token::COMMA));
+  }
+  Expect(Token::RPAREN, CHECK_OK_CUSTOM(NullStatement));
+  Expect(Token::SEMICOLON, CHECK_OK_CUSTOM(NullStatement));
+  return impl()->DeclareNative(name, pos, ok);
+}
+
+template <typename Impl>
 void ParserBase<Impl>::CheckArityRestrictions(int param_count,
                                               FunctionKind function_kind,
                                               bool has_rest,
@@ -4017,6 +4108,67 @@ ParserBase<Impl>::ParseArrowFunctionLiteral(
 }
 
 template <typename Impl>
+typename ParserBase<Impl>::ExpressionT ParserBase<Impl>::ParseClassLiteral(
+    IdentifierT name, Scanner::Location class_name_location,
+    bool name_is_strict_reserved, int pos, bool* ok) {

[marja, 2016/09/26 09:35:26]

Can you rename pos into something more illustrative? It seems to be
class_token_position on the caller's side.

Can the positions be reordered; we're passing both class_name_location and
class_token_position, could we pass them in the opposite order so that the
smaller one is first?

[nickie, 2016/09/26 12:19:59]

The 'pos' is the position of the class token, not the class name (which need not
exist).  I'm renaming it to make this clear.

+  // All parts of a ClassDeclaration and ClassExpression are strict code.
+  if (name_is_strict_reserved) {
+    impl()->ReportMessageAt(class_name_location,
+                            MessageTemplate::kUnexpectedStrictReserved);
+    *ok = false;
+    return impl()->EmptyExpression();
+  }
+  if (impl()->IsEvalOrArguments(name)) {
+    impl()->ReportMessageAt(class_name_location,
+                            MessageTemplate::kStrictEvalArguments);
+    *ok = false;
+    return impl()->EmptyExpression();
+  }
+
+  BlockState block_state(&scope_state_);
+  RaiseLanguageMode(STRICT);
+
+  ClassInfo class_info(this);
+  impl()->DeclareClassVariable(name, block_state.scope(), &class_info, pos,
+                               CHECK_OK);
+
+  if (Check(Token::EXTENDS)) {
+    block_state.set_start_position(scanner()->location().end_pos);
+    ExpressionClassifier extends_classifier(this);
+    class_info.extends = ParseLeftHandSideExpression(CHECK_OK);
+    CheckNoTailCallExpressions(CHECK_OK);
+    impl()->RewriteNonPattern(CHECK_OK);
+    impl()->AccumulateFormalParameterContainmentErrors();
+  } else {
+    block_state.set_start_position(scanner()->location().end_pos);
+  }
+
+  ClassLiteralChecker checker(this);
+
+  Expect(Token::LBRACE, CHECK_OK);
+
+  const bool has_extends = !impl()->IsEmptyExpression(class_info.extends);
+  while (peek() != Token::RBRACE) {
+    if (Check(Token::SEMICOLON)) continue;
+    FuncNameInferrer::State fni_state(fni_);
+    bool is_computed_name = false;  // Classes do not care about computed
+                                    // property names here.
+    ExpressionClassifier property_classifier(this);
+    ClassLiteralPropertyT property = ParseClassPropertyDefinition(
+        &checker, has_extends, &is_computed_name,
+        &class_info.has_seen_constructor, CHECK_OK);
+    impl()->RewriteNonPattern(CHECK_OK);
+    impl()->AccumulateFormalParameterContainmentErrors();
+
+    impl()->DeclareClassProperty(name, property, &class_info, CHECK_OK);
+    impl()->Infer();
+  }
+
+  Expect(Token::RBRACE, CHECK_OK);
+  return impl()->RewriteClassLiteral(name, &class_info, pos, ok);
+}
+
+template <typename Impl>
 typename ParserBase<Impl>::ExpressionT ParserBase<Impl>::ParseTemplateLiteral(
     ExpressionT tag, int start, bool* ok) {
   // A TemplateLiteral is made up of 0 or more TEMPLATE_SPAN tokens (literal
@@ -4313,7 +4465,7 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseStatementListItem(
       return ParseHoistableDeclaration(nullptr, false, ok);
     case Token::CLASS:
       Consume(Token::CLASS);
-      return impl()->ParseClassDeclaration(nullptr, false, ok);
+      return ParseClassDeclaration(nullptr, false, ok);
     case Token::VAR:
     case Token::CONST:
       return ParseVariableStatement(kStatementListItem, nullptr, ok);
@@ -4591,7 +4743,7 @@ ParserBase<Impl>::ParseExpressionOrLabelledStatement(
   if (extension_ != nullptr && peek() == Token::FUNCTION &&
       !scanner()->HasAnyLineTerminatorBeforeNext() && impl()->IsNative(expr) &&
       !scanner()->literal_contains_escapes()) {
-    return impl()->ParseNativeDeclaration(ok);
+    return ParseNativeDeclaration(ok);
   }
 
   // Parsed expression statement, followed by semicolon.