[Index] Return SourceLocation to consumers, not FileID/Offset pair.

[sammccall]

Summary

The FileID/Offset conversion is lossy. The code takes the fileLoc, which loses
e.g. the spelling location in some macro cases.
Instead, pass the original SourceLocation which preserves all information, and
update consumers to match current behavior.

This allows us to fix two bugs in clangd that need the spelling location.

[sammccall]

Ping...

@akyrtzi @arphaman I'm confident there's no functional change in this patch so I'm happy to land it with just a review from clangd folks, but would be great to know if you're happy with the interface change.

[sammccall]

Oops, I got my reviews crossed and thought @ilya-biryukov had already approved it (we chatted about this today)

[ilya-biryukov]

LGTM, but let's watch out for @akyrtzi's and @arphaman's comments, just in case they're not happy with the change. In that case we'll have to revert it.

[hans]

Some bots are sad:
http://lab.llvm.org:8011/builders/clang-atom-d525-fedora-rel/builds/16346/
http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/17364/
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/28078/

[sammccall]

The matching clangd patch is r329571, I got distracted and it took me a few
minutes to land it. Apologies!

include/clang/Index/IndexDataConsumer.h

public:
 
   virtual void initialize(ASTContext &Ctx) {}
 
   virtual void setPreprocessor(std::shared_ptr<Preprocessor> PP) {}
 
   /// \returns true to continue indexing, or false to abort.
   virtual bool handleDeclOccurence(const Decl *D, SymbolRoleSet Roles,
                                    ArrayRef<SymbolRelation> Relations,
-                                   FileID FID, unsigned Offset,
-                                   ASTNodeInfo ASTNode);
+                                   SourceLocation Loc, ASTNodeInfo ASTNode);
 
   /// \returns true to continue indexing, or false to abort.
   virtual bool handleMacroOccurence(const IdentifierInfo *Name,
                                     const MacroInfo *MI, SymbolRoleSet Roles,
-                                    FileID FID, unsigned Offset);
+                                    SourceLocation Loc);
 
   /// \returns true to continue indexing, or false to abort.
   virtual bool handleModuleOccurence(const ImportDecl *ImportD,
-                                     SymbolRoleSet Roles,
-                                     FileID FID, unsigned Offset);
+                                     SymbolRoleSet Roles, SourceLocation Loc);
 
   virtual void finish() {}
 
 private:
   virtual void _anchor();
 };
 
 } // namespace index
 } // namespace clang
 
 #endif

