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 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
