[clangd] Report only decl of overridding method in xref.

[usaxena95]

Summary

See: https://github.com/clangd/clangd/issues/668

struct A { virtual void foo(); };
struct B : A { void foo() override; };

Find refs on A::foo() will show:

• decls of A::foo()
• decls of B::foo()
• refs to A::foo()
• no refs to B::foo().

[usaxena95]

Move Index query to more logical place.

[sammccall]

This looks about right to me...
Unfortunately I landed ff4832dbff0ccf1fd29f726efe72fd1220cd645a and 1eb7fd089e2fcf3fe211f865b28e2fed12128c3f meanwhile so this will need a rebase :-(

The first of those patches introduces flags on returned refs to indicate which are decls/defs. I'd suggest adding an "override" flag and populating it for these results.
Hopefully we'll have a sensible way to expose these flags over LSP someday, meanwhile I think our internal embedders may like to surface that.

(I guess the bug includes the other side of this, where if the method is an override, calls to base methods are reported. Makes sense to split it into a separate patch...)

[usaxena95]

Rebase + Add OverriddenBy flag for such references.

[usaxena95]

In D95812#2535077, @sammccall wrote:

This looks about right to me...
Unfortunately I landed ff4832dbff0ccf1fd29f726efe72fd1220cd645a and 1eb7fd089e2fcf3fe211f865b28e2fed12128c3f meanwhile so this will need a rebase :-(

The first of those patches introduces flags on returned refs to indicate which are decls/defs. I'd suggest adding an "override" flag and populating it for these results.

Thanks.

(I guess the bug includes the other side of this, where if the method is an override, calls to base methods are reported. Makes sense to split it into a separate patch...)

Yes. I plan to do that in a separate patch.

[usaxena95]

Print value of new OverriddenBy flag.

[usaxena95]

Remove unintended changes.

[hokein]

Thanks, this looks great.

clang-tools-extra/clangd/XRefs.cpp

1374

I guess we should include the definition location as well?

clang-tools-extra/clangd/XRefs.h

86 ↗

(On Diff #320685)

nit: add some doc, I'd just name it override as this field indicates this is an occurrence overrides the base method.

[usaxena95]

Addressed comments.

clang-tools-extra/clangd/XRefs.cpp

DeclRelationSet Relations =
         DeclRelation::TemplatePattern | DeclRelation::Alias;
     std::vector<const NamedDecl *> Decls =
         getDeclAtPosition(AST, *CurLoc, Relations);
     llvm::DenseSet<SymbolID> Targets;
     for (const NamedDecl *D : Decls)
       if (auto ID = getSymbolID(D))
         Targets.insert(ID);
 
-    llvm::DenseSet<SymbolID> Overrides;
+    RelationsRequest OverriddenBy;
     if (Index) {
-      RelationsRequest FindOverrides;
-      FindOverrides.Predicate = RelationKind::OverriddenBy;
+      OverriddenBy.Predicate = RelationKind::OverriddenBy;
       for (const NamedDecl *ND : Decls) {
-        // Special case: virtual void meth^od() = 0 includes refs of overrides.
+        // Special case: Inlcude declaration of overridding methods.
         if (const auto *CMD = llvm::dyn_cast<CXXMethodDecl>(ND)) {
-          if (CMD->isPure())
+          if (CMD->isVirtual())
             if (IdentifierAtCursor && SM.getSpellingLoc(CMD->getLocation()) ==
                                           IdentifierAtCursor->location())
               if (auto ID = getSymbolID(CMD))
-                FindOverrides.Subjects.insert(ID);
+                OverriddenBy.Subjects.insert(ID);
         }
       }
-      if (!FindOverrides.Subjects.empty())
-        Index->relations(FindOverrides,
-                         [&](const SymbolID &Subject, const Symbol &Object) {
-                           Overrides.insert(Object.ID);
-                         });
-      Targets.insert(Overrides.begin(), Overrides.end());
     }
 
     // We traverse the AST to find references in the main file.
     auto MainFileRefs = findRefs(Targets, AST);
     // We may get multiple refs with the same location and different Roles, as
     // cross-reference is only interested in locations, we deduplicate them
     // by the location to avoid emitting duplicated locations.
     MainFileRefs.erase(std::unique(MainFileRefs.begin(), MainFileRefs.end(),
                                    [](const ReferenceFinder::Reference &L,
                                       const ReferenceFinder::Reference &R) {
                                      return L.SpelledTok.location() ==
                                             R.SpelledTok.location();
                                    }),
                        MainFileRefs.end());
     for (const auto &Ref : MainFileRefs) {
       Location Result;
       Result.range = Ref.range(SM);
       Result.uri = URIMainFile;
       Results.References.push_back(std::move(Result));
     }
+    // Add declaration of overridding methods.
+    if (Index && Results.References.size() <= Limit &&
+        !OverriddenBy.Subjects.empty())
+      Index->relations(
+          OverriddenBy, [&](const SymbolID &Subject, const Symbol &Object) {
+            if (auto LSPLoc =
+                    toLSPLocation(Object.CanonicalDeclaration, *MainFilePath))
+              Results.References.push_back(std::move(*LSPLoc));
+          });
+
     if (Index && Results.References.size() <= Limit) {
-      Req.IDs = std::move(Overrides);
       for (const Decl *D : Decls) {
         // Not all symbols can be referenced from outside (e.g.
         // function-locals).
         // TODO: we could skip TU-scoped symbols here (e.g. static functions) if
         // we know this file isn't a header. The details might be tricky.
         if (D->getParentFunctionOrMethod())
           continue;
         if (auto ID = getSymbolID(D))
           Req.IDs.insert(ID);
       }
     }
   }
+
   // Now query the index for references from other files.
   if (!Req.IDs.empty() && Index && Results.References.size() <= Limit) {
     Req.Limit = Limit;
     Results.HasMore |= Index->refs(Req, [&](const Ref &R) {
       // No need to continue process if we reach the limit.
       if (Results.References.size() > Limit)
         return;
       auto LSPLoc = toLSPLocation(R.Location, *MainFilePath);

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

TEST(FindReferences, WithinAST) {
   }
 }
 
 TEST(FindReferences, IncludeOverrides) {
   llvm::StringRef Test =
       R"cpp(
         class Base {
         public:
-          virtual void [[f^unc]]() = 0;
+          virtual void [[f^unc]]();
         };
         class Derived : public Base {
         public:
           void [[func]]() override;
         };
         void test(Derived* D) {
-          D->[[func]]();
+          D->func();  // No references to the overrides.
         })cpp";
   Annotations T(Test);
   auto TU = TestTU::withCode(T.code());
   auto AST = TU.build();
   std::vector<Matcher<Location>> ExpectedLocations;
   for (const auto &R : T.ranges())
     ExpectedLocations.push_back(RangeIs(R));
   EXPECT_THAT(findReferences(AST, T.point(), 0, TU.index().get()).References,