Split the AST representation of class properties from object properties

src/parsing/parser-base.h

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef V8_PARSING_PARSER_BASE_H
 #define V8_PARSING_PARSER_BASE_H
 
 #include "src/ast/scopes.h"
 #include "src/bailout-reason.h"
 #include "src/base/hashmap.h"
@@ skipping 165 matching lines @@
 //   typedef Base;
 //   typedef Impl;
 //   // TODO(nikolaos): this one will probably go away, as it is
 //   // not related to pure parsing.
 //   typedef GeneratorVariable;
 //   // Return types for traversing functions.
 //   typedef Identifier;
 //   typedef Expression;
 //   typedef FunctionLiteral;
 //   typedef ObjectLiteralProperty;
+//   typedef ClassLiteralProperty;
 //   typedef ExpressionList;
 //   typedef PropertyList;
 //   typedef FormalParameters;
 //   typedef StatementList;
 //   typedef Block;
 //   // For constructing objects returned by the traversing functions.
 //   typedef Factory;
 // };
 
 template <typename Impl>
 struct ParserTypes;
 
 template <typename Impl>
 class ParserBase {
  public:
   // Shorten type names defined by ParserTypes<Impl>.
   typedef ParserTypes<Impl> Types;
   typedef typename Types::Identifier IdentifierT;
   typedef typename Types::Expression ExpressionT;
   typedef typename Types::FunctionLiteral FunctionLiteralT;
   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::StatementList StatementListT;
   typedef typename Types::Block BlockT;
   typedef typename v8::internal::ExpressionClassifier<Types>
       ExpressionClassifier;
 
   // All implementation-specific methods must be called through this.
   Impl* impl() { return static_cast<Impl*>(this); }
@@ skipping 70 matching lines @@
   };
 
   enum VariableDeclarationContext {
     kStatementListItem,
     kStatement,
     kForStatement
   };
   // clang-format on
 
   class Checkpoint;
-  class ObjectLiteralCheckerBase;
+  class ClassLiteralChecker;
+  class ObjectLiteralChecker;
 
   // ---------------------------------------------------------------------------
   // ScopeState and its subclasses implement the parser's scope stack.
   // ScopeState keeps track of the current scope, and the outer ScopeState. The
   // parser's scope_state_ points to the top ScopeState. ScopeState's
   // constructor push on the scope stack and the destructors pop. BlockState and
   // FunctionState are used to hold additional per-block and per-function state.
   class ScopeState BASE_EMBEDDED {
    public:
     V8_INLINE Scope* scope() const { return scope_; }
@@ skipping 817 matching lines @@
   enum class PropertyKind {
     kAccessorProperty,
     kValueProperty,
     kShorthandProperty,
     kMethodProperty,
     kNotSet
   };
 
   bool SetPropertyKindFromToken(Token::Value token, PropertyKind* kind);
   ExpressionT ParsePropertyName(IdentifierT* name, PropertyKind* kind,
-                                bool in_class, bool* is_generator, bool* is_get,
-                                bool* is_set, bool* is_async, bool* is_static,
-                                bool* is_computed_name, bool* ok);
-
+                                bool* is_generator, bool* is_get, bool* is_set,
+                                bool* is_async, bool* is_computed_name,
+                                bool* ok);
   ExpressionT ParseObjectLiteral(bool* ok);
-  ObjectLiteralPropertyT ParsePropertyDefinition(
-      ObjectLiteralCheckerBase* checker, bool in_class, bool has_extends,
-      bool* is_computed_name, bool* has_seen_constructor, IdentifierT* name,
+  ClassLiteralPropertyT ParseClassPropertyDefinition(
+      ClassLiteralChecker* checker, bool has_extends, bool* is_computed_name,
+      bool* has_seen_constructor, IdentifierT* name, bool* ok);
+  ObjectLiteralPropertyT ParseObjectPropertyDefinition(
+      ObjectLiteralChecker* checker, bool* is_computed_name, IdentifierT* name,
       bool* ok);
   ExpressionListT ParseArguments(Scanner::Location* first_spread_pos,
                                  bool maybe_arrow, bool* ok);
   ExpressionListT ParseArguments(Scanner::Location* first_spread_pos,
                                  bool* ok) {
     return ParseArguments(first_spread_pos, false, ok);
   }
 
   ExpressionT ParseAssignmentExpression(bool accept_IN, bool* ok);
   ExpressionT ParseYieldExpression(bool accept_IN, bool* ok);
@@ skipping 65 matching lines @@
       if (is_sloppy(scope->language_mode())) {
         // For sloppy scopes we also have to record the call at function level,
         // in case it includes declarations that will be hoisted.
         scope->GetDeclarationScope()->RecordEvalCall();
       }
       return Call::IS_POSSIBLY_EVAL;
     }
     return Call::NOT_EVAL;
   }
 
