[parser] report errors for invalid binding patterns in async formal parameters

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 879 matching lines @@
   }
 
   bool IsValidArrowFormalParametersStart(Token::Value token) {
     return is_any_identifier(token) || token == Token::LPAREN;
   }
 
   void ValidateArrowFormalParameters(const ExpressionClassifier* classifier,
                                      ExpressionT expr,
                                      bool parenthesized_formals, bool is_async,
                                      bool* ok) {
-    if (classifier->is_valid_binding_pattern()) {
+    if (!is_async && classifier->is_valid_binding_pattern()) {
       // A simple arrow formal parameter: IDENTIFIER => BODY.
       if (!this->IsIdentifier(expr)) {
         Traits::ReportMessageAt(scanner()->location(),
                                 MessageTemplate::kUnexpectedToken,
                                 Token::String(scanner()->current_token()));
         *ok = false;
       }
     } else if (!classifier->is_valid_arrow_formal_parameters()) {
       // If after parsing the expr, we see an error but the expression is
       // neither a valid binding pattern nor a valid parenthesized formal
       // parameter list, show the "arrow formal parameters" error if the formals
       // started with a parenthesis, and the binding pattern error otherwise.
       const typename ExpressionClassifier::Error& error =
           parenthesized_formals ? classifier->arrow_formal_parameters_error()
                                 : classifier->binding_pattern_error();
       ReportClassifierError(error);
       *ok = false;
     }
-    if (is_async && !classifier->is_valid_async_arrow_formal_parameters()) {
-      const typename ExpressionClassifier::Error& error =
-          classifier->async_arrow_formal_parameters_error();
-      ReportClassifierError(error);
-      *ok = false;
+    if (is_async) {
+      if (!classifier->is_valid_async_arrow_formal_parameters()) {
+        const typename ExpressionClassifier::Error& error =
+            classifier->async_arrow_formal_parameters_error();
+        ReportClassifierError(error);
+        *ok = false;
+      }
+      if (!classifier->is_valid_binding_pattern()) {

[nickie, 2016/07/11 13:19:50]

Is it possible that we have both !is_valid_async_arrow_formal_parameters and
!is_valid_binding_pattern?  In that case, we'd be reporting two errors here. 
Maybe switch the "if" to an "else if"?

BTW, the same seems to be the case with the error message in line 916.  Can it
be that we have !is_valid_arrow_formal_parameters, is_async, and at least one of
!is_valid_async_arrow_formal_parameters and !is_valid_binding_pattern?  If yes,
maybe add "*ok &&" to the condition of line 919.

[caitp, 2016/07/11 13:24:42]

Yeah, this is true --- the WebKit style is to just add `return` statements
rather than using `else if` control flow, which I think I prefer. Not sure what
Chromium/Google's styleguide says about this. If people are okay with it, I
prefer to just add return statements where they're missing.

[nickie, 2016/07/11 13:32:18]

Yes, I think that adding return statements is a better option here.

+        ReportClassifierError(classifier->binding_pattern_error());
+        *ok = false;
+      }
     }
   }
 
   void ValidateLetPattern(const ExpressionClassifier* classifier, bool* ok) {
     if (!classifier->is_valid_let_pattern()) {
       ReportClassifierError(classifier->let_pattern_error());
       *ok = false;
     }
   }
 
@@ skipping 83 matching lines @@
       Scanner::Location* first_spread_pos, bool maybe_arrow,
       ExpressionClassifier* classifier, bool* ok);
   typename Traits::Type::ExpressionList ParseArguments(
       Scanner::Location* first_spread_pos, ExpressionClassifier* classifier,
       bool* ok) {
     return ParseArguments(first_spread_pos, false, classifier, ok);
   }
 
   ExpressionT ParseAssignmentExpression(bool accept_IN,
                                         ExpressionClassifier* classifier,
-                                        bool* ok);
+                                        bool marge_partterns, bool* ok);

[nickie, 2016/07/11 13:19:50]

Unless I'm missing the pun, "merge_patterns" :-)

[caitp, 2016/07/11 13:24:42]

just a typo, will fix

