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