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 <[email protected]> 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 7cb70b834af6ef5c263edd23baedc402ab76db59 Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> 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 | 39 +++++++++++++------ .../unittests/tweaks/ExtractFunctionTests.cpp | 23 +++++++++++ 2 files changed, 50 insertions(+), 12 deletions(-) diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp index 624b11094ef09..479e3e7724b49 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. @@ -154,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; } 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 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
