Support for extracting lambda expressions, e.g. extracting a lambda from a callexpr (e.g. algorithms/ranges) to a named variable.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
LGTM, thanks!
clang-tools-extra/docs/ReleaseNotes.rst | ||
---|---|---|
86 | Maybe add a "Code Actions" section for this? |
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 | ||
---|---|---|
177–193 | nit: can we re-write this as: !isa<LambdaExpr>(Stmt) || InsertionPoint->Selected == SelectionTree::Complete ? | |
clang-tools-extra/docs/ReleaseNotes.rst | ||
86 | +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.
- 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.
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp | ||
---|---|---|
104 | 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. |
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp | ||
---|---|---|
106 | 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.
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
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp | ||
---|---|---|
178 | Noticed that this can be removed because extracting from captures is allowed. Will change in the next update | |
182–184 | 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. |
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
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...
Haven't looked at everything yet, but wanted to mention one thing I noticed.
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp | ||
---|---|---|
91 | 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. | |
182–184 | I'm having trouble following this comment. Can you give the testcase that would produce this invalid code? |
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp | ||
---|---|---|
91 | I'll update the diff when I've implemented this. A requires clause may reference a variable like a as well. | |
182–184 | I provided test cases in lines 147-150 in ExtractVariableTests.cpp. For the example given in my comment, the test case would be: void foo() { [](int x = [[42]]) {}; } (erroneous) Post: void foo() { int placeholder = 42; [](int x = placeholder) {}; } |
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 | ||
---|---|---|
91 |
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. | |
182–184 | 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. | |
187 | 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 | ||
149 | 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 |
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
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp | ||
---|---|---|
91 | 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. | |
187 | 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 | ||
149 | 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). |
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp | ||
---|---|---|
187 | 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. |
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
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp | ||
---|---|---|
187 | 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. To answer the question anyway: |
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.
Thank you. Could you please commit this for me with Julian Schmidt <44101708+5chmidti@users.noreply.github.com>?
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:
Here, extracting the lambda succeeds, and the reference to a in decltype(a) becomes dangling.