[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

1408

it might be nicer to make IDs a parameter too.

1410

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> IDs, 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.
     if (auto MacroSID = getSymbolID(Macro->Name, Macro->Info, SM)) {
       // Collect macro references from main file.
       const auto &IDToRefs = AST.getMacros().MacroRefs;
       auto Refs = IDToRefs.find(MacroSID);
       if (Refs != IDToRefs.end()) {
         for (const auto &Ref : Refs->second) {
           ReferencesResult::Reference Result;
           Result.Loc.range = Ref.Rng;
           Result.Loc.uri = URIMainFile;
           if (Ref.IsDefinition) {
             Result.Attributes |= ReferencesResult::Declaration;
             Result.Attributes |= ReferencesResult::Definition;
           }
           Results.References.push_back(std::move(Result));
         }
       }
-      Req.IDs.insert(MacroSID);
+      IDs.insert(MacroSID);
     }
   } else {
     // Handle references to Decls.
 
     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);
+          IDs.insert(ID);
       }
     }
   }
   // Now query the index for references from other files.
-  if (!Req.IDs.empty() && Index && Results.References.size() <= Limit) {
+  auto QueryIndex = [&](llvm::DenseSet<SymbolID> IDs, bool AllowAttributes) {
+    RefsRequest Req;
+    Req.IDs = std::move(IDs);
     Req.Limit = Limit;
+    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(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);
   }
   return Results;
 }
 
 std::vector<SymbolDetails> getSymbolInfo(ParsedAST &AST, Position Pos) {

clang-tools-extra/clangd/index/Index.h

public:
 
   /// Looks up symbols with any of the given symbol IDs and applies \p Callback
   /// on each matched symbol.
   /// The returned symbol must be deep-copied if it's used outside Callback.
   virtual void
   lookup(const LookupRequest &Req,
          llvm::function_ref<void(const Symbol &)> Callback) const = 0;
 
-  /// Finds all occurrences (e.g. references, declarations, definitions) of a
-  /// symbol and applies \p Callback on each result.
+  /// Finds all occurrences (e.g. references, declarations, definitions) of
+  /// symbols and applies \p Callback on each result.
   ///
   /// Results should be returned in arbitrary order.
   /// The returned result must be deep-copied if it's used outside Callback.
+  /// FIXME: there's no indication which result references which symbol.
   ///
   /// Returns true if there will be more results (limited by Req.Limit);
   virtual bool refs(const RefsRequest &Req,
                     llvm::function_ref<void(const Ref &)> Callback) const = 0;
 
   /// Finds all relations (S, P, O) stored in the index such that S is among
   /// Req.Subjects and P is Req.Predicate, and invokes \p Callback for (S, O) in
   /// each.

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