[clang-tools-extra] 63b5d13 - [clangd] Don't offer Extract to Function for control-flow conditions (#223409)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Sep 22 07:49:03 PDT 2026
Author: Christian Kandeler
Date: 2026-09-22T16:48:57+02:00
New Revision: 63b5d1388f6ca68954ad22bc36fb73613507ceb1
URL: https://github.com/llvm/llvm-project/commit/63b5d1388f6ca68954ad22bc36fb73613507ceb1
DIFF: https://github.com/llvm/llvm-project/commit/63b5d1388f6ca68954ad22bc36fb73613507ceb1.diff
LOG: [clangd] Don't offer Extract to Function for control-flow conditions (#223409)
Extracting the condition of an `if`/`while`/`do`/`for`/`switch` (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, e.g.:
```cpp
void extracted(int &event1, bool &event2, double &event3) {
event1 == 2 && event2 && event3 == 10.3;
}
int example(int event1, bool event2, double event3) {
if (extracted(event1, event2, event3)) // doesn't compile
return 1;
```
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 in #219945 to allow extracting genuine
expression-statements. This adds back a narrower, correctly-scoped
check instead: reject when the selected node occupies a
condition slot specifically, which also finally
resolves two long-standing FIXMEs about this exact gap.
Assisted-by: Claude
Added:
Modified:
clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
index 479e3e7724b49..a68b48536a22b 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -104,11 +104,49 @@ bool isUnselectedRootStmtCandidate(const Node *N) {
return N->ASTNode.get<DeclStmt>() || N->ASTNode.get<CXXOperatorCallExpr>();
}
+// 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->getConditionVariableDeclStmt();
+ if (const auto *For = llvm::dyn_cast<ForStmt>(Parent))
+ return Child == For->getCond() ||
+ 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->getConditionVariableDeclStmt();
+ if (const auto *ForRange = llvm::dyn_cast<CXXForRangeStmt>(Parent))
+ return Child == ForRange->getRangeInit();
+ 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 +154,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 (isConditionClause(Parent, S))
+ return false;
return true;
}
@@ -337,8 +378,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..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"));
- // FIXME: ExtractFunction should be unavailable inside loop construct
- // initializer/condition.
+ // 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
@@ -678,6 +686,68 @@ 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' 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) { switch ([[x + 1]]) {} }"), "unavailable");
+ // A condition-variable declaration (`if (T x = ...)`) is likewise not a
+ // 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) 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