[clang-tools-extra] [clangd] Do not offer extraction to variable for decl init expression (PR #69477)
Christian Kandeler via cfe-commits
cfe-commits at lists.llvm.org
Thu Jan 11 02:42:11 PST 2024
https://github.com/ckandeler updated https://github.com/llvm/llvm-project/pull/69477
>From 06666ad973446c970e110f1b9c1213e97a7d3da8 Mon Sep 17 00:00:00 2001
From: Christian Kandeler <christian.kandeler at qt.io>
Date: Wed, 18 Oct 2023 17:24:04 +0200
Subject: [PATCH] [clangd] Do not offer extraction to variable for decl init
expression
That would turn:
int x = f() + 1;
into:
auto placeholder = f() + 1;
int x = placeholder;
which makes little sense and is clearly not intended, as stated
explicitly by a comment in eligibleForExtraction(). It appears that the
declaration case was simply forgotten (the assignment case was already
implemented).
---
.../refactor/tweaks/ExtractVariable.cpp | 44 +++++++++++++++++--
.../unittests/tweaks/ExtractVariableTests.cpp | 27 +++++++-----
2 files changed, 55 insertions(+), 16 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
index 79bf962544242b..3b378153eafd52 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
@@ -468,7 +468,8 @@ bool eligibleForExtraction(const SelectionTree::Node *N) {
// Extracting Exprs like a = 1 gives placeholder = a = 1 which isn't useful.
// FIXME: we could still hoist the assignment, and leave the variable there?
ParsedBinaryOperator BinOp;
- if (BinOp.parse(*N) && BinaryOperator::isAssignmentOp(BinOp.Kind))
+ bool IsBinOp = BinOp.parse(*N);
+ if (IsBinOp && BinaryOperator::isAssignmentOp(BinOp.Kind))
return false;
const SelectionTree::Node &OuterImplicit = N->outerImplicit();
@@ -483,13 +484,48 @@ bool eligibleForExtraction(const SelectionTree::Node *N) {
OuterImplicit.ASTNode.get<Expr>()))
return false;
+ std::function<bool(const SelectionTree::Node *)> IsFullySelected =
+ [&](const SelectionTree::Node *N) {
+ if (N->ASTNode.getSourceRange().isValid() &&
+ N->Selected != SelectionTree::Complete)
+ return false;
+ for (const auto *Child : N->Children) {
+ if (!IsFullySelected(Child))
+ return false;
+ }
+ return true;
+ };
+ auto ExprIsFullySelectedTargetNode = [&](const Expr *E) {
+ if (E != OuterImplicit.ASTNode.get<Expr>())
+ return false;
+
+ // The above condition is the only relevant one except for binary operators.
+ // Without the following code, we would fail to offer extraction for e.g.:
+ // int x = 1 + 2 + [[3 + 4 + 5]];
+ // See the documentation of ParsedBinaryOperator for further details.
+ if (!IsBinOp)
+ return true;
+ return IsFullySelected(N);
+ };
+
// Disable extraction of full RHS on assignment operations, e.g:
- // auto x = [[RHS_EXPR]];
+ // x = [[RHS_EXPR]];
// This would just result in duplicating the code.
if (const auto *BO = Parent->ASTNode.get<BinaryOperator>()) {
- if (BO->isAssignmentOp() &&
- BO->getRHS() == OuterImplicit.ASTNode.get<Expr>())
+ if (BO->isAssignmentOp() && ExprIsFullySelectedTargetNode(BO->getRHS()))
+ return false;
+ }
+
+ // The same logic as for assignments applies to initializations.
+ // However, we do allow extracting the RHS of an init capture, as it is
+ // a valid use case to move non-trivial expressions out of the capture clause.
+ // FIXME: In that case, the extracted variable should be captured directly,
+ // rather than an explicit copy.
+ if (const auto *Decl = Parent->ASTNode.get<VarDecl>()) {
+ if (!Decl->isInitCapture() &&
+ ExprIsFullySelectedTargetNode(Decl->getInit())) {
return false;
+ }
}
return true;
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
index eb5b06cfee4316..42dd612eeeec46 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
@@ -27,10 +27,10 @@ TEST_F(ExtractVariableTest, Test) {
return [[[[t.b[[a]]r]]([[t.z]])]];
}
void f() {
- int a = [[5 +]] [[4 * [[[[xyz]]()]]]];
+ int a = 5 + [[4 * [[[[xyz]]()]]]];
// multivariable initialization
if(1)
- int x = [[1]], y = [[a + 1]], a = [[1]], z = a + 1;
+ int x = [[1]] + 1, y = a + [[1]], a = [[1]] + 2, z = a + 1;
// if without else
if([[1]])
a = [[1]] + 1;
@@ -61,7 +61,7 @@ TEST_F(ExtractVariableTest, Test) {
ExtraArgs = {"-xc"};
const char *AvailableC = R"cpp(
void foo() {
- int x = [[1]];
+ int x = [[1]] + 1;
})cpp";
EXPECT_AVAILABLE(AvailableC);
@@ -79,7 +79,7 @@ TEST_F(ExtractVariableTest, Test) {
@end
@implementation Foo
- (void)method {
- int x = [[1 + 2]];
+ int x = [[1]] + 2;
}
@end)cpp";
EXPECT_AVAILABLE(AvailableObjC);
@@ -103,6 +103,9 @@ TEST_F(ExtractVariableTest, Test) {
}
int z = [[1]];
} t;
+ int x = [[1 + 2]];
+ int y;
+ y = [[1 + 2]];
return [[t]].bar([[t]].z);
}
void v() { return; }
@@ -430,8 +433,8 @@ TEST_F(ExtractVariableTest, Test) {
int member = 42;
};
)cpp"},
- {R"cpp(void f() { auto x = [[ [](){ return 42; }]]; })cpp",
- R"cpp(void f() { auto placeholder = [](){ return 42; }; auto x = placeholder; })cpp"},
+ {R"cpp(void f() { auto x = +[[ [](){ return 42; }]]; })cpp",
+ R"cpp(void f() { auto placeholder = [](){ return 42; }; auto x = + placeholder; })cpp"},
{R"cpp(
template <typename T>
auto sink(T f) { return f(); }
@@ -515,13 +518,13 @@ TEST_F(ExtractVariableTest, Test) {
{R"cpp(
template <typename ...Ts>
void foo(Ts ...args) {
- auto x = [[ [&args...]() {} ]];
+ auto x = +[[ [&args...]() {} ]];
}
)cpp",
R"cpp(
template <typename ...Ts>
void foo(Ts ...args) {
- auto placeholder = [&args...]() {}; auto x = placeholder ;
+ auto placeholder = [&args...]() {}; auto x = + placeholder ;
}
)cpp"},
{R"cpp(
@@ -533,7 +536,7 @@ TEST_F(ExtractVariableTest, Test) {
int main() {
Coordinates c = {};
const auto [x, y] = c;
- auto f = [[ [&]() { return x + y; } ]];
+ auto f = [[ [&]() { return x + y; } ]]();
}
)cpp",
R"cpp(
@@ -545,7 +548,7 @@ TEST_F(ExtractVariableTest, Test) {
int main() {
Coordinates c = {};
const auto [x, y] = c;
- auto placeholder = [&]() { return x + y; }; auto f = placeholder ;
+ auto placeholder = [&]() { return x + y; }; auto f = placeholder ();
}
)cpp"},
{R"cpp(
@@ -557,7 +560,7 @@ TEST_F(ExtractVariableTest, Test) {
int main() {
Coordinates c = {};
if (const auto [x, y] = c; x > y) {
- auto f = [[ [&]() { return x + y; } ]];
+ auto f = [[ [&]() { return x + y; } ]]();
}
}
)cpp",
@@ -570,7 +573,7 @@ TEST_F(ExtractVariableTest, Test) {
int main() {
Coordinates c = {};
if (const auto [x, y] = c; x > y) {
- auto placeholder = [&]() { return x + y; }; auto f = placeholder ;
+ auto placeholder = [&]() { return x + y; }; auto f = placeholder ();
}
}
)cpp"},
More information about the cfe-commits
mailing list