Improve attribute range handling for attributed function types in sel… #163926

quanzhuo · 2025-10-17T08:34:36Z

…ection

github-actions · 2025-10-17T08:35:03Z

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 @ followed by their GitHub username.

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.

llvmbot · 2025-10-17T08:35:39Z

@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:

(modified) clang-tools-extra/clangd/Selection.cpp (+12)
(modified) clang-tools-extra/clangd/unittests/SelectionTests.cpp (+9)

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"},

llvmbot · 2025-10-17T08:35:39Z

@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:

(modified) clang-tools-extra/clangd/Selection.cpp (+12)
(modified) clang-tools-extra/clangd/unittests/SelectionTests.cpp (+9)

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"},

Copilot

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.

Copilot · 2025-10-17T08:35:55Z

clang-tools-extra/clangd/Selection.cpp

+        if (ATL.getModifiedLoc().getAs<FunctionTypeLoc>()) {
+          // Only claim the attribute's source range, not the whole type.
+          claimRange(ATL.getLocalSourceRange(), Result);


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()).

Suggested change

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);

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 AttributedTypeLoc and 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.

quanzhuo · 2025-10-17T09:36:06Z

Hi Commander, Could you please help to review this pr ?
Give me some hint if the change is not suitable, Thank you
@HighCommander4

HighCommander4

Thanks for the patch! Looks great, I just have a suggestion for one additonal test case.

HighCommander4 · 2025-10-31T06:24:55Z

clang-tools-extra/clangd/unittests/SelectionTests.cpp

+       "FunctionDecl"},
+      {"struct X { [[void ^foo() const [[clang::lifetimebound]]]]; };",
+       "FunctionDecl"},
+      {"struct X { [[const int* ^Get() const [[clang::lifetimebound]]]]; };",


Could you add a case where we successfully hit the AttributedTypeLoc (e.g. [[clang::^lifetimebound]])?

HighCommander4

CI is showing the test is failing, that will need to be fixed.

HighCommander4 · 2025-10-31T06:33:43Z

CI is showing the test is failing, that will need to be fixed.

Ah, the reason for the test failure is slightly annoying: there's a conflict between the C++ attribute syntax, and the syntax of the annotations used to mark up our test cases (see the documentation of the annotation syntax and this FIXME.)

This can be worked around by using an alternative syntax for C++ attributes, <:<:attribute:>:>, as in this example test.

quanzhuo · 2025-11-01T07:09:55Z

Hi @HighCommander4

Thanks for the review! I've added the test for selecting AttributedTypeLoc, when the cursor is on the attribute. However, I'm having trouble with the second test case I added. It's failing with a range mismatch:

D:\repos\llvm-project\clang-tools-extra\clangd\unittests\SelectionTests.cpp:635
Expected equality of these values:
  nodeRange(T.commonAncestor(), AST)
    Which is: 0:17-0:57
  Test.range()
    Which is: 0:17-0:60
struct X { const [[int* Foo() const <:[clang::life^timebound]:>]] {return nullptr;}; };
 TranslationUnitDecl
   CXXRecordDecl struct X {}
     CXXMethodDecl const int *Foo() const
      .AttributedTypeLoc const int *() const [[clang::lifetimebound]]

The issue appears to be that the test annotation parser stops at the ] in ]:>, which truncates the expected range marker.

Thanks for your guidance!

HighCommander4 · 2025-11-02T07:22:35Z

The issue appears to be that the test annotation parser stops at the ] in ]:>, which truncates the expected range marker.

It looks like the problem is not with the expected range marker (this is Test.range(), which is 0:17-0:60, which looks correct), but with the actual range of the AttributedTypeLoc node, which is 0:17-0:57.

It looks like the AST is reporting that the AttributedTypeLoc node ends at the lifetimebound token, rather than the :> token. This is a bug in the AST modeling of AttributedTypeLoc (or perhaps, based on this comment, a deliberate quirk).

We can revise the test to expect the node to end there, i.e.:

struct X { const [[int* Foo() const <:[clang::life^timebound]]]:>

though I find it more readable to use the digraph for both brackets:

struct X { const [[int* Foo() const <:<:clang::life^timebound]]:>:>

and maybe add a comment saying that this is a quirk in the AST modeling of AttributedTypeLoc.

quanzhuo · 2025-11-02T11:48:43Z

Hi @HighCommander4,

Thank you for the detailed explanation! I've updated the test case and comment as you suggested. The test should pass now. Thanks again for your patient guidance!

HighCommander4

Thanks, LGTM!

github-actions · 2025-11-02T22:21:21Z

@quanzhuo Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

This ensures a method name continues to target the method's declaration even if the method's type uses an attribute. Before this change, the AttributedTypeLoc would claim the method name. Fixes clangd/clangd#2488

Improve attribute range handling for attributed function types in sel…

4239775

…ection This fix clangd/clangd#2488

Copilot AI review requested due to automatic review settings October 17, 2025 08:34

llvmbot added clang-tools-extra clangd labels Oct 17, 2025

Copilot AI reviewed Oct 17, 2025

View reviewed changes

HighCommander4 self-requested a review October 19, 2025 02:19

HighCommander4 approved these changes Oct 31, 2025

View reviewed changes

HighCommander4 requested changes Oct 31, 2025

View reviewed changes

quanzhuo force-pushed the main branch from ca03904 to 4239775 Compare October 31, 2025 08:45

Add test for AttributedTypeLoc

d23f381

Update test for AttributedTypeLoc

342c87f

HighCommander4 approved these changes Nov 2, 2025

View reviewed changes

HighCommander4 merged commit c1d1a40 into llvm:main Nov 2, 2025
10 checks passed

Improve attribute range handling for attributed function types in sel… #163926

Improve attribute range handling for attributed function types in sel… #163926

Uh oh!

Conversation

quanzhuo commented Oct 17, 2025

Uh oh!

github-actions bot commented Oct 17, 2025

Uh oh!

llvmbot commented Oct 17, 2025

Uh oh!

llvmbot commented Oct 17, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

quanzhuo Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

quanzhuo commented Oct 17, 2025

Uh oh!

HighCommander4 left a comment

Choose a reason for hiding this comment

Uh oh!

HighCommander4 Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

HighCommander4 left a comment

Choose a reason for hiding this comment

Uh oh!

HighCommander4 commented Oct 31, 2025

Uh oh!

quanzhuo commented Nov 1, 2025

Uh oh!

HighCommander4 commented Nov 2, 2025

Uh oh!

quanzhuo commented Nov 2, 2025

Uh oh!

HighCommander4 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Nov 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants