[clangd] Adapt index interfaces to D45014, and fix the old bugs.

[sammccall]

Summary

[clangd] Adapt index interfaces to D45014, and fix the old bugs.

• don't count occurrences of decls where we don't spell the name

[sammccall]

Also fix source/expansion location confusion in XRefs (findDefinition et al)

[ilya-biryukov]

LGTM

clangd/XRefs.cpp

auto Last = std::unique(MacroInfos.begin(), MacroInfos.end(),
                               return Left.Info == Right.Info;
                             });
     MacroInfos.erase(Last, MacroInfos.end());
     return std::move(MacroInfos);
   }
 
   bool
   handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
-                      ArrayRef<index::SymbolRelation> Relations, FileID FID,
-                      unsigned Offset,
+                      ArrayRef<index::SymbolRelation> Relations,
+                      SourceLocation Loc,
                       index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
-    if (isSearchedLocation(FID, Offset)) {
+    if (Loc == SearchedLocation) {
       // Find and add definition declarations (for GoToDefinition).
       // We don't use parameter `D`, as Parameter `D` is the canonical
       // declaration, which is the first declaration of a redeclarable
       // declaration, and it could be a forward declaration.
       if (const auto *Def = GetDefinition(D)) {
         Decls.push_back(Def);
       } else {
         // Couldn't find a definition, fall back to use `D`.
         Decls.push_back(D);
       }
     }
     return true;
   }
 
 private:
-  bool isSearchedLocation(FileID FID, unsigned Offset) const {
-    const SourceManager &SourceMgr = AST.getSourceManager();
-    return SourceMgr.getFileOffset(SearchedLocation) == Offset &&
-           SourceMgr.getFileID(SearchedLocation) == FID;
-  }
-
   void finish() override {
     // Also handle possible macro at the searched location.
     Token Result;
     auto &Mgr = AST.getSourceManager();
-    if (!Lexer::getRawToken(SearchedLocation, Result, Mgr, AST.getLangOpts(),
-                            false)) {
+    if (!Lexer::getRawToken(Mgr.getSpellingLoc(SearchedLocation), Result, Mgr,
+                            AST.getLangOpts(), false)) {
       if (Result.is(tok::raw_identifier)) {
         PP.LookUpIdentifierInfo(Result);
       }
       IdentifierInfo *IdentifierInfo = Result.getIdentifierInfo();
       if (IdentifierInfo && IdentifierInfo->hadMacroDefinition()) {
         std::pair<FileID, unsigned int> DecLoc =
             Mgr.getDecomposedExpansionLoc(SearchedLocation);
         // Get the definition just before the searched location so that a macro
         // referenced in a '#undef MACRO' can still be found.
         SourceLocation BeforeSearchedLocation = Mgr.getMacroArgExpandedLocation(
             Mgr.getLocForStartOfFile(DecLoc.first)
                 .getLocWithOffset(DecLoc.second - 1));
         MacroDefinition MacroDef =
             PP.getMacroDefinitionAtLoc(IdentifierInfo, BeforeSearchedLocation);
         MacroInfo *MacroInf = MacroDef.getMacroInfo();
         if (MacroInf) {
           MacroInfos.push_back(MacroDecl{IdentifierInfo->getName(), MacroInf});
-          // Clear all collected delcarations if this is a macro search.
-          //
-          // In theory, there should be no declarataions being collected when we
-          // search a source location that refers to a macro.
-          // The occurrence location returned by `handleDeclOccurence` is
-          // limited (FID, Offset are from expansion location), we will collect
-          // all declarations inside the macro.
-          //
-          // FIXME: Avoid adding decls from inside macros in handlDeclOccurence.
-          Decls.clear();
+          assert(Decls.empty());
         }
       }
     }
   }
 };
 
 llvm::Optional<Location>
 makeLocation(ParsedAST &AST, const SourceRange &ValSourceRange) {
std::vector<DocumentHighlight> takeHighlights() {
     auto Last =
         std::unique(DocumentHighlights.begin(), DocumentHighlights.end());
     DocumentHighlights.erase(Last, DocumentHighlights.end());
     return std::move(DocumentHighlights);
   }
 
   bool
   handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
-                      ArrayRef<index::SymbolRelation> Relations, FileID FID,
-                      unsigned Offset,
+                      ArrayRef<index::SymbolRelation> Relations,
+                      SourceLocation Loc,
                       index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
     const SourceManager &SourceMgr = AST.getSourceManager();
-    if (SourceMgr.getMainFileID() != FID ||
+    SourceLocation HighlightStartLoc = SourceMgr.getFileLoc(Loc);
+    if (SourceMgr.getMainFileID() != SourceMgr.getFileID(HighlightStartLoc) ||
         std::find(Decls.begin(), Decls.end(), D) == Decls.end()) {
       return true;
     }
     SourceLocation End;
     const LangOptions &LangOpts = AST.getLangOpts();
-    SourceLocation StartOfFileLoc = SourceMgr.getLocForStartOfFile(FID);
-    SourceLocation HightlightStartLoc = StartOfFileLoc.getLocWithOffset(Offset);
-    End =
-        Lexer::getLocForEndOfToken(HightlightStartLoc, 0, SourceMgr, LangOpts);
-    SourceRange SR(HightlightStartLoc, End);
+    End = Lexer::getLocForEndOfToken(HighlightStartLoc, 0, SourceMgr, LangOpts);
+    SourceRange SR(HighlightStartLoc, End);
 
     DocumentHighlightKind Kind = DocumentHighlightKind::Text;
     if (static_cast<index::SymbolRoleSet>(index::SymbolRole::Write) & Roles)
       Kind = DocumentHighlightKind::Write;
     else if (static_cast<index::SymbolRoleSet>(index::SymbolRole::Read) & Roles)
       Kind = DocumentHighlightKind::Read;
 
     DocumentHighlights.push_back(getDocumentHighlight(SR, Kind));

clangd/index/SymbolCollector.h

public:
   void initialize(ASTContext &Ctx) override;
 
   void setPreprocessor(std::shared_ptr<Preprocessor> PP) override {
     this->PP = std::move(PP);
   }
 
   bool
   handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
-                      ArrayRef<index::SymbolRelation> Relations, FileID FID,
-                      unsigned Offset,
+                      ArrayRef<index::SymbolRelation> Relations,
+                      SourceLocation Loc,
                       index::IndexDataConsumer::ASTNodeInfo ASTNode) override;
 
   SymbolSlab takeSymbols() { return std::move(Symbols).build(); }
 
   void finish() override;
 
 private:
   const Symbol *addDeclaration(const NamedDecl &, SymbolID);

clangd/index/SymbolCollector.cpp

void SymbolCollector::initialize(ASTContext &Ctx) {
   CompletionAllocator = std::make_shared<GlobalCodeCompletionAllocator>();
   CompletionTUInfo =
       llvm::make_unique<CodeCompletionTUInfo>(CompletionAllocator);
 }
 
 // Always return true to continue indexing.
 bool SymbolCollector::handleDeclOccurence(
     const Decl *D, index::SymbolRoleSet Roles,
-    ArrayRef<index::SymbolRelation> Relations, FileID FID, unsigned Offset,
+    ArrayRef<index::SymbolRelation> Relations, SourceLocation Loc,
     index::IndexDataConsumer::ASTNodeInfo ASTNode) {
   assert(ASTCtx && PP.get() && "ASTContext and Preprocessor must be set.");
   assert(CompletionAllocator && CompletionTUInfo);
   const NamedDecl *ND = llvm::dyn_cast<NamedDecl>(D);
   if (!ND)
     return true;
 
   // Mark D as referenced if this is a reference coming from the main file.
   // D may not be an interesting symbol, but it's cheaper to check at the end.
+  auto &SM = ASTCtx->getSourceManager();
   if (Opts.CountReferences &&
       (Roles & static_cast<unsigned>(index::SymbolRole::Reference)) &&
-      ASTCtx->getSourceManager().getMainFileID() == FID)
+      SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
     ReferencedDecls.insert(ND);
 
   // Don't continue indexing if this is a mere reference.
   if (!(Roles & static_cast<unsigned>(index::SymbolRole::Declaration) ||
         Roles & static_cast<unsigned>(index::SymbolRole::Definition)))
     return true;
   if (shouldFilterDecl(ND, ASTCtx, Opts))
     return true;

unittests/clangd/SymbolCollectorTests.cpp

 
 TEST_F(SymbolCollectorTest, References) {
   const std::string Header = R"(
     class W;
     class X {};
     class Y;
     class Z {}; // not used anywhere
     Y* y = nullptr;  // used in header doesn't count
+    #define GLOBAL_Z(name) Z name;
   )";
   const std::string Main = R"(
     W* w = nullptr;
     W* w2 = nullptr; // only one usage counts
     X x();
     class V;
     V* v = nullptr; // Used, but not eligible for indexing.
     class Y{}; // definition doesn't count as a reference
+    GLOBAL_Z(z); // Not a reference to Z, we don't spell the type.
   )";
   CollectorOpts.CountReferences = true;
   runSymbolCollector(Header, Main);
   EXPECT_THAT(Symbols,
               UnorderedElementsAre(AllOf(QName("W"), Refs(1)),
                                    AllOf(QName("X"), Refs(1)),
                                    AllOf(QName("Y"), Refs(0)),
                                    AllOf(QName("Z"), Refs(0)), QName("y")));

unittests/clangd/XRefsTests.cpp

const char *Tests[] = {
        class TTT { public: int a; };
        #define [[FF]](S) if (int b = S.a) {}
        void f() {
          TTT t;
          F^F(t);
        }
       )cpp",
 
+      R"cpp(// Macro argument
+       int [[i]];
+       #define ADDRESSOF(X) &X;
+       int *j = ADDRESSOF(^i);
+      )cpp",
+
+      R"cpp(// Symbol concatenated inside macro (not supported)
+       int *pi;
+       #define POINTER(X) p # X;
+       int i = *POINTER(^i);
+      )cpp",
+
       R"cpp(// Forward class declaration
         class Foo;
         class [[Foo]] {};
         F^oo* foo();
       )cpp",
 
       R"cpp(// Function declaration
         void foo();
const char *Tests[] = {
          #define FF() class [[Test]] {};
          FF();
          void f() { T^est a; }
       )cpp",
   };
   for (const char *Test : Tests) {
     Annotations T(Test);
     auto AST = build(T.code());
+    std::vector<Matcher<Location>> ExpectedLocations;
+    for (const auto &R : T.ranges())
+      ExpectedLocations.push_back(RangeIs(R));
     EXPECT_THAT(findDefinitions(AST, T.point()),
-                ElementsAre(RangeIs(T.range())))
+                ElementsAreArray(ExpectedLocations))
         << Test;
   }
 }
 
 TEST(GoToDefinition, RelPathsInCompileCommand) {
   Annotations SourceAnnotations(R"cpp(
 int [[foo]];
 int baz = f^oo;