https://github.com/ckandeler updated https://github.com/llvm/llvm-project/pull/223409
>From 75a12228620def0500ebf69a04ea816c118e4761 Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> Date: Mon, 14 Sep 2026 15:26:09 +0200 Subject: [PATCH 1/2] [clangd] Don't offer Extract to Function for control-flow conditions Extracting the condition of an if/while/do/for/switch (or a for/range- for's init/increment clause, or a condition-variable declaration) was treated the same as extracting a discardable expression-statement, producing a void-returning function called where the construct needs a value. This slot was previously unreachable in practice because a blanket "never extract a single Expr" check happened to also block it, but that check was removed to allow extracting genuine expression- statements. Add back a narrower, correctly-scoped check instead: reject when the selected node occupies a condition/init/increment slot specifically, which also resolves a long-standing FIXME about this exact gap. Assisted-by: Claude --- .../refactor/tweaks/ExtractFunction.cpp | 40 +++++++++++++++++-- .../unittests/tweaks/ExtractFunctionTests.cpp | 34 ++++++++++++++-- 2 files changed, 68 insertions(+), 6 deletions(-) diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp index 479e3e7724b49..6ebf35506ef48 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp @@ -104,11 +104,44 @@ bool isUnselectedRootStmtCandidate(const Node *N) { return N->ASTNode.get<DeclStmt>() || N->ASTNode.get<CXXOperatorCallExpr>(); } +// Whether Child is the condition, init-statement, increment, or +// condition-variable declaration of a control-flow Parent, as opposed to its +// "body" (then/else/loop-body/switch-body) -- the only slot(s) that behave +// like genuine statement positions. The value of a condition (or the side +// effect of an init/increment clause) is consumed by the construct itself, +// so treating it as a discardable statement and replacing it with a call to +// an extracted function would either not compile (if a value is expected, +// e.g. an `if` condition) or silently change what the code does. +bool isConditionOrInitClause(const Stmt *Parent, const Stmt *Child) { + if (const auto *If = llvm::dyn_cast<IfStmt>(Parent)) + return Child == If->getCond() || Child == If->getInit() || + Child == If->getConditionVariableDeclStmt(); + if (const auto *For = llvm::dyn_cast<ForStmt>(Parent)) + return Child == For->getCond() || Child == For->getInit() || + Child == For->getInc() || + Child == For->getConditionVariableDeclStmt(); + if (const auto *While = llvm::dyn_cast<WhileStmt>(Parent)) + return Child == While->getCond() || + Child == While->getConditionVariableDeclStmt(); + if (const auto *Do = llvm::dyn_cast<DoStmt>(Parent)) + return Child == Do->getCond(); + if (const auto *Switch = llvm::dyn_cast<SwitchStmt>(Parent)) + return Child == Switch->getCond() || Child == Switch->getInit() || + Child == Switch->getConditionVariableDeclStmt(); + if (const auto *ForRange = llvm::dyn_cast<CXXForRangeStmt>(Parent)) + return Child == ForRange->getInit() || Child == ForRange->getCond() || + Child == ForRange->getInc() || Child == ForRange->getBeginStmt() || + Child == ForRange->getEndStmt() || + Child == ForRange->getLoopVarStmt(); + return false; +} + // 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. bool isRootStmt(const Node *N) { - if (!N->ASTNode.get<Stmt>()) + const Stmt *S = N->ASTNode.get<Stmt>(); + if (!S) return false; // Root statement cannot be partially selected. if (N->Selected == SelectionTree::Partial) @@ -116,6 +149,9 @@ bool isRootStmt(const Node *N) { if (N->Selected == SelectionTree::Unselected && !isUnselectedRootStmtCandidate(N)) return false; + if (const Stmt *Parent = N->Parent ? N->Parent->ASTNode.get<Stmt>() : nullptr) + if (isConditionOrInitClause(Parent, S)) + return false; return true; } @@ -337,8 +373,6 @@ bool validSingleChild(const Node *Child, const FunctionDecl *EnclosingFunc) { return true; } -// FIXME: Check we're not extracting from the initializer/condition of a control -// flow structure. std::optional<ExtractionZone> findExtractionZone(const Node *CommonAnc, const SourceManager &SM, const LangOptions &LangOpts) { diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp index 00e549d4e0f88..d012eebb03073 100644 --- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp @@ -36,9 +36,9 @@ TEST_F(ExtractFunctionTest, FunctionTest) { // Ensure that end of Zone and Beginning of PostZone being adjacent doesn't // lead to break being included in the extraction zone. EXPECT_THAT(apply("for(;;) { [[int x;]]break; }"), HasSubstr("extracted")); - // FIXME: ExtractFunction should be unavailable inside loop construct - // initializer/condition. - EXPECT_THAT(apply(" for([[int i = 0;]];);"), HasSubstr("extracted")); + // ExtractFunction is unavailable inside a loop construct's + // initializer/condition/increment. + EXPECT_THAT(apply(" for([[int i = 0;]];);"), HasSubstr("unavailable")); // Extract certain return EXPECT_THAT(apply(" if(true) [[{ return; }]] "), HasSubstr("extracted")); // Don't extract uncertain return @@ -678,6 +678,34 @@ TEST_F(ExtractFunctionTest, SingleStatement) { "unavailable"); } +TEST_F(ExtractFunctionTest, ControlFlowConditions) { + Context = File; + // The condition of an `if` is not a discardable statement -- its value is + // consumed by the `if` itself. + EXPECT_EQ(apply(R"cpp( + int example(int event1, bool event2, double event3) { + if ([[event1 == 2 && event2 && event3 == 10.3]]) + return 1; + return 0; + })cpp"), + "unavailable"); + // Same, but for other control-flow constructs' condition/init/increment + // clauses. + EXPECT_EQ(apply("void f(int x) { while ([[x > 0]]) --x; }"), "unavailable"); + EXPECT_EQ(apply("void f(int x) { do {} while ([[x > 0]]); }"), "unavailable"); + EXPECT_EQ(apply("void f(int x) { for (; [[x > 0]];) ; }"), "unavailable"); + EXPECT_EQ(apply("void f(int x) { for (;; [[--x]]) ; }"), "unavailable"); + EXPECT_EQ(apply("void f(int x) { switch ([[x + 1]]) {} }"), "unavailable"); + // A condition-variable declaration (`if (T x = ...)`) is likewise not a + // discardable statement. + EXPECT_EQ(apply("bool cond(); void f() { if ([[bool b = cond()]]) ; }"), + "unavailable"); + // Sanity check: extraction from the *body* of these constructs (as opposed + // to their condition/init/increment) is unaffected. + EXPECT_THAT(apply("void f(int x) { if (x > 0) [[x = x * 2;]] }"), + HasSubstr("extracted")); +} + } // namespace } // namespace clangd } // namespace clang >From ad5d0b8a7f052b41a12ae0590b76cc4aed00fa5a Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> Date: Tue, 22 Sep 2026 15:09:27 +0200 Subject: [PATCH 2/2] [clangd] Address review: only block the condition, not init/increment A loop's init-statement and increment clause don't have the same problem as the condition: their value is discarded just like an ordinary expression-statement (e.g. `for (global_var = 0; ; func())`), so there's no reason to block extracting them. Any hazard from extracting a *declaration* that's used later (e.g. `for (int i = 0; i < 10; ++i) use(i);`) is already caught independently by ExtractionZone::requiresHoisting() in prepare(), regardless of which clause it's in. Narrow isConditionOrInitClause (renamed isConditionClause) down to just the condition and condition-variable declaration, for if/for/while/do/switch. Also, CXXForRangeStmt handling didn't actually work for the case it was meant to protect: `for (auto X : [[V]])` was still offered. clangd's SelectionTree has a custom TraverseCXXForRangeStmt override that visits only the init-statement, loop variable, range-expression, and body of a range-based for -- the compiler-synthesized condition/increment/begin/end (which the previous checks targeted) never become SelectionTree nodes at all, so those checks were dead code. The actual user-visible, problematic slot is the range-expression itself (its value is consumed to build the hidden begin/end iterators), checked via getRangeInit() instead. Assisted-by: Claude --- .../refactor/tweaks/ExtractFunction.cpp | 41 +++++++------ .../unittests/tweaks/ExtractFunctionTests.cpp | 58 ++++++++++++++++--- 2 files changed, 73 insertions(+), 26 deletions(-) diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp index 6ebf35506ef48..a68b48536a22b 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp @@ -104,21 +104,29 @@ bool isUnselectedRootStmtCandidate(const Node *N) { return N->ASTNode.get<DeclStmt>() || N->ASTNode.get<CXXOperatorCallExpr>(); } -// Whether Child is the condition, init-statement, increment, or -// condition-variable declaration of a control-flow Parent, as opposed to its -// "body" (then/else/loop-body/switch-body) -- the only slot(s) that behave -// like genuine statement positions. The value of a condition (or the side -// effect of an init/increment clause) is consumed by the construct itself, -// so treating it as a discardable statement and replacing it with a call to -// an extracted function would either not compile (if a value is expected, -// e.g. an `if` condition) or silently change what the code does. -bool isConditionOrInitClause(const Stmt *Parent, const Stmt *Child) { +// Whether Child is the condition (or condition-variable declaration) of a +// control-flow Parent, or the range-expression of a range-based for. These +// are the only slots whose *value* is actually consumed by the construct +// itself -- to decide whether to keep looping/branching, or to build the +// hidden begin/end iterators -- so replacing them with a call to a +// void-returning extracted function would not compile. Other slots, like a +// loop's init-statement or increment expression, have their value discarded +// just like an ordinary expression-statement (and any hazard from +// extracting a declaration that's used later is already caught by +// ExtractionZone::requiresHoisting), so they remain extractable. +// +// For CXXForRangeStmt, only RangeInit is ever reachable here: clangd's +// SelectionTree has a custom traversal for range-based for loops (see +// TraverseCXXForRangeStmt in Selection.cpp) that visits only the +// init-statement, loop variable, range-expression, and body -- the +// compiler-synthesized condition/increment/begin/end never become +// SelectionTree nodes at all. +bool isConditionClause(const Stmt *Parent, const Stmt *Child) { if (const auto *If = llvm::dyn_cast<IfStmt>(Parent)) - return Child == If->getCond() || Child == If->getInit() || + return Child == If->getCond() || Child == If->getConditionVariableDeclStmt(); if (const auto *For = llvm::dyn_cast<ForStmt>(Parent)) - return Child == For->getCond() || Child == For->getInit() || - Child == For->getInc() || + return Child == For->getCond() || Child == For->getConditionVariableDeclStmt(); if (const auto *While = llvm::dyn_cast<WhileStmt>(Parent)) return Child == While->getCond() || @@ -126,13 +134,10 @@ bool isConditionOrInitClause(const Stmt *Parent, const Stmt *Child) { if (const auto *Do = llvm::dyn_cast<DoStmt>(Parent)) return Child == Do->getCond(); if (const auto *Switch = llvm::dyn_cast<SwitchStmt>(Parent)) - return Child == Switch->getCond() || Child == Switch->getInit() || + return Child == Switch->getCond() || Child == Switch->getConditionVariableDeclStmt(); if (const auto *ForRange = llvm::dyn_cast<CXXForRangeStmt>(Parent)) - return Child == ForRange->getInit() || Child == ForRange->getCond() || - Child == ForRange->getInc() || Child == ForRange->getBeginStmt() || - Child == ForRange->getEndStmt() || - Child == ForRange->getLoopVarStmt(); + return Child == ForRange->getRangeInit(); return false; } @@ -150,7 +155,7 @@ bool isRootStmt(const Node *N) { !isUnselectedRootStmtCandidate(N)) return false; if (const Stmt *Parent = N->Parent ? N->Parent->ASTNode.get<Stmt>() : nullptr) - if (isConditionOrInitClause(Parent, S)) + if (isConditionClause(Parent, 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 d012eebb03073..02e99b45cc263 100644 --- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp @@ -36,9 +36,17 @@ TEST_F(ExtractFunctionTest, FunctionTest) { // Ensure that end of Zone and Beginning of PostZone being adjacent doesn't // lead to break being included in the extraction zone. EXPECT_THAT(apply("for(;;) { [[int x;]]break; }"), HasSubstr("extracted")); - // ExtractFunction is unavailable inside a loop construct's - // initializer/condition/increment. - EXPECT_THAT(apply(" for([[int i = 0;]];);"), HasSubstr("unavailable")); + // A loop's initializer has its value discarded just like an ordinary + // statement, so it remains extractable (unlike the condition; see + // ControlFlowConditions below). + EXPECT_THAT(apply(" for([[int i = 0;]];);"), HasSubstr("extracted")); + // ...but if the declared name is used later (in the condition, + // increment, or body), extraction is unavailable regardless -- not + // because of any condition/init-specific logic, but because + // requiresHoisting() (checked in ExtractFunction::prepare(), independent + // of what kind of statement is being extracted) catches it. + EXPECT_EQ(apply("void use(int); for([[int i = 0;]] i < 10; ++i) use(i);"), + "unavailable"); // Extract certain return EXPECT_THAT(apply(" if(true) [[{ return; }]] "), HasSubstr("extracted")); // Don't extract uncertain return @@ -689,23 +697,57 @@ TEST_F(ExtractFunctionTest, ControlFlowConditions) { return 0; })cpp"), "unavailable"); - // Same, but for other control-flow constructs' condition/init/increment - // clauses. + // Same, but for other control-flow constructs' conditions. EXPECT_EQ(apply("void f(int x) { while ([[x > 0]]) --x; }"), "unavailable"); EXPECT_EQ(apply("void f(int x) { do {} while ([[x > 0]]); }"), "unavailable"); EXPECT_EQ(apply("void f(int x) { for (; [[x > 0]];) ; }"), "unavailable"); - EXPECT_EQ(apply("void f(int x) { for (;; [[--x]]) ; }"), "unavailable"); EXPECT_EQ(apply("void f(int x) { switch ([[x + 1]]) {} }"), "unavailable"); // A condition-variable declaration (`if (T x = ...)`) is likewise not a - // discardable statement. + // discardable statement: its truthiness *is* the condition. EXPECT_EQ(apply("bool cond(); void f() { if ([[bool b = cond()]]) ; }"), "unavailable"); + // Unlike the condition, a loop's initializer and increment clauses have + // their value discarded just like an ordinary statement, so they remain + // extractable (any hazard from extracting a declaration used later is + // already caught by ExtractionZone::requiresHoisting, independently of + // this). + EXPECT_THAT(apply("void f(int x) { for ([[x = 0]]; x < 10; ++x) ; }"), + HasSubstr("extracted")); + EXPECT_THAT(apply("void f(int x) { for (;; [[--x]]) ; }"), + HasSubstr("extracted")); + // Likewise, an `if`/`switch` init-statement (C++17) is extractable. + ExtraArgs.push_back("-std=c++17"); + EXPECT_THAT(apply("void f(int x) { if ([[x = 0]]; x > 0) ; }"), + HasSubstr("extracted")); + EXPECT_THAT(apply("void f(int x) { switch ([[x = 0]]; x) {} }"), + HasSubstr("extracted")); // Sanity check: extraction from the *body* of these constructs (as opposed - // to their condition/init/increment) is unaffected. + // to their condition) is unaffected. EXPECT_THAT(apply("void f(int x) { if (x > 0) [[x = x * 2;]] }"), HasSubstr("extracted")); } +TEST_F(ExtractFunctionTest, RangeBasedFor) { + Context = File; + // The range-expression of a range-based for is consumed to build the + // hidden begin/end iterators, so it's not a discardable statement either + // (same category as an ordinary condition). + EXPECT_EQ(apply(R"cpp( + struct Vec { int *begin(); int *end(); }; + Vec V; + void f() { for (auto X : [[V]]) {} } + )cpp"), + "unavailable"); + // Extraction from the body is unaffected. + EXPECT_THAT(apply(R"cpp( + struct Vec { int *begin(); int *end(); }; + Vec V; + void foo(int); + void f() { for (auto X : V) { [[foo(X);]] } } + )cpp"), + HasSubstr("extracted")); +} + } // namespace } // namespace clangd } // namespace clang _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