+  ExpressionT ParseAssignmentExpression(bool accept_IN,
+                                        ExpressionClassifier* classifier,
+                                        bool* ok) {
+    return ParseAssignmentExpression(accept_IN, classifier, false, ok);
+  }
   ExpressionT ParseYieldExpression(bool accept_IN,
                                    ExpressionClassifier* classifier, bool* ok);
   ExpressionT ParseTailCallExpression(ExpressionClassifier* classifier,
                                       bool* ok);
   ExpressionT ParseConditionalExpression(bool accept_IN,
                                          ExpressionClassifier* classifier,
                                          bool* ok);
   ExpressionT ParseBinaryExpression(int prec, bool accept_IN,
                                     ExpressionClassifier* classifier, bool* ok);
   ExpressionT ParseUnaryExpression(ExpressionClassifier* classifier, bool* ok);
@@ skipping 1127 matching lines @@
   Expect(Token::LPAREN, CHECK_OK_CUSTOM(NullExpressionList));
   bool done = (peek() == Token::RPAREN);
   bool was_unspread = false;
   int unspread_sequences_count = 0;
   while (!done) {
     int start_pos = peek_position();
     bool is_spread = Check(Token::ELLIPSIS);
     int expr_pos = peek_position();
 
     ExpressionT argument = this->ParseAssignmentExpression(
-        true, classifier, CHECK_OK_CUSTOM(NullExpressionList));
+        true, classifier, maybe_arrow, CHECK_OK_CUSTOM(NullExpressionList));
     CheckNoTailCallExpressions(classifier, CHECK_OK_CUSTOM(NullExpressionList));
     if (!maybe_arrow) {
       Traits::RewriteNonPattern(classifier,
                                 CHECK_OK_CUSTOM(NullExpressionList));
     }
     if (is_spread) {
       if (!spread_arg.IsValid()) {
         spread_arg.beg_pos = start_pos;
         spread_arg.end_pos = peek_position();
       }
@@ skipping 46 matching lines @@
   }
 
   return result;
 }
 
 // Precedence = 2
 template <class Traits>
 typename ParserBase<Traits>::ExpressionT
 ParserBase<Traits>::ParseAssignmentExpression(bool accept_IN,
                                               ExpressionClassifier* classifier,
-                                              bool* ok) {
+                                              bool merge_patterns, bool* ok) {
   // AssignmentExpression ::
   //   ConditionalExpression
   //   ArrowFunction
   //   YieldExpression
   //   LeftHandSideExpression AssignmentOperator AssignmentExpression
   bool is_destructuring_assignment = false;
   int lhs_beg_pos = peek_position();
 
   if (peek() == Token::YIELD && is_generator()) {
     return this->ParseYieldExpression(accept_IN, classifier, ok);
@@ skipping 71 matching lines @@
     return expression;
   }
 
   if (this->IsValidReferenceExpression(expression)) {
     arrow_formals_classifier.ForgiveAssignmentPatternError();
   }
 
   // "expression" was not itself an arrow function parameter list, but it might
   // form part of one.  Propagate speculative formal parameter error locations.
   // Do not merge pending non-pattern expressions yet!
-  classifier->Accumulate(
-      &arrow_formals_classifier,
+  int productions_to_propagate =
       ExpressionClassifier::StandardProductions |
-          ExpressionClassifier::FormalParametersProductions |
-          ExpressionClassifier::CoverInitializedNameProduction |
-          ExpressionClassifier::AsyncArrowFormalParametersProduction |
-          ExpressionClassifier::AsyncBindingPatternProduction,
-      false);
+      ExpressionClassifier::FormalParametersProductions |
+      ExpressionClassifier::CoverInitializedNameProduction |
+      ExpressionClassifier::AsyncArrowFormalParametersProduction |
+      ExpressionClassifier::AsyncBindingPatternProduction;
+  if (merge_patterns) {

[Dan Ehrenberg, 2016/07/11 23:22:47]

I find this logic of when to merge_patterns to be really confusing. Maybe better
to notice the binding pattern error when it's generated, rather than propagating
it like this and noticing it later. I tried that approach out at
https://codereview.chromium.org/2139063002/ .

[caitp, 2016/07/11 23:32:24]

That doesn't really work, because then we could fail when parsing
`async(<assignment pattern>);` for no real reason...

[Dan Ehrenberg, 2016/07/11 23:37:57]

I don't understand the issue. The binding pattern is only checked if it's
followed by an arrow (the same case for turning it into a comma expression). Do
you have a test case in mind?

[nickie, 2016/07/12 13:11:53]

I tried the following with Dan's patch and they seem to work fine, so I don't
understand what Caitlin suggests here.

d8> function async(x) { print("async", x, "FTW!"); }
d8> var await=42;

d8> f = async(1);
async 1 FTW!

d8> f = async(x=1);
async 1 FTW!

d8> f = async([x,y]=[1,2]);
async 1,2 FTW!

d8> async(x=1);
async 1 FTW!

d8> async([x,y]=[1,2]);
async 1,2 FTW!

d8> async(await);
async 42 FTW!

d8> async(await=17);
async 17 FTW!

[caitp, 2016/07/12 14:38:30]

What are either of you even talking about? Using plain english, what he
suggested does not work. What he implements in his patch is different from what
he suggested here --- so that's besides the point. Anyways, his patch works,
cool, lets do it.

+    productions_to_propagate |= ExpressionClassifier::BindingPatternProduction;
+  }
+
+  classifier->Accumulate(&arrow_formals_classifier, productions_to_propagate,
+                         false);
 
   if (!Token::IsAssignmentOp(peek())) {
     // Parsed conditional expression only (no assignment).
     // Now pending non-pattern expressions must be merged.
     classifier->MergeNonPatterns(&arrow_formals_classifier);
     return expression;
   }
 
   // Now pending non-pattern expressions must be discarded.
   arrow_formals_classifier.Discard();
@@ skipping 446 matching lines @@
         Traits::RewriteNonPattern(classifier, CHECK_OK);
         result = factory()->NewProperty(result, index, pos);
         Expect(Token::RBRACK, CHECK_OK);
         break;
       }
 
       case Token::LPAREN: {
         CheckNoTailCallExpressions(classifier, CHECK_OK);
         int pos;
         Traits::RewriteNonPattern(classifier, CHECK_OK);
-        BindingPatternUnexpectedToken(classifier);
+
+        const char* arg;
+        MessageTemplate::Template message = MessageTemplate::kUnexpectedToken;
+        Scanner::Location location = scanner()->peek_location();
+        GetUnexpectedTokenMessage(peek(), &message, &location, &arg);
+
+        if (!is_async) {
+          classifier->RecordBindingPatternError(location, message, arg);
+        }
+
         if (scanner()->current_token() == Token::IDENTIFIER ||
             scanner()->current_token() == Token::SUPER ||
             scanner()->current_token() == Token::ASYNC) {
           // For call of an identifier we want to report position of
           // the identifier as position of the call in the stack trace.
           pos = position();
         } else {
           // For other kinds of calls we record position of the parenthesis as
           // position of the call. Note that this is extremely important for
           // expressions of the form function(){...}() for which call position
@@ skipping 11 matching lines @@
         typename Traits::Type::ExpressionList args =
             ParseArguments(&spread_pos, is_async, classifier, CHECK_OK);
 
         if (V8_UNLIKELY(is_async && peek() == Token::ARROW)) {
           if (args->length()) {
             // async ( Arguments ) => ...
             return Traits::ExpressionListToExpression(args);
           }
           // async () => ...
           return factory()->NewEmptyParentheses(pos);
+        } else {
+          classifier->ReplaceBindingPatternError(location, message, arg);

[Dan Ehrenberg, 2016/07/11 23:22:47]

I don't understand why this case is added when we're going to reach
ArrowFormalParametersUnexpectedToken a couple lines lower. Also, why replace
here rather than add to the list like normal? Do you have an example of an error
message that improves due to this? If this is about getting a better error
message rather than reporting an error in the first place, we should have a
message test for it.

[caitp, 2016/07/11 23:32:24]

ArrowFormalParameters don't really matter for async functions. What matters is
that the parameters to the `async(params...)` call are each valid binding
patterns. (And also, valid AsyncArrowFormalParameters because of the banned
`await` word).

[Dan Ehrenberg, 2016/07/11 23:37:57]

Could you write a test for it?

         }
 
         ArrowFormalParametersUnexpectedToken(classifier);
 
         // Keep track of eval() calls since they disable all local variable
         // optimizations.
         // The calls that need special treatment are the
         // direct eval calls. These calls are all of the form eval(...), with
         // no explicit receiver.
         // These calls are marked as potentially direct eval calls. Whether
@@ skipping 801 matching lines @@
     has_seen_constructor_ = true;
     return;
   }
 }
 
 
 }  // namespace internal
 }  // namespace v8
 
 #endif  // V8_PARSING_PARSER_BASE_H