-  class ObjectLiteralCheckerBase {
-   public:
-    explicit ObjectLiteralCheckerBase(ParserBase* parser) : parser_(parser) {}
-
-    virtual void CheckProperty(Token::Value property, PropertyKind type,
-                               MethodKind method_type, bool* ok) = 0;
-
-    virtual ~ObjectLiteralCheckerBase() {}
-
-   protected:
-    ParserBase* parser() const { return parser_; }
-    Scanner* scanner() const { return parser_->scanner(); }
-
-   private:
-    ParserBase* parser_;
-  };
-
   // Validation per ES6 object literals.
-  class ObjectLiteralChecker : public ObjectLiteralCheckerBase {
+  class ObjectLiteralChecker {
    public:
     explicit ObjectLiteralChecker(ParserBase* parser)
-        : ObjectLiteralCheckerBase(parser), has_seen_proto_(false) {}
+        : parser_(parser), has_seen_proto_(false) {}
 
-    void CheckProperty(Token::Value property, PropertyKind type,
-                       MethodKind method_type, bool* ok) override;
+    void CheckDuplicateProto(Token::Value property);
 
    private:
     bool IsProto() { return this->scanner()->LiteralMatches("__proto__", 9); }
 
+    ParserBase* parser() const { return parser_; }
+    Scanner* scanner() const { return parser_->scanner(); }
+
+    ParserBase* parser_;
     bool has_seen_proto_;
   };
 
   // Validation per ES6 class literals.
-  class ClassLiteralChecker : public ObjectLiteralCheckerBase {
+  class ClassLiteralChecker {
    public:
     explicit ClassLiteralChecker(ParserBase* parser)
-        : ObjectLiteralCheckerBase(parser), has_seen_constructor_(false) {}
+        : parser_(parser), has_seen_constructor_(false) {}
 
-    void CheckProperty(Token::Value property, PropertyKind type,
-                       MethodKind method_type, bool* ok) override;
+    void CheckClassMethodName(Token::Value property, PropertyKind type,
+                              MethodKind method_type, bool* ok);
 
    private:
     bool IsConstructor() {
       return this->scanner()->LiteralMatches("constructor", 11);
     }
     bool IsPrototype() {
       return this->scanner()->LiteralMatches("prototype", 9);
     }
 
+    ParserBase* parser() const { return parser_; }
+    Scanner* scanner() const { return parser_->scanner(); }
+
+    ParserBase* parser_;
     bool has_seen_constructor_;
   };
 
   ModuleDescriptor* module() const {
     return scope()->AsModuleScope()->module();
   }
   Scope* scope() const { return scope_state_->scope(); }
 
   // Stack of expression classifiers.
   // The top of the stack is always pointed to by classifier().
@@ skipping 622 matching lines @@
       *kind = PropertyKind::kMethodProperty;
       return true;
     default:
       break;
   }
   return false;
 }
 
 template <class Impl>
 typename ParserBase<Impl>::ExpressionT ParserBase<Impl>::ParsePropertyName(
-    IdentifierT* name, PropertyKind* kind, bool in_class, bool* is_generator,
-    bool* is_get, bool* is_set, bool* is_async, bool* is_static,
-    bool* is_computed_name, bool* ok) {
-  DCHECK(*kind == PropertyKind::kNotSet);
+    IdentifierT* name, PropertyKind* kind, bool* is_generator, bool* is_get,
+    bool* is_set, bool* is_async, bool* is_computed_name, bool* ok) {
+  DCHECK(*kind == PropertyKind::kNotSet ||
+         *kind == PropertyKind::kMethodProperty);  // The second case occurs
+                                                   // when parsing static fields
+                                                   // of classes.
   DCHECK(!*is_generator);
   DCHECK(!*is_get);
   DCHECK(!*is_set);
   DCHECK(!*is_async);
-  DCHECK(!*is_static);
   DCHECK(!*is_computed_name);
 
   *is_generator = Check(Token::MUL);
   if (*is_generator) {
     *kind = PropertyKind::kMethodProperty;
   }
 
   Token::Value token = peek();
   int pos = peek_position();
 
-  if (in_class && !*is_generator && token == Token::STATIC) {
-    Consume(Token::STATIC);
-    *kind = PropertyKind::kMethodProperty;
-    if (peek() == Token::LPAREN) {
-      *name = impl()->GetSymbol();  // TODO(bakkot) specialize on 'static'
-      return factory()->NewStringLiteral(*name, pos);
-    }
-    *is_generator = Check(Token::MUL);
-    *is_static = true;
-    token = peek();
-    pos = peek_position();
-  }
-
   if (allow_harmony_async_await() && !*is_generator && token == Token::ASYNC &&
       !scanner()->HasAnyLineTerminatorAfterNext()) {
     Consume(Token::ASYNC);
     token = peek();
     if (SetPropertyKindFromToken(token, kind)) {
       *name = impl()->GetSymbol();  // TODO(bakkot) specialize on 'async'
       if (fni_ != nullptr) {
         impl()->PushLiteralName(fni_, *name);
       }
       return factory()->NewStringLiteral(*name, pos);
     }
     *kind = PropertyKind::kMethodProperty;
     *is_async = true;
     pos = peek_position();
   }
 
   if (token == Token::IDENTIFIER && !*is_generator && !*is_async) {
-    // This is checking for 'get' and 'set' in particular. Any other identifier
-    // must be the property name, and so SetPropertyKindFromToken will return
-    // true (unless there's a syntax error).
+    // This is checking for 'get' and 'set' in particular.
     Consume(Token::IDENTIFIER);
     token = peek();
-    if (SetPropertyKindFromToken(token, kind)) {
+    if (SetPropertyKindFromToken(token, kind) ||
+        !scanner()->IsGetOrSet(is_get, is_set)) {
       *name = impl()->GetSymbol();
       if (fni_ != nullptr) {
         impl()->PushLiteralName(fni_, *name);
       }
       return factory()->NewStringLiteral(*name, pos);
     }
-    scanner()->IsGetOrSet(is_get, is_set);
-    if (!*is_get && !*is_set) {  // TODO(bakkot) have IsGetOrSet return a bool
-      Token::Value next = Next();
-      ReportUnexpectedToken(next);
-      *ok = false;
-      return impl()->EmptyExpression();
-    }
     *kind = PropertyKind::kAccessorProperty;
     pos = peek_position();
   }
 
   // For non computed property names we normalize the name a bit:
   //
   //   "12" -> 12
   //   12.3 -> "12.3"
   //   12.30 -> "12.3"
   //   identifier -> "identifier"
@@ skipping 28 matching lines @@
       Expect(Token::RBRACK, CHECK_OK);
       break;
     }
 
     default:
       *name = ParseIdentifierName(CHECK_OK);
       break;
   }
 
   if (*kind == PropertyKind::kNotSet) {
-    if (!SetPropertyKindFromToken(peek(), kind) ||
-        (*kind == PropertyKind::kShorthandProperty &&
-         !Token::IsIdentifier(token, language_mode(), this->is_generator(),
-                              parsing_module_ || is_async_function()))) {
-      Token::Value next = Next();
-      ReportUnexpectedToken(next);
-      *ok = false;
-      return impl()->EmptyExpression();
-    }
+    SetPropertyKindFromToken(peek(), kind);
   }
-  DCHECK(*kind != PropertyKind::kNotSet);
 
   if (*is_computed_name) {
     return expression;
   }
 
   if (fni_ != nullptr) {
     impl()->PushLiteralName(fni_, *name);
   }
 
   uint32_t index;
   return impl()->IsArrayIndex(*name, &index)
              ? factory()->NewNumberLiteral(index, pos)
              : factory()->NewStringLiteral(*name, pos);
 }
 
 template <typename Impl>
-typename ParserBase<Impl>::ObjectLiteralPropertyT
-ParserBase<Impl>::ParsePropertyDefinition(ObjectLiteralCheckerBase* checker,
-                                          bool in_class, bool has_extends,
-                                          bool* is_computed_name,
-                                          bool* has_seen_constructor,
-                                          IdentifierT* name, bool* ok) {
-  DCHECK(!in_class || has_seen_constructor != nullptr);
+typename ParserBase<Impl>::ClassLiteralPropertyT
+ParserBase<Impl>::ParseClassPropertyDefinition(ClassLiteralChecker* checker,
+                                               bool has_extends,
+                                               bool* is_computed_name,
+                                               bool* has_seen_constructor,
+                                               IdentifierT* name, bool* ok) {
+  DCHECK(has_seen_constructor != nullptr);
   bool is_get = false;
   bool is_set = false;
   bool is_generator = false;
   bool is_async = false;
   bool is_static = false;
   PropertyKind kind = PropertyKind::kNotSet;
 
   Token::Value name_token = peek();
+
+  ExpressionT name_expression;
+  if (name_token == Token::STATIC) {
+    Consume(Token::STATIC);
+    kind = PropertyKind::kMethodProperty;

[adamk, 2016/09/01 21:18:23]

If you move this into the LPAREN branch, then you don't need the weird || or
comment in the kNotSet DCHECK in ParsePropertyName.

[bakkot, 2016/09/01 23:24:31]

Done. This was historical, not really intentional. It had the effect that
ParsePropertyName would necessarily set kind to either Method or Accessor, so
that if the name was followed by a colon it would fail in the Method branch
below. With this change, 'static x :' will be interpreted as a ValueProperty by
ParsePropertyName (since it will only see the 'x :' part), and then the error
will be thrown by hitting the ValueProperty branch.

+    if (peek() == Token::LPAREN) {
+      *name = impl()->GetSymbol();  // TODO(bakkot) specialize on 'static'
+      name_expression = factory()->NewStringLiteral(*name, position());
+    } else {
+      is_static = true;
+      name_expression = ParsePropertyName(
+          name, &kind, &is_generator, &is_get, &is_set, &is_async,
+          is_computed_name, CHECK_OK_CUSTOM(EmptyClassLiteralProperty));
+    }
+  } else {
+    name_expression = ParsePropertyName(
+        name, &kind, &is_generator, &is_get, &is_set, &is_async,
+        is_computed_name, CHECK_OK_CUSTOM(EmptyClassLiteralProperty));
+  }
+
+  switch (kind) {
+    case PropertyKind::kShorthandProperty:
+    case PropertyKind::kValueProperty: {
+      Token::Value next = Next();

[adamk, 2016/09/01 21:18:23]

If you inline the Next() call into the call to ReportUnexpectedToken, you don't
need the curlies around this case block (also saves a line). Similar comment for
other calls to ReportUnexpectedToken().

[bakkot, 2016/09/01 23:24:31]

Done.

+      ReportUnexpectedToken(next);
+      *ok = false;
+      return impl()->EmptyClassLiteralProperty();
+    }
+
+    case PropertyKind::kMethodProperty: {
+      DCHECK(!is_get && !is_set);
+
+      MethodKind method_kind = MethodKind::kNormal;
+      if (is_generator) {
+        method_kind |= MethodKind::kGenerator;

[adamk, 2016/09/01 21:18:23]

I'm not sure MethodKind is buying us a whole lot of readability. The only
callers of CheckClassMethodName already have bools available called
is_generator, is_async, and is_static, and already have to pass a bunch of bools
in the correct order to ParsePropertyName. And then we turn them back into bools
inside!

What would you think of just having CheckClassMethodName take bools, and getting
rid of all these bitwise operations?

[bakkot, 2016/09/01 23:24:31]

Done.

Yeah, that's much better. MethodKind requires much more code than it's worth.
It's been getting progressively less useful with these patches.

+      }
+      if (is_async) {
+        method_kind |= MethodKind::kAsync;
+      }
+      if (is_static) {
+        method_kind |= MethodKind::kStatic;
+      }
+
+      // MethodDefinition
+      //    PropertyName '(' StrictFormalParameters ')' '{' FunctionBody '}'
+      //    '*' PropertyName '(' StrictFormalParameters ')' '{' FunctionBody '}'
+
+      if (!*is_computed_name) {
+        checker->CheckClassMethodName(
+            name_token, PropertyKind::kMethodProperty, method_kind,
+            CHECK_OK_CUSTOM(EmptyClassLiteralProperty));
+      }
+
+      FunctionKind kind = is_generator
+                              ? FunctionKind::kConciseGeneratorMethod
+                              : is_async ? FunctionKind::kAsyncConciseMethod
+                                         : FunctionKind::kConciseMethod;
+
+      if (!is_static && impl()->IsConstructor(*name)) {
+        *has_seen_constructor = true;
+        kind = has_extends ? FunctionKind::kSubclassConstructor
+                           : FunctionKind::kBaseConstructor;
+      }
+
+      ExpressionT value = impl()->ParseFunctionLiteral(
+          *name, scanner()->location(), kSkipFunctionNameCheck, kind,
+          kNoSourcePosition, FunctionLiteral::kAccessorOrMethod,
+          language_mode(), CHECK_OK_CUSTOM(EmptyClassLiteralProperty));
+
+      return factory()->NewClassLiteralProperty(name_expression, value,
+                                                ClassLiteralProperty::METHOD,
+                                                is_static, *is_computed_name);
+    }
+
+    case PropertyKind::kAccessorProperty: {
+      DCHECK((is_get || is_set) && !is_generator && !is_async);
+
+      MethodKind method_kind = MethodKind::kNormal;
+      if (is_static) {
+        method_kind |= MethodKind::kStatic;
+      }
+
+      if (!*is_computed_name) {
+        checker->CheckClassMethodName(
+            name_token, PropertyKind::kAccessorProperty, method_kind,
+            CHECK_OK_CUSTOM(EmptyClassLiteralProperty));
+        // Make sure the name expression is a string since we need a Name for
+        // Runtime_DefineAccessorPropertyUnchecked and since we can determine
+        // this statically we can skip the extra runtime check.
+        name_expression =
+            factory()->NewStringLiteral(*name, name_expression->position());
+      }
+
+      FunctionKind kind = is_get ? FunctionKind::kGetterFunction
+                                 : FunctionKind::kSetterFunction;
+
+      ExpressionT value = impl()->ParseFunctionLiteral(
+          *name, scanner()->location(), kSkipFunctionNameCheck, kind,
+          kNoSourcePosition, FunctionLiteral::kAccessorOrMethod,
+          language_mode(), CHECK_OK_CUSTOM(EmptyClassLiteralProperty));
+
+      return factory()->NewClassLiteralProperty(
+          name_expression, value,
+          is_get ? ClassLiteralProperty::GETTER : ClassLiteralProperty::SETTER,
+          is_static, *is_computed_name);
+    }
+
+    case PropertyKind::kNotSet: {
+      Token::Value next = Next();
+      ReportUnexpectedToken(next);
+      *ok = false;
+      return impl()->EmptyClassLiteralProperty();
+    }
+  }
+  UNREACHABLE();
+  return impl()->EmptyClassLiteralProperty();
+}
+
+template <typename Impl>
+typename ParserBase<Impl>::ObjectLiteralPropertyT
+ParserBase<Impl>::ParseObjectPropertyDefinition(ObjectLiteralChecker* checker,
+                                                bool* is_computed_name,
+                                                IdentifierT* name, bool* ok) {
+  bool is_get = false;
+  bool is_set = false;
+  bool is_generator = false;
+  bool is_async = false;
+  PropertyKind kind = PropertyKind::kNotSet;
+
+  Token::Value name_token = peek();
   int next_beg_pos = scanner()->peek_location().beg_pos;
   int next_end_pos = scanner()->peek_location().end_pos;
 
-  ExpressionT name_expression =
-      ParsePropertyName(name, &kind, in_class, &is_generator, &is_get, &is_set,
-                        &is_async, &is_static, is_computed_name,
-                        CHECK_OK_CUSTOM(EmptyObjectLiteralProperty));
+  ExpressionT name_expression = ParsePropertyName(
+      name, &kind, &is_generator, &is_get, &is_set, &is_async, is_computed_name,
+      CHECK_OK_CUSTOM(EmptyObjectLiteralProperty));
 
   switch (kind) {
     case PropertyKind::kValueProperty: {
-      if (in_class) {
-        Token::Value next = Next();
-        ReportUnexpectedToken(next);
-        *ok = false;
-        return impl()->EmptyObjectLiteralProperty();
-      }
-
-      DCHECK(!is_get && !is_set && !is_generator && !is_async && !is_static);
+      DCHECK(!is_get && !is_set && !is_generator && !is_async);
 
       if (!*is_computed_name) {
-        checker->CheckProperty(name_token, PropertyKind::kValueProperty,
-                               MethodKind::kNormal,
-                               CHECK_OK_CUSTOM(EmptyObjectLiteralProperty));
+        checker->CheckDuplicateProto(name_token);
       }
       Consume(Token::COLON);
       int beg_pos = peek_position();
       ExpressionT value = ParseAssignmentExpression(
           true, CHECK_OK_CUSTOM(EmptyObjectLiteralProperty));
       CheckDestructuringElement(value, beg_pos, scanner()->location().end_pos);
 
       return factory()->NewObjectLiteralProperty(name_expression, value,
-                                                 is_static, *is_computed_name);
+                                                 *is_computed_name);
     }
 
     case PropertyKind::kShorthandProperty: {
       // PropertyDefinition
       //    IdentifierReference
       //    CoverInitializedName
       //
       // CoverInitializedName
       //    IdentifierReference Initializer?
-      if (in_class) {
+      DCHECK(!is_get && !is_set && !is_generator && !is_async);
+
+      if (!Token::IsIdentifier(name_token, language_mode(),
+                               this->is_generator(),
+                               parsing_module_ || is_async_function())) {
         Token::Value next = Next();
         ReportUnexpectedToken(next);
         *ok = false;
         return impl()->EmptyObjectLiteralProperty();
       }
 
-      DCHECK(!is_get && !is_set && !is_generator && !is_async && !is_static &&
-             !*is_computed_name);
+      DCHECK(!*is_computed_name);
 
       if (classifier()->duplicate_finder() != nullptr &&
           scanner()->FindSymbol(classifier()->duplicate_finder(), 1) != 0) {
         classifier()->RecordDuplicateFormalParameterError(
             scanner()->location());
       }
 
       if (impl()->IsEvalOrArguments(*name) && is_strict(language_mode())) {
         classifier()->RecordBindingPatternError(
             scanner()->location(), MessageTemplate::kStrictEvalArguments);
@@ skipping 26 matching lines @@
         classifier()->RecordExpressionError(
             Scanner::Location(next_beg_pos, scanner()->location().end_pos),
             MessageTemplate::kInvalidCoverInitializedName);
 
         impl()->SetFunctionNameFromIdentifierRef(rhs, lhs);
       } else {
         value = lhs;
       }
 
       return factory()->NewObjectLiteralProperty(
-          name_expression, value, ObjectLiteralProperty::COMPUTED, is_static,
-          false);
+          name_expression, value, ObjectLiteralProperty::COMPUTED, false);
     }
 
     case PropertyKind::kMethodProperty: {
       DCHECK(!is_get && !is_set);
 
-      MethodKind method_kind = MethodKind::kNormal;
-      if (is_generator) {
-        method_kind |= MethodKind::kGenerator;
-      }
-      if (is_async) {
-        method_kind |= MethodKind::kAsync;
-      }
-      if (is_static) {
-        method_kind |= MethodKind::kStatic;
-      }
-
       // MethodDefinition
       //    PropertyName '(' StrictFormalParameters ')' '{' FunctionBody '}'
       //    '*' PropertyName '(' StrictFormalParameters ')' '{' FunctionBody '}'
 
       classifier()->RecordPatternError(
           Scanner::Location(next_beg_pos, scanner()->location().end_pos),
           MessageTemplate::kInvalidDestructuringTarget);
 
-      if (!*is_computed_name) {
-        checker->CheckProperty(name_token, PropertyKind::kMethodProperty,
-                               method_kind,
-                               CHECK_OK_CUSTOM(EmptyObjectLiteralProperty));
-      }
-
       FunctionKind kind = is_generator
                               ? FunctionKind::kConciseGeneratorMethod
                               : is_async ? FunctionKind::kAsyncConciseMethod
                                          : FunctionKind::kConciseMethod;
 
-      if (in_class && !is_static && impl()->IsConstructor(*name)) {
-        *has_seen_constructor = true;
-        kind = has_extends ? FunctionKind::kSubclassConstructor
-                           : FunctionKind::kBaseConstructor;
-      }
-
       ExpressionT value = impl()->ParseFunctionLiteral(
           *name, scanner()->location(), kSkipFunctionNameCheck, kind,
           kNoSourcePosition, FunctionLiteral::kAccessorOrMethod,
           language_mode(), CHECK_OK_CUSTOM(EmptyObjectLiteralProperty));
 
       return factory()->NewObjectLiteralProperty(
-          name_expression, value, ObjectLiteralProperty::COMPUTED, is_static,
+          name_expression, value, ObjectLiteralProperty::COMPUTED,
           *is_computed_name);
     }
 
     case PropertyKind::kAccessorProperty: {
       DCHECK((is_get || is_set) && !is_generator && !is_async);
 
-      MethodKind method_kind = MethodKind::kNormal;
-      if (is_static) {
-        DCHECK(in_class);
-        method_kind |= MethodKind::kStatic;
-      }
-
       classifier()->RecordPatternError(
           Scanner::Location(next_beg_pos, scanner()->location().end_pos),
           MessageTemplate::kInvalidDestructuringTarget);
+
       if (!*is_computed_name) {
-        checker->CheckProperty(name_token, PropertyKind::kAccessorProperty,
-                               method_kind,
-                               CHECK_OK_CUSTOM(EmptyObjectLiteralProperty));
-      }
-
-      FunctionLiteralT value = impl()->ParseFunctionLiteral(
-          *name, scanner()->location(), kSkipFunctionNameCheck,
-          is_get ? FunctionKind::kGetterFunction
-                 : FunctionKind::kSetterFunction,
-          kNoSourcePosition, FunctionLiteral::kAccessorOrMethod,
-          language_mode(), CHECK_OK_CUSTOM(EmptyObjectLiteralProperty));
-
-      // Make sure the name expression is a string since we need a Name for
-      // Runtime_DefineAccessorPropertyUnchecked and since we can determine this
-      // statically we can skip the extra runtime check.
-      if (!*is_computed_name) {
+        // Make sure the name expression is a string since we need a Name for
+        // Runtime_DefineAccessorPropertyUnchecked and since we can determine
+        // this statically we can skip the extra runtime check.
         name_expression =
             factory()->NewStringLiteral(*name, name_expression->position());
       }
 
+      FunctionKind kind = is_get ? FunctionKind::kGetterFunction
+                                 : FunctionKind::kSetterFunction;
+
+      ExpressionT value = impl()->ParseFunctionLiteral(
+          *name, scanner()->location(), kSkipFunctionNameCheck, kind,
+          kNoSourcePosition, FunctionLiteral::kAccessorOrMethod,
+          language_mode(), CHECK_OK_CUSTOM(EmptyObjectLiteralProperty));
+
       return factory()->NewObjectLiteralProperty(
           name_expression, value, is_get ? ObjectLiteralProperty::GETTER
                                          : ObjectLiteralProperty::SETTER,
-          is_static, *is_computed_name);
+          *is_computed_name);
     }
 
-    case PropertyKind::kNotSet:
-      UNREACHABLE();
+    case PropertyKind::kNotSet: {

[adamk, 2016/09/01 21:18:23]

Can you explain why it's better to have duplication of this error-reporting
rather than handling it in ParsePropertyName? I suspect I'm missing something.

[bakkot, 2016/09/01 23:24:31]

Discussed offline. The real reason is that it makes adding class fields later
easier. The ideological reason is that ParsePropertyName shouldn't really be
throwing errors for unexpected tokens past the end of the property name; that's
the responsibility of whatever wants to consume that token.

+      Token::Value next = Next();
+      ReportUnexpectedToken(next);
+      *ok = false;
+      return impl()->EmptyObjectLiteralProperty();
+    }
   }
   UNREACHABLE();
   return impl()->EmptyObjectLiteralProperty();
 }
 
 template <typename Impl>
 typename ParserBase<Impl>::ExpressionT ParserBase<Impl>::ParseObjectLiteral(
     bool* ok) {
   // ObjectLiteral ::
   // '{' (PropertyDefinition (',' PropertyDefinition)* ','? )? '}'
 
   int pos = peek_position();
   PropertyListT properties = impl()->NewPropertyList(4);
   int number_of_boilerplate_properties = 0;
   bool has_computed_names = false;
   ObjectLiteralChecker checker(this);
 
   Expect(Token::LBRACE, CHECK_OK);
 
   while (peek() != Token::RBRACE) {
     FuncNameInferrer::State fni_state(fni_);
 
-    const bool in_class = false;
-    const bool has_extends = false;
     bool is_computed_name = false;
     IdentifierT name = impl()->EmptyIdentifier();
-    ObjectLiteralPropertyT property =
-        ParsePropertyDefinition(&checker, in_class, has_extends,
-                                &is_computed_name, nullptr, &name, CHECK_OK);
+    ObjectLiteralPropertyT property = ParseObjectPropertyDefinition(
+        &checker, &is_computed_name, &name, CHECK_OK);
 
     if (is_computed_name) {
       has_computed_names = true;
     }
 
     // Count CONSTANT or COMPUTED properties to maintain the enumeration order.
     if (!has_computed_names && impl()->IsBoilerplateProperty(property)) {
       number_of_boilerplate_properties++;
     }
     properties->Add(property, zone());
@@ skipping 1604 matching lines @@
     classifier()->RecordAssignmentPatternError(
         Scanner::Location(begin, end),
         MessageTemplate::kInvalidDestructuringTarget);
   }
 }
 
 #undef CHECK_OK
 #undef CHECK_OK_CUSTOM
 
 template <typename Impl>
