(Pre)Parser: Simplify NewExpression handling (fixed).

Notes:
- We use simple recursion to keep track of how many "new" operators we have seen
  and where.
- This makes the self-baked stack class PositionStack in parser.cc unnecessary.
- Now the logic is also unified between Parser and PreParser.
- This is a fixed version of r19386.

R=ulan@chromium.org
BUG=v8:3126
LOG=N

Review URL: https://codereview.chromium.org/168583008

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@19417 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/parser.cc

@@ -46,49 +46,6 @@
 namespace v8 {
 namespace internal {
 
-// PositionStack is used for on-stack allocation of token positions for
-// new expressions. Please look at ParseNewExpression.
-
-class PositionStack  {
- public:
-  explicit PositionStack(bool* ok) : top_(NULL), ok_(ok) {}
-  ~PositionStack() {
-    ASSERT(!*ok_ || is_empty());
-    USE(ok_);
-  }
-
-  class Element  {
-   public:
-    Element(PositionStack* stack, int value) {
-      previous_ = stack->top();
-      value_ = value;
-      stack->set_top(this);
-    }
-
-   private:
-    Element* previous() { return previous_; }
-    int value() { return value_; }
-    friend class PositionStack;
-    Element* previous_;
-    int value_;
-  };
-
-  bool is_empty() { return top_ == NULL; }
-  int pop() {
-    ASSERT(!is_empty());
-    int result = top_->value();
-    top_ = top_->previous();
-    return result;
-  }
-
- private:
-  Element* top() { return top_; }
-  void set_top(Element* value) { top_ = value; }
-  Element* top_;
-  bool* ok_;
-};
-
-
 RegExpBuilder::RegExpBuilder(Zone* zone)
     : zone_(zone),
       pending_empty_(false),
@@ -3292,12 +3249,7 @@ Expression* Parser::ParseLeftHandSideExpression(bool* ok) {
   // LeftHandSideExpression ::
   //   (NewExpression | MemberExpression) ...
 
-  Expression* result;
-  if (peek() == Token::NEW) {
-    result = ParseNewExpression(CHECK_OK);
-  } else {
-    result = ParseMemberExpression(CHECK_OK);
-  }
+  Expression* result = ParseMemberWithNewPrefixesExpression(CHECK_OK);
 
   while (true) {
     switch (peek()) {
@@ -3366,54 +3318,54 @@ Expression* Parser::ParseLeftHandSideExpression(bool* ok) {
 }
 
 
-Expression* Parser::ParseNewPrefix(PositionStack* stack, bool* ok) {
+Expression* Parser::ParseMemberWithNewPrefixesExpression(bool* ok) {
   // NewExpression ::
   //   ('new')+ MemberExpression
 
-  // The grammar for new expressions is pretty warped. The keyword
-  // 'new' can either be a part of the new expression (where it isn't
-  // followed by an argument list) or a part of the member expression,
-  // where it must be followed by an argument list. To accommodate
-  // this, we parse the 'new' keywords greedily and keep track of how
-  // many we have parsed. This information is then passed on to the
-  // member expression parser, which is only allowed to match argument
-  // lists as long as it has 'new' prefixes left
-  Expect(Token::NEW, CHECK_OK);
-  PositionStack::Element pos(stack, position());
-
-  Expression* result;
-  if (peek() == Token::NEW) {
-    result = ParseNewPrefix(stack, CHECK_OK);
-  } else {
-    result = ParseMemberWithNewPrefixesExpression(stack, CHECK_OK);
-  }
-
-  if (!stack->is_empty()) {
-    int last = stack->pop();
-    result = factory()->NewCallNew(
-        result, new(zone()) ZoneList<Expression*>(0, zone()), last);
-  }
-  return result;
-}
+  // The grammar for new expressions is pretty warped. We can have several 'new'
+  // keywords following each other, and then a MemberExpression. When we see '('
+  // after the MemberExpression, it's associated with the rightmost unassociated
+  // 'new' to create a NewExpression with arguments. However, a NewExpression
+  // can also occur without arguments.
 
+  // Examples of new expression:
+  // new foo.bar().baz means (new (foo.bar)()).baz
+  // new foo()() means (new foo())()
+  // new new foo()() means (new (new foo())())
+  // new new foo means new (new foo)
+  // new new foo() means new (new foo())
+  // new new foo().bar().baz means (new (new foo()).bar()).baz
 
-Expression* Parser::ParseNewExpression(bool* ok) {
-  PositionStack stack(ok);
-  return ParseNewPrefix(&stack, ok);
+  if (peek() == Token::NEW) {
+    Consume(Token::NEW);
+    int new_pos = position();
+    Expression* result = ParseMemberWithNewPrefixesExpression(CHECK_OK);
+    if (peek() == Token::LPAREN) {
+      // NewExpression with arguments.
+      ZoneList<Expression*>* args = ParseArguments(CHECK_OK);
+      result = factory()->NewCallNew(result, args, new_pos);
+      // The expression can still continue with . or [ after the arguments.
+      result = ParseMemberExpressionContinuation(result, CHECK_OK);
+      return result;
+    }
+    // NewExpression without arguments.
+    return factory()->NewCallNew(
+        result, new(zone()) ZoneList<Expression*>(0, zone()), new_pos);
+  }
+  // No 'new' keyword.
+  return ParseMemberExpression(ok);
 }
 
 
 Expression* Parser::ParseMemberExpression(bool* ok) {
-  return ParseMemberWithNewPrefixesExpression(NULL, ok);
-}
-
-
-Expression* Parser::ParseMemberWithNewPrefixesExpression(PositionStack* stack,
-                                                         bool* ok) {
   // MemberExpression ::
   //   (PrimaryExpression | FunctionLiteral)
   //     ('[' Expression ']' | '.' Identifier | Arguments)*
 
+  // The '[' Expression ']' and '.' Identifier parts are parsed by
+  // ParseMemberExpressionContinuation, and the Arguments part is parsed by the
+  // caller.
+
   // Parse the initial primary or function expression.
   Expression* result = NULL;
   if (peek() == Token::FUNCTION) {
@@ -3442,13 +3394,22 @@ Expression* Parser::ParseMemberWithNewPrefixesExpression(PositionStack* stack,
     result = ParsePrimaryExpression(CHECK_OK);
   }
 
+  result = ParseMemberExpressionContinuation(result, CHECK_OK);
+  return result;
+}
+
+
+Expression* Parser::ParseMemberExpressionContinuation(Expression* expression,
+                                                      bool* ok) {
+  // Parses this part of MemberExpression:
+  // ('[' Expression ']' | '.' Identifier)*
   while (true) {
     switch (peek()) {
       case Token::LBRACK: {
         Consume(Token::LBRACK);
         int pos = position();
         Expression* index = ParseExpression(true, CHECK_OK);
-        result = factory()->NewProperty(result, index, pos);
+        expression = factory()->NewProperty(expression, index, pos);
         if (fni_ != NULL) {
           if (index->IsPropertyName()) {
             fni_->PushLiteralName(index->AsLiteral()->AsPropertyName());
@@ -3464,23 +3425,17 @@ Expression* Parser::ParseMemberWithNewPrefixesExpression(PositionStack* stack,
         Consume(Token::PERIOD);
         int pos = position();
         Handle<String> name = ParseIdentifierName(CHECK_OK);
-        result = factory()->NewProperty(
-            result, factory()->NewLiteral(name, pos), pos);
+        expression = factory()->NewProperty(
+            expression, factory()->NewLiteral(name, pos), pos);
         if (fni_ != NULL) fni_->PushLiteralName(name);
         break;
       }
-      case Token::LPAREN: {
-        if ((stack == NULL) || stack->is_empty()) return result;
-        // Consume one of the new prefixes (already parsed).
-        ZoneList<Expression*>* args = ParseArguments(CHECK_OK);
-        int pos = stack->pop();
-        result = factory()->NewCallNew(result, args, pos);
-        break;
-      }
       default:
-        return result;
+        return expression;
     }
   }
+  ASSERT(false);
+  return NULL;
 }
 
 

src/parser.h

@@ -638,11 +638,10 @@ class Parser : public ParserBase<ParserTraits> {
   Expression* ParseUnaryExpression(bool* ok);
   Expression* ParsePostfixExpression(bool* ok);
   Expression* ParseLeftHandSideExpression(bool* ok);
-  Expression* ParseNewExpression(bool* ok);
+  Expression* ParseMemberWithNewPrefixesExpression(bool* ok);
   Expression* ParseMemberExpression(bool* ok);
-  Expression* ParseNewPrefix(PositionStack* stack, bool* ok);
-  Expression* ParseMemberWithNewPrefixesExpression(PositionStack* stack,
-                                                   bool* ok);
+  Expression* ParseMemberExpressionContinuation(Expression* expression,
+                                                bool* ok);
   Expression* ParseArrayLiteral(bool* ok);
   Expression* ParseObjectLiteral(bool* ok);
 

src/preparser.cc

@@ -1007,12 +1007,7 @@ PreParser::Expression PreParser::ParseLeftHandSideExpression(bool* ok) {
   // LeftHandSideExpression ::
   //   (NewExpression | MemberExpression) ...
 
-  Expression result = Expression::Default();
-  if (peek() == Token::NEW) {
-    result = ParseNewExpression(CHECK_OK);
-  } else {
-    result = ParseMemberExpression(CHECK_OK);
-  }
+  Expression result = ParseMemberWithNewPrefixesExpression(CHECK_OK);
 
   while (true) {
     switch (peek()) {
@@ -1052,39 +1047,38 @@ PreParser::Expression PreParser::ParseLeftHandSideExpression(bool* ok) {
 }
 
 
-PreParser::Expression PreParser::ParseNewExpression(bool* ok) {
+PreParser::Expression PreParser::ParseMemberWithNewPrefixesExpression(
+    bool* ok) {
   // NewExpression ::
   //   ('new')+ MemberExpression
 
-  // The grammar for new expressions is pretty warped. The keyword
-  // 'new' can either be a part of the new expression (where it isn't
-  // followed by an argument list) or a part of the member expression,
-  // where it must be followed by an argument list. To accommodate
-  // this, we parse the 'new' keywords greedily and keep track of how
-  // many we have parsed. This information is then passed on to the
-  // member expression parser, which is only allowed to match argument
-  // lists as long as it has 'new' prefixes left
-  unsigned new_count = 0;
-  do {
-    Consume(Token::NEW);
-    new_count++;
-  } while (peek() == Token::NEW);
+  // See Parser::ParseNewExpression.
 
-  return ParseMemberWithNewPrefixesExpression(new_count, ok);
+  if (peek() == Token::NEW) {
+    Consume(Token::NEW);
+    ParseMemberWithNewPrefixesExpression(CHECK_OK);
+    if (peek() == Token::LPAREN) {
+      // NewExpression with arguments.
+      ParseArguments(CHECK_OK);
+      // The expression can still continue with . or [ after the arguments.
+      ParseMemberExpressionContinuation(Expression::Default(), CHECK_OK);
+    }
+    return Expression::Default();
+  }
+  // No 'new' keyword.
+  return ParseMemberExpression(ok);
 }
 
 
 PreParser::Expression PreParser::ParseMemberExpression(bool* ok) {
-  return ParseMemberWithNewPrefixesExpression(0, ok);
-}
-
-
-PreParser::Expression PreParser::ParseMemberWithNewPrefixesExpression(
-    unsigned new_count, bool* ok) {
   // MemberExpression ::
   //   (PrimaryExpression | FunctionLiteral)
   //     ('[' Expression ']' | '.' Identifier | Arguments)*
 
+  // The '[' Expression ']' and '.' Identifier parts are parsed by
+  // ParseMemberExpressionContinuation, and the Arguments part is parsed by the
+  // caller.
+
   // Parse the initial primary or function expression.
   Expression result = Expression::Default();
   if (peek() == Token::FUNCTION) {
@@ -1107,42 +1101,44 @@ PreParser::Expression PreParser::ParseMemberWithNewPrefixesExpression(
   } else {
     result = ParsePrimaryExpression(CHECK_OK);
   }
+  result = ParseMemberExpressionContinuation(result, CHECK_OK);
+  return result;
+}
+
 
+PreParser::Expression PreParser::ParseMemberExpressionContinuation(
+    PreParserExpression expression, bool* ok) {
+  // Parses this part of MemberExpression:
+  // ('[' Expression ']' | '.' Identifier)*
   while (true) {
     switch (peek()) {
       case Token::LBRACK: {
         Consume(Token::LBRACK);
         ParseExpression(true, CHECK_OK);
         Expect(Token::RBRACK, CHECK_OK);
-        if (result.IsThis()) {
-          result = Expression::ThisProperty();
+        if (expression.IsThis()) {
+          expression = Expression::ThisProperty();
         } else {
-          result = Expression::Default();
+          expression = Expression::Default();
         }
         break;
       }
       case Token::PERIOD: {
         Consume(Token::PERIOD);
         ParseIdentifierName(CHECK_OK);
-        if (result.IsThis()) {
-          result = Expression::ThisProperty();
+        if (expression.IsThis()) {
+          expression = Expression::ThisProperty();
         } else {
-          result = Expression::Default();
+          expression = Expression::Default();
         }
         break;
       }
-      case Token::LPAREN: {
-        if (new_count == 0) return result;
-        // Consume one of the new prefixes (already parsed).
-        ParseArguments(CHECK_OK);
-        new_count--;
-        result = Expression::Default();
-        break;
-      }
       default:
-        return result;
+        return expression;
     }
   }
+  ASSERT(false);
+  return PreParserExpression::Default();
 }
 
 

src/preparser.h

@@ -854,9 +854,10 @@ class PreParser : public ParserBase<PreParserTraits> {
   Expression ParseUnaryExpression(bool* ok);
   Expression ParsePostfixExpression(bool* ok);
   Expression ParseLeftHandSideExpression(bool* ok);
-  Expression ParseNewExpression(bool* ok);
   Expression ParseMemberExpression(bool* ok);
-  Expression ParseMemberWithNewPrefixesExpression(unsigned new_count, bool* ok);
+  Expression ParseMemberExpressionContinuation(PreParserExpression expression,
+                                               bool* ok);
+  Expression ParseMemberWithNewPrefixesExpression(bool* ok);
   Expression ParseArrayLiteral(bool* ok);
   Expression ParseObjectLiteral(bool* ok);
   Expression ParseV8Intrinsic(bool* ok);

test/cctest/test-parsing.cc

@@ -2063,3 +2063,61 @@ TEST(Intrinsics) {
   // or not.
   RunParserSyncTest(context_data, statement_data, kSuccessOrError);
 }
+
+
+TEST(NoErrorsNewExpression) {
+  const char* context_data[][2] = {
+    {"", ""},
+    {"var f =", ""},
+    { NULL, NULL }
+  };
+
+  const char* statement_data[] = {
+    "new foo",
+    "new foo();",
+    "new foo(1);",
+    "new foo(1, 2);",
+    // The first () will be processed as a part of the NewExpression and the
+    // second () will be processed as part of LeftHandSideExpression.
+    "new foo()();",
+    // The first () will be processed as a part of the inner NewExpression and
+    // the second () will be processed as a part of the outer NewExpression.
+    "new new foo()();",
+    "new foo.bar;",
+    "new foo.bar();",
+    "new foo.bar.baz;",
+    "new foo.bar().baz;",
+    "new foo[bar];",
+    "new foo[bar]();",
+    "new foo[bar][baz];",
+    "new foo[bar]()[baz];",
+    "new foo[bar].baz(baz)()[bar].baz;",
+    "new \"foo\"",  // Runtime error
+    "new 1",  // Runtime error
+    "new foo++",
+    // This even runs:
+    "(new new Function(\"this.x = 1\")).x;",
+    "new new Test_Two(String, 2).v(0123).length;",
+    NULL
+  };
+
+  RunParserSyncTest(context_data, statement_data, kSuccess);
+}
+
+
+TEST(ErrorsNewExpression) {
+  const char* context_data[][2] = {
+    {"", ""},
+    {"var f =", ""},
+    { NULL, NULL }
+  };
+
+  const char* statement_data[] = {
+    "new foo bar",
+    "new ) foo",
+    "new ++foo",
+    NULL
+  };
+
+  RunParserSyncTest(context_data, statement_data, kError);
+}