feat: Support forward declarations (#148)

Author: varungandhi-src

docs/Design.md

@@ -46,15 +46,15 @@ by a significant amount for an extended period of time.
 
 ### Disk I/O
 
-Workers write out partial SCIP indexes based on paths
+Workers write out shards (incomplete SCIP indexes) based on paths
 provided by the driver after completing an indexing job.
 This write is done after each job is complete,
 instead of after completing all jobs,
 because it reduces RSS as well as the blast radius
 in case the worker gets killed or crashes later.
 
 After all indexing work is completed, the driver
-assembles the partial indexes into a full SCIP index.
+assembles the shards into a full SCIP index.
 
 ### Bazel and distributed builds
 
@@ -69,11 +69,11 @@ because they would do redundant work.
 "Flattening" the job DAG would get rid of
 intermediate link jobs.)
 
-Each job could output a partial index,
+Each job could output some shards,
 along with hashes (for faster de-duplication
 during the merge step).
 The final merge step would take these
-hashes and partial indexes
+hashes and shards
 and assemble them into an Index.
 
 NOTE: As of Jan 24 2023, this functionality is planned

indexer/Driver.cc

@@ -403,7 +403,7 @@ class Scheduler final {
           return fmt::format("emitting an index for '{}'",
                              fileInfoIt->path.asStringRef());
         }
-        return "emitting a partial index";
+        return "emitting a shard";
       }
     }());
   }
@@ -581,7 +581,7 @@ class Driver {
   FileIndexingPlanner planner;
 
   std::vector<std::pair<JobId, IndexingStatistics>> allStatistics;
-  std::vector<AbsolutePath> indexPartPaths;
+  std::vector<ShardPaths> shardPaths;
 
   /// Total number of commands in the compilation database.
   size_t compdbCommandCount = 0;
@@ -593,8 +593,7 @@ class Driver {
 
   Driver(std::string driverId, DriverOptions &&options)
       : options(std::move(options)), id(driverId), scheduler(),
-        planner(this->options.projectRootPath), indexPartPaths(),
-        compdbParser() {
+        planner(this->options.projectRootPath), shardPaths(), compdbParser() {
     MessageQueues::deleteIfPresent(this->id, this->numWorkers());
     this->queues = MessageQueues(this->id, this->numWorkers(),
                                  {IPC_BUFFER_MAX_SIZE, IPC_BUFFER_MAX_SIZE});
@@ -675,20 +674,23 @@ class Driver {
 
     scip::Index fullIndex{};
     if (this->options.deterministic) {
-      // Sorting before merging so that mergeIndexParts can be const
-      absl::c_sort(this->indexPartPaths,
-                   [](const AbsolutePath &p1, const AbsolutePath &p2) -> bool {
-                     auto cmp = p1 <=> p2;
-                     ENFORCE(cmp != 0, "2+ index parts have same path '{}'",
-                             p1.asStringRef());
-                     return cmp == std::strong_ordering::less;
-                   });
-    }
-    this->mergeIndexParts(fullIndex);
+      // Sorting before merging so that mergeShards can be const
+      absl::c_sort(
+          this->shardPaths, [](const auto &paths1, const auto &paths2) -> bool {
+            auto cmp = paths1.docsAndExternals <=> paths2.docsAndExternals;
+            ENFORCE(cmp != 0, "2+ index parts have same path '{}'",
+                    paths1.docsAndExternals.asStringRef());
+            ENFORCE(paths1.forwardDecls != paths2.forwardDecls,
+                    "2+ index parts have same path '{}'",
+                    paths1.forwardDecls.asStringRef());
+            return cmp == std::strong_ordering::less;
+          });
+    }
+    this->mergeShards(fullIndex);
     fullIndex.SerializeToOstream(&outputStream);
   }
 
-  void mergeIndexParts(scip::Index &fullIndex) const {
+  void mergeShards(scip::Index &fullIndex) const {
     LogTimerRAII timer("index merging");
 
     scip::ToolInfo toolInfo;
@@ -721,23 +723,31 @@ class Driver {
     // a dependency on a library with a concurrent hash table.
     *fullIndex.mutable_metadata() = std::move(metadata);
 
-    scip::IndexBuilder builder{fullIndex};
-    // TODO: Measure how much time this is taking and parallelize if too slow.
-    for (auto &indexPartAbsPath : this->indexPartPaths) {
-      auto &indexPartPath = indexPartAbsPath.asStringRef();
-      std::ifstream inputStream(indexPartPath,
+    auto readIndexShard = [](const AbsolutePath &path,
+                             scip::Index &indexShard) -> bool {
+      auto &shardPath = path.asStringRef();
+      std::ifstream inputStream(shardPath,
                                 std::ios_base::in | std::ios_base::binary);
       if (inputStream.fail()) {
-        spdlog::warn("failed to open partial index at '{}' ({})", indexPartPath,
+        spdlog::warn("failed to open shard at '{}' ({})", shardPath,
                      std::strerror(errno));
-        continue;
+        return false;
+      }
+      if (!indexShard.ParseFromIstream(&inputStream)) {
+        spdlog::warn("failed to parse shard at '{}'", shardPath);
+        return false;
       }
-      scip::Index partialIndex;
-      if (!partialIndex.ParseFromIstream(&inputStream)) {
-        spdlog::warn("failed to parse partial index at '{}'", indexPartPath);
+      return true;
+    };
+
+    scip::IndexBuilder builder{fullIndex};
+    // TODO: Measure how much time this is taking and parallelize if too slow.
+    for (auto &paths : this->shardPaths) {
+      scip::Index indexShard;
+      if (!readIndexShard(paths.docsAndExternals, indexShard)) {
         continue;
       }
-      for (auto &doc : *partialIndex.mutable_documents()) {
+      for (auto &doc : *indexShard.mutable_documents()) {
         bool isMultiplyIndexed = this->planner.isMultiplyIndexed(
             RootRelativePathRef{doc.relative_path(), RootKind::Project});
         builder.addDocument(std::move(doc), isMultiplyIndexed);
@@ -746,10 +756,24 @@ class Driver {
       // deterministic mode, indexes should be the same, and iterated over in
       // sorted order. So if external symbol emission in each part is
       // deterministic, addExternalSymbol will be called in deterministic order.
-      for (auto &extSym : *partialIndex.mutable_external_symbols()) {
+      for (auto &extSym : *indexShard.mutable_external_symbols()) {
         builder.addExternalSymbol(std::move(extSym));
       }
     }
+
+    auto symbolToInfoMap = builder.populateSymbolToInfoMap();
+
+    for (auto &paths : this->shardPaths) {
+      scip::Index indexShard;
+      if (!readIndexShard(paths.forwardDecls, indexShard)) {
+        continue;
+      }
+      for (auto &forwardDeclSym : *indexShard.mutable_external_symbols()) {
+        builder.addForwardDeclaration(*symbolToInfoMap,
+                                      std::move(forwardDeclSym));
+      }
+    }
+
     builder.finish(this->options.deterministic);
   }
 
@@ -877,8 +901,7 @@ class Driver {
         this->allStatistics.emplace_back(response.jobId,
                                          std::move(result.statistics));
       }
-      this->indexPartPaths.emplace_back(
-          std::move(response.result.emitIndex.indexPartPath));
+      this->shardPaths.emplace_back(std::move(result.shardPaths));
       break;
     }
     }

indexer/Indexer.cc

@@ -263,7 +263,8 @@ TuIndexer::TuIndexer(const clang::SourceManager &sourceManager,
                      GetStableFileId getStableFileId)
     : sourceManager(sourceManager), langOptions(langOptions),
       astContext(astContext), symbolFormatter(symbolFormatter), documentMap(),
-      getStableFileId(getStableFileId) {}
+      getStableFileId(getStableFileId), externalSymbols(),
+      forwardDeclarations() {}
 
 void TuIndexer::saveBindingDecl(const clang::BindingDecl &bindingDecl) {
   auto optSymbol = this->symbolFormatter.getBindingSymbol(bindingDecl);
@@ -358,8 +359,8 @@ void TuIndexer::saveFunctionDecl(const clang::FunctionDecl &functionDecl) {
     this->saveDefinition(symbol, functionDecl.getLocation(),
                          std::move(symbolInfo));
   } else {
-    // See TODO(ref: handle-forward-decls)
-    this->saveReference(symbol, functionDecl.getLocation());
+    this->saveForwardDeclaration(symbol, functionDecl.getLocation(),
+                                 this->tryGetDocComment(functionDecl));
   }
 }
 
@@ -484,11 +485,8 @@ void TuIndexer::saveTagDecl(const clang::TagDecl &tagDecl) {
   auto symbol = optSymbol.value();
 
   if (!tagDecl.isThisDeclarationADefinition()) {
-    // 1. We should emit an external symbol here in some situations
-    //    See TODO(ref: handle-forward-decls)
-    // 2. Pass in a forward declaration SymbolRole here
-    //    once https://github.com/sourcegraph/scip/issues/131 is fixed.
-    this->saveReference(symbol, tagDecl.getLocation());
+    this->saveForwardDeclaration(symbol, tagDecl.getLocation(),
+                                 this->tryGetDocComment(tagDecl));
     return;
   }
 
@@ -681,6 +679,34 @@ void TuIndexer::emitDocumentOccurrencesAndSymbols(
           }));
 }
 
+void TuIndexer::emitExternalSymbols(bool deterministic,
+                                    scip::Index &indexShard) {
+  scip_clang::extractTransform(
+      std::move(this->externalSymbols), deterministic,
+      absl::FunctionRef<void(std::string_view &&, scip::SymbolInformation &&)>(
+          [&](auto &&symbol, auto &&symbolInfo) {
+            symbolInfo.set_symbol(symbol.data(), symbol.size());
+            *indexShard.add_external_symbols() = std::move(symbolInfo);
+          }));
+}
+
+void TuIndexer::emitForwardDeclarations(bool deterministic,
+                                        scip::Index &forwardDeclIndex) {
+  scip_clang::extractTransform(
+      std::move(this->forwardDeclarations), deterministic,
+      absl::FunctionRef<void(std::string_view &&, DocComment &&)>(
+          [&](auto &&symbol, auto &&docComment) {
+            scip::SymbolInformation symbolInfo{};
+            for (auto &line : docComment.lines) {
+              *symbolInfo.add_documentation() = std::move(line);
+            }
+            symbolInfo.set_symbol(symbol.data(), symbol.size());
+            // Add a forward declaration SymbolRole here
+            // once https://github.com/sourcegraph/scip/issues/131 is fixed.
+            *forwardDeclIndex.add_external_symbols() = std::move(symbolInfo);
+          }));
+}
+
 std::pair<FileLocalSourceRange, clang::FileID>
 TuIndexer::getTokenExpansionRange(
     clang::SourceLocation startExpansionLoc) const {
@@ -692,6 +718,17 @@ TuIndexer::getTokenExpansionRange(
                                             {startExpansionLoc, endLoc});
 }
 
+void TuIndexer::saveForwardDeclaration(std::string_view symbol,
+                                       clang::SourceLocation loc,
+                                       DocComment &&docComments) {
+  this->saveReference(symbol, loc);
+  auto [it, inserted] = this->forwardDeclarations.emplace(symbol, docComments);
+  if (!inserted && it->second.lines.empty() && !docComments.lines.empty()) {
+    it->second.lines = std::move(docComments.lines);
+  }
+  return;
+}
+
 void TuIndexer::saveReference(std::string_view symbol,
                               clang::SourceLocation loc, int32_t extraRoles) {
   auto expansionLoc = this->sourceManager.getExpansionLoc(loc);
@@ -721,6 +758,19 @@ void TuIndexer::saveDefinition(
     if (optSymbolInfo.has_value()) {
       doc.symbolInfos.emplace(symbol, std::move(optSymbolInfo.value()));
     }
+  } else if (optSymbolInfo.has_value()) {
+    this->saveExternalSymbol(symbol, std::move(*optSymbolInfo));
+  }
+}
+
+void TuIndexer::saveExternalSymbol(std::string_view symbol,
+                                   scip::SymbolInformation &&symbolInfo) {
+  auto [it, inserted] =
+      this->externalSymbols.emplace(symbol, std::move(symbolInfo));
+  if (!inserted && it->second.documentation().empty()
+      && !symbolInfo.documentation().empty()) {
+    *it->second.mutable_documentation() =
+        std::move(*symbolInfo.mutable_documentation());
   }
 }
 

indexer/Indexer.h

@@ -214,6 +214,11 @@ class TuIndexer final {
       documentMap;
   GetStableFileId getStableFileId;
 
+  absl::flat_hash_map</*name*/ std::string_view, scip::SymbolInformation>
+      externalSymbols;
+
+  absl::flat_hash_map<std::string_view, DocComment> forwardDeclarations;
+
 public:
   TuIndexer(const clang::SourceManager &, const clang::LangOptions &,
             const clang::ASTContext &, SymbolFormatter &, GetStableFileId);
@@ -242,10 +247,19 @@ class TuIndexer final {
   void emitDocumentOccurrencesAndSymbols(bool deterministic, clang::FileID,
                                          scip::Document &);
 
+  void emitExternalSymbols(bool deterministic, scip::Index &);
+  void emitForwardDeclarations(bool deterministic, scip::Index &);
+
 private:
   std::pair<FileLocalSourceRange, clang::FileID>
   getTokenExpansionRange(clang::SourceLocation startExpansionLoc) const;
 
+  /// Helper method for recording forward declarations.
+  ///
+  /// Prefer this over \c saveReference or \c saveOccurrence.
+  void saveForwardDeclaration(std::string_view symbol,
+                              clang::SourceLocation loc, DocComment &&);
+
   void saveReference(std::string_view symbol, clang::SourceLocation loc,
                      int32_t extraRoles = 0);
 
@@ -259,6 +273,9 @@ class TuIndexer final {
                       std::optional<scip::SymbolInformation> &&symbolInfo,
                       int32_t extraRoles = 0);
 
+  /// Only for use inside \c saveDefinition.
+  void saveExternalSymbol(std::string_view symbol, scip::SymbolInformation &&);
+
   /// Lower-level method for only saving a Occurrence.
   ///
   /// Returns a \c PartialDocument reference so that \c scip::SymbolInformation

indexer/IpcMessages.cc

@@ -152,7 +152,8 @@ DERIVE_SERIALIZE_1_NEWTYPE(scip_clang::EmitIndexJobDetails, filesToBeIndexed)
 DERIVE_SERIALIZE_1_NEWTYPE(scip_clang::IpcTestMessage, content)
 DERIVE_SERIALIZE_1_NEWTYPE(scip_clang::SemanticAnalysisJobDetails, command)
 
-DERIVE_SERIALIZE_2(scip_clang::EmitIndexJobResult, statistics, indexPartPath)
+DERIVE_SERIALIZE_2(scip_clang::ShardPaths, docsAndExternals, forwardDecls)
+DERIVE_SERIALIZE_2(scip_clang::EmitIndexJobResult, statistics, shardPaths)
 DERIVE_SERIALIZE_2(scip_clang::PreprocessedFileInfo, path, hashValue)
 DERIVE_SERIALIZE_2(scip_clang::PreprocessedFileInfoMulti, path, hashValues)
 DERIVE_SERIALIZE_2(scip_clang::IndexJobRequest, id, job)
@@ -185,4 +186,4 @@ bool fromJSON(const llvm::json::Value &value, IndexJobResponse &r,
          && mapper.map("jobId", r.jobId) && mapper.map("result", r.result);
 }
 
-} // namespace scip_clang
+} // namespace scip_clang

indexer/IpcMessages.h

@@ -153,9 +153,15 @@ struct IndexingStatistics {
 };
 SERIALIZABLE(IndexingStatistics)
 
+struct ShardPaths {
+  AbsolutePath docsAndExternals;
+  AbsolutePath forwardDecls;
+};
+SERIALIZABLE(ShardPaths)
+
 struct EmitIndexJobResult {
   IndexingStatistics statistics;
-  AbsolutePath indexPartPath;
+  ShardPaths shardPaths;
 };
 SERIALIZABLE(EmitIndexJobResult)