[clang-tools-extra] 800e7f8 - [clangd] Offer Extract to Function for single expression-statements (#219945)
via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 11 11:12:32 PDT 2026
Author: Christian Kandeler
Date: 2026-09-11T20:12:25+02:00
New Revision: 800e7f8f308196b27cec5880789a1519e0bee70a
URL: https://github.com/llvm/llvm-project/commit/800e7f8f308196b27cec5880789a1519e0bee70a
DIFF: https://github.com/llvm/llvm-project/commit/800e7f8f308196b27cec5880789a1519e0bee70a.diff
LOG: [clangd] Offer Extract to Function for single expression-statements (#219945)
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
Added:
Modified:
clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
clang-tools-extra/docs/ReleaseNotes.md
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
index bc9a790232507..479e3e7724b49 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,16 +113,35 @@ 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;
}
+// 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
@@ -130,7 +158,16 @@ 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 = getEnclosingStmt(CommonAnc);
+ break;
+ }
// Ensure all Children are RootStmts.
Parent = CommonAnc;
break;
@@ -138,17 +175,11 @@ const Node *getParentOfRootStmts(const Node *CommonAnc) {
// Only a fully-selected single statement can be selected.
return nullptr;
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;
}
@@ -298,11 +329,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..00e549d4e0f88 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,54 @@ 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"));
+ // 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
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md
index e865792b05ff5..6fff0359afd91 100644
--- a/clang-tools-extra/docs/ReleaseNotes.md
+++ b/clang-tools-extra/docs/ReleaseNotes.md
@@ -87,6 +87,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
More information about the cfe-commits
mailing list