[clangd] references: decls of overrides of x are refs to x, not decls

[sammccall]

Summary

This requires a second index query for refs to overrides, as the refs
call doesn't tell you which ref points at which symbol.

[kadircet]

thanks, lgtm!

clang-tools-extra/clangd/XRefs.cpp

1406

it might be nicer to make IDs a parameter too.

1409

just thinking out loud, i wonder why we don't just provide a symbolid in this callback too. the interface currently says refs will be returned in any order, and i am not sure about all the index implementations we have, but i think they should be able to provide that for each reference. that way we could get rid of multiple index queries (not that it matters too much currently, but it might one day..)

clang-tools-extra/clangd/XRefs.cpp

 namespace {
 
 /// Collects references to symbols within the main file.
 class ReferenceFinder : public index::IndexDataConsumer {
 public:
   struct Reference {
     syntax::Token SpelledTok;
     index::SymbolRoleSet Role;
+    SymbolID Target;
 
     Range range(const SourceManager &SM) const {
       return halfOpenToRange(SM, SpelledTok.range(SM).toCharRange(SM));
     }
   };
 
   ReferenceFinder(const ParsedAST &AST,
                   const llvm::DenseSet<SymbolID> &TargetIDs)
public:
   }
 
   bool
   handleDeclOccurrence(const Decl *D, index::SymbolRoleSet Roles,
                        llvm::ArrayRef<index::SymbolRelation> Relations,
                        SourceLocation Loc,
                        index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
     const SourceManager &SM = AST.getSourceManager();
-    if (!isInsideMainFile(Loc, SM) ||
-        TargetIDs.find(getSymbolID(D)) == TargetIDs.end())
+    if (!isInsideMainFile(Loc, SM))
+      return true;
+    SymbolID ID = getSymbolID(D);
+    if (!TargetIDs.contains(ID))
       return true;
     const auto &TB = AST.getTokens();
     Loc = SM.getFileLoc(Loc);
     if (const auto *Tok = TB.spelledTokenAt(Loc))
-      References.push_back({*Tok, Roles});
+      References.push_back({*Tok, Roles, ID});
     return true;
   }
 
 private:
   std::vector<Reference> References;
   const ParsedAST &AST;
   const llvm::DenseSet<SymbolID> &TargetIDs;
 };
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 {};
   }
 
   RefsRequest Req;
+  llvm::DenseSet<SymbolID> Overrides;
 
   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);
 
-    llvm::DenseSet<SymbolID> Overrides;
     if (Index) {
       RelationsRequest FindOverrides;
       FindOverrides.Predicate = RelationKind::OverriddenBy;
       for (const NamedDecl *ND : Decls) {
         // Special case: virtual void meth^od() = 0 includes refs of overrides.
         if (const auto *CMD = llvm::dyn_cast<CXXMethodDecl>(ND)) {
           if (CMD->isPure())
             if (IdentifierAtCursor && SM.getSpellingLoc(CMD->getLocation()) ==
MainFileRefs.erase(std::unique(MainFileRefs.begin(), MainFileRefs.end(),
                                      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));
     }
     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;
+  auto QueryIndex = [&](bool AllowAttributes) {
+    if (Req.IDs.empty() || !Index || Results.References.size() > Limit)
+      return;
     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);
       // Avoid indexed results for the main file - the AST is authoritative.
       if (!LSPLoc || LSPLoc->uri.file() == *MainFilePath)
         return;
       ReferencesResult::Reference Result;
       Result.Loc = std::move(*LSPLoc);
+      if (AllowAttributes) {
         if ((R.Kind & RefKind::Declaration) == RefKind::Declaration)
           Result.Attributes |= ReferencesResult::Declaration;
         // FIXME: our index should definitely store def | decl separately!
         if ((R.Kind & RefKind::Definition) == RefKind::Definition)
           Result.Attributes |=
               ReferencesResult::Declaration | ReferencesResult::Definition;
+      }
       Results.References.push_back(std::move(Result));
     });
-  }
+  };
+  QueryIndex(/*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?
+  Req.IDs = std::move(Overrides);
+  QueryIndex(/*AllowAttributes=*/false);
   if (Results.References.size() > Limit) {
     Results.HasMore = true;
     Results.References.resize(Limit);
   }
   return Results;
 }
 
 std::vector<SymbolDetails> getSymbolInfo(ParsedAST &AST, Position Pos) {

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

TEST(FindReferences, IncludeOverrides) {
   llvm::StringRef Test =
       R"cpp(
         class Base {
         public:
           virtual void $decl[[f^unc]]() = 0;
         };
         class Derived : public Base {
         public:
-          void $decl[[func]]() override; // FIXME: ref, not decl
+          void [[func]]() override;
         };
         void test(Derived* D) {
           D->[[func]]();
         })cpp";
   checkFindRefs(Test, /*UseIndex=*/true);
 }
 
 TEST(FindReferences, MainFileReferencesOnly) {