[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

1387

I guess we should include the definition location as well?

clang-tools-extra/clangd/XRefs.h

86

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.h

 std::vector<DocumentHighlight> findDocumentHighlights(ParsedAST &AST,
                                                       Position Pos);
 
 struct ReferencesResult {
   // Bitmask describing whether the occurrence is a declaration, definition etc.
   enum ReferenceAttributes : unsigned {
     Declaration = 1 << 0,
     Definition = 1 << 1,
+    // The occurrence is an override of the target base method.
+    Override = 1 << 2,
   };
   struct Reference {
     Location Loc;
     unsigned Attributes = 0;
   };
   std::vector<Reference> References;
   bool HasMore = false;
 };

clang-tools-extra/clangd/XRefs.cpp

ReferencesResult findReferences(ParsedAST &AST, Position Pos, uint32_t Limit,
   }
   auto URIMainFile = URIForFile::canonicalize(*MainFilePath, *MainFilePath);
   auto CurLoc = sourceLocationInMainFile(SM, Pos);
   if (!CurLoc) {
     llvm::consumeError(CurLoc.takeError());
     return {};
   }
 
-  llvm::DenseSet<SymbolID> IDs, Overrides;
+  llvm::DenseSet<SymbolID> IDs;
 
   const auto *IdentifierAtCursor =
       syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens());
   llvm::Optional<DefinedMacro> Macro;
   if (IdentifierAtCursor)
     Macro = locateMacroAt(*IdentifierAtCursor, AST.getPreprocessor());
   if (Macro) {
     // Handle references to macro.
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);
 
+    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) {
       ReferencesResult::Reference Result;
       Result.Loc.range = Ref.range(SM);
       Result.Loc.uri = URIMainFile;
-      // Overrides are always considered references, not defs/decls.
-      if (!Overrides.contains(Ref.Target)) {
-        if (Ref.Role & static_cast<unsigned>(index::SymbolRole::Declaration))
-          Result.Attributes |= ReferencesResult::Declaration;
-        // clang-index doesn't report definitions as declarations, but they are.
-        if (Ref.Role & static_cast<unsigned>(index::SymbolRole::Definition))
-          Result.Attributes |=
-              ReferencesResult::Definition | ReferencesResult::Declaration;
-      }
+      Results.References.push_back(std::move(Result));
+    // Add decl/def 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)) {
+              ReferencesResult::Reference Result;
+              Result.Loc = std::move(*LSPLoc);
+              Result.Attributes =
+                  ReferencesResult::Declaration | ReferencesResult::Override;
               Results.References.push_back(std::move(Result));
             }
+            if (auto LSPLoc = toLSPLocation(Object.Definition, *MainFilePath)) {
+              ReferencesResult::Reference Result;
+              Result.Loc = std::move(*LSPLoc);
+              Result.Attributes = ReferencesResult::Declaration |
+                                  ReferencesResult::Definition |
+                                  ReferencesResult::Override;
+              Results.References.push_back(std::move(Result));
+            }
+          });
+
     if (Index && Results.References.size() <= Limit) {
       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;
Results.HasMore |= Index->refs(Req, [&](const Ref &R) {
         if ((R.Kind & RefKind::Definition) == RefKind::Definition)
           Result.Attributes |=
               ReferencesResult::Declaration | ReferencesResult::Definition;
       }
       Results.References.push_back(std::move(Result));
     });
   };
   QueryIndex(std::move(IDs), /*AllowAttributes=*/true);
-  // FIXME: Currently we surface decls/defs/refs of derived methods.
-  // Maybe we'd prefer decls/defs of derived methods, and refs of base methods?
-  QueryIndex(std::move(Overrides), /*AllowAttributes=*/false);
   if (Results.References.size() > Limit) {
     Results.HasMore = true;
     Results.References.resize(Limit);
   }
+  // FIXME: Report refs of base methods.
   return Results;
 }
 
 std::vector<SymbolDetails> getSymbolInfo(ParsedAST &AST, Position Pos) {
   const SourceManager &SM = AST.getSourceManager();
   auto CurLoc = sourceLocationInMainFile(SM, Pos);
   if (!CurLoc) {
     llvm::consumeError(CurLoc.takeError());

 
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
                               const ReferencesResult::Reference &R) {
   OS << R.Loc;
   if (R.Attributes & ReferencesResult::Declaration)
     OS << " [decl]";
   if (R.Attributes & ReferencesResult::Definition)
     OS << " [def]";
+  if (R.Attributes & ReferencesResult::Override)
+    OS << " [override]";
   return OS;
 }
 
 template <typename HierarchyItem>
 static llvm::Optional<HierarchyItem> declToHierarchyItem(const NamedDecl &ND) {
   ASTContext &Ctx = ND.getASTContext();
   auto &SM = Ctx.getSourceManager();
   SourceLocation NameLoc = nameLocation(ND, Ctx.getSourceManager());

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

void checkFindRefs(llvm::StringRef Test, bool UseIndex = false) {
   // If we have cases that are definition-only, we should change this.
   for (const auto &R : T.ranges("def"))
     ExpectedLocations.push_back(
         AllOf(RangeIs(R), AttrsAre(ReferencesResult::Definition |
                                    ReferencesResult::Declaration)));
   for (const auto &R : T.ranges("decl"))
     ExpectedLocations.push_back(
         AllOf(RangeIs(R), AttrsAre(ReferencesResult::Declaration)));
+  for (const auto &R : T.ranges("overridedecl"))
+    ExpectedLocations.push_back(AllOf(
+        RangeIs(R),
+        AttrsAre(ReferencesResult::Declaration | ReferencesResult::Override)));
+  for (const auto &R : T.ranges("overridedef"))
+    ExpectedLocations.push_back(
+        AllOf(RangeIs(R), AttrsAre(ReferencesResult::Declaration |
+                                   ReferencesResult::Definition |
+                                   ReferencesResult::Override)));
   EXPECT_THAT(
       findReferences(AST, T.point(), 0, UseIndex ? TU.index().get() : nullptr)
           .References,
       UnorderedElementsAreArray(ExpectedLocations))
       << Test;
 }
 
 TEST(FindReferences, WithinAST) {
TEST(FindReferences, IncludeOverrides) {
   llvm::StringRef Test =
       R"cpp(
         class Base {
         public:
           virtual void $decl[[f^unc]]() = 0;
         };
         class Derived : public Base {
         public:
-          void [[func]]() override;
+          void $overridedecl[[func]]() override;
         };
+        void Derived::$overridedef[[func]]() {}
         void test(Derived* D) {
-          D->[[func]]();
+          D->func();  // No references to the overrides.
         })cpp";
   checkFindRefs(Test, /*UseIndex=*/true);
 }
 
 TEST(FindReferences, MainFileReferencesOnly) {
   llvm::StringRef Test =
       R"cpp(
         void test() {