Improve attribute range handling for attributed function types in sel… #163926
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang-tools-extra Author: Quan Zhuo (quanzhuo) Changes…ection This fix clangd/clangd#2488 Full diff: https://github.com/llvm/llvm-project/pull/163926.diff 2 Files Affected:
diff --git a/clang-tools-extra/clangd/Selection.cpp b/clang-tools-extra/clangd/Selection.cpp
index 06165dfbbcdd2..faa00d20497fa 100644
--- a/clang-tools-extra/clangd/Selection.cpp
+++ b/clang-tools-extra/clangd/Selection.cpp
@@ -958,6 +958,18 @@ class SelectionVisitor : public RecursiveASTVisitor<SelectionVisitor> {
claimRange(SourceRange(FTL.getLParenLoc(), FTL.getEndLoc()), Result);
return;
}
+ if (auto ATL = TL->getAs<AttributedTypeLoc>()) {
+ // For attributed function types like `int foo() [[attr]]`, the
+ // AttributedTypeLoc's range includes the function name. We want to
+ // allow the function name to be associated with the FunctionDecl
+ // rather than the AttributedTypeLoc, so we only claim the attribute
+ // range itself.
+ if (ATL.getModifiedLoc().getAs<FunctionTypeLoc>()) {
+ // Only claim the attribute's source range, not the whole type.
+ claimRange(ATL.getLocalSourceRange(), Result);
+ return;
+ }
+ }
}
claimRange(getSourceRange(N), Result);
}
diff --git a/clang-tools-extra/clangd/unittests/SelectionTests.cpp b/clang-tools-extra/clangd/unittests/SelectionTests.cpp
index 3df19d8fc174d..103c00ebd5696 100644
--- a/clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -311,6 +311,15 @@ TEST(SelectionTest, CommonAncestor) {
{"[[void foo^()]];", "FunctionProtoTypeLoc"},
{"[[^void foo^()]];", "FunctionDecl"},
{"[[void ^foo()]];", "FunctionDecl"},
+ // Tricky case: with function attributes, the AttributedTypeLoc's range
+ // includes the function name, but we want the name to be associated with
+ // the FunctionDecl.
+ {"struct X { [[void ^foo() [[clang::lifetimebound]]]]; };",
+ "FunctionDecl"},
+ {"struct X { [[void ^foo() const [[clang::lifetimebound]]]]; };",
+ "FunctionDecl"},
+ {"struct X { [[const int* ^Get() const [[clang::lifetimebound]]]]; };",
+ "FunctionDecl"},
// Tricky case: two VarDecls share a specifier.
{"[[int ^a]], b;", "VarDecl"},
{"[[int a, ^b]];", "VarDecl"},
|
|
@llvm/pr-subscribers-clangd Author: Quan Zhuo (quanzhuo) Changes…ection This fix clangd/clangd#2488 Full diff: https://github.com/llvm/llvm-project/pull/163926.diff 2 Files Affected:
diff --git a/clang-tools-extra/clangd/Selection.cpp b/clang-tools-extra/clangd/Selection.cpp
index 06165dfbbcdd2..faa00d20497fa 100644
--- a/clang-tools-extra/clangd/Selection.cpp
+++ b/clang-tools-extra/clangd/Selection.cpp
@@ -958,6 +958,18 @@ class SelectionVisitor : public RecursiveASTVisitor<SelectionVisitor> {
claimRange(SourceRange(FTL.getLParenLoc(), FTL.getEndLoc()), Result);
return;
}
+ if (auto ATL = TL->getAs<AttributedTypeLoc>()) {
+ // For attributed function types like `int foo() [[attr]]`, the
+ // AttributedTypeLoc's range includes the function name. We want to
+ // allow the function name to be associated with the FunctionDecl
+ // rather than the AttributedTypeLoc, so we only claim the attribute
+ // range itself.
+ if (ATL.getModifiedLoc().getAs<FunctionTypeLoc>()) {
+ // Only claim the attribute's source range, not the whole type.
+ claimRange(ATL.getLocalSourceRange(), Result);
+ return;
+ }
+ }
}
claimRange(getSourceRange(N), Result);
}
diff --git a/clang-tools-extra/clangd/unittests/SelectionTests.cpp b/clang-tools-extra/clangd/unittests/SelectionTests.cpp
index 3df19d8fc174d..103c00ebd5696 100644
--- a/clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -311,6 +311,15 @@ TEST(SelectionTest, CommonAncestor) {
{"[[void foo^()]];", "FunctionProtoTypeLoc"},
{"[[^void foo^()]];", "FunctionDecl"},
{"[[void ^foo()]];", "FunctionDecl"},
+ // Tricky case: with function attributes, the AttributedTypeLoc's range
+ // includes the function name, but we want the name to be associated with
+ // the FunctionDecl.
+ {"struct X { [[void ^foo() [[clang::lifetimebound]]]]; };",
+ "FunctionDecl"},
+ {"struct X { [[void ^foo() const [[clang::lifetimebound]]]]; };",
+ "FunctionDecl"},
+ {"struct X { [[const int* ^Get() const [[clang::lifetimebound]]]]; };",
+ "FunctionDecl"},
// Tricky case: two VarDecls share a specifier.
{"[[int ^a]], b;", "VarDecl"},
{"[[int a, ^b]];", "VarDecl"},
|
There was a problem hiding this comment.
Pull Request Overview
Fix selection mapping for attributed function types so that the function name is associated with the FunctionDecl, not the AttributedTypeLoc.
- Add unit tests covering function declarations with trailing attributes and const-qualified methods.
- Adjust SelectionVisitor to claim only the attribute token range for AttributedTypeLoc wrapping function types.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| clang-tools-extra/clangd/Selection.cpp | Special-case AttributedTypeLoc wrapping function types to claim only attribute tokens during selection. |
| clang-tools-extra/clangd/unittests/SelectionTests.cpp | Add tests ensuring the function name maps to FunctionDecl when attributes are present on the function type. |
| if (ATL.getModifiedLoc().getAs<FunctionTypeLoc>()) { | ||
| // Only claim the attribute's source range, not the whole type. | ||
| claimRange(ATL.getLocalSourceRange(), Result); |
There was a problem hiding this comment.
Returning immediately after claiming the attribute range prevents the existing FunctionProtoTypeLoc handling (at lines 958–960) from running for attributed function types. As a result, the function's parameter list and trailing part (from lParen to EndLoc) will not be claimed by the type loc, potentially leaving those tokens unclaimed or attributed to the wrong node. Suggestion: claim the attribute range, then also claim the inner FunctionProtoTypeLoc's range before returning, e.g., check ATL.getModifiedLoc().getAs() and claim SourceRange(FTL.getLParenLoc(), FTL.getEndLoc()).
| if (ATL.getModifiedLoc().getAs<FunctionTypeLoc>()) { | |
| // Only claim the attribute's source range, not the whole type. | |
| claimRange(ATL.getLocalSourceRange(), Result); | |
| if (auto FTL = ATL.getModifiedLoc().getAs<FunctionTypeLoc>()) { | |
| // Claim the attribute's source range. | |
| claimRange(ATL.getLocalSourceRange(), Result); | |
| // Also claim the function's parameter list and trailing part. | |
| claimRange(SourceRange(FTL.getLParenLoc(), FTL.getEndLoc()), Result); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
The RecursiveASTVisitor automatically traverses the inner type of AttributedTypeLoc. Looking at the RAV implementation in RecursiveASTVisitor.h:
DEF_TRAVERSE_TYPELOC(AttributedType,
{ TRY_TO(TraverseTypeLoc(TL.getModifiedLoc())); })This means when we visit an AttributedTypeLoc, the RAV will automatically traverse its ModifiedLoc (the inner FunctionTypeLoc).
So the flow is:
- We encounter
AttributedTypeLocand only claim the attribute's source range - RAV automatically traverses the inner
FunctionTypeLoc - The existing special handling for
FunctionTypeLoc(lines 957-959) claims the parameter list range
If we claim the function's parameter list in the AttributedTypeLoc handler as suggested, it would be claimed twice: once in the AttributedTypeLoc case and again when the inner FunctionTypeLoc is traversed.
|
Hi Commander, Could you please help to review this pr ? |
2 participants
…ection
This fix clangd/clangd#2488