[index][clangd] Consider labels when indexing function bodies

[ckandeler]

Summary

It's valuable to have document highlights for labels and be able to find
references to them.

[ilya-biryukov]

Thanks! This looks like a useful addition.

Could you add a test into libIndex for this too? Just for the sake of having better coverage, some folks probably don't run clang-tools-extra tests.
Also, could we add a test that a label does not get indexed if local function indexing is off?

clang/lib/Index/IndexBody.cpp

148

IndexingContext::handleReference already has this check, we should call handleReference unconditionally.

158

We should call handleDecl, indexing output can differentiate between a declarations and a references.
LabelStmt is exactly the node that declares a LabelDecl.

[ilya-biryukov]

In clangd we also have findExplicitReferences and targetDecl functions that seem to handle the GoToStmt.
In fact, I believe they are used in findDocumentHighlights, so I'm not sure why your test did not work before this patch.

I have also not looked into libIndex for quite a long time, so looping in @kadircet for a second pair of eyes.

[ckandeler]

In D150124#4329163, @ilya-biryukov wrote:

In clangd we also have findExplicitReferences and targetDecl functions that seem to handle the GoToStmt.

According to the commit message of the patch that added this, it was for "go to definition" functionality (which did indeed work already).

[ckandeler]

Addressed review comments.

[ckandeler]

ping

[nridge]

In D150124#4329163, @ilya-biryukov wrote:

In clangd we also have findExplicitReferences and targetDecl functions that seem to handle the GoToStmt.
In fact, I believe they are used in findDocumentHighlights, so I'm not sure why your test did not work before this patch.
findDocumentHighlights

I can explain this part: findDocumentHighlights uses targetDecl to associate the input cursor position with a symbol, but then it uses findRefs (which uses libIndex) to locate other usages of the symbol in the file.

clang/lib/Index/IndexBody.cpp

150

NameReference was introduced in https://github.com/llvm/llvm-project/commit/e7eb27a9a0edd859de49bcc9af7ca27dbb435886 to handle the somewhat unique situation with constructors and destructors where the constructor/destructor references the class by name but semantically denotes a separate entity.

Why is that applicable here?

Note that handleReference() will automatically add SymbolRole::Reference here.

clang/unittests/Index/IndexTests.cpp

233 ↗

(On Diff #521577)

I think the outer AllOf() here is a no-op, since it has only one argument. (AllOf(x, y, z) means "each of the matchers x, y, and z match").

[ckandeler]

Addressed review comment.

[kadircet]

sorry for missing this review. Nathan's explanation around targetDecl vs findRefs is absolutely right (btw, thanks a lot Nathan for taking good care of clangd, I don't think I say that enough). hopefully one day we can switch clangd's usage of libindex to our internal find target/explicitrefs implementation, but until then we actually need to address such things in both places to make sure functionality is actually consistent.

clang/lib/Index/IndexBody.cpp

150

+1 we shouldn't be setting namereference here.

[ckandeler]

clang/lib/Index/IndexBody.cpp

150

So set nothing at all here?

[nridge]

clang/lib/Index/IndexBody.cpp

150

Yes, I think we should pass a default constructed SymbolRoleSet() here, and handleReference() will add SymbolRole::Reference into it automatically.

[nridge]

In D150124#4542032, @kadircet wrote:

btw, thanks a lot Nathan for taking good care of clangd, I don't think I say that enough

I'm happy to help!

[ckandeler]

Addressed another review comment.

[nridge]

LGTM

[ckandeler]

Build failures seem unrelated.

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

const char *Tests[] = {
         @end
         @implementation Foo
           +(void) [[x]]:(int)a [[y]]:(int)b {}
         @end
         void go() {
           [Foo [[x]]:2 [[^y]]:4];
         }
       )cpp",
+      R"cpp( // Label
+        int main() {
+          goto [[^theLabel]];
+          [[theLabel]]:
+            return 1;
+        }
+      )cpp",
   };
   for (const char *Test : Tests) {
     Annotations T(Test);
     auto TU = TestTU::withCode(T.code());
     TU.ExtraArgs.push_back("-xobjective-c++");
     auto AST = TU.build();
     EXPECT_THAT(findDocumentHighlights(AST, T.point()), highlightsFrom(T))
         << Test;

clang/lib/Index/IndexBody.cpp

public:
 
   bool VisitDeclRefExpr(DeclRefExpr *E) {
     SmallVector<SymbolRelation, 4> Relations;
     SymbolRoleSet Roles = getRolesForRef(E, Relations);
     return IndexCtx.handleReference(E->getDecl(), E->getLocation(),
                                     Parent, ParentDC, Roles, Relations, E);
   }
 
+  bool VisitGotoStmt(GotoStmt *S) {
+    if (IndexCtx.shouldIndexFunctionLocalSymbols()) {
+      return IndexCtx.handleReference(S->getLabel(), S->getLabelLoc(), Parent,
+                                      ParentDC,
+                                      unsigned(SymbolRole::NameReference));
+    }
+    return true;
+  }
+
+  bool VisitLabelStmt(LabelStmt *S) {
+    if (IndexCtx.shouldIndexFunctionLocalSymbols()) {
+      return IndexCtx.handleReference(S->getDecl(), S->getIdentLoc(), Parent,
+                                      ParentDC, {});
+    }
+    return true;
+  }
+
   bool VisitMemberExpr(MemberExpr *E) {
     SourceLocation Loc = E->getMemberLoc();
     if (Loc.isInvalid())
       Loc = E->getBeginLoc();
     SmallVector<SymbolRelation, 4> Relations;
     SymbolRoleSet Roles = getRolesForRef(E, Relations);
     return IndexCtx.handleReference(E->getMemberDecl(), Loc,
                                     Parent, ParentDC, Roles, Relations, E);