[clangd] Do not offer extraction to variable for decl init expression (#69477)

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

Author: ckandeler

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;

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"},