[clang-tools-extra] [clangd] Don't offer Extract to Function for control-flow conditions (PR #223409)
Christian Kandeler via cfe-commits
cfe-commits at lists.llvm.org
Tue Sep 22 06:14:24 PDT 2026
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 <christian.kandeler at qt.io>
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 <christian.kandeler at qt.io>
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
More information about the cfe-commits
mailing list