lib/Index/IndexingAction.cpp

 
 using namespace clang;
 using namespace clang::index;
 
 void IndexDataConsumer::_anchor() {}
 
 bool IndexDataConsumer::handleDeclOccurence(const Decl *D, SymbolRoleSet Roles,
                                             ArrayRef<SymbolRelation> Relations,
-                                            FileID FID, unsigned Offset,
+                                            SourceLocation Loc,
                                             ASTNodeInfo ASTNode) {
   return true;
 }
 
 bool IndexDataConsumer::handleMacroOccurence(const IdentifierInfo *Name,
-                                             const MacroInfo *MI, SymbolRoleSet Roles,
-                                             FileID FID, unsigned Offset) {
+                                             const MacroInfo *MI,
+                                             SymbolRoleSet Roles,
+                                             SourceLocation Loc) {
   return true;
 }
 
 bool IndexDataConsumer::handleModuleOccurence(const ImportDecl *ImportD,
                                               SymbolRoleSet Roles,
-                                              FileID FID, unsigned Offset) {
+                                              SourceLocation Loc) {
   return true;
 }
 
 namespace {
 
 class IndexASTConsumer : public ASTConsumer {
   std::shared_ptr<Preprocessor> PP;
   IndexingContext &IndexCtx;

lib/Index/IndexingContext.cpp

 
 bool IndexingContext::importedModule(const ImportDecl *ImportD) {
   SourceLocation Loc;
   auto IdLocs = ImportD->getIdentifierLocs();
   if (!IdLocs.empty())
     Loc = IdLocs.front();
   else
     Loc = ImportD->getLocation();
-  SourceManager &SM = Ctx->getSourceManager();
-  Loc = SM.getFileLoc(Loc);
-  if (Loc.isInvalid())
-    return true;
 
-  FileID FID;
-  unsigned Offset;
-  std::tie(FID, Offset) = SM.getDecomposedLoc(Loc);
+  SourceManager &SM = Ctx->getSourceManager();
+  FileID FID = SM.getFileID(SM.getFileLoc(Loc));
   if (FID.isInvalid())
     return true;
 
   bool Invalid = false;
   const SrcMgr::SLocEntry &SEntry = SM.getSLocEntry(FID, &Invalid);
   if (Invalid || !SEntry.isFile())
     return true;
 
   if (SEntry.getFile().getFileCharacteristic() != SrcMgr::C_User) {
     switch (IndexOpts.SystemSymbolFilter) {
     case IndexingOptions::SystemSymbolFilterKind::None:
       return true;
     case IndexingOptions::SystemSymbolFilterKind::DeclarationsOnly:
     case IndexingOptions::SystemSymbolFilterKind::All:
       break;
     }
   }
 
   SymbolRoleSet Roles = (unsigned)SymbolRole::Declaration;
   if (ImportD->isImplicit())
     Roles |= (unsigned)SymbolRole::Implicit;
 
-  return DataConsumer.handleModuleOccurence(ImportD, Roles, FID, Offset);
+  return DataConsumer.handleModuleOccurence(ImportD, Roles, Loc);
 }
 
 bool IndexingContext::isTemplateImplicitInstantiation(const Decl *D) {
   TemplateSpecializationKind TKind = TSK_Undeclared;
   if (const ClassTemplateSpecializationDecl *
       SD = dyn_cast<ClassTemplateSpecializationDecl>(D)) {
     TKind = SD->getSpecializationKind();
   } else if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
bool IndexingContext::handleDeclOccurrence(const Decl *D, SourceLocation Loc,
                                            const Decl *OrigD,
                                            const DeclContext *ContainerDC) {
   if (D->isImplicit() && !isa<ObjCMethodDecl>(D))
     return true;
   if (!isa<NamedDecl>(D) || shouldSkipNamelessDecl(cast<NamedDecl>(D)))
     return true;
 
   SourceManager &SM = Ctx->getSourceManager();
-  Loc = SM.getFileLoc(Loc);
-  if (Loc.isInvalid())
-    return true;
-
-  FileID FID;
-  unsigned Offset;
-  std::tie(FID, Offset) = SM.getDecomposedLoc(Loc);
+  FileID FID = SM.getFileID(SM.getFileLoc(Loc));
   if (FID.isInvalid())
     return true;
 
   bool Invalid = false;
   const SrcMgr::SLocEntry &SEntry = SM.getSLocEntry(FID, &Invalid);
   if (Invalid || !SEntry.isFile())
     return true;
 
if (Parent) {
     }
   }
 
   for (auto &Rel : Relations) {
     addRelation(SymbolRelation(Rel.Roles,
                                Rel.RelatedSymbol->getCanonicalDecl()));
   }
 
-  IndexDataConsumer::ASTNodeInfo Node{ OrigE, OrigD, Parent, ContainerDC };
-  return DataConsumer.handleDeclOccurence(D, Roles, FinalRelations, FID, Offset,
-                                          Node);
+  IndexDataConsumer::ASTNodeInfo Node{OrigE, OrigD, Parent, ContainerDC};
+  return DataConsumer.handleDeclOccurence(D, Roles, FinalRelations, Loc, Node);
 }

tools/c-index-test/core_main.cpp

public:
   }
 
   void initialize(ASTContext &Ctx) override {
     CGNameGen.reset(new CodegenNameGenerator(Ctx));
   }
 
   bool handleDeclOccurence(const Decl *D, SymbolRoleSet Roles,
                            ArrayRef<SymbolRelation> Relations,
-                           FileID FID, unsigned Offset,
-                           ASTNodeInfo ASTNode) override {
+                           SourceLocation Loc, ASTNodeInfo ASTNode) override {
     ASTContext &Ctx = D->getASTContext();
     SourceManager &SM = Ctx.getSourceManager();
 
-    unsigned Line = SM.getLineNumber(FID, Offset);
-    unsigned Col = SM.getColumnNumber(FID, Offset);
+    Loc = SM.getFileLoc(Loc);
+    FileID FID = SM.getFileID(Loc);
+    unsigned Line = SM.getLineNumber(FID, SM.getFileOffset(Loc));
+    unsigned Col = SM.getColumnNumber(FID, SM.getFileOffset(Loc));
     OS << Line << ':' << Col << " | ";
 
     printSymbolInfo(getSymbolInfo(D), OS);
     OS << " | ";
 
     printSymbolNameAndUSR(D, Ctx, OS);
     OS << " | ";
 
for (auto &SymRel : Relations) {
       printSymbolNameAndUSR(SymRel.RelatedSymbol, Ctx, OS);
       OS << '\n';
     }
 
     return true;
   }
 
   bool handleModuleOccurence(const ImportDecl *ImportD, SymbolRoleSet Roles,
-                             FileID FID, unsigned Offset) override {
+                             SourceLocation Loc) override {
     ASTContext &Ctx = ImportD->getASTContext();
     SourceManager &SM = Ctx.getSourceManager();
 
-    unsigned Line = SM.getLineNumber(FID, Offset);
-    unsigned Col = SM.getColumnNumber(FID, Offset);
+    Loc = SM.getFileLoc(Loc);
+    FileID FID = SM.getFileID(Loc);
+    unsigned Line = SM.getLineNumber(FID, SM.getFileOffset(Loc));
+    unsigned Col = SM.getColumnNumber(FID, SM.getFileOffset(Loc));
     OS << Line << ':' << Col << " | ";
 
     printSymbolInfo(getSymbolInfo(ImportD), OS);
     OS << " | ";
 
     OS << ImportD->getImportedModule()->getFullModuleName() << " | ";
 
     printSymbolRoles(Roles, OS);

tools/libclang/CXIndexDataConsumer.h

public:
   CXIdxClientEntity getClientEntity(const Decl *D) const;
   void setClientEntity(const Decl *D, CXIdxClientEntity client);
 
   static bool isTemplateImplicitInstantiation(const Decl *D);
 
 private:
   bool handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
                            ArrayRef<index::SymbolRelation> Relations,
-                           FileID FID, unsigned Offset,
-                           ASTNodeInfo ASTNode) override;
+                           SourceLocation Loc, ASTNodeInfo ASTNode) override;
 
   bool handleModuleOccurence(const ImportDecl *ImportD,
                              index::SymbolRoleSet Roles,
-                             FileID FID, unsigned Offset) override;
+                             SourceLocation Loc) override;
 
   void finish() override;
 
   bool handleDecl(const NamedDecl *D,
                   SourceLocation Loc, CXCursor Cursor,
                   DeclInfo &DInfo,
                   const DeclContext *LexicalDC = nullptr,
                   const DeclContext *SemaDC = nullptr);

