[clangd] Disallow extraction of expression-statements.

[sammccall]

Summary

I split out the "extract parent instead of this" logic from the "this isn't
worth extracting" logic (now in eligibleForExtraction()), because I found it
hard to reason about.

While here, handle overloaded as well as builtin assignment operators.

Also this uncovered a bug in getCallExpr() which I fixed.

[SureYeaah]

What was the bug in getCallExpr() ?

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

391 ↗

(On Diff #211960)

Remove dump.

445 ↗

(On Diff #211960)

Check if parent is an assignment binaryoperator or a vardecl?

461 ↗

(On Diff #211960)

Can we combine both these IFs and remove the unused assignment?

[SureYeaah]

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

329 ↗

(On Diff #211960)

Change to a=[[1]];?

[sammccall]

In D65337#1604324, @SureYeaah wrote:

What was the bug in getCallExpr() ?

It could find calls where the DeclRef was an arbitrary subexpression of the callee, not exactly the callee (modulo implicit nodes).

It allowed extraction of [[t]].bar(t.z), because it recognized t as a DeclRefExpr (maybe a function we're calling!) and then walked up the stack looking for a call.
It found one, and verified that the callee was the top of the stack (which was [[t.bar]](t.z) at that point).

In the previous version of the code, this was filtered out by one of the redundant checks, but after removing them the problem showed up. Once here I decided to remove the stack (only the top element was ever used) and reuse outerImplicit since we had it anyway and it avoided explicit looping.

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

445 ↗

(On Diff #211960)

Right. I want to keep that out of this patch though: more tests, more changes to existing tests, more risk that it's not exactly what we want (e.g. extraction of *constants* to a named variable might be fine...)

461 ↗

(On Diff #211960)

Done. Also removed the clobbering of TargetNode if there's no call, this decision is already covered by eligibleForExtraction. And then cleaned up some widely-scoped variables and impossible conditions here.

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

329 ↗

(On Diff #211960)

Sure, though the next patch may disallow this too :-)

[sammccall]

Address review comments.

[sammccall]

Add basic test for outerImplicit.

clang-tools-extra/trunk/clangd/Selection.h

struct Node {
     // Walk up the AST to get the DeclContext of this Node,
     // which is not the node itself.
     const DeclContext& getDeclContext() const;
     // Printable node kind, like "CXXRecordDecl" or "AutoTypeLoc".
     std::string kind() const;
     // If this node is a wrapper with no syntax (e.g. implicit cast), return
     // its contents. (If multiple wrappers are present, unwraps all of them).
     const Node& ignoreImplicit() const;
+    // If this node is inside a wrapper with no syntax (e.g. implicit cast),
+    // return that wrapper. (If multiple are present, unwraps all of them).
+    const Node& outerImplicit() const;
   };
   // The most specific common ancestor of all the selected nodes.
   // Returns nullptr if the common ancestor is the root.
   // (This is to avoid accidentally traversing the TUDecl and thus preamble).
   const Node *commonAncestor() const;
   // The selection node corresponding to TranslationUnitDecl.
   const Node &root() const { return *Root; }
 

clang-tools-extra/trunk/clangd/Selection.cpp

 
 const SelectionTree::Node &SelectionTree::Node::ignoreImplicit() const {
   if (Children.size() == 1 &&
       Children.front()->ASTNode.getSourceRange() == ASTNode.getSourceRange())
     return Children.front()->ignoreImplicit();
   return *this;
 }
 
+const SelectionTree::Node &SelectionTree::Node::outerImplicit() const {
+  if (Parent && Parent->ASTNode.getSourceRange() == ASTNode.getSourceRange())
+    return Parent->outerImplicit();
+  return *this;
+}
+
 } // namespace clangd
 } // namespace clang

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

Expected<Tweak::Effect> ExtractVariable::apply(const Selection &Inputs) {
   if (auto Err = Result.add(Target->insertDeclaration(VarName, Range)))
     return std::move(Err);
   // replace expression with variable name
   if (auto Err = Result.add(Target->replaceWithVar(Range, VarName)))
     return std::move(Err);
   return Effect::applyEdit(Result);
 }
 
-// Find the CallExpr whose callee is an ancestor of the DeclRef
+// Find the CallExpr whose callee is the (possibly wrapped) 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);
-  }
+  const SelectionTree::Node &MaybeCallee = DeclRef->outerImplicit();
+  const SelectionTree::Node *MaybeCall = MaybeCallee.Parent;
+  if (!MaybeCall)
+  const CallExpr *CE =
+      llvm::dyn_cast_or_null<CallExpr>(MaybeCall->ASTNode.get<Expr>());
+  if (!CE)
+    return nullptr;
+  if (CE->getCallee() != MaybeCallee.ASTNode.get<Expr>())
     return nullptr;
+  return MaybeCall;
 }
 
-// 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();
+// Returns true if Inner (which is a direct child of Outer) is appearing as
+// a statement rather than an expression whose value can be used.
+bool childExprIsStmt(const Stmt *Outer, const Expr *Inner) {
+  if (!Outer || !Inner)
+    return false;
+  // Blacklist the most common places where an expr can appear but be unused.
+  if (llvm::isa<CompoundStmt>(Outer))
+    return true;
+  if (llvm::isa<SwitchCase>(Outer))
+    return true;
+  // Control flow statements use condition etc, but not the body.
+  if (const auto* WS = llvm::dyn_cast<WhileStmt>(Outer))
+    return Inner == WS->getBody();
+  if (const auto* DS = llvm::dyn_cast<DoStmt>(Outer))
+    return Inner == DS->getBody();
+  if (const auto* FS = llvm::dyn_cast<ForStmt>(Outer))
+    return Inner == FS->getBody();
+  if (const auto* FS = llvm::dyn_cast<CXXForRangeStmt>(Outer))
+    return Inner == FS->getBody();
+  if (const auto* IS = llvm::dyn_cast<IfStmt>(Outer))
+    return Inner == IS->getThen() || Inner == IS->getElse();
+  // Assume all other cases may be actual expressions.
+  // This includes the important case of subexpressions (where Outer is Expr).
+  return false;
+}
+
+// check if N can and should be extracted (e.g. is not void-typed).
+bool eligibleForExtraction(const SelectionTree::Node *N) {
+  const Expr *E = N->ASTNode.get<Expr>();
+  if (!E)
+    return false;
+
+  // Void expressions can't be assigned to variables.
+  if (const Type *ExprType = E->getType().getTypePtrOrNull())
+    if (ExprType->isVoidType())
+      return false;
+
+  // A plain reference to a name (e.g. variable) isn't  worth extracting.
+  // FIXME: really? What if it's e.g. `std::is_same<void, void>::value`?
+  if (llvm::isa<DeclRefExpr>(E) || llvm::isa<MemberExpr>(E))
+    return false;
+
+  // Extracting Exprs like a = 1 gives dummy = 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))
+    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>(),
+                      OuterImplicit.ASTNode.get<Expr>()))
+    return false;
+
+  // FIXME: ban extracting the RHS of an assignment: `a = [[foo()]]`
   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->isAssignmentOp())
-      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))
+  // For function and member function DeclRefs, extract the whole call.
+  if (const Expr *E = N->ASTNode.get<clang::Expr>())
+    if (llvm::isa<DeclRefExpr>(E) || llvm::isa<MemberExpr>(E))
+      if (const SelectionTree::Node *Call = getCallExpr(N))
+        TargetNode = Call;
+  if (!eligibleForExtraction(TargetNode))
     return false;
   Target = llvm::make_unique<ExtractionContext>(TargetNode, SM, Ctx);
   return Target->isExtractable();
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang

clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp

TEST(SelectionTest, Implicit) {
   auto T = makeSelectionTree(Test, AST);
 
   const SelectionTree::Node *Str = T.commonAncestor();
   EXPECT_EQ("StringLiteral", nodeKind(Str)) << "Implicit selected?";
   EXPECT_EQ("ImplicitCastExpr", nodeKind(Str->Parent));
   EXPECT_EQ("CXXConstructExpr", nodeKind(Str->Parent->Parent));
   EXPECT_EQ(Str, &Str->Parent->Parent->ignoreImplicit())
       << "Didn't unwrap " << nodeKind(&Str->Parent->Parent->ignoreImplicit());
+
+  EXPECT_EQ("CXXConstructExpr", nodeKind(&Str->outerImplicit()));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang

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

void f() {
         a = [[4]];
       else
         a = [[5]];
       // for loop
       for(a = [[1]]; a > [[[[3]] + [[4]]]]; a++)
         a = [[2]];
       // while
       while(a < [[1]])
-        [[a++]];
+        a = [[1]];
       // do while
       do
         a = [[1]];
       while(a < [[3]]);
     }
   )cpp");
   // Should not crash.
   checkNotAvailable(ID, R"cpp(
void f(int b = [[1]]) {
         if([[a + 1]] == 4)
           a = [[[[a]] +]] 1;
       // for loop
       for(int a = 1, b = 2, c = 3; a > [[b + c]]; [[a++]])
         a = [[a + 1]];
       // lambda
       auto lamb = [&[[a]], &[[b]]](int r = [[1]]) {return 1;}
       // assigment
-      [[a = 5]];
-      [[a >>= 5]];
-      [[a *= 5]];
+      xyz([[a = 5]]);
+      xyz([[a *= 5]]);
       // Variable DeclRefExpr
       a = [[b]];
+      // statement expression
+      [[xyz()]];
+      while (a)
+        [[++a]];
       // label statement
       goto label;
       label:
         a = [[1]];
     }
   )cpp");
   // vector of pairs of input and output strings
   const std::vector<std::pair<llvm::StringLiteral, llvm::StringLiteral>>
const std::vector<std::pair<llvm::StringLiteral, llvm::StringLiteral>>
                              break;
                            default:
                              break;
                        }
                  })cpp"},*/
           // Macros
           {R"cpp(#define PLUS(x) x++
                  void f(int a) {
-                   PLUS([[1+a]]);
+                   int y = PLUS([[1+a]]);
                  })cpp",
           /*FIXME: It should be extracted like this.
            R"cpp(#define PLUS(x) x++
                  void f(int a) {
                    auto dummy = 1+a; int y = PLUS(dummy);
                  })cpp"},*/
            R"cpp(#define PLUS(x) x++
                  void f(int a) {
-                   auto dummy = PLUS(1+a); dummy;
+                   auto dummy = PLUS(1+a); int y = dummy;
                  })cpp"},
           // ensure InsertionPoint isn't inside a macro
           {R"cpp(#define LOOP(x) while (1) {a = x;}
                  void f(int a) {
                    if(1)
                     LOOP(5 + [[3]])
                  })cpp",
           /*FIXME: It should be extracted like this. SelectionTree needs to be