[clang-tools-extra] 82a7182 - [clangd] Disable extract variable for RHS of assignments

[Kadir Cetinkaya via cfe-commits]

Author: Kadir Cetinkaya
Date: 2020-10-14T14:22:47+02:00
New Revision: 82a71822a54d76c62bf730d8c0e8e86d68c60159

URL: https://github.com/llvm/llvm-project/commit/82a71822a54d76c62bf730d8c0e8e86d68c60159
DIFF: https://github.com/llvm/llvm-project/commit/82a71822a54d76c62bf730d8c0e8e86d68c60159.diff

LOG: [clangd] Disable extract variable for RHS of assignments

Differential Revision: https://reviews.llvm.org/D89307

Added: 
    

Modified: 
    clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
    clang-tools-extra/clangd/unittests/TweakTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
index 8b668be5f2f9..8feef4e84722 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
@@ -382,17 +382,27 @@ bool eligibleForExtraction(const SelectionTree::Node *N) {
   if (BinOp.parse(*N) && BinaryOperator::isAssignmentOp(BinOp.Kind))
     return false;
 
+  const SelectionTree::Node &OuterImplicit = N->outerImplicit();
+  const auto *Parent = OuterImplicit.Parent;
+  if (!Parent)
+    return false;
   // We don't want to extract expressions used as statements, that would leave
   // a `dummy;` around that has no effect.
   // Unfortunately because the AST doesn't have ExprStmt, we have to check in
   // this roundabout way.
-  const SelectionTree::Node &OuterImplicit = N->outerImplicit();
-  if (!OuterImplicit.Parent ||
-      childExprIsStmt(OuterImplicit.Parent->ASTNode.get<Stmt>(),
+  if (childExprIsStmt(Parent->ASTNode.get<Stmt>(),
                       OuterImplicit.ASTNode.get<Expr>()))
     return false;
 
-  // FIXME: ban extracting the RHS of an assignment: `a = [[foo()]]`
+  // Disable extraction of full RHS on assignment operations, e.g:
+  // auto 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>())
+      return false;
+  }
+
   return true;
 }
 

diff  --git a/clang-tools-extra/clangd/unittests/TweakTests.cpp b/clang-tools-extra/clangd/unittests/TweakTests.cpp
index f64d42a7eed7..718b84d03990 100644
--- a/clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -215,26 +215,26 @@ TEST_F(ExtractVariableTest, Test) {
         int x = [[1]], y = [[a + 1]], a = [[1]], z = a + 1;
       // if without else
       if([[1]])
-        a = [[1]];
+        a = [[1]] + 1;
       // if with else
       if(a < [[3]])
         if(a == [[4]])
-          a = [[5]];
+          a = [[5]] + 1;
         else
-          a = [[5]];
+          a = [[5]] + 1;
       else if (a < [[4]])
-        a = [[4]];
+        a = [[4]] + 1;
       else
-        a = [[5]];
+        a = [[5]] + 1;
       // for loop
-      for(a = [[1]]; a > [[[[3]] + [[4]]]]; a++)
-        a = [[2]];
+      for(a = [[1]] + 1; a > [[[[3]] + [[4]]]]; a++)
+        a = [[2]] + 1;
       // while
       while(a < [[1]])
-        a = [[1]];
+        a = [[1]] + 1;
       // do while
       do
-        a = [[1]];
+        a = [[1]] + 1;
       while(a < [[3]]);
     }
   )cpp";
@@ -291,6 +291,7 @@ TEST_F(ExtractVariableTest, Test) {
       xyz([[a *= 5]]);
       // Variable DeclRefExpr
       a = [[b]];
+      a = [[xyz()]];
       // statement expression
       [[xyz()]];
       while (a)
@@ -373,10 +374,10 @@ TEST_F(ExtractVariableTest, Test) {
                  })cpp"},
           // attribute testing
           {R"cpp(void f(int a) {
-                    [ [gsl::suppress("type")] ] for (;;) a = [[1]];
+                    [ [gsl::suppress("type")] ] for (;;) a = [[1]] + 1;
                  })cpp",
            R"cpp(void f(int a) {
-                    auto dummy = 1; [ [gsl::suppress("type")] ] for (;;) a = dummy;
+                    auto dummy = 1; [ [gsl::suppress("type")] ] for (;;) a = dummy + 1;
                  })cpp"},
           // MemberExpr
           {R"cpp(class T {