[clang] [Clang] Improve error recovery for invalid calls (PR #136295)
Younan Zhang via cfe-commits
cfe-commits at lists.llvm.org
Fri Apr 18 04:47:30 PDT 2025
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 <zyn7109 at 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 at -1{{expected '(' for function-style cast or type construction}}
+// expected-error at -1{{expected '(' for function-style cast or type construction}} \
+// expected-error at -2{{parentheses are required around this expression in a requires clause}}
template<typename T>
void bar() requires foo<T>();
More information about the cfe-commits
mailing list