[Clangd] Fixed ExtractVariable for MemberExprs and Assignment Exprs

[SureYeaah]

Summary

• Modified ExtractVariable to stop extraction for MemberExpr, DeclRefExpr and Assignment Expr
• Fixed unittests

[SureYeaah]

Fixed comment

[kadircet]

clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp

80–81

nit: Could you reduce nesting by early returns and add braces to outer if statements if there are any nested statements left.

clang-tools-extra/clangd/unittests/TweakTests.cpp

304

left out this one ?

307

how come these changes part of that patch?

is it possible that this tweak was changed to not trigger on empty selections, but tests were not updated? If that's the case could you please send these on a different patch and rebase this patch on that one?

307

how come these changes part of that patch?

how come these changes *are* part of that patch?

[SureYeaah]

Removed unrelated changes

[SureYeaah]

clang-tools-extra/clangd/unittests/TweakTests.cpp

304

Since that's an IntegerLiteral, this patch doesn't apply to it?

307

This patch doesn't involve empty selection triggers. Reverted these changes.

[sammccall]

Are you sure we want to disable extraction here, rather than just do the extraction at a higher level?

E.g. if bar(1,2,3, f[[o]]o(4,5)); seems like it should extract the call too foo(4,5), not fail to trigger entirely.

clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp

81

this comment repeats the implementation, but should explain why instead

[kadircet]

clang-tools-extra/clangd/unittests/TweakTests.cpp

304

nvm this one, it was for "empyt selection triggering" case.

328–329

I thought extractor didn't handle this case(missing braces).

What is the extraction in this case? Because if it is

auto dummy = a++;
while (a < 1)
   dummy++;

it is not going to be semantically same.

[SureYeaah]

In D64717#1585512, @sammccall wrote:

Are you sure we want to disable extraction here, rather than just do the extraction at a higher level?

E.g. if bar(1,2,3, f[[o]]o(4,5)); seems like it should extract the call too foo(4,5), not fail to trigger entirely.

Selecting f[[o]]o(4,5) will just extract the foo which would be a DeclRefExpr. We want to extract the CallExpr for which we would need to select the brackets as well.

[SureYeaah]

clang-tools-extra/clangd/unittests/TweakTests.cpp

328–329

We don't check for missing braces and continuing traversing up the AST until we find the CompoundStmt.

Yes, it's not going to be semantically the same. We only check if after extraction, the extracted expression will reference a variable that hasn't been declared till that point.

[kadircet]

clang-tools-extra/clangd/unittests/TweakTests.cpp

328–329

I believe this behavior is not expected and we should address that as well, the expressions you extract might have weird side effects, therefore if some expression is within a loop body it should stay there.
No need to address in this patch though, could you (leave a fixme || create a bug) to make sure extraction of such expressions doesn't go to outer scope, but rather stays within the loop body?

[SureYeaah]

Added comments

[sammccall]

In D64717#1585542, @SureYeaah wrote:

In D64717#1585512, @sammccall wrote:

Are you sure we want to disable extraction here, rather than just do the extraction at a higher level?

E.g. if bar(1,2,3, f[[o]]o(4,5)); seems like it should extract the call too foo(4,5), not fail to trigger entirely.

Selecting f[[o]]o(4,5) will just extract the foo which would be a DeclRefExpr. We want to extract the CallExpr for which we would need to select the brackets as well.

I agree, but this patch doesn't do that. (I think?)
Instead it bans extraction entirely in this case.

[SureYeaah]

In D64717#1585632, @sammccall wrote:

In D64717#1585542, @SureYeaah wrote:

In D64717#1585512, @sammccall wrote:

Are you sure we want to disable extraction here, rather than just do the extraction at a higher level?

E.g. if bar(1,2,3, f[[o]]o(4,5)); seems like it should extract the call too foo(4,5), not fail to trigger entirely.

Selecting f[[o]]o(4,5) will just extract the foo which would be a DeclRefExpr. We want to extract the CallExpr for which we would need to select the brackets as well.

