[clang-tools-extra] [clangd] Extract to function: Suppress pure forwarding wrappers (PR #232654)
Christian Kandeler via cfe-commits
cfe-commits at lists.llvm.org
Fri Oct 16 02:47:38 PDT 2026
https://github.com/ckandeler created https://github.com/llvm/llvm-project/pull/232654
Extracting a single statement that's just a lone call (or a return of
one), where every operand that would need to become a parameter is
already exactly that parameter value unchanged, produces nothing but a
renamed call: `foo(x);` becomes `void extracted(int x) { foo(x); }`
called as `extracted(x);`, with no abstraction gained. A value-less
`return;` is the same kind of no-op: there's nothing to abstract over
at all. Both are now rejected.
This is deliberately narrow and per-operand, not a naive argument-count
check: `foo(100)` (a literal), `foo(globalVar)` (an already-visible
global), and `foo(x + y)` (real computation) are all still offered,
since each does something a plain rename wouldn't.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
>From 7c8179289c4ed2a23369779c70db1f54ec3c73a6 Mon Sep 17 00:00:00 2001
From: Christian Kandeler <christian.kandeler at qt.io>
Date: Fri, 16 Oct 2026 11:35:42 +0200
Subject: [PATCH] [clangd] Extract to function: Suppress pure forwarding
wrappers
Extracting a single statement that's just a lone call (or a return of
one), where every operand that would need to become a parameter is
already exactly that parameter value unchanged, produces nothing but a
renamed call: `foo(x);` becomes `void extracted(int x) { foo(x); }`
called as `extracted(x);`, with no abstraction gained. A value-less
`return;` is the same kind of no-op: there's nothing to abstract over
at all. Both are now rejected.
This is deliberately narrow and per-operand, not a naive argument-count
check: `foo(100)` (a literal), `foo(globalVar)` (an already-visible
global), and `foo(x + y)` (real computation) are all still offered,
since each does something a plain rename wouldn't.
Assisted-by: Claude Code
---
.../refactor/tweaks/ExtractFunction.cpp | 78 ++++++++-
.../unittests/tweaks/ExtractFunctionTests.cpp | 162 ++++++++++++++++--
2 files changed, 222 insertions(+), 18 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
index de9a5fa45b9c0..3e43e6c61f3ad 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -390,13 +390,89 @@ computeEnclosingFuncRange(const FunctionDecl *EnclosingFunction,
return toHalfOpenFileRange(SM, LangOpts, EnclosingFunction->getSourceRange());
}
+// Whether D would need to become a forwarded parameter of the extracted
+// function if referenced from inside the extraction zone: a parameter or
+// local variable of the enclosing function. A static/extern/thread-local
+// variable doesn't need forwarding -- it's already visible wherever the
+// extracted function ends up, same as a global.
+bool mustBeForwarded(const Decl *D) {
+ if (isa<ParmVarDecl>(D))
+ return true;
+ if (const auto *VD = dyn_cast<VarDecl>(D))
+ return VD->hasLocalStorage();
+ return false;
+}
+
+// Whether E, as an operand of a call being considered by isTrivialExtraction,
+// demonstrates no abstraction of its own: a bare (parenthesized/implicitly
+// cast) reference to a Decl that would need to become a parameter, forwarded
+// unchanged. Anything else -- a literal, a reference to an already-visible
+// global, or any non-bare expression -- means this operand alone justifies
+// offering the extraction (it hardcodes or computes something a plain
+// rename wouldn't), so it's not "bare forwarding".
+bool isBareForwardedOperand(const Expr *E) {
+ const auto *DRE = dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts());
+ return DRE && mustBeForwarded(DRE->getDecl());
+}
+
+// The operands of Call relevant to isTrivialExtraction: the receiver of
+// a method call, a non-method callee that isn't a plain reference to a
+// fixed function (e.g. a function-pointer variable instead), and every
+// explicit argument. An operator call's operands are already included in
+// its own arguments() (clang models e.g. `a + b` as `operator+(a, b)`),
+// so its callee -- always a reference to the operator function/overload
+// itself, never itself customizable -- is always excluded.
+llvm::SmallVector<const Expr *, 4> relevantCallOperands(const CallExpr *Call) {
+ llvm::SmallVector<const Expr *, 4> Operands;
+ if (const auto *MCE = dyn_cast<CXXMemberCallExpr>(Call)) {
+ Operands.push_back(MCE->getImplicitObjectArgument());
+ } else if (!isa<CXXOperatorCallExpr>(Call)) {
+ const Expr *Callee = Call->getCallee()->IgnoreParenImpCasts();
+ if (!(isa<DeclRefExpr>(Callee) &&
+ isa<FunctionDecl>(cast<DeclRefExpr>(Callee)->getDecl())))
+ Operands.push_back(Call->getCallee());
+ }
+ Operands.append(Call->arg_begin(), Call->arg_end());
+ return Operands;
+}
+
+// Whether extracting S on its own, as a single root statement, would add
+// no real abstraction over leaving it where it is:
+// - A value-less `return;` has nothing to abstract over at all.
+// - A call (to a function, method, or operator), or a return of one, is
+// a pure 1:1 forwarding wrapper if every relevant operand (see
+// relevantCallOperands) is already exactly the parameter value it
+// would become, unchanged (or there are none). Extracting e.g.
+// `foo();`, `foo(x)` (x a local), or `return foo(x);` this way would
+// just rename the call; `foo(100)`, `foo(globalVar)`, or `foo(x + y)`
+// would not, since each does something a plain rename wouldn't
+// (hardcode a value/global, or compute one). For the return case, the
+// returned value may need an implicit conversion to the enclosing
+// function's own return type (e.g. foo returns int, the enclosing
+// function long); IgnoreImplicit() looks through that -- the
+// conversion doesn't change the verdict, since the extracted function
+// would perform the exact same conversion internally.
+bool isTrivialExtraction(const Stmt *S) {
+ if (const auto *RS = dyn_cast<ReturnStmt>(S)) {
+ if (!RS->getRetValue())
+ return true;
+ S = RS->getRetValue()->IgnoreImplicit();
+ }
+ const auto *Call = dyn_cast<CallExpr>(S);
+ return Call &&
+ llvm::all_of(relevantCallOperands(Call), isBareForwardedOperand);
+}
+
// returns true if Child can be a single RootStmt being extracted from
// EnclosingFunc.
bool validSingleChild(const Node *Child, const FunctionDecl *EnclosingFunc) {
// Extracting the body of EnclosingFunc would remove it's definition.
assert(EnclosingFunc->hasBody() &&
"We should always be extracting from a function body.");
- if (Child->ASTNode.get<Stmt>() == EnclosingFunc->getBody())
+ const Stmt *S = Child->ASTNode.get<Stmt>();
+ if (S == EnclosingFunc->getBody())
+ return false;
+ if (isTrivialExtraction(S))
return false;
return true;
}
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
index fd2472655af37..bed7666575303 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -744,7 +744,7 @@ TEST_F(ExtractFunctionTest, RangeBasedFor) {
struct Vec { int *begin(); int *end(); };
Vec V;
void foo(int);
- void f() { for (auto X : V) { [[foo(X);]] } }
+ void f() { for (auto X : V) { [[foo(X + 1);]] } }
)cpp"),
HasSubstr("extracted"));
}
@@ -782,10 +782,11 @@ TEST_F(ExtractFunctionTest, ConstParameters) {
Context = File;
// A captured scalar that's only read becomes a by-value parameter;
// non-scalars instead become a const reference (see the `S` cases
- // below).
+ // below). The duplicated calls below are there to prevent "pure forwarding"
+ // suppression.
EXPECT_THAT(apply(R"cpp(
void use(int);
- void f(int x) { [[use(x);]] }
+ void f(int x) { [[use(x); use(x);]] }
)cpp"),
HasSubstr("void extracted(int x)"));
// Direct assignment: stays non-const.
@@ -800,45 +801,46 @@ TEST_F(ExtractFunctionTest, ConstParameters) {
// A non-const method call may mutate the object: stays non-const.
EXPECT_THAT(apply(R"cpp(
struct S { void mutate(); };
- void f(S s) { [[s.mutate();]] }
+ void f(S s) { [[s.mutate(); s.mutate();]] }
)cpp"),
HasSubstr("void extracted(S &s)"));
// A const method call cannot mutate the object: becomes const.
EXPECT_THAT(apply(R"cpp(
struct S { void inspect() const; };
- void f(S s) { [[s.inspect();]] }
+ void f(S s) { [[s.inspect(); s.inspect();]] }
)cpp"),
HasSubstr("void extracted(const S &s)"));
// Passed to a parameter taking a non-const reference: stays non-const,
// since the callee could mutate it through that reference.
EXPECT_THAT(apply(R"cpp(
void mayMutate(int &);
- void f(int x) { [[mayMutate(x);]] }
+ void f(int x) { [[mayMutate(x); mayMutate(x);]] }
)cpp"),
HasSubstr("void extracted(int &x)"));
// Passed to a parameter taking a const reference or by value: becomes
// an unmutated scalar, so by value.
EXPECT_THAT(apply(R"cpp(
void readOnly(const int &);
- void f(int x) { [[readOnly(x);]] }
+ void f(int x) { [[readOnly(x); readOnly(x);]] }
)cpp"),
HasSubstr("void extracted(int x)"));
EXPECT_THAT(apply(R"cpp(
void byValue(int);
- void f(int x) { [[byValue(x);]] }
+ void f(int x) { [[byValue(x); byValue(x);]] }
)cpp"),
HasSubstr("void extracted(int x)"));
// A scalar parameter that's already declared const keeps that
// qualifier when passed by value: It might be relevant for overload
// resolution.
- EXPECT_THAT(apply("void use(int); void f(const int x) { [[use(x);]] }"),
- HasSubstr("void extracted(const int x)"));
+ EXPECT_THAT(
+ apply("void use(int); void f(const int x) { [[use(x); use(x);]] }"),
+ HasSubstr("void extracted(const int x)"));
// A non-scalar parameter that's already declared const keeps that
// qualifier, since it's still passed by reference.
EXPECT_THAT(apply(R"cpp(
struct S {};
void use(const S &);
- void f(const S s) { [[use(s);]] }
+ void f(const S s) { [[use(s); use(s);]] }
)cpp"),
HasSubstr("void extracted(const S &s)"));
}
@@ -895,7 +897,7 @@ TEST_F(ExtractFunctionTest, ConstParametersMemberCallArguments) {
// the argument check from the object check.
EXPECT_THAT(apply(R"cpp(
struct S { void mayMutate(int &) const; };
- void f(S s, int x) { [[s.mayMutate(x);]] }
+ void f(S s, int x) { [[s.mayMutate(x); s.mayMutate(x);]] }
)cpp"),
HasSubstr("extracted(const S &s, int &x)"));
// Same, but through an overloaded operator: the implicit object is
@@ -903,7 +905,7 @@ TEST_F(ExtractFunctionTest, ConstParametersMemberCallArguments) {
// found at the right offset.
EXPECT_THAT(apply(R"cpp(
struct S { void operator()(int &) const; };
- void f(S s, int x) { [[s(x);]] }
+ void f(S s, int x) { [[s(x); s(x);]] }
)cpp"),
HasSubstr("extracted(const S &s, int &x)"));
// A free (non-member) operator overload has no implicit object, so all
@@ -911,7 +913,7 @@ TEST_F(ExtractFunctionTest, ConstParametersMemberCallArguments) {
EXPECT_THAT(apply(R"cpp(
struct S {};
void operator+(const S &, int &);
- void f(S s, int x) { [[s + x;]] }
+ void f(S s, int x) { [[s + x; s + x;]] }
)cpp"),
HasSubstr("extracted(const S &s, int &x)"));
}
@@ -970,7 +972,7 @@ TEST_F(ExtractFunctionTest, ConstParametersStaticOperatorCall) {
ExtraArgs.push_back("-std=c++23");
EXPECT_THAT(apply(R"cpp(
struct S { static void operator()(int); };
- void f(S s, int x) { [[s(x);]] }
+ void f(S s, int x) { [[s(x); s(x);]] }
)cpp"),
HasSubstr("extracted(const S &s, int x)"));
}
@@ -998,13 +1000,13 @@ TEST_F(ExtractFunctionTest, ConstParametersFunctionReference) {
TEST_F(ExtractFunctionTest, ConstParametersScalarsByValue) {
Context = File;
// An unmutated pointer is a scalar too: passed by value.
- EXPECT_THAT(apply("void use(int *); void f(int *p) { [[use(p);]] }"),
+ EXPECT_THAT(apply("void use(int *); void f(int *p) { [[use(p); use(p);]] }"),
HasSubstr("extracted(int *p)"));
// An unmutated enum: passed by value.
EXPECT_THAT(apply(R"cpp(
enum E { A, B };
void use(E);
- void f(E e) { [[use(e);]] }
+ void f(E e) { [[use(e); use(e);]] }
)cpp"),
HasSubstr("extracted(E e)"));
// A class type, even one that's small and trivially copyable, is never
@@ -1332,6 +1334,132 @@ TEST_F(ExtractFunctionTest, CFileRejectMutatedPointerToVariableLengthArray) {
"fail: Too complex to extract.");
}
+TEST_F(ExtractFunctionTest, RejectPureForwardingZeroArgCall) {
+ Context = File;
+ // No abstraction gained: the extracted function would just rename the
+ // call, with no customizable behavior at all.
+ EXPECT_EQ(apply("void foo(); void f() { [[foo();]] }"), "unavailable");
+}
+
+TEST_F(ExtractFunctionTest, RejectPureForwardingSingleArgCall) {
+ Context = File;
+ // x is forwarded to foo unchanged: still just a rename.
+ EXPECT_EQ(apply("void foo(int); void f(int x) { [[foo(x);]] }"),
+ "unavailable");
+}
+
+TEST_F(ExtractFunctionTest, OfferDespitePureForwarding_LiteralArg) {
+ Context = File;
+ // Hardcoding 100 is a real specialization, unlike a plain rename.
+ EXPECT_THAT(apply("void foo(int); void f() { [[foo(100);]] }"),
+ HasSubstr("extracted"));
+}
+
+TEST_F(ExtractFunctionTest, OfferDespitePureForwarding_GlobalArg) {
+ Context = File;
+ // Hardcoding which already-visible global to use is likewise a real
+ // specialization: unlike a captured local, globalVar doesn't need to
+ // become a parameter at all.
+ EXPECT_THAT(apply(R"cpp(
+ int globalVar;
+ void foo(int);
+ void f() { [[foo(globalVar);]] }
+ )cpp"),
+ HasSubstr("extracted"));
+}
+
+TEST_F(ExtractFunctionTest, OfferDespitePureForwarding_ComputedArg) {
+ Context = File;
+ // foo(x + y) needs 2 parameters for 1 argument slot: real computation,
+ // not just forwarding x and y unchanged.
+ EXPECT_THAT(apply("void foo(int); void f(int x, int y) { [[foo(x + y);]] }"),
+ HasSubstr("extracted"));
+}
+
+TEST_F(ExtractFunctionTest, RejectPureForwardingMethodCall) {
+ Context = File;
+ // Both the receiver and the argument are forwarded unchanged.
+ EXPECT_EQ(apply(R"cpp(
+ struct S { void method(int); };
+ void f(S s, int x) { [[s.method(x);]] }
+ )cpp"),
+ "unavailable");
+}
+
+TEST_F(ExtractFunctionTest, OfferDespitePureForwarding_GlobalReceiver) {
+ Context = File;
+ // Hardcoding which already-visible object to call the method on is a
+ // real specialization, same as a global argument.
+ EXPECT_THAT(apply(R"cpp(
+ struct S { void method(); };
+ S globalS;
+ void f() { [[globalS.method();]] }
+ )cpp"),
+ HasSubstr("extracted"));
+}
+
+TEST_F(ExtractFunctionTest, RejectPureForwardingOperatorCall) {
+ Context = File;
+ // Both operands of a + b are forwarded unchanged; the operator function
+ // itself (never customizable) is correctly excluded from the check.
+ EXPECT_EQ(apply(R"cpp(
+ struct S {};
+ void operator+(const S &, int);
+ void f(S s, int x) { [[s + x;]] }
+ )cpp"),
+ "unavailable");
+}
+
+TEST_F(ExtractFunctionTest, RejectPureForwardingFunctionPointerCall) {
+ Context = File;
+ // Unlike an ordinary function name, a function-pointer variable callee
+ // does need to become a parameter -- but it's still forwarded
+ // unchanged here, same as x.
+ EXPECT_EQ(apply("void f(void (*fp)(int), int x) { [[fp(x);]] }"),
+ "unavailable");
+}
+
+TEST_F(ExtractFunctionTest, OfferDespitePureForwarding_ComputedCallee) {
+ Context = File;
+ // Which function pointer gets called is itself computed here, not a
+ // bare forwarded reference: real logic, not just forwarding.
+ EXPECT_THAT(
+ apply("void f(void (*fp[2])(int), int i, int x) { [[fp[i](x);]] }"),
+ HasSubstr("extracted"));
+}
+
+TEST_F(ExtractFunctionTest, RejectPureForwardingReturnStmt) {
+ Context = File;
+ // A call wrapped in a ReturnStmt is just as much a pure rename as a
+ // bare call statement.
+ EXPECT_EQ(apply("int foo(int); int f(int x) { [[return foo(x);]] }"),
+ "unavailable");
+}
+
+TEST_F(ExtractFunctionTest, RejectPureForwardingReturnStmtImplicitConversion) {
+ Context = File;
+ // The implicit int-to-long conversion doesn't change the verdict: the
+ // extracted function would perform the exact same conversion
+ // internally, so this is still just a rename.
+ EXPECT_EQ(apply("int foo(int); long f(int x) { [[return foo(x);]] }"),
+ "unavailable");
+}
+
+TEST_F(ExtractFunctionTest, OfferDespitePureForwarding_ReturnStmtComputedArg) {
+ Context = File;
+ // Same as the non-return case: a computed argument is real abstraction.
+ EXPECT_THAT(
+ apply("int foo(int); int f(int x, int y) { [[return foo(x + y);]] }"),
+ HasSubstr("extracted"));
+}
+
+TEST_F(ExtractFunctionTest, RejectBareReturnStmt) {
+ Context = File;
+ // A value-less return has nothing to abstract over at all: extracting
+ // it would just wrap it in a function that does the exact same thing.
+ EXPECT_EQ(apply("void f() { [[return;]] }"), "unavailable");
+}
+
} // namespace
} // namespace clangd
} // namespace clang
More information about the cfe-commits
mailing list