tools/libclang/CXIndexDataConsumer.cpp

 };
 
 CXSymbolRole getSymbolRole(SymbolRoleSet Role) {
   // CXSymbolRole mirrors low 9 bits of clang::index::SymbolRole.
   return CXSymbolRole(static_cast<uint32_t>(Role) & ((1 << 9) - 1));
 }
 }
 
-bool CXIndexDataConsumer::handleDeclOccurence(const Decl *D,
-                                              SymbolRoleSet Roles,
-                                             ArrayRef<SymbolRelation> Relations,
-                                              FileID FID, unsigned Offset,
-                                              ASTNodeInfo ASTNode) {
-  SourceLocation Loc = getASTContext().getSourceManager()
-      .getLocForStartOfFile(FID).getLocWithOffset(Offset);
+bool CXIndexDataConsumer::handleDeclOccurence(
+    const Decl *D, SymbolRoleSet Roles, ArrayRef<SymbolRelation> Relations,
+    SourceLocation Loc, ASTNodeInfo ASTNode) {
+  Loc = getASTContext().getSourceManager().getFileLoc(Loc);
 
   if (Roles & (unsigned)SymbolRole::Reference) {
     const NamedDecl *ND = dyn_cast<NamedDecl>(D);
     if (!ND)
       return true;
 
     if (auto *ObjCID = dyn_cast_or_null<ObjCInterfaceDecl>(ASTNode.OrigD)) {
       if (!ObjCID->isThisDeclarationADefinition() &&
if (Roles & (unsigned)SymbolRole::Reference) {
     IndexingDeclVisitor(*this, Loc, LexicalDC).Visit(ASTNode.OrigD);
   }
 
   return !shouldAbort();
 }
 
 bool CXIndexDataConsumer::handleModuleOccurence(const ImportDecl *ImportD,
                                                 SymbolRoleSet Roles,
-                                                FileID FID,
-                                                unsigned Offset) {
+                                                SourceLocation Loc) {
   IndexingDeclVisitor(*this, SourceLocation(), nullptr).Visit(ImportD);
   return !shouldAbort();
 }
 
 void CXIndexDataConsumer::finish() {
   indexDiagnostics();
 }