I agree, but this patch doesn't do that. (I think?)
Instead it bans extraction entirely in this case.

Yes it doesn't. Our current model doesn't extend the user selection and as such we only check the commonAncestor of whatever the user has selected. Should this be changed then?

[sammccall]

In D64717#1585682, @SureYeaah wrote:

In D64717#1585632, @sammccall wrote:

In D64717#1585542, @SureYeaah wrote:

In D64717#1585512, @sammccall wrote:

Are you sure we want to disable extraction here, rather than just do the extraction at a higher level?

E.g. if bar(1,2,3, f[[o]]o(4,5)); seems like it should extract the call too foo(4,5), not fail to trigger entirely.

Selecting f[[o]]o(4,5) will just extract the foo which would be a DeclRefExpr. We want to extract the CallExpr for which we would need to select the brackets as well.

I agree, but this patch doesn't do that. (I think?)
Instead it bans extraction entirely in this case.

Yes it doesn't. Our current model doesn't extend the user selection and as such we only check the commonAncestor of whatever the user has selected. Should this be changed then?

Yes I think you want to walk up the tree like you do for finding the target stmt, giving up when you hit a non-expr.

This didn't matter prior to this patch as void subexprs are extremely rare. But declrefs are not!

[SureYeaah]

Added fix for selecting the callExpr of a MemberExpr/Function DeclRefExpr

[SureYeaah]

Fixed wrong function name

[SureYeaah]

Fixed crash

[kadircet]

clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp

198

comment seems to be repeating the code, instead lets comment about the assumptions, like not working on declrefexpr's etc.

214

maybe instead of checking internals like Target just make computeExtractionContext return an llvm::Error and check for success?

233

I don't think traversing the tree without any conditions is a good idea, for example:

struct Foo {
int bar(int);
int z;
};

void foo() {
Foo f;
f.bar([[f.z]]);
}

above selection is on a memberexpr, that has a parent membercallexpr but it is actually not related to selection at all but I believe we'll extract the whole thing in that case.
I don't think that is the intended behavior, could you also add a test case for that?

275

nit: braces

278

nit: instead of checking success check failure and bail out?

if(!TargetNode || !canBeass...)
  return;

[SureYeaah]

clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp

214

Should it instead return a bool since there's actually no error?

[SureYeaah]

Refactored code

[kadircet]

clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp

214

Yes that's also plausible

296

return Target->InsertionPoint?

clang-tools-extra/clangd/unittests/TweakTests.cpp

304

another case on bar ?

[[[[t.b[[a]]r]](t.z)]]

350

also on t:

return [[t]].bar([[[[t]].z]]);

[SureYeaah]

Fixed tests

[SureYeaah]

clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp

296

Changed to checking if target is extractable.

[SureYeaah]

Merged with trigger on non empty selection only

[kadircet]

clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp

214

i was trying to say:

having a function named computeExtractionContext that will require you to check some other state even when it returns true sounds like a bad idea.

296

any reasons for not returning Target->isExtractable() ?

clang-tools-extra/clangd/unittests/TweakTests.cpp

304

i suppose this is done

427–445

I think auto dummy = 1; should be within the label. otherwise we might break codes like(which is basically anything with labels):

void foo() {
goto label;
label:
 a = 1;
}

I don't think it is that important though, and have no idea about the effort necessary feel free to just add a FIXME if it turns out hard.

[SureYeaah]

Minor changes and disabled extraction from label statement

[kadircet]

Thanks, LGTM from my side. Do you have any concerns @sammccall ?

clang-tools-extra/clangd/unittests/TweakTests.cpp

427–445

could you keep the previous test case within a checkNotAvailable block ?

[SureYeaah]

Added test for label

[SureYeaah]

clang-tools-extra/clangd/unittests/TweakTests.cpp

427–445

I've disabled extraction from label statements and added a fixme because the fix for all kinds of loops as well as if statement needs another patch.

clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp

 //===--- ExtractVariable.cpp ------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #include "ClangdUnit.h"
 #include "Logger.h"
 #include "Protocol.h"
 #include "Selection.h"
 #include "SourceCode.h"
 #include "refactor/Tweak.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Expr.h"
