[clangd] allow extracting to variable for lambda expressions

[5chmidti]

Summary

Support for extracting lambda expressions, e.g. extracting a lambda from a callexpr (e.g. algorithms/ranges) to a named variable.

[5chmidti]

Add change to release notes

[nridge]

LGTM, thanks!

clang-tools-extra/docs/ReleaseNotes.rst

86

Maybe add a "Code Actions" section for this?

[kadircet]

branch cut is upon us, so it'd be great if you can land this soon (or let us know if you don't have commit access)

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

176–178

nit: can we re-write this as: !isa<LambdaExpr>(Stmt) || InsertionPoint->Selected == SelectionTree::Complete ?

clang-tools-extra/docs/ReleaseNotes.rst

86

+1

[5chmidti]

1. addressed comments

2 bugfix (sorry, should've said something)
Local variables inside the lambda were previously added to the ReferencedDecls vector and would block the action inside of exprIsValidOutside (declarations are inside of the lambda).
Now only consider DeclRefExprs in the lambda captures.

3. I don't have commit access, someone else would need to commit this.

I don't know if you want to delay this patch due to the bugfix or not, either way is fine with me.

[kadircet]

sorry i'll need to take a look at this again.

[nridge]

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

84

What about variables referenced implicitly via a capture-default?

For example, consider:

int main() {
  if (int a = 1)
    if ([[ [&](){ return a + 1; } ]]() == 4)
      a = a + 1;
}

Here, the attempted extraction point is before the outer if, which is before the declaration of a, and the fact that the lambda references a should prevent that.

[nridge]

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

86

On the other hand, the exclusion of local variables declared inside lambdas should also apply to lambdas which are subexpressions of the selected expression. Consider:

template <typename T>
auto call(T t) { return t(); }

int main() {
  return [[ call([](){ int a = 1; return a + 1; }) ]] + 5;
}

Here, the presence of the local variable a inside the lambda should not interfere with the ability to extract the call(...) expression.

[5chmidti]

You're right, I'll incorporate the logic for lambdas into FindDeclRefsVisitor and change the docs/comments/commit message to reflect this. I will try to think of more edge cases to test as well. Though I'll be busy until mid march with uni, so there will be no new revisions until then.

[5chmidti]

I took a little break, but here are the changes/fixes:

• moved the logic for variables referenced in captures into the visitor
  • short circuiting the TraverseLambdaExpression and using TraverseLambdaCapture to handle variable captures and the initialization fo init-captures
  • fixes both problems mentioned by nridge
• fix for immediately invoked lambda expressions
  • the selection of an IIL would mark the call operator as a referenced decl (fixed in VisitDeclRefExpr)
• fix condition in CanExtractOutside to allow an unselected parent that is a lambda expression
  • allows for the extraction of an initializer for init-capture variables
• block default arguments from prarameters of a lambda from being extracted to a function-local scope (fixed in CanExtractOutside)
• added more tests

[5chmidti]

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

177

Noticed that this can be removed because extracting from captures is allowed. Will change in the next update

181–183

This is supposed to stop the following invalid code from happening:

void foo() {
  int placeholder = 42;
  [](int x = placeholder {};
  extern void bar(int x = placeholder);
}

clangd does not seem to support extracting from the initializers of defaulted parameters, should I keep the condition as is, or should I do something different here? It is legal to have default arguments in global scope (examples below).

The following insertions could exist (out of scope for this patch):

int placeholder = 42;
void foo() {
  [](int x = placeholder {};
  extern void bar(int x = placeholder);
}

struct X {
  static inline int placeholder = 42;
  void foo(int x = placeholder) {}
};

Either way, I'll have to adjust the comment because this is not just to stop default parameter initializers in lambdas from being extracted to a local scope.

[5chmidti]

Ping, and:

• removed mention of captures in the comment from the condition about blocking extraction because that is allowed
• removed (of a lambda) from the following comment because it is not just about defaulted parameters of lambdas, but of all functions

[5chmidti]

Ping

[5chmidti]

Ping

[nridge]

I promise I haven't forgotten about this. It's just been a challenge finding a large enough chunk of time to page in all the relevant context and think through the issues. Maybe this weekend...

[nridge]

Haven't looked at everything yet, but wanted to mention one thing I noticed.

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

82

Looking at RecursiveASTVisitor::TraverseLambdaExpr, there are a few things it traverses in addition to the lambda's parameters and body (which we are saying are ok to skip) and the lambda's captures (which we are traversing).

For example, the lambda may have an explicit result type which references a variable in scope:

int main() {
  if (int a = 1)
    if ([[ []() -> decltype(a){ return 1; } ]] () == 4)
      a = a + 1;
}

Here, extracting the lambda succeeds, and the reference to a in decltype(a) becomes dangling.

181–183

I'm having trouble following this comment. Can you give the testcase that would produce this invalid code?

[5chmidti]

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

82

I'll update the diff when I've implemented this. A requires clause may reference a variable like a as well.

181–183

I provided test cases in lines 147-150 in ExtractVariableTests.cpp. For the example given in my comment, the test case would be:
Pre:

void foo() {
  [](int x = [[42]]) {};
}

(erroneous) Post:

void foo() {
  int placeholder = 42;
  [](int x = placeholder) {};
}

[nridge]

Ok, I've finished looking through the patch; sorry it took so long to get to.

It generally looks pretty good to me, I just have some fairly minor comments.

Thanks again for your work on this!

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

82

A requires clause may reference a variable like a as well.

Technically, an explicit template parameter list can also reference a local variable via e.g. a default argument for a non-type parameter.

I appreciate that we're getting into increasingly obscure edge cases here, so please feel free to use your judgment and draw a line somewhere; the refactoring introducing a compiler error when invoked on some obscure/unlikely piece of code is not that big of a deal.

181–183

Thanks. I think the reason for my confusion was that the code snippet in your original comment was missing a ')`, and I thought perhaps the refactoring introduced that syntax error, but I understand now that the actual issue is a semantic error where a default argument references a local variable which isn't allowed.

I think this is fine, the only cleanup I think is needed here is to collapse the "Allow all expressions..." comment at the top of the block, and the "Allow expressions..." comment just below, into one.

186

I think it's worth adding to the comment *why* we allow unselected lambda expressions (to allow extracting from capture initializers), as it's not obvious

clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp

165

It's easy to overlook the purpose of this line amidst all the brackets, I would suggest adding a comment like:

// Extracting from a capture initializer is usually fine, but not if
// the capture itself is nested inside a default argument

[5chmidti]

Addressed comments:

• added explicit template parameters, trailing return-type declarations and attributes to the visitor that finds DeclRefExprs
• added a few more comments about why an extraction is expected to be unavailable

[5chmidti]

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

82

I have added support for attributes, trailing return-type decls and explicit template parameters to the visitor. I think that is every edge case covered.

186

I changed the previous return of !isa<LambdaExpr>(Stmt) || InsertionPoint->Selected != SelectionTree::Partial; to a simple return true;. None of my partial selection tests fail and I can't think of a case where the lambda would be partially selected.

clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp

165

I added some comments like the one for this example to indicate why an extraction is expected to be unavailable. The same goes for the tests just above (ln134-189).

[5chmidti]

Ping

[nridge]

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

93

nit: just if (clang::Expr *const RequiresClause = LExpr->getTrailingRequiresClause()) is equivalent

186

Hmm, so what actually happens in these testcases?

// lambdas: partially selected
[][[(){}]];
[]()[[{}]];
[ [[ ](){}]];

[nridge]

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

186

I checked, and in these cases we disallow the extraction here before we get to creating an ExtractionContext.

If the testcases are modified as follows:

template <typename T> void sink(T) {}
...
void f() {
  // lambdas: partially selected
  sink([][[(){}]]);
  sink([]()[[{}]]);
  sink([ [[ ](){}]]);
}

now they fail, unless the check for partial selection here is restored.

[nridge]

(Otherwise the updates look good!)

[5chmidti]

addressed comments

[5chmidti]

Fixup to last revision change:

• do not block the partial selection of lambdas

There is no reason to block the partial selection of lambdas. Other expressions can be partially selected as well and still offer an extraction:

4[[0 +]] 2

[5chmidti]

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

186

As stated in the update comment, I see no reason why we actually block partial selection of lambdas. It's okay to extract from the initializer of a capture (one of your comments above):

int foo();

void bar() {
  [x = [[foo()]] ]() {};
}

The tests in ln 138-143 (titled lambdas: captures) check that we don't extract the wrong things from lambda captures.
Do you see a way that extracting from a lambda capture or any kind of partial selection could be problematic?

To answer the question anyway:
Hmm, so what actually happens in these testcases?
The original diff had a sink that I probably removed because the tests would succeed even without the sink. However, those tests then tested a different condition and never hit the check for partial selection.

[nridge]

I'm fine with allowing partial selection for consistency with other expressions.

I think this patch is in a good state. Thanks for your patience with the review :) Let me know if you need me to commit it for you.

[5chmidti]

Thank you. Could you please commit this for me with Julian Schmidt <[e-mail]>?

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 "AST.h"
 #include "ParsedAST.h"
 #include "Protocol.h"
 #include "Selection.h"
 #include "SourceCode.h"
 #include "refactor/Tweak.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.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"
class FindDeclRefsVisitor
       : public clang::RecursiveASTVisitor<FindDeclRefsVisitor> {
   public:
     std::vector<Decl *> ReferencedDecls;
     bool VisitDeclRefExpr(DeclRefExpr *DeclRef) { // NOLINT
       ReferencedDecls.push_back(DeclRef->getDecl());
       return true;
     }
   };
+
+  // Local variables declared inside of the selected lambda cannot go out of
+  // scope. The DeclRefExprs that are important are the lambda captures and
+  // capture var intitializers.
+  if (const auto *const LExpr = llvm::dyn_cast<LambdaExpr>(Expr)) {
+    FindDeclRefsVisitor Visitor;
+    for (const auto &Capture : LExpr->captures()) {
+      if (Capture.capturesVariable()) {
+        auto *const CapturedVar = Capture.getCapturedVar();
+        if (auto *const VDecl = llvm::dyn_cast<VarDecl>(CapturedVar)) {
+          Visitor.TraverseStmt(VDecl->getInit());
+        }
+      }
+    }
+    return Visitor.ReferencedDecls;
+  }
+
   FindDeclRefsVisitor Visitor;
   Visitor.TraverseStmt(const_cast<Stmt *>(cast<Stmt>(Expr)));
   return Visitor.ReferencedDecls;
 }
 
 static QualType computeVariableType(const Expr *Expr, const ASTContext &Ctx) {
   if (Ctx.getLangOpts().CPlusPlus11)
     return Ctx.getAutoDeductType();

 // 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
+      // Allow all expressions except partial LambdaExpr selections since we
+      // don't want to extract from the captures/default arguments of a lambda
       if (isa<clang::Expr>(Stmt))
-        return !isa<LambdaExpr>(Stmt);
+        return !isa<LambdaExpr>(Stmt) ||
+               InsertionPoint->Selected == SelectionTree::Complete;
+
       // 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<ReturnStmt>(Stmt) || isa<WhileStmt>(Stmt);
     }
     if (InsertionPoint->ASTNode.get<VarDecl>())
       return true;
     return false;
   };
   for (const SelectionTree::Node *CurNode = getExprNode();
        CurNode->Parent && CanExtractOutside(CurNode);

clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp

int xyz(int a = [[1]]) {
         int bar(int a = [[1]]) {
           int b = [[z]];
         }
         int z = [[1]];
       } t;
       return [[t]].bar([[t]].z);
     }
     void v() { return; }
