llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clangd
Author: Christian Kandeler (ckandeler)
<details>
<summary>Changes</summary>
Selecting just the initializer of a variable declaration, e.g.
`auto A = [[func()]];`, was incorrectly offered for extraction,
producing a void-returning function called where a value is required:
```cpp
void extracted() { func(); }
void f() { auto A = extracted(); }
```
This doesn't compile (auto can't deduce from void; a fixed type would
likewise fail to convert from void).
`getEnclosingStmt()` ascends from the selected node to what should be
its enclosing statement, rejecting the case where that turns out to be
an `Expr` (meaning the selection is a subexpression of a larger
expression, not a genuine statement). A `VarDecl`'s initializer isn't
caught by that check, though: ascending from it lands on the `VarDecl`
itself, which is a `Decl`, not a `Stmt` or `Expr`. Broaden the check to
reject unless the ascended-to parent is a `Stmt` (note `Expr` is itself
a `Stmt` subtype in Clang's AST, so this still needs the separate
`Expr` check to catch the original case).
Assisted-by: Claude
---
Full diff: https://github.com/llvm/llvm-project/pull/225434.diff
2 Files Affected:
- (modified) clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
(+11-5)
- (modified) clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
(+29)
``````````diff
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
index a68b48536a22b..eb7a9faa65a81 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -170,15 +170,21 @@ bool isRootStmt(const Node *N) {
// 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.
+// Returns null if the (possibly DeclStmt-adjusted) parent isn't a "plain"
+// Stmt, or is an Expr: this means Child is merely a subexpression of a
+// larger expression or declaration rather than a genuine standalone
+// statement, and extracting it would produce broken code. This covers two
+// distinct cases:
+// - Parent is an Expr, e.g. selecting just the "3" in `stream << 3;`
+// (Child is a subexpression of a larger expression).
+// - Parent isn't a Stmt at all, e.g. selecting just the "func()" in
+// `auto A = func();` (Child is a VarDecl's initializer, so Parent is
+// that VarDecl -- a Decl, not a Stmt).
const Node *getEnclosingStmt(const Node *Child) {
const Node *Parent = Child->Parent;
if (Parent->ASTNode.get<DeclStmt>())
Parent = Parent->Parent;
- if (Parent->ASTNode.get<Expr>())
+ if (!Parent->ASTNode.get<Stmt>() || Parent->ASTNode.get<Expr>())
return nullptr;
return Parent;
}
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
index 02e99b45cc263..c55fc17ae83e9 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -748,6 +748,35 @@ TEST_F(ExtractFunctionTest, RangeBasedFor) {
HasSubstr("extracted"));
}
+TEST_F(ExtractFunctionTest, VarDeclInitializer) {
+ Context = File;
+ // The initializer of a variable declaration is not a discardable
+ // statement either: its value is required to initialize the variable, so
+ // replacing it with a call to a void-returning extracted function would
+ // not compile. Ascending from the initializer lands on the VarDecl itself (a
+ // Decl, not a Stmt or Expr), which the general "is this a genuine statement"
+ // check must also reject.
+ EXPECT_EQ(apply(R"cpp(
+ int func();
+ void f() { auto A = [[func()]]; }
+ )cpp"),
+ "unavailable");
+ // Same without `auto`: the type doesn't matter, only that a value is
+ // required.
+ EXPECT_EQ(apply(R"cpp(
+ int func();
+ void f() { int A = [[func()]]; }
+ )cpp"),
+ "unavailable");
+ // Sanity check: extracting the whole declaration statement (as opposed to
+ // just its initializer) is unaffected.
+ EXPECT_THAT(apply(R"cpp(
+ int func();
+ void f() { [[int A = func();]] }
+ )cpp"),
+ HasSubstr("extracted"));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
``````````
</details>
https://github.com/llvm/llvm-project/pull/225434
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