+#include "clang/AST/ExprCXX.h"
 #include "clang/AST/OperationKinds.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/StmtCXX.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Core/Replacement.h"
bool VisitDeclRefExpr(DeclRefExpr *DeclRef) { // NOLINT
       ReferencedDecls.push_back(DeclRef->getDecl());
       return true;
     }
   };
   FindDeclRefsVisitor Visitor;
   Visitor.TraverseStmt(const_cast<Stmt *>(dyn_cast<Stmt>(Expr)));
   return Visitor.ReferencedDecls;
 }
 
-// An expr is not extractable if it's null or an expression of type void
-// FIXME: Ignore assignment (a = 1) Expr since it is extracted as dummy = a =
-static bool isExtractableExpr(const clang::Expr *Expr) {
-  if (Expr) {
-    const Type *ExprType = Expr->getType().getTypePtrOrNull();
-    // FIXME: check if we need to cover any other types
-    if (ExprType)
-      return !ExprType->isVoidType();
-  }
-  return false;
-}
-
 ExtractionContext::ExtractionContext(const SelectionTree::Node *Node,
                                      const SourceManager &SM,
                                      const ASTContext &Ctx)
     : ExprNode(Node), SM(SM), Ctx(Ctx) {
   Expr = Node->ASTNode.get<clang::Expr>();
-  if (isExtractableExpr(Expr)) {
-    ReferencedDecls = computeReferencedDecls(Expr);
-    InsertionPoint = computeInsertionPoint();
-    if (InsertionPoint)
-      Extractable = true;
-  }
-}
 
 // checks whether extracting before InsertionPoint will take a
 // variable reference out of scope
 bool ExtractionContext::exprIsValidOutside(const clang::Stmt *Scope) const {
   SourceLocation ScopeBegin = Scope->getBeginLoc();
   SourceLocation ScopeEnd = Scope->getEndLoc();
   for (const Decl *ReferencedDecl : ReferencedDecls) {
     if (SM.isPointWithin(ReferencedDecl->getBeginLoc(), ScopeBegin, ScopeEnd) &&
         SM.isPointWithin(ReferencedDecl->getEndLoc(), ScopeBegin, ScopeEnd))
       return false;
   }
   return true;
 }
 
 // Return the Stmt before which we need to insert the extraction.
 // To find the Stmt, we go up the AST Tree and if the Parent of the current
 // Stmt is a CompoundStmt, we can extract inside this CompoundStmt just before
 // the current Stmt. We ALWAYS insert before a Stmt whose parent is a
 // CompoundStmt
 //
-
-// FIXME: Extraction from switch and case statements
+// FIXME: Extraction from label, switch and case statements
 // FIXME: Doens't work for FoldExpr
+// FIXME: Ensure extraction from loops doesn't change semantics.
 const clang::Stmt *ExtractionContext::computeInsertionPoint() const {
   // returns true if we can extract before InsertionPoint
   auto CanExtractOutside =
       [](const SelectionTree::Node *InsertionPoint) -> bool {
     if (const clang::Stmt *Stmt = InsertionPoint->ASTNode.get<clang::Stmt>()) {
       // Allow all expressions except LambdaExpr since we don't want to extract
       // from the captures/default arguments of a lambda
       if (isa<clang::Expr>(Stmt))
         return !isa<LambdaExpr>(Stmt);
       // We don't yet allow extraction from switch/case stmt as we would need to
       // jump over the switch stmt even if there is a CompoundStmt inside the
       // switch. And there are other Stmts which we don't care about (e.g.
       // continue and break) as there can never be anything to extract from
       // them.
       return isa<AttributedStmt>(Stmt) || isa<CompoundStmt>(Stmt) ||
              isa<CXXForRangeStmt>(Stmt) || isa<DeclStmt>(Stmt) ||
              isa<DoStmt>(Stmt) || isa<ForStmt>(Stmt) || isa<IfStmt>(Stmt) ||
-             isa<LabelStmt>(Stmt) || isa<ReturnStmt>(Stmt) ||
-             isa<WhileStmt>(Stmt);
+             isa<ReturnStmt>(Stmt) || isa<WhileStmt>(Stmt);
     }
     if (InsertionPoint->ASTNode.get<VarDecl>())
       return true;
     return false;
   };
   for (const SelectionTree::Node *CurNode = getExprNode();
        CurNode->Parent && CanExtractOutside(CurNode);
        CurNode = CurNode->Parent) {

 public:
   const char *id() const override final;
   bool prepare(const Selection &Inputs) override;
   Expected<Effect> apply(const Selection &Inputs) override;
   std::string title() const override {
     return "Extract subexpression to variable";
   }
   Intent intent() const override { return Refactor; }
+  // Compute the extraction context for the Selection
+  bool computeExtractionContext(const SelectionTree::Node *N,
+                                const SourceManager &SM, const ASTContext &Ctx);
 
 private:
   // the expression to extract
   std::unique_ptr<ExtractionContext> Target;
 };
 REGISTER_TWEAK(ExtractVariable)
 bool ExtractVariable::prepare(const Selection &Inputs) {
+  // we don't trigger on empty selections for now
+  if (Inputs.SelectionBegin == Inputs.SelectionEnd)
+    return false;
   const ASTContext &Ctx = Inputs.AST.getASTContext();
   const SourceManager &SM = Inputs.AST.getSourceManager();
   const SelectionTree::Node *N = Inputs.ASTSelection.commonAncestor();
-  // we don't trigger on empty selections for now
-  if (!N || Inputs.SelectionBegin == Inputs.SelectionEnd)
-    return false;
-  Target = llvm::make_unique<ExtractionContext>(N, SM, Ctx);
-  return Target->isExtractable();
+  return computeExtractionContext(N, SM, Ctx);
 }
 
 Expected<Tweak::Effect> ExtractVariable::apply(const Selection &Inputs) {
   tooling::Replacements Result;
   // FIXME: get variable name from user or suggest based on type
   std::string VarName = "dummy";
   // insert new variable declaration
   if (auto Err = Result.add(Target->insertDeclaration(VarName)))
     return std::move(Err);
   // replace expression with variable name
   if (auto Err = Result.add(Target->replaceWithVar(VarName)))
     return std::move(Err);
   return Effect::applyEdit(Result);
 }
 
+// Find the CallExpr whose callee is an ancestor of the DeclRef
+const SelectionTree::Node *getCallExpr(const SelectionTree::Node *DeclRef) {
+  // we maintain a stack of all exprs encountered while traversing the
+  // selectiontree because the callee of the callexpr can be an ancestor of the
+  // DeclRef. e.g. Callee can be an ImplicitCastExpr.
+  std::vector<const clang::Expr *> ExprStack;
+  for (auto *CurNode = DeclRef; CurNode; CurNode = CurNode->Parent) {
+    const Expr *CurExpr = CurNode->ASTNode.get<Expr>();
+    if (const CallExpr *CallPar = CurNode->ASTNode.get<CallExpr>()) {
+      // check whether the callee of the callexpr is present in Expr stack.
+      if (std::find(ExprStack.begin(), ExprStack.end(), CallPar->getCallee()) !=
+          ExprStack.end())
+        return CurNode;
+      return nullptr;
+    }
+    ExprStack.push_back(CurExpr);
+  }
+  return nullptr;
+}
+
+// check if Expr can be assigned to a variable i.e. is non-void type
+bool canBeAssigned(const SelectionTree::Node *ExprNode) {
+  const clang::Expr *Expr = ExprNode->ASTNode.get<clang::Expr>();
+  if (const Type *ExprType = Expr->getType().getTypePtrOrNull())
+    // FIXME: check if we need to cover any other types
+    return !ExprType->isVoidType();
+  return true;
+}
+
+// Find the node that will form our ExtractionContext.
+// We don't want to trigger for assignment expressions and variable/field
+// DeclRefs. For function/member function, we want to extract the entire
+// function call.
+bool ExtractVariable::computeExtractionContext(const SelectionTree::Node *N,
+                                               const SourceManager &SM,
+                                               const ASTContext &Ctx) {
+  if (!N)
+    return false;
+  const clang::Expr *SelectedExpr = N->ASTNode.get<clang::Expr>();
+  const SelectionTree::Node *TargetNode = N;
+  if (!SelectedExpr)
+    return false;
+  // Extracting Exprs like a = 1 gives dummy = a = 1 which isn't useful.
+  if (const BinaryOperator *BinOpExpr =
+          dyn_cast_or_null<BinaryOperator>(SelectedExpr)) {
+    if (BinOpExpr->getOpcode() == BinaryOperatorKind::BO_Assign)
+      return false;
+  }
+  // For function and member function DeclRefs, we look for a parent that is a
+  // CallExpr
+  if (const DeclRefExpr *DeclRef =
+          dyn_cast_or_null<DeclRefExpr>(SelectedExpr)) {
+    // Extracting just a variable isn't that useful.
+    if (!isa<FunctionDecl>(DeclRef->getDecl()))
+      return false;
+    TargetNode = getCallExpr(N);
+  }
+  if (const MemberExpr *Member = dyn_cast_or_null<MemberExpr>(SelectedExpr)) {
+    // Extracting just a field member isn't that useful.
+    if (!isa<CXXMethodDecl>(Member->getMemberDecl()))
+      return false;
+    TargetNode = getCallExpr(N);
+  }
+  if (!TargetNode || !canBeAssigned(TargetNode))
+    return false;
+  Target = llvm::make_unique<ExtractionContext>(TargetNode, SM, Ctx);
+  return Target->isExtractable();
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/TweakTests.cpp

TEST(TweakTest, DumpRecordLayout) {
   checkNotAvailable(ID, "struct X { int ^a; };");
   checkNotAvailable(ID, "struct ^X;");
   checkNotAvailable(ID, "template <typename T> struct ^X { T t; };");
   checkNotAvailable(ID, "enum ^X {};");
 
   const char *Input = "struct ^X { int x; int y; }";
   EXPECT_THAT(getMessage(ID, Input), ::testing::HasSubstr("0 |   int x"));
 }
+
 TEST(TweakTest, ExtractVariable) {
   llvm::StringLiteral ID = "ExtractVariable";
   checkAvailable(ID, R"cpp(
-    int xyz() {
+    int xyz(int a = 1) {
+      struct T {
+        int bar(int a = 1);
+        int z;
+      } t;
       // return statement
-      return [[1]];
+      return [[[[t.b[[a]]r]](t.z)]];
     }
     void f() {
       int a = [[5 +]] [[4 * [[[[xyz]]()]]]];
       // multivariable initialization
       if(1)
-        int x = [[1]], y = [[a]] + 1, a = [[1]], z = a + 1;
+        int x = [[1]], y = [[a + 1]], a = [[1]], z = a + 1;
       // if without else
       if([[1]])
         a = [[1]];
       // if with else
       if(a < [[3]])
         if(a == [[4]])
           a = [[5]];
         else
           a = [[5]];
       else if (a < [[4]])
         a = [[4]];
       else
         a = [[5]];
-      // for loop 
+      // for loop
       for(a = [[1]]; a > [[[[3]] + [[4]]]]; a++)
         a = [[2]];
-      // while 
+      // while
       while(a < [[1]])
-        [[a]]++;
-      // do while 
+        [[a++]];
+      // do while
       do
         a = [[1]];
       while(a < [[3]]);
     }
   )cpp");
   // Should not crash.
   checkNotAvailable(ID, R"cpp(
     template<typename T, typename ...Args>
     struct Test<T, Args...> {
-    Test(const T &v) :val(^) {}
+    Test(const T &v) :val[[(^]]) {}
       T val;
     };
   )cpp");
   checkNotAvailable(ID, R"cpp(
     int xyz(int a = [[1]]) {
-      return 1;
-      class T {
-        T(int a = [[1]]) {};
-        int xyz = [[1]];
-      };
+      struct T {
+        int bar(int a = [[1]]);
+        int z = [[1]];
+      } t;
+      return [[t]].bar([[[[t]].z]]);
     }
+    void v() { return; }
     // function default argument
     void f(int b = [[1]]) {
       // empty selection
       int a = ^1 ^+ ^2;
       // void expressions
       auto i = new int, j = new int;
       [[[[delete i]], delete j]];
+      [[v]]();
       // if
       if(1)
         int x = 1, y = a + 1, a = 1, z = [[a + 1]];
       if(int a = 1)
-        if([[a]] == 4)
+        if([[a + 1]] == 4)
           a = [[[[a]] +]] 1;
-      // for loop 
-      for(int a = 1, b = 2, c = 3; [[a]] > [[b + c]]; [[a]]++)
+      // for loop
+      for(int a = 1, b = 2, c = 3; a > [[b + c]]; [[a++]])
         a = [[a + 1]];
-      // lambda 
+      // lambda
       auto lamb = [&[[a]], &[[b]]](int r = [[1]]) {return 1;}
+      // assigment
+      [[a = 5]];
+      // Variable DeclRefExpr
+      a = [[b]];
     }
   )cpp");
   // vector of pairs of input and output strings
   const std::vector<std::pair<llvm::StringLiteral, llvm::StringLiteral>>
       InputOutputs = {
           // extraction from variable declaration/assignment
           {R"cpp(void varDecl() {
                    int a = 5 * (4 + (3 [[- 1)]]);
const std::vector<std::pair<llvm::StringLiteral, llvm::StringLiteral>>
                    if(1)
                     LOOP(5 + [[3]])
                  })cpp",
            R"cpp(#define LOOP(x) {int a = x + 1;}
                  void f(int a) {
                    auto dummy = 3; if(1)
                     LOOP(5 + dummy)
                  })cpp"},
-          // label and attribute testing
+          // attribute testing
           {R"cpp(void f(int a) {
-                    label: [ [gsl::suppress("type")] ] for (;;) a = [[1]];
+                    [ [gsl::suppress("type")] ] for (;;) a = [[1]];
                  })cpp",
            R"cpp(void f(int a) {
-                    auto dummy = 1; label: [ [gsl::suppress("type")] ] for (;;) a = dummy;
+                    auto dummy = 1; [ [gsl::suppress("type")] ] for (;;) a = dummy;
+                 })cpp"},
+          // MemberExpr
+          {R"cpp(class T {
+                   T f() {
+                     return [[T().f()]].f();
+                   }
+                 };)cpp",
+           R"cpp(class T {
+                   T f() {
+                     auto dummy = T().f(); return dummy.f();
+                   }
+                 };)cpp"},
+          // Function DeclRefExpr
+          {R"cpp(int f() {
+                   return [[f]]();
+                 })cpp",
+           R"cpp(int f() {
+                   auto dummy = f(); return dummy;
                  })cpp"},
           // FIXME: Doesn't work because bug in selection tree
           /*{R"cpp(#define PLUS(x) x++
                  void f(int a) {
                    PLUS([[a]]);
                  })cpp",
            R"cpp(#define PLUS(x) x++
                  void f(int a) {
                    auto dummy = a; PLUS(dummy);
                  })cpp"},*/
-          // FIXME: Doesn't work correctly for \[\[clang::uninitialized\]\] int
-          // b = [[1]]; since the attr is inside the DeclStmt and the bounds of
-          // DeclStmt don't cover the attribute
+          // FIXME: Wrong result for \[\[clang::uninitialized\]\] int b = [[1]];
+          // since the attr is inside the DeclStmt and the bounds of
+          // DeclStmt don't cover the attribute.
       };
   for (const auto &IO : InputOutputs) {
     checkTransform(ID, IO.first, IO.second);
   }
 }
 
 TEST(TweakTest, AnnotateHighlightings) {
   llvm::StringLiteral ID = "AnnotateHighlightings";