+    template <typename T> void callable_sink(T) {}
     // 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]]();
void f(int b = [[1]]) {
       // statement expression
       [[xyz()]];
       while (a)
         [[++a]];
       // label statement
       goto label;
       label:
         a = [[1]];
+
+      // lambdas
+      callable_sink([][[(){}]]);
+
+      // captures
+      int x = 0;
+      callable_sink([ [[=]] ](){});
+      callable_sink([ [[&]] ](){});
+      callable_sink([ [[x]] ](){});
+      callable_sink([ [[&x] ]](){});
+      callable_sink([y = [[x]] ](){});
+      callable_sink([ [[y = x]] ](){});
+
+      // default args
+      callable_sink([](int x = [[10]]){});
     }
   )cpp";
   EXPECT_UNAVAILABLE(UnavailableCases);
 
   // vector of pairs of input and output strings
   std::vector<std::pair<std::string, std::string>> InputOutputs = {
       // extraction from variable declaration/assignment
       {R"cpp(void varDecl() {
                    int a = 5 * (4 + (3 [[- 1)]]);
                  })cpp",
        R"cpp(void varDecl() {
                    auto placeholder = (3 - 1); int a = 5 * (4 + placeholder);
                  })cpp"},
       // FIXME: extraction from switch case
       /*{R"cpp(void f(int a) {
                if(1)
                  while(a < 1)
                    switch (1) {
                        case 1:
                          a = [[1 + 2]];
                          break;
                        default:
                          break;
                    }
std::vector<std::pair<std::string, std::string>> InputOutputs = {
        R"cpp(struct S {
                    S(int);
                  };
                  S operator+(S, S);
 
                  void f() {
                    auto placeholder = S(2) + S(3) + S(4); S x = S(1) + placeholder + S(5);
                  })cpp"},
+      // Complete lambda expressions
+      {R"cpp(template <typename T> void f(T) {}
+                void f2() {
+                  f([[ [](){ return 42; }]]);
+                }
+                )cpp",
+       R"cpp(template <typename T> void f(T) {}
+                void f2() {
+                  auto placeholder = [](){ return 42; }; f( placeholder);
+                }
+                )cpp"},
+      {R"cpp(auto foo(int VarA) {
+                  return [VarA]() {
+                    return [[ [VarA, VarC = 42 + VarA](int VarB) { return VarA + VarB + VarC; }]];
+                  };
+                }
+                )cpp",
+       R"cpp(auto foo(int VarA) {
+                  return [VarA]() {
+                    auto placeholder = [VarA, VarC = 42 + VarA](int VarB) { return VarA + VarB + VarC; }; return  placeholder;
+                  };
+                }
+                )cpp"},
+      {R"cpp(template <typename T> void f(T) {}
+                void f2(int var) {
+                  f([[ [&var](){ auto internal_val = 42; return var + internal_val; }]]);
+                }
+                )cpp",
+       R"cpp(template <typename T> void f(T) {}
+                void f2(int var) {
+                  auto placeholder = [&var](){ auto internal_val = 42; return var + internal_val; }; f( placeholder);
+                }
+                )cpp"},
+      {R"cpp(template <typename T> void f(T) { }
+                struct A {
+                    void f2(int& var) {
+                        auto local_var = 42;
+                        f([[ [&var, &local_var, this]() {
+                            auto internal_val = 42;
+                            return var + local_var + internal_val + member;
+                        }]]);
+                    }
+
+                    int member = 42;
+};
+                )cpp",
+       R"cpp(template <typename T> void f(T) { }
+                struct A {
+                    void f2(int& var) {
+                        auto local_var = 42;
+                        auto placeholder = [&var, &local_var, this]() {
+                            auto internal_val = 42;
+                            return var + local_var + internal_val + member;
+                        }; f( placeholder);
+                    }
+
+                    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"},
       // Don't try to analyze across macro boundaries
       // FIXME: it'd be nice to do this someday (in a safe way)
       {R"cpp(#define ECHO(X) X
                  void f() {
                    int x = 1 + [[ECHO(2 + 3) + 4]] + 5;
                  })cpp",
        R"cpp(#define ECHO(X) X
                  void f() {

clang-tools-extra/docs/ReleaseNotes.rst

 ^^^^^^^^^^^^^
 
 Hover
 ^^^^^
 
 Code completion
 ^^^^^^^^^^^^^^^
 
+Code actions
+^^^^^^^^^^^^
+
+- The extract variable tweak gained support for extracting complete lambda expressions to a variable.
+
 Signature help
 ^^^^^^^^^^^^^^
 
 Cross-references
 ^^^^^^^^^^^^^^^^
 
 Objective-C
 ^^^^^^^^^^^
 
 Miscellaneous
 ^^^^^^^^^^^^^
 
 Improvements to clang-doc
 -------------------------
 
 - The default executor was changed to standalone to match other tools.
 
 Improvements to clang-query
 ---------------------------
 
 The improvements are...