[clangd] Handle lambda scopes inside Node::getDeclContext() #76329
Conversation
|
@llvm/pr-subscribers-clangd Author: Younan Zhang (zyn0217) ChangesWe used to consider the For example, void foo();
auto lambda = [] {
return ^foo();
};where Seemingly, this affects nothing currently. Full diff: https://github.com/llvm/llvm-project/pull/76329.diff 2 Files Affected:
diff --git a/clang-tools-extra/clangd/Selection.cpp b/clang-tools-extra/clangd/Selection.cpp
index 8c6d5750ecefdb..a7413485b04ac3 100644
--- a/clang-tools-extra/clangd/Selection.cpp
+++ b/clang-tools-extra/clangd/Selection.cpp
@@ -1113,6 +1113,9 @@ const DeclContext &SelectionTree::Node::getDeclContext() const {
return *DC;
return *Current->getLexicalDeclContext();
}
+ if (auto *LE = CurrentNode->ASTNode.get<LambdaExpr>())
+ if (CurrentNode != this)
+ return *LE->getLambdaClass();
}
llvm_unreachable("A tree must always be rooted at TranslationUnitDecl.");
}
diff --git a/clang-tools-extra/clangd/unittests/SelectionTests.cpp b/clang-tools-extra/clangd/unittests/SelectionTests.cpp
index 4c019a1524f3c3..cdb50dfc1c3095 100644
--- a/clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -880,6 +880,19 @@ TEST(SelectionTest, DeclContextIsLexical) {
}
}
+TEST(SelectionTest, DeclContextLambda) {
+ llvm::Annotations Test(R"cpp(
+ void foo();
+ auto lambda = [] {
+ return $1^foo();
+ };
+ )cpp");
+ auto AST = TestTU::withCode(Test.code()).build();
+ auto ST = SelectionTree::createRight(AST.getASTContext(), AST.getTokens(),
+ Test.point("1"), Test.point("1"));
+ EXPECT_FALSE(ST.commonAncestor()->getDeclContext().isTranslationUnit());
+}
+
} // namespace
} // namespace clangd
} // namespace clang
|
|
Ping. And Happy New Year 2024! |
We used to consider the DeclContext for selection nodes inside
a lambda as the scope of the lambda expression locates in.
For example,
```cpp
void foo();
auto lambda = [] {
return ^foo();
};
```
where N is the selection node for the expression `foo()`,
`N.getDeclContext()` returns the TranslationUnitDecl previously,
which IMO is wrong, since the RecordDecl of the lambda is closer.
There was a problem hiding this comment.
Thanks, the change looks good to me!
I went through the existing callers of Node::getDeclContext(), and I was able to construct a test case where the patch actually changes behaviour (in a good way):
namespace NS {
void unrelated();
void foo();
}
auto L = [] {
using NS::unrelated;
NS::foo();
};Here, if the "add using-declaration" code action is invoked on NS::foo, before this patch the result is:
namespace NS {
void unrelated();
void foo();
}
using NS::foo;
auto L = [] {
using NS::unrelated;
foo();
};but after this patch the result is:
namespace NS {
void unrelated();
void foo();
}
auto L = [] {
using NS::foo;
using NS::unrelated;
foo();
};Let's add this test case to AddUsingTests while we're at it.
You too! |
|
Sorry for being silent for a few days, I've been a bit occupied recently and hopefully I could get back to the heuristic patch for templates soon.
Thank you for the exploration! I've added the test case, although it appears a bit odd: auto L = [] {
using NS::foo;using NS::unrelated; // Shouldn't it be placed on a new line?
foo();
};)cppHowever, I've seen a similar tweaking style (e.g. |
zyn0217
force-pushed
the
selection-tree-lambda
branch
from
18274ed to
e289f7f
Compare
|
(I'll land it once the CI turns green.) |
I think what may be happening here is that the refactorings try to avoid duplicating the work of the formatter, and just generate syntactically valid code that the formatter will then format according to the codebase's clang-format settings. |
|
The CI failure on windows seems irrelevant, so I'm landing it anyway. |
We used to consider the `DeclContext` for selection nodes inside a
lambda as the enclosing scope of the lambda expression, rather than the
lambda itself.
For example,
```cpp
void foo();
auto lambda = [] {
return ^foo();
};
```
where `N` is the selection node for the expression `foo()`,
`N.getDeclContext()` returns the `TranslationUnitDecl` previously, which
IMO is wrong, since the method `operator()` of the lambda is closer.
Incidentally, this fixes a glitch in add-using-declaration tweaks.
(Thanks @HighCommander4 for the test case.)
We used to consider the
DeclContextfor selection nodes inside a lambda as the enclosing scope of the lambda expression, rather than the lambda itself.For example,
where
Nis the selection node for the expressionfoo(),N.getDeclContext()returns theTranslationUnitDeclpreviously, which IMO is wrong, since the methodoperator()of the lambda is closer.Incidentally, this fixes a glitch in add-using-declaration tweaks. (Thanks @HighCommander4 for the test case.)