-void ParserBase<Impl>::ObjectLiteralChecker::CheckProperty(
-    Token::Value property, PropertyKind type, MethodKind method_type,
-    bool* ok) {
-  DCHECK(!IsStaticMethod(method_type));
-  DCHECK(!IsSpecialMethod(method_type) ||
-         type == PropertyKind::kMethodProperty);
-
+void ParserBase<Impl>::ObjectLiteralChecker::CheckDuplicateProto(
+    Token::Value property) {
   if (property == Token::SMI || property == Token::NUMBER) return;
 
-  if (type == PropertyKind::kValueProperty && IsProto()) {
+  if (IsProto()) {
     if (has_seen_proto_) {
       this->parser()->classifier()->RecordExpressionError(
           this->scanner()->location(), MessageTemplate::kDuplicateProto);
       return;
     }
     has_seen_proto_ = true;
   }
 }
 
 template <typename Impl>
-void ParserBase<Impl>::ClassLiteralChecker::CheckProperty(
+void ParserBase<Impl>::ClassLiteralChecker::CheckClassMethodName(
     Token::Value property, PropertyKind type, MethodKind method_type,
     bool* ok) {
   DCHECK(type == PropertyKind::kMethodProperty ||
          type == PropertyKind::kAccessorProperty);
 
   if (property == Token::SMI || property == Token::NUMBER) return;
 
   if (IsStaticMethod(method_type)) {
     if (IsPrototype()) {
       this->parser()->ReportMessage(MessageTemplate::kStaticPrototype);
@@ skipping 20 matching lines @@
     has_seen_constructor_ = true;
     return;
   }
 }
 
 
 }  // namespace internal
 }  // namespace v8
 
 #endif  // V8_PARSING_PARSER_BASE_H