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

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 a946cfa6bc825bbca6162cdaf4d97105f6c9aa4c..71b622c96662af3d3a2c07c2f9ca5dea4723032d 100644
--- a/src/parsing/parser-base.h
+++ b/src/parsing/parser-base.h
@@ -897,7 +897,7 @@ class ParserBase : public Traits {
                                      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(),
@@ -916,11 +916,17 @@ class ParserBase : public Traits {
       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;
+      }
     }
   }
 
@@ -1024,7 +1030,12 @@ class ParserBase : public Traits {
 
   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,
@@ -2172,7 +2183,7 @@ typename Traits::Type::ExpressionList ParserBase<Traits>::ParseArguments(
     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,
@@ -2239,7 +2250,7 @@ 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
@@ -2331,14 +2342,18 @@ ParserBase<Traits>::ParseAssignmentExpression(bool accept_IN,
   // "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).
@@ -2805,7 +2820,16 @@ ParserBase<Traits>::ParseLeftHandSideExpression(
         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) {
@@ -2837,6 +2861,8 @@ ParserBase<Traits>::ParseLeftHandSideExpression(
           }
           // 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);