[Clang] Improve error recovery for invalid calls (llvm#136295)

It doesn't make sense that we only build a RecoveryExpr for expressions
with invalid trailing commas. This patch extends it so that we now
always build up a RecoveryExpr whenever the call contains anything
invalid. As a result, we can back out HasTrailingComma.

There is only one diagnostic change as to concepts, where a RecoveryExpr
than an ExprError is now used to model an invalid requires clause, for
which we suggest adding parentheses around it. (This looks like what GCC
diagnoses)

Author: zyn0217

clang/docs/ReleaseNotes.rst

@@ -398,14 +398,16 @@ Improvements to Clang's diagnostics
   constructors to initialize their non-modifiable members. The diagnostic is
   not new; being controlled via a warning group is what's new. Fixes #GH41104
 
+
+- Improved Clang's error recovery for invalid function calls.
+
 - Improved bit-field diagnostics to consider the type specified by the
   ``preferred_type`` attribute. These diagnostics are controlled by the flags
   ``-Wpreferred-type-bitfield-enum-conversion`` and
   ``-Wpreferred-type-bitfield-width``. These warnings are on by default as they
   they're only triggered if the authors are already making the choice to use
   ``preferred_type`` attribute.
 
-
 Improvements to Clang's time-trace
 ----------------------------------
 

clang/include/clang/Parse/Parser.h

@@ -1942,8 +1942,7 @@ class Parser : public CodeCompletionHandler {
                            llvm::function_ref<void()> ExpressionStarts =
                                llvm::function_ref<void()>(),
                            bool FailImmediatelyOnInvalidExpr = false,
-                           bool EarlyTypoCorrection = false,
-                           bool *HasTrailingComma = nullptr);
+                           bool EarlyTypoCorrection = false);
 
   /// ParseSimpleExpressionList - A simple comma-separated list of expressions,
   /// used for misc language extensions.

clang/lib/Parse/ParseExpr.cpp

@@ -2218,27 +2218,20 @@ Parser::ParsePostfixExpressionSuffix(ExprResult LHS) {
         CalledSignatureHelp = true;
         return PreferredType;
       };
+      bool ExpressionListIsInvalid = false;
       if (OpKind == tok::l_paren || !LHS.isInvalid()) {
         if (Tok.isNot(tok::r_paren)) {
-          bool HasTrailingComma = false;
-          bool HasError = ParseExpressionList(
-              ArgExprs,
-              [&] {
-                PreferredType.enterFunctionArgument(Tok.getLocation(),
-                                                    RunSignatureHelp);
-              },
-              /*FailImmediatelyOnInvalidExpr*/ false,
-              /*EarlyTypoCorrection*/ false, &HasTrailingComma);
-
-          if (HasError && !HasTrailingComma) {
+          if ((ExpressionListIsInvalid = ParseExpressionList(ArgExprs, [&] {
+                 PreferredType.enterFunctionArgument(Tok.getLocation(),
+                                                     RunSignatureHelp);
+               }))) {
             (void)Actions.CorrectDelayedTyposInExpr(LHS);
             // If we got an error when parsing expression list, we don't call
             // the CodeCompleteCall handler inside the parser. So call it here
             // to make sure we get overload suggestions even when we are in the
             // middle of a parameter.
             if (PP.isCodeCompletionReached() && !CalledSignatureHelp)
               RunSignatureHelp();
-            LHS = ExprError();
           } else if (LHS.isInvalid()) {
             for (auto &E : ArgExprs)
               Actions.CorrectDelayedTyposInExpr(E);
@@ -2249,6 +2242,12 @@ Parser::ParsePostfixExpressionSuffix(ExprResult LHS) {
       // Match the ')'.
       if (LHS.isInvalid()) {
         SkipUntil(tok::r_paren, StopAtSemi);
+      } else if (ExpressionListIsInvalid) {
+        Expr *Fn = LHS.get();
+        ArgExprs.insert(ArgExprs.begin(), Fn);
+        LHS = Actions.CreateRecoveryExpr(Fn->getBeginLoc(), Tok.getLocation(),
+                                         ArgExprs);
+        SkipUntil(tok::r_paren, StopAtSemi);
       } else if (Tok.isNot(tok::r_paren)) {
         bool HadDelayedTypo = false;
         if (Actions.CorrectDelayedTyposInExpr(LHS).get() != LHS.get())
@@ -3700,8 +3699,7 @@ void Parser::injectEmbedTokens() {
 bool Parser::ParseExpressionList(SmallVectorImpl<Expr *> &Exprs,
                                  llvm::function_ref<void()> ExpressionStarts,
                                  bool FailImmediatelyOnInvalidExpr,
-                                 bool EarlyTypoCorrection,
-                                 bool *HasTrailingComma) {
+                                 bool EarlyTypoCorrection) {
   bool SawError = false;
   while (true) {
     if (ExpressionStarts)
@@ -3744,11 +3742,6 @@ bool Parser::ParseExpressionList(SmallVectorImpl<Expr *> &Exprs,
     Token Comma = Tok;
     ConsumeToken();
     checkPotentialAngleBracketDelimiter(Comma);
-
-    if (Tok.is(tok::r_paren)) {
-      if (HasTrailingComma)
-        *HasTrailingComma = true;
-    }
   }
   if (SawError) {
     // Ensure typos get diagnosed when errors were encountered while parsing the

clang/test/AST/ast-dump-recovery.cpp

@@ -34,21 +34,22 @@ void test_invalid_call_1(int s) {
 
 int some_func2(int a, int b);
 void test_invalid_call_2() {
-  // CHECK:   -RecoveryExpr {{.*}} 'int' contains-errors
+  // CHECK:   -RecoveryExpr {{.*}} '<dependent type>' contains-errors
   // CHECK-NEXT: `-UnresolvedLookupExpr {{.*}} '<overloaded function type>' lvalue (ADL) = 'some_func2'
   some_func2(,);
 
-  // CHECK:   -RecoveryExpr {{.*}} 'int' contains-errors
+  // CHECK:   -RecoveryExpr {{.*}} '<dependent type>' contains-errors
   // CHECK-NEXT: `-UnresolvedLookupExpr {{.*}} '<overloaded function type>' lvalue (ADL) = 'some_func2'
   some_func2(,,);
 
-  // CHECK:   `-RecoveryExpr {{.*}} 'int' contains-errors
+  // CHECK:   -RecoveryExpr {{.*}} '<dependent type>' contains-errors
   // CHECK-NEXT: |-UnresolvedLookupExpr {{.*}} '<overloaded function type>' lvalue (ADL) = 'some_func2'
   // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1
   some_func2(1,);
 
-  // FIXME: Handle invalid argument with recovery
-  // CHECK-NOT: `-RecoveryExpr
+  // CHECK:   -RecoveryExpr {{.*}} '<dependent type>' contains-errors
+  // CHECK-NEXT: |-UnresolvedLookupExpr {{.*}} '<overloaded function type>' lvalue (ADL) = 'some_func2'
+  // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1
   some_func2(,1);
 }
 

clang/test/AST/new-unknown-type.cpp

@@ -17,9 +17,15 @@ namespace b {
 // CHECK:      |-NamespaceDecl 0x{{[^ ]*}} <line:5:1, line:11:1> line:5:11 a
 // CHECK-NEXT: | `-FunctionDecl 0x{{[^ ]*}} <line:6:3, line:10:3> line:6:8 computeSomething 'void ()'
 // CHECK-NEXT: |   `-CompoundStmt 0x{{[^ ]*}} <col:27, line:10:3>
+// CHECK-NEXT: |   |-RecoveryExpr {{.*}} '<dependent type>' contains-errors
+// CHECK-NEXT: |   | `-UnresolvedLookupExpr {{.*}} '<overloaded function type>' lvalue (ADL) = 'foo'
+// CHECK-NEXT: |   |-RecoveryExpr {{.*}} '<dependent type>' contains-errors
+// CHECK-NEXT: |   | `-UnresolvedLookupExpr {{.*}} '<overloaded function type>' lvalue (ADL) = 'foo'
+// CHECK-NEXT: |   `-RecoveryExpr {{.*}} '<dependent type>' contains-errors
+// CHECK-NEXT: |    `-UnresolvedLookupExpr {{.*}} '<overloaded function type>' lvalue (ADL) = 'foo'
 // CHECK-NEXT: |-NamespaceDecl 0x{{[^ ]*}} <line:13:1, line:15:1> line:13:11 b
 // CHECK-NEXT: | `-CXXRecordDecl 0x{{[^ ]*}} <line:14:3, col:14> col:10 referenced struct Bar definition
 
 static b::Bar bar;
-// CHECK:      `-VarDecl 0x{{[^ ]*}} <line:23:1, col:15> col:15 bar 'b::Bar' static callinit
+// CHECK:      `-VarDecl 0x{{[^ ]*}} <line:29:1, col:15> col:15 bar 'b::Bar' static callinit
 // CHECK-NEXT:   `-CXXConstructExpr 0x{{[^ ]*}} <col:15> 'b::Bar' 'void () noexcept'

clang/test/Parser/cxx-concepts-requires-clause.cpp

@@ -119,7 +119,8 @@ template<typename T> requires 0
 
 template<typename T> requires foo<T>
 (int) bar() { };
-// expected-error@-1{{expected '(' for function-style cast or type construction}}
+// expected-error@-1{{expected '(' for function-style cast or type construction}} \
+// expected-error@-2{{parentheses are required around this expression in a requires clause}}
 
 template<typename T>
 void bar() requires foo<T>();