[PATCH] D141757: [clangd] allow extracting to variable for lambda expressions

[Nathan Ridge via Phabricator via cfe-commits]

nridge requested changes to this revision.
nridge added a comment.
This revision now requires changes to proceed.

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!



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:182-184
+        if (InsertionPoint->Parent->ASTNode.get<ParmVarDecl>() != nullptr) {
+          return false;
+        }
----------------
5chmidti wrote:
> nridge wrote:
> > 5chmidti wrote:
> > > 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.
> > I'm having trouble following this comment. Can you give the testcase that would produce this invalid code?
> 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) {};
> }
> ```
> 
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.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:91
+
+    // Local variables declared inside of the selected lambda cannot go out of
+    // scope. The DeclRefExprs that are important are the variables captured and
----------------
5chmidti wrote:
> nridge wrote:
> > Looking at [RecursiveASTVisitor::TraverseLambdaExpr](https://searchfox.org/llvm/rev/094539cfcb46f55824402565e5c18580df689a67/clang/include/clang/AST/RecursiveASTVisitor.h#2652-2691), 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.
> I'll update the diff when I've implemented this. A requires clause may reference a variable like `a` as well.
> 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.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:187
+        // Allow expressions, but only allow completely selected lambda
+        // expressions or unselected lambda expressions that are the parent of
+        // the originally selected node, not partially selected lambda
----------------
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


================
Comment at: clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp:149
+      [](int x = [[10]]){};
+      [](auto h = [i = [[ [](){} ]]](){}) {};
+      [](auto h = [[ [i = [](){}](){} ]]) {};
----------------
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
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141757/new/

https://reviews.llvm.org/D141757