[clangd] Do not offer extraction to variable for decl init expression

[ckandeler]

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).

[llvmbot]

@llvm/pr-subscribers-clangd

Author: None (ckandeler)

Changes

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).

---

Full diff: https://github.com/llvm/llvm-project/pull/69477.diff

2 Files Affected:

• (modified) clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp (+9-1)
• (modified) clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp (+3-67)

diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
index 1e4edc6d6b4bb39..26fa94ded1f012e 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
@@ -467,7 +467,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();
@@ -490,6 +491,13 @@ bool eligibleForExtraction(const SelectionTree::Node *N) {
         BO->getRHS() == OuterImplicit.ASTNode.get<Expr>())
       return false;
   }
+  if (const auto *Decl = Parent->ASTNode.get<VarDecl>()) {
+    if (!Decl->isInitCapture() &&
+        Decl->getInit() == OuterImplicit.ASTNode.get<Expr>() &&
+        (!IsBinOp || !BinOp.associative())) {
+      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 3d74a941071f849..7ca717747e49a7b 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);
   ExtraArgs = {"-xobjective-c"};
@@ -422,8 +422,6 @@ 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(
         template <typename T>
         auto sink(T f) { return f(); }
@@ -504,68 +502,6 @@ TEST_F(ExtractVariableTest, Test) {
         int main() {
           auto placeholder = []() { return 42; }(); return  placeholder ;
         })cpp"},
-      {R"cpp(
-        template <typename ...Ts>
-        void foo(Ts ...args) {
-          auto x = [[ [&args...]() {} ]];
-        }
-      )cpp",
-       R"cpp(
-        template <typename ...Ts>
-        void foo(Ts ...args) {
-          auto placeholder = [&args...]() {}; auto x =  placeholder ;
-        }
-      )cpp"},
-      {R"cpp(
-        struct Coordinates {
-          int x{};
-          int y{};
-        };
-
-        int main() {
-          Coordinates c = {};
-          const auto [x, y] = c;
-          auto f = [[ [&]() { return x + y; } ]];
-        }
-        )cpp",
-       R"cpp(
-        struct Coordinates {
-          int x{};
-          int y{};
-        };
-
-        int main() {
-          Coordinates c = {};
-          const auto [x, y] = c;
-          auto placeholder = [&]() { return x + y; }; auto f =  placeholder ;
-        }
-        )cpp"},
-      {R"cpp(
-        struct Coordinates {
-          int x{};
-          int y{};
-        };
-
-        int main() {
-          Coordinates c = {};
-          if (const auto [x, y] = c; x > y) {
-            auto f = [[ [&]() { return x + y; } ]];
-          }
-        }
-        )cpp",
-       R"cpp(
-        struct Coordinates {
-          int x{};
-          int y{};
-        };
-
-        int main() {
-          Coordinates c = {};
-          if (const auto [x, y] = c; x > y) {
-            auto placeholder = [&]() { return x + y; }; auto f =  placeholder ;
-          }
-        }
-        )cpp"},
       // Don't try to analyze across macro boundaries
       // FIXME: it'd be nice to do this someday (in a safe way)
       {R"cpp(#define ECHO(X) X

[ckandeler]

ping

[ckandeler]

ping

[ckandeler]

The latest patch set addresses all the comments.

[ckandeler]

ping

[kadircet]

thanks, lgtm!