fix: Replace canonical paths with StableFileId (#143)

varungandhi-src · web-flow · commit 5796298a4c4c · 2023-03-26T10:30:12.000+09:00
diff --git a/indexer/Path.cc b/indexer/Path.cc
@@ -43,6 +43,15 @@ AbsolutePathRef::makeRelative(AbsolutePathRef longerPath) {
   return {};
 }
 
+std::optional<std::string_view> AbsolutePathRef::fileName() const {
+  auto sview = this->asStringView();
+  auto i = sview.find_last_of(std::filesystem::path::preferred_separator);
+  if (i == std::string::npos || i == sview.size() - 1) {
+    return {};
+  }
+  return sview.substr(i + 1);
+}
+
 AbsolutePathRef AbsolutePath::asRef() const {
   auto sv = std::string_view(this->value.data(), this->value.size());
   auto optRef = AbsolutePathRef::tryFrom(sv);
@@ -93,6 +102,14 @@ RootRelativePath::RootRelativePath(RootRelativePathRef ref)
   ENFORCE(llvm::sys::path::is_relative(this->value));
 }
 
+RootRelativePath::RootRelativePath(std::string &&path, RootKind kind)
+    : value(std::move(path)), _kind(kind) {
+  if (this->value.empty()) {
+    return;
+  }
+  ENFORCE(llvm::sys::path::is_relative(this->value));
+}
+
 void RootRelativePath::replaceExtension(std::string_view newExtension) {
   auto it = this->value.rfind('.');
   if (it == std::string::npos) {
diff --git a/indexer/Path.h b/indexer/Path.h
@@ -40,6 +40,10 @@ class AbsolutePathRef {
 
   // Basic prefix-based implementation; does not handle lexical normalization.
   std::optional<std::string_view> makeRelative(AbsolutePathRef longerPath);
+
+  /// Try to get the file name by slicing off the prefix till the last
+  /// path separator.
+  std::optional<std::string_view> fileName() const;
 };
 
 /// Typically used when referring to paths for files which may or may not
@@ -124,6 +128,8 @@ class RootRelativePath {
 
   explicit RootRelativePath(RootRelativePathRef ref);
 
+  RootRelativePath(std::string &&, RootKind);
+
   const std::string &asStringRef() const {
     return this->value;
   }
diff --git a/indexer/SymbolFormatter.cc b/indexer/SymbolFormatter.cc
@@ -136,16 +136,16 @@ std::string_view SymbolFormatter::getMacroSymbol(clang::SourceLocation defLoc) {
   auto defPLoc =
       this->sourceManager.getPresumedLoc(defLoc, /*UseLineDirectives*/ false);
   ENFORCE(defPLoc.isValid());
-  std::string_view filename;
-  if (auto optRelPath = this->getCanonicalPath(defPLoc.getFileID())) {
-    filename = optRelPath->asStringView();
+  std::string_view filepath;
+  if (auto optStableFileId = this->getStableFileId(defPLoc.getFileID())) {
+    filepath = optStableFileId->path.asStringView();
   } else {
-    filename = std::string_view(defPLoc.getFilename());
+    filepath = std::string_view(defPLoc.getFilename());
   }
 
   // Technically, ':' is used by SCIP for <meta>, but using ':'
   // here lines up with other situations like compiler errors.
-  auto name = this->formatTemporary("{}:{}:{}", filename, defPLoc.getLine(),
+  auto name = this->formatTemporary("{}:{}:{}", filepath, defPLoc.getLine(),
                                     defPLoc.getColumn());
   std::string out{};
   SymbolBuilder{.packageName = "todo-pkg",
@@ -313,10 +313,10 @@ SymbolFormatter::getTagSymbol(const clang::TagDecl &tagDecl) {
       //
       // If we don't include the hash, the anonymous structs will end up with
       // the same symbol name.
-      if (auto optRelativePath = this->getCanonicalPath(defFileId)) {
+      if (auto optStableFileId = this->getStableFileId(defFileId)) {
         descriptor.name = this->formatTemporary(
             "$anonymous_type_{:x}_{}",
-            HashValue::forText(optRelativePath->asStringView()), counter);
+            HashValue::forText(optStableFileId->path.asStringView()), counter);
       }
     }
     if (descriptor.name.empty()) {
@@ -444,23 +444,12 @@ SymbolFormatter::getNamespaceSymbol(const clang::NamespaceDecl &namespaceDecl) {
         if (namespaceDecl.isAnonymousNamespace()) {
           auto mainFileId = this->sourceManager.getMainFileID();
           ENFORCE(mainFileId.isValid());
-          auto path = this->getCanonicalPath(mainFileId);
-          if (!path.has_value()) {
-            // Strictly speaking, this will be suboptimal in the following case:
-            // - header 1: in source tree (has canonical path), uses
-            //     namespace {..}
-            // - header 2: in source tree (has canonical path), uses
-            //     namespace {..}
-            // - generated C++ file: in build tree only (no canonical path), and
-            //   includes header 1 and 2.
-            // We will not emit a symbol that connects the anonymous namespace
-            // in header 1 and header 2. However, that is OK, because it is
-            // unclear how to handle this case anyways, and anonymous namespaces
-            // are rarely (if ever) used in headers.
-            return {};
-          }
+          auto optStableFileId = this->getStableFileId(mainFileId);
+          ENFORCE(optStableFileId.has_value(),
+                  "main file always has a valid StableFileId");
+          auto path = optStableFileId->path;
           descriptor.name = this->formatTemporary("$anonymous_namespace_{}",
-                                                  path->asStringView());
+                                                  path.asStringView());
         } else {
           descriptor.name = this->formatTemporary(namespaceDecl);
         }
diff --git a/indexer/SymbolFormatter.h b/indexer/SymbolFormatter.h
@@ -53,9 +53,6 @@ class raw_ostream;
 
 namespace scip_clang {
 
-using GetCanonicalPath =
-    absl::FunctionRef<std::optional<RootRelativePathRef>(clang::FileID)>;
-
 /// Type similar to \c scip::Descriptor but carrying \c string_view fields
 /// instead to avoid redundant intermediate allocations.
 struct DescriptorBuilder {
@@ -93,9 +90,36 @@ struct SymbolBuilder {
                                       const DescriptorBuilder &descriptor);
 };
 
+/// An identifier for a file that is stable across indexing runs,
+/// represented as a path.
+///
+/// There are 4 kinds of files:
+/// 1. In-project files.
+/// 2. Generated files: These are present in the build root,
+///    but not in the project root.
+/// 3. External files: From libraries (stdlib, SDKs etc.)
+/// 4. Magic files: Corresponding to the builtin header,
+///    and command-line arguments.
+///
+/// For 2, 3 and 4, we make up fake paths that are likely to be
+/// distinct from actual in-project paths.
+///
+/// In the future, for cross-repo, a directory layout<->project mapping
+/// may be supplied or inferred, which would enable us to use non-synthetic
+/// paths for external files.
+struct StableFileId {
+  RootRelativePathRef path;
+  bool isInProject;
+  /// Track this for debugging.
+  bool isSynthetic;
+};
+
+using GetStableFileId =
+    absl::FunctionRef<std::optional<StableFileId>(clang::FileID)>;
+
 class SymbolFormatter final {
   const clang::SourceManager &sourceManager;
-  GetCanonicalPath getCanonicalPath;
+  GetStableFileId getStableFileId;
 
   // Q: Should we allocate into an arena instead of having standalone
   // std::string values here?
@@ -112,8 +136,8 @@ class SymbolFormatter final {
 
 public:
   SymbolFormatter(const clang::SourceManager &sourceManager,
-                  GetCanonicalPath getCanonicalPath)
-      : sourceManager(sourceManager), getCanonicalPath(getCanonicalPath),
+                  GetStableFileId getStableFileId)
+      : sourceManager(sourceManager), getStableFileId(getStableFileId),
         locationBasedCache(), declBasedCache(), scratchBuffer() {}
   SymbolFormatter(const SymbolFormatter &) = delete;
   SymbolFormatter &operator=(const SymbolFormatter &) = delete;
diff --git a/indexer/Worker.cc b/indexer/Worker.cc
@@ -616,24 +616,32 @@ using FileIdsToBeIndexedSet =
 /// In the future, for cross-repo, a directory layout<->project mapping
 /// may be supplied or inferred, which would provide canonical relative
 /// paths for more files.
-class CanonicalPathMap final {
+class StableFileIdMap final {
+  std::vector<RootRelativePath> storage;
+
+  struct ExternalFileEntry {
+    AbsolutePathRef absPath;
+    // Points to storage
+    RootRelativePathRef fakeRelativePath;
+  };
+
   absl::flat_hash_map<llvm_ext::AbslHashAdapter<clang::FileID>,
-                      std::variant<RootRelativePathRef, AbsolutePathRef>>
+                      std::variant<RootRelativePathRef, ExternalFileEntry>>
       map;
 
   const RootPath &projectRootPath;
 
   const RootPath &buildRootPath;
 
 public:
-  CanonicalPathMap() = delete;
-  CanonicalPathMap(const RootPath &projectRootPath,
-                   const RootPath &buildRootPath)
+  StableFileIdMap() = delete;
+  StableFileIdMap(const RootPath &projectRootPath,
+                  const RootPath &buildRootPath)
       : map(), projectRootPath(projectRootPath), buildRootPath(buildRootPath) {}
-  CanonicalPathMap(CanonicalPathMap &&other) = default;
-  CanonicalPathMap &operator=(CanonicalPathMap &&) = delete;
-  CanonicalPathMap(const CanonicalPathMap &) = delete;
-  CanonicalPathMap &operator=(const CanonicalPathMap &) = delete;
+  StableFileIdMap(StableFileIdMap &&other) = default;
+  StableFileIdMap &operator=(StableFileIdMap &&) = delete;
+  StableFileIdMap(const StableFileIdMap &) = delete;
+  StableFileIdMap &operator=(const StableFileIdMap &) = delete;
 
   void populate(const ClangIdLookupMap &clangIdLookupMap) {
     this->map.reserve(clangIdLookupMap.size());
@@ -683,28 +691,40 @@ class CanonicalPathMap final {
                    this->projectRootPath.tryMakeRelative(absPathRef)) {
       return insertRelPath(optProjectRootRelPath.value());
     }
-    return this->map.insert({{fileId}, absPathRef}).second;
+    auto optFileName = absPathRef.fileName();
+    ENFORCE(optFileName.has_value(),
+            "Clang returned file path {} without a file name",
+            absPathRef.asStringView());
+    this->storage.emplace_back(
+        fmt::format("<external>/{}/{}",
+                    HashValue::forText(absPathRef.asStringView()),
+                    *optFileName),
+        RootKind::Build); // fake value to satisfy the RootRelativePathRef API
+    return this->map
+        .insert({{fileId},
+                 ExternalFileEntry{absPathRef, this->storage.back().asRef()}})
+        .second;
   }
 
   bool contains(clang::FileID fileId) const {
     return this->map.contains({fileId});
   }
 
-  /// See the doc comment on \c CanonicalPathMap
-  std::optional<RootRelativePathRef>
-  tryGetCanonicalPath(clang::FileID fileId) const {
+  /// See the doc comment on \c StableFileIdMap
+  std::optional<StableFileId> getStableFileId(clang::FileID fileId) const {
     auto it = this->map.find({fileId});
-    // TODO: Some headers like <built-in> and those in the Xcode SDK
-    // are not getting populated here. That's fine, because they are
-    // out-of-project anyways, but it seems weird that the populate(...)
-    // call isn't adding them.
     if (it == this->map.end()) {
       return {};
     }
     if (std::holds_alternative<RootRelativePathRef>(it->second)) {
-      return std::get<RootRelativePathRef>(it->second);
+      return StableFileId{.path = std::get<RootRelativePathRef>(it->second),
+                          .isInProject = true,
+                          .isSynthetic = false};
     }
-    return {};
+    return StableFileId{
+        .path = std::get<ExternalFileEntry>(it->second).fakeRelativePath,
+        .isInProject = false,
+        .isSynthetic = true};
   }
 };
 
@@ -726,17 +746,17 @@ struct PartialScipIndex {
 class IndexerAstVisitor : public clang::RecursiveASTVisitor<IndexerAstVisitor> {
   using Base = RecursiveASTVisitor;
 
-  const CanonicalPathMap &pathMap;
+  const StableFileIdMap &stableFileIdMap;
   FileIdsToBeIndexedSet toBeIndexed;
   bool deterministic;
 
   TuIndexer &tuIndexer;
 
 public:
-  IndexerAstVisitor(const CanonicalPathMap &pathMap,
-                    FileIdsToBeIndexedSet &&map, bool deterministic,
+  IndexerAstVisitor(const StableFileIdMap &pathMap,
+                    FileIdsToBeIndexedSet &&toBeIndexed, bool deterministic,
                     TuIndexer &tuIndexer)
-      : pathMap(pathMap), toBeIndexed(std::move(map)),
+      : stableFileIdMap(pathMap), toBeIndexed(std::move(toBeIndexed)),
         deterministic(deterministic), tuIndexer(tuIndexer) {}
 
   // See clang/include/clang/Basic/DeclNodes.td for list of declarations.
@@ -807,13 +827,15 @@ class IndexerAstVisitor : public clang::RecursiveASTVisitor<IndexerAstVisitor> {
 
   void writeIndex(SymbolFormatter &&symbolFormatter, MacroIndexer &&macroIndex,
                   scip::Index &scipIndex) {
-    std::vector<std::pair<RootRelativePathRef, clang::FileID>> relativePaths;
-    for (auto wrappedFileId : toBeIndexed) {
-      // We may have been asked to index files outside the project,
-      // which lack canonical paths.
-      if (auto optRelPathRef =
-              pathMap.tryGetCanonicalPath(wrappedFileId.data)) {
-        relativePaths.push_back({optRelPathRef.value(), wrappedFileId.data});
+    std::vector<std::pair<RootRelativePathRef, clang::FileID>>
+        indexedProjectFiles;
+    for (auto wrappedFileId : this->toBeIndexed) {
+      if (auto optStableFileId =
+              this->stableFileIdMap.getStableFileId(wrappedFileId.data)) {
+        if (optStableFileId->isInProject) {
+          indexedProjectFiles.emplace_back(optStableFileId->path,
+                                           wrappedFileId.data);
+        }
       }
     }
     if (this->deterministic) {
@@ -824,10 +846,10 @@ class IndexerAstVisitor : public clang::RecursiveASTVisitor<IndexerAstVisitor> {
                 p1.first.asStringView());
         return cmp == std::strong_ordering::less;
       };
-      absl::c_sort(relativePaths, comparePaths);
+      absl::c_sort(indexedProjectFiles, comparePaths);
     }
 
-    for (auto [relPathRef, fileId] : relativePaths) {
+    for (auto [relPathRef, fileId] : indexedProjectFiles) {
       scip::Document document;
       auto relPath = relPathRef.asStringView();
       document.set_relative_path(relPath.data(), relPath.size());
@@ -898,22 +920,22 @@ class IndexerAstConsumer : public clang::SemaConsumer {
       return;
     }
 
-    CanonicalPathMap canonicalPathMap{this->options.projectRootPath,
-                                      this->options.buildRootPath};
+    StableFileIdMap stableFileIdMap{this->options.projectRootPath,
+                                    this->options.buildRootPath};
     FileIdsToBeIndexedSet toBeIndexed{};
     this->computeFileIdsToBeIndexed(astContext, emitIndexDetails,
-                                    clangIdLookupMap, canonicalPathMap,
+                                    clangIdLookupMap, stableFileIdMap,
                                     toBeIndexed);
 
-    auto getRelativePath =
-        [&](clang::FileID fileId) -> std::optional<RootRelativePathRef> {
-      return canonicalPathMap.tryGetCanonicalPath(fileId);
+    auto getStableFileId =
+        [&](clang::FileID fileId) -> std::optional<StableFileId> {
+      return stableFileIdMap.getStableFileId(fileId);
     };
-    SymbolFormatter symbolFormatter{sourceManager, getRelativePath};
+    SymbolFormatter symbolFormatter{sourceManager, getStableFileId};
     TuIndexer tuIndexer{sourceManager, this->sema->getLangOpts(),
                         this->sema->getASTContext(), symbolFormatter};
 
-    IndexerAstVisitor visitor{canonicalPathMap, std::move(toBeIndexed),
+    IndexerAstVisitor visitor{stableFileIdMap, std::move(toBeIndexed),
                               this->options.deterministic, tuIndexer};
     visitor.TraverseAST(astContext);
 
@@ -933,16 +955,16 @@ class IndexerAstConsumer : public clang::SemaConsumer {
   void computeFileIdsToBeIndexed(const clang::ASTContext &astContext,
                                  const EmitIndexJobDetails &emitIndexDetails,
                                  const ClangIdLookupMap &clangIdLookupMap,
-                                 CanonicalPathMap &canonicalPathMap,
+                                 StableFileIdMap &stableFileIdMap,
                                  FileIdsToBeIndexedSet &toBeIndexed) {
     auto &sourceManager = astContext.getSourceManager();
     auto mainFileId = sourceManager.getMainFileID();
 
-    canonicalPathMap.populate(clangIdLookupMap);
+    stableFileIdMap.populate(clangIdLookupMap);
     if (auto *mainFileEntry = sourceManager.getFileEntryForID(mainFileId)) {
       if (auto optMainFileAbsPath =
               AbsolutePathRef::tryFrom(mainFileEntry->tryGetRealPathName())) {
-        canonicalPathMap.insert(mainFileId, optMainFileAbsPath.value());
+        stableFileIdMap.insert(mainFileId, optMainFileAbsPath.value());
         toBeIndexed.insert({mainFileId});
       } else {
         spdlog::debug(
@@ -971,7 +993,7 @@ class IndexerAstConsumer : public clang::SemaConsumer {
       std::string message;
       auto check = [&](auto wrappedFileId) -> bool {
         auto fileId = wrappedFileId.data;
-        if (canonicalPathMap.contains(fileId)) {
+        if (stableFileIdMap.contains(fileId)) {
           return true;
         }
         message = fmt::format("missing fileId {} for path: {}",
