[clang-tools-extra] [clangd] Offer Extract to Function for single expression-statements (PR #219945)
Christian Kandeler via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 11 05:25:27 PDT 2026
https://github.com/ckandeler updated https://github.com/llvm/llvm-project/pull/219945
>From 1a33d51e61a26bfb42d44950598f7486cba32ea9 Mon Sep 17 00:00:00 2001
From: Christian Kandeler <christian.kandeler at qt.io>
Date: Mon, 31 Aug 2026 13:01:53 +0200
Subject: [PATCH 1/2] [clangd] Offer Extract to Function for single
expression-statements
The tweak refused to trigger whenever the extraction zone contained a
single statement that was an expression, e.g. a lone call like
`log("connection failed");` or an overloaded-operator statement like
`std::cout << "x";`. The former was blocked by an overly broad check
in validSingleChild(); the latter additionally required
getParentOfRootStmts() to recognize that such a statement can be its
own root statement even while marked Unselected, rather than being
treated as a container of root statements.
Fixes clangd/clangd#698
Fixes clangd/clangd#1254
Assisted-by: Claude
---
.../refactor/tweaks/ExtractFunction.cpp | 35 ++++++++++------
.../unittests/tweaks/ExtractFunctionTests.cpp | 41 +++++++++++++++----
clang-tools-extra/docs/ReleaseNotes.md | 5 +++
3 files changed, 61 insertions(+), 20 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
index bc9a790232507..624b11094ef09 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -95,6 +95,15 @@ enum FunctionDeclKind {
OutOfLineDefinition
};
+// Whether N, despite being Unselected, may still be a single RootStmt: a
+// DeclStmt can be unselected since VarDecls claim the entire selection range
+// in the selection tree. Similarly, a CXXOperatorCallExpr of a binary
+// operation can be unselected because its children (the operands) claim the
+// entire selection range in the selection tree (e.g. <<).
+bool isUnselectedRootStmtCandidate(const Node *N) {
+ return N->ASTNode.get<DeclStmt>() || N->ASTNode.get<CXXOperatorCallExpr>();
+}
+
// A RootStmt is a statement that's fully selected including all its children
// and its parent is unselected.
// Check if a node is a root statement.
@@ -104,12 +113,8 @@ bool isRootStmt(const Node *N) {
// Root statement cannot be partially selected.
if (N->Selected == SelectionTree::Partial)
return false;
- // A DeclStmt can be an unselected RootStmt since VarDecls claim the entire
- // selection range in selectionTree. Additionally, a CXXOperatorCallExpr of a
- // binary operation can be unselected because its children claim the entire
- // selection range in the selection tree (e.g. <<).
- if (N->Selected == SelectionTree::Unselected && !N->ASTNode.get<DeclStmt>() &&
- !N->ASTNode.get<CXXOperatorCallExpr>())
+ if (N->Selected == SelectionTree::Unselected &&
+ !isUnselectedRootStmtCandidate(N))
return false;
return true;
}
@@ -130,7 +135,18 @@ const Node *getParentOfRootStmts(const Node *CommonAnc) {
const Node *Parent = nullptr;
switch (CommonAnc->Selected) {
case SelectionTree::Selection::Unselected:
- // Typically a block, with the { and } unselected, could also be ForStmt etc
+ // Typically a block, with the { and } unselected, could also be ForStmt
+ // etc. However, CommonAnc may instead be a single statement that is
+ // itself Unselected only because all of its own tokens are claimed by
+ // its children (see isUnselectedRootStmtCandidate); in that case it's a
+ // root statement in its own right, and we need its actual parent, same
+ // as in the Complete case below.
+ if (isUnselectedRootStmtCandidate(CommonAnc)) {
+ Parent = CommonAnc->Parent;
+ if (Parent->ASTNode.get<DeclStmt>())
+ Parent = Parent->Parent;
+ break;
+ }
// Ensure all Children are RootStmts.
Parent = CommonAnc;
break;
@@ -298,11 +314,6 @@ computeEnclosingFuncRange(const FunctionDecl *EnclosingFunction,
// returns true if Child can be a single RootStmt being extracted from
// EnclosingFunc.
bool validSingleChild(const Node *Child, const FunctionDecl *EnclosingFunc) {
- // Don't extract expressions.
- // FIXME: We should extract expressions that are "statements" i.e. not
- // subexpressions
- if (Child->ASTNode.get<Expr>())
- return false;
// Extracting the body of EnclosingFunc would remove it's definition.
assert(EnclosingFunc->hasBody() &&
"We should always be extracting from a function body.");
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
index eff4d0f43595c..edad883950071 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -24,8 +24,8 @@ TEST_F(ExtractFunctionTest, FunctionTest) {
// Root statements should have common parent.
EXPECT_EQ(apply("for(;;) [[1+2; 1+2;]]"), "unavailable");
- // Expressions aren't extracted.
- EXPECT_EQ(apply("int x = 0; [[x++;]]"), "unavailable");
+ // Single expression-statements can be extracted.
+ EXPECT_THAT(apply("int x = 0; [[x++;]]"), HasSubstr("extracted"));
// We don't support extraction from lambdas.
EXPECT_EQ(apply("auto lam = [](){ [[int x;]] }; "), "unavailable");
// Partial statements aren't extracted.
@@ -192,16 +192,16 @@ F (extracted();)
EXPECT_EQ(apply(CompoundFailInput), "unavailable");
ExtraArgs.push_back("-std=c++14");
- // FIXME: Expressions are currently not extracted
- EXPECT_EQ(apply(R"cpp(
+ // A bare expression-statement can be extracted (the semicolon isn't part
+ // of the selection either way, since it isn't owned by any AST node).
+ EXPECT_THAT(apply(R"cpp(
void call() { [[1+1]]; }
)cpp"),
- "unavailable");
- // FIXME: Single expression statements are currently not extracted
- EXPECT_EQ(apply(R"cpp(
+ HasSubstr("extracted"));
+ EXPECT_THAT(apply(R"cpp(
void call() { [[1+1;]] }
)cpp"),
- "unavailable");
+ HasSubstr("extracted"));
}
TEST_F(ExtractFunctionTest, DifferentHeaderSourceTest) {
@@ -630,6 +630,31 @@ int main() {
EXPECT_EQ(apply(Before), After);
}
+TEST_F(ExtractFunctionTest, SingleStatement) {
+ Context = File;
+ // https://github.com/clangd/clangd/issues/698
+ // A single call-expression-statement can be extracted.
+ EXPECT_THAT(apply(R"cpp(
+ void foo(int, int);
+ void bar() {
+ [[foo(1, 2);]]
+ })cpp"),
+ HasSubstr("extracted"));
+ // https://github.com/clangd/clangd/issues/1254
+ // A single statement consisting of an overloaded binary operator call can
+ // be extracted, even though the SelectionTree marks the
+ // CXXOperatorCallExpr itself as Unselected (its operands claim all the
+ // characters).
+ EXPECT_THAT(apply(R"cpp(
+ struct Stream {};
+ Stream &operator<<(Stream &, const char *);
+ Stream stream;
+ int main() {
+ [[stream << "x";]]
+ })cpp"),
+ HasSubstr("extracted"));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md
index 633418a2abb98..ba00888b1adb3 100644
--- a/clang-tools-extra/docs/ReleaseNotes.md
+++ b/clang-tools-extra/docs/ReleaseNotes.md
@@ -82,6 +82,11 @@ infrastructure are described first, followed by tool-specific sections.
- clangd now applies clang-tidy fix-it post-processing before exposing fixes.
+- The `Extract to function` tweak is now offered for selections consisting of
+ a single expression-statement (e.g. a lone function call or an overloaded
+ operator call such as `stream << 42;`), which it previously refused to
+ extract.
+
#### Signature help
#### Cross-references
>From 2350f25a6f5610243d07a0ff352f80c879d6460a Mon Sep 17 00:00:00 2001
From: Christian Kandeler <christian.kandeler at qt.io>
Date: Fri, 11 Sep 2026 13:59:54 +0200
Subject: [PATCH 2/2] [clangd] Address review: dedupe ascend logic, reject
subexpressions
Factor the repeated "ascend to parent, skipping over an intervening
DeclStmt" logic out of getParentOfRootStmts() into a shared
getEnclosingStmt() helper, instead of duplicating it (and its
explanatory comment) across both the Unselected and Complete cases.
While doing so, add a check that the resulting parent isn't itself an
Expr. Without it, selecting a bare subexpression of a larger
expression-statement (e.g. the "3" in `stream << 3;`) was incorrectly
offered for extraction, producing broken code like
`void extracted() { 3; } ... stream << extracted();`.
Assisted-by: Claude
---
.../refactor/tweaks/ExtractFunction.cpp | 37 ++++++++++++++-----
.../unittests/tweaks/ExtractFunctionTests.cpp | 23 ++++++++++++
2 files changed, 50 insertions(+), 10 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
index 624b11094ef09..c2dc9b96c263a 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -119,6 +119,29 @@ bool isRootStmt(const Node *N) {
return true;
}
+// Given a Child that is itself a single RootStmt (either because it's
+// completely selected, or because it's an isUnselectedRootStmtCandidate),
+// returns Child's enclosing statement, which is where we'll look for
+// Child's RootStmt siblings.
+//
+// If parent is a DeclStmt, even though it's unselected, we consider it a
+// root statement and return its parent instead. This is done because the
+// VarDecls claim the entire selection range of the Declaration and DeclStmt
+// is always unselected.
+//
+// Returns null if the (possibly DeclStmt-adjusted) parent is an Expr: this
+// means Child is merely a subexpression of a larger expression rather than
+// a genuine standalone statement, e.g. selecting just the "3" in
+// `stream << 3;`, and extracting it would produce broken code.
+const Node *getEnclosingStmt(const Node *Child) {
+ const Node *Parent = Child->Parent;
+ if (Parent->ASTNode.get<DeclStmt>())
+ Parent = Parent->Parent;
+ if (Parent->ASTNode.get<Expr>())
+ return nullptr;
+ return Parent;
+}
+
// Returns the (unselected) parent of all RootStmts given the commonAncestor.
// Returns null if:
// 1. any node is partially selected
@@ -142,9 +165,7 @@ const Node *getParentOfRootStmts(const Node *CommonAnc) {
// root statement in its own right, and we need its actual parent, same
// as in the Complete case below.
if (isUnselectedRootStmtCandidate(CommonAnc)) {
- Parent = CommonAnc->Parent;
- if (Parent->ASTNode.get<DeclStmt>())
- Parent = Parent->Parent;
+ Parent = getEnclosingStmt(CommonAnc);
break;
}
// Ensure all Children are RootStmts.
@@ -156,15 +177,11 @@ const Node *getParentOfRootStmts(const Node *CommonAnc) {
case SelectionTree::Selection::Complete:
// If the Common Ancestor is completely selected, then it's a root statement
// and its parent will be unselected.
- Parent = CommonAnc->Parent;
- // If parent is a DeclStmt, even though it's unselected, we consider it a
- // root statement and return its parent. This is done because the VarDecls
- // claim the entire selection range of the Declaration and DeclStmt is
- // always unselected.
- if (Parent->ASTNode.get<DeclStmt>())
- Parent = Parent->Parent;
+ Parent = getEnclosingStmt(CommonAnc);
break;
}
+ if (!Parent)
+ return nullptr;
// Ensure all Children are RootStmts.
return llvm::all_of(Parent->Children, isRootStmt) ? Parent : nullptr;
}
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
index edad883950071..00e549d4e0f88 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -653,6 +653,29 @@ TEST_F(ExtractFunctionTest, SingleStatement) {
[[stream << "x";]]
})cpp"),
HasSubstr("extracted"));
+ // Selecting a subexpression of an operator call (rather than the whole
+ // statement) must not be extracted: it is not a standalone statement, and
+ // "extracting" it would replace only part of the expression.
+ EXPECT_EQ(apply(R"cpp(
+ struct Stream {};
+ Stream &operator<<(Stream &, int);
+ Stream stream;
+ void test() {
+ stream << [[3]];
+ })cpp"),
+ "unavailable");
+ // Same as above, but the selected subexpression is itself an
+ // (Unselected-but-fully-covered) CXXOperatorCallExpr nested as an argument
+ // of an outer call, rather than a Complete leaf expression.
+ EXPECT_EQ(apply(R"cpp(
+ struct Stream {};
+ Stream &operator<<(Stream &, int);
+ void foo(Stream &, int);
+ Stream stream;
+ void test() {
+ foo([[stream << 3]], 4);
+ })cpp"),
+ "unavailable");
}
} // namespace
More information about the cfe-commits
mailing list