Page MenuHomePhabricator

[clangd] allow extracting to variable for lambda expressions
Needs ReviewPublic

Authored by 5chmidti on Jan 14 2023, 5:12 AM.



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

Diff Detail

Event Timeline

5chmidti created this revision.Jan 14 2023, 5:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2023, 5:12 AM
5chmidti requested review of this revision.Jan 14 2023, 5:12 AM
5chmidti updated this revision to Diff 489232.Jan 14 2023, 5:16 AM

Add change to release notes

LGTM, thanks!

81 ↗(On Diff #489232)

Maybe add a "Code Actions" section for this?

kadircet accepted this revision.Jan 23 2023, 12:05 AM

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)


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

81 ↗(On Diff #489232)


This revision is now accepted and ready to land.Jan 23 2023, 12:05 AM
5chmidti updated this revision to Diff 491319.Jan 23 2023, 5:25 AM
  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.

  1. 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.

5chmidti marked 3 inline comments as done.Jan 23 2023, 5:25 AM
kadircet requested changes to this revision.Jan 23 2023, 9:09 AM

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

This revision now requires changes to proceed.Jan 23 2023, 9:09 AM
nridge added inline comments.Feb 21 2023, 1:20 AM

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 added inline comments.Feb 21 2023, 1:29 AM

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.

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 updated this revision to Diff 518159.Apr 29 2023, 5:15 AM

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 marked 2 inline comments as done.Apr 29 2023, 5:16 AM
5chmidti retitled this revision from [clangd] allow extracting to variable for complete lambda expressions to [clangd] allow extracting to variable for lambda expressions.Apr 29 2023, 6:00 AM
5chmidti edited the summary of this revision. (Show Details)
5chmidti added inline comments.Apr 29 2023, 4:25 PM

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


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.