https://github.com/zyn0217 created https://github.com/llvm/llvm-project/pull/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 invalud requires clause, for which we suggest adding parentheses around it. (This looks like what GCC diagnoses: https://gcc.godbolt.org/z/GT4TzbqTq) >From 8efadc35d64f2724d5610a7ae66fa70a7c8e3d46 Mon Sep 17 00:00:00 2001 From: Younan Zhang <zyn7...@gmail.com> Date: Fri, 18 Apr 2025 19:20:49 +0800 Subject: [PATCH] [Clang] Improve error recovery for invalid calls 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 used to model an invalud requires clause, for which we now suggest adding parenthesis around it. --- clang/docs/ReleaseNotes.rst | 2 ++ clang/include/clang/Parse/Parser.h | 3 +- clang/lib/Parse/ParseExpr.cpp | 31 +++++++------------ clang/test/AST/ast-dump-recovery.cpp | 11 ++++--- clang/test/AST/new-unknown-type.cpp | 8 ++++- .../Parser/cxx-concepts-requires-clause.cpp | 3 +- 6 files changed, 30 insertions(+), 28 deletions(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index f5cd1fbeabcfe..613b79ca2c3d8 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -395,6 +395,8 @@ 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. + Improvements to Clang's time-trace ---------------------------------- diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h index 662f54d0e8d8a..40dbe23434d04 100644 --- a/clang/include/clang/Parse/Parser.h +++ b/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. diff --git a/clang/lib/Parse/ParseExpr.cpp b/clang/lib/Parse/ParseExpr.cpp index 3e3e7cbcd68b2..df015564849da 100644 --- a/clang/lib/Parse/ParseExpr.cpp +++ b/clang/lib/Parse/ParseExpr.cpp @@ -2218,19 +2218,13 @@ Parser::ParsePostfixExpressionSuffix(ExprResult LHS) { CalledSignatureHelp = true; return PreferredType; }; + bool KnownInvalidCall = 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 ((KnownInvalidCall = 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 @@ -2238,7 +2232,6 @@ Parser::ParsePostfixExpressionSuffix(ExprResult LHS) { // 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 (KnownInvalidCall) { + 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 diff --git a/clang/test/AST/ast-dump-recovery.cpp b/clang/test/AST/ast-dump-recovery.cpp index b59fa3778192f..b8195950f2fa1 100644 --- a/clang/test/AST/ast-dump-recovery.cpp +++ b/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); } diff --git a/clang/test/AST/new-unknown-type.cpp b/clang/test/AST/new-unknown-type.cpp index e7dd3c90145d2..9d6b19acba739 100644 --- a/clang/test/AST/new-unknown-type.cpp +++ b/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' diff --git a/clang/test/Parser/cxx-concepts-requires-clause.cpp b/clang/test/Parser/cxx-concepts-requires-clause.cpp index 13b4e8439882d..b921be5dbfd6d 100644 --- a/clang/test/Parser/cxx-concepts-requires-clause.cpp +++ b/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>(); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
