[clangd] Consider expression statements in ExtractVariable tweak

[ckandeler]

For instance:
int func();
int main()
{
func(); // => auto placeholder = func();
}

[llvmbot]

@llvm/pr-subscribers-clangd

@llvm/pr-subscribers-clang-tools-extra

Author: Christian Kandeler (ckandeler)

Changes

For instance:
int func();
int main()
{
func(); // => auto placeholder = func();
}

---

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

2 Files Affected:

• (modified) clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp (+27-7)
• (modified) clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp (+11-1)

diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
index 3b378153eafd52..77eeb3df4773f2 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
@@ -49,7 +49,8 @@ class ExtractionContext {
                                       llvm::StringRef VarName) const;
   // Generate Replacement for declaring the selected Expr as a new variable
   tooling::Replacement insertDeclaration(llvm::StringRef VarName,
-                                         SourceRange InitChars) const;
+                                         SourceRange InitChars,
+                                         bool AddSemicolon) const;
 
 private:
   bool Extractable = false;
@@ -252,7 +253,8 @@ ExtractionContext::replaceWithVar(SourceRange Chars,
 // returns the Replacement for declaring a new variable storing the extraction
 tooling::Replacement
 ExtractionContext::insertDeclaration(llvm::StringRef VarName,
-                                     SourceRange InitializerChars) const {
+                                     SourceRange InitializerChars,
+                                     bool AddSemicolon) const {
   llvm::StringRef ExtractionCode = toSourceCode(SM, InitializerChars);
   const SourceLocation InsertionLoc =
       toHalfOpenFileRange(SM, Ctx.getLangOpts(),
@@ -260,7 +262,9 @@ ExtractionContext::insertDeclaration(llvm::StringRef VarName,
           ->getBegin();
   std::string ExtractedVarDecl =
       printType(VarType, ExprNode->getDeclContext(), VarName) + " = " +
-      ExtractionCode.str() + "; ";
+      ExtractionCode.str();
+  if (AddSemicolon)
+    ExtractedVarDecl += "; ";
   return tooling::Replacement(SM, InsertionLoc, 0, ExtractedVarDecl);
 }
 
@@ -423,8 +427,6 @@ bool childExprIsStmt(const Stmt *Outer, const Expr *Inner) {
   if (!Outer || !Inner)
     return false;
   // Exclude 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.
@@ -516,6 +518,12 @@ bool eligibleForExtraction(const SelectionTree::Node *N) {
       return false;
   }
 
+  // If e.g. a capture clause was selected, the target node is the lambda
+  // expression. We only want to offer the extraction if the entire lambda
+  // expression was selected.
+  if (llvm::isa<LambdaExpr>(E))
+    return N->Selected == SelectionTree::Complete;
+
   // 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.
@@ -599,10 +607,22 @@ Expected<Tweak::Effect> ExtractVariable::apply(const Selection &Inputs) {
   // FIXME: get variable name from user or suggest based on type
   std::string VarName = "placeholder";
   SourceRange Range = Target->getExtractionChars();
+
+  const SelectionTree::Node &OuterImplicit =
+      Target->getExprNode()->outerImplicit();
+  assert(OuterImplicit.Parent);
+  bool IsStmtExpr = llvm::isa_and_nonnull<CompoundStmt>(
+      OuterImplicit.Parent->ASTNode.get<Stmt>());
+
   // insert new variable declaration
-  if (auto Err = Result.add(Target->insertDeclaration(VarName, Range)))
+  if (auto Err =
+          Result.add(Target->insertDeclaration(VarName, Range, !IsStmtExpr)))
     return std::move(Err);
-  // replace expression with variable name
+
+  // replace expression with variable name, unless it's a statement expression,
+  // in which case we remove it.
+  if (IsStmtExpr)
+    VarName.clear();
   if (auto Err = Result.add(Target->replaceWithVar(Range, VarName)))
     return std::move(Err);
   return Effect::mainFileEdit(Inputs.AST->getSourceManager(),
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
index 656b62c9a1f4e1..c813f3ddee89e4 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
@@ -152,7 +152,7 @@ TEST_F(ExtractVariableTest, Test) {
       a = [[b]];
       a = [[xyz()]];
       // statement expression
-      [[xyz()]];
+      [[v()]];
       while (a)
         [[++a]];
       // label statement
@@ -493,6 +493,16 @@ TEST_F(ExtractVariableTest, Test) {
               a = a + 1;
           }
         })cpp"},
+      {R"cpp(
+        int func() { return 0; }
+        int main() {
+          [[func()]];
+        })cpp",
+       R"cpp(
+        int func() { return 0; }
+        int main() {
+          auto placeholder = func();
+        })cpp"},
       {R"cpp(
         template <typename T>
         auto call(T t) { return t(); }

[HighCommander4]

@ckandeler Could you elaborate on the motivation for this change?

In my mind, the value proposition of the "extract variable" refactoring is that it saves you the work of moving the expression from one location to another, and typing the name of the variable twice (once at the declaration and once at the use).

In the expression-statement case, the expression isn't moving to a new location, and the variable name is only mentioned in one place. In essence, the refactoring amounts to prepending auto placeholder = to the expression. Given that you're going to be (very likely) renaming placeholder anyways, does this really add anything meaningful over just typing auto VariableNameYouWant = "manually"?

[ckandeler]

In the expression-statement case, the expression isn't moving to a new location, and the variable name is only mentioned in
one place. In essence, the refactoring amounts to prepending auto placeholder = to the expression. Given that you're
going to be (very likely) renaming placeholder anyways, does this really add anything meaningful over just typing auto VariableNameYouWant = "manually"?

My thinking was that because the expression is assignable and it's not clearly pointless to do so, the tweak should be available (plus it still saves some keystrokes). My concrete motivation was due to user requests on the client side, e.g.:
https://bugreports.qt.io/browse/QTCREATORBUG-9363
https://bugreports.qt.io/browse/QTCREATORBUG-9578
Maybe some developers have the habit of writing just the rhs of an init statement and then let tooling insert the lhs? It's possible, I suppose. E.g. if the return type is complex and you want to have it spelt out in the declaration (as per the project's coding conventions), then you could apply this tweak and then the ExpandDeducedType one.
In general, I'm not a big fan of going out of my way to prevent users from doing something. And once you start deliberating about degrees of usefulness, you quickly run into problems deciding where to draw the line.

[HighCommander4]

Ok, fair enough.

[ckandeler]

Rebased.

[ckandeler]

Addressed review comments.

[HighCommander4]

Thanks, LGTM