Tighten new comments across SCIP shard code

Author: jupblb

build.sbt

@@ -519,8 +519,7 @@ lazy val semanticdbKotlincMinimized = project
           val snapDir =
             (baseDirectory.value / "src" / "generatedSnapshots" / "resources")
               .getAbsolutePath
-          // Place the aggregated `index.scip` outside the shard-scanned
-          // targetroot so a subsequent run doesn't re-ingest it as a shard.
+          // Write `index.scip` outside the shard-scanned targetroot to avoid re-ingestion.
           val scipOut = (target.value / "scip-index" / "index.scip")
             .getAbsolutePath
           val mainCls = "com.sourcegraph.scip_java.ScipJava"

scip-java/src/main/scala/com/sourcegraph/scip_java/commands/IndexSemanticdbCommand.scala

@@ -61,12 +61,8 @@ final case class IndexSemanticdbCommand(
     )
     allowExportingGlobalSymbolsFromDirectoryEntries: Boolean = true,
     @Description(
-      "If true, walk targetroots for *.scip shards (META-INF/scip/...) emitted by the " +
-        "compiler plug-ins instead of *.semanticdb files. The aggregator rewrites placeholder " +
-        "symbols into the final 'scip-java' scheme and merges per-source shards into the " +
-        "output index. Defaults to true now that both the javac and kotlinc plug-ins emit " +
-        "shards. Pass --use-scip-shards=false to fall back to the legacy SemanticDB-based " +
-        "aggregator."
+      "If true, aggregate *.scip shards under META-INF/scip/ instead of *.semanticdb files. " +
+        "Pass --use-scip-shards=false to fall back to the SemanticDB-based aggregator."
     )
     useScipShards: Boolean = true,
     @Inline()

scip-java/src/main/scala/com/sourcegraph/scip_java/commands/SnapshotCommand.scala

@@ -52,9 +52,7 @@ case class SnapshotCommand(
           ): FileVisitResult = {
             if (scipPattern.matches(file)) {
               val index = Index.parseFrom(Files.readAllBytes(file))
-              // Skip per-source shards emitted by the compiler plugin (those don't have a
-              // project_root). The aggregator produces a single top-level index file that
-              // carries the project_root and is the canonical input for snapshot rendering.
+              // Skip per-source shards (no project_root); only the aggregator output carries it.
               val rawProjectRoot = index.getMetadata.getProjectRoot
               if (rawProjectRoot.nonEmpty) {
                 foundScipFile = true

scip-semanticdb/src/main/java/com/sourcegraph/scip_semanticdb/ScipShardAggregator.java

@@ -27,19 +27,9 @@
 import java.util.jar.JarFile;
 
 /**
- * Aggregates per-source {@code *.scip} shards into a single {@link Index}.
- *
- * <p>Steps:
- *
- * <ol>
- *   <li>Walk all targetroots for {@code *.scip} shards (and {@code *.jar} files containing them).
- *   <li>Parse each shard into a {@link Index} containing exactly one {@link Document}.
- *   <li>Rewrite placeholder global symbols ({@code ". . . . "} prefix) into the final {@code
- *       "scip-java maven g a v ..."} form via {@link SymbolRewriter}.
- *   <li>Deduplicate documents by {@code relative_path}, merging occurrences/symbols.
- *   <li>Compute inverse {@code is_implementation+is_reference} relationships across the project.
- *   <li>Emit the metadata block and all documents through {@link ScipWriter}.
- * </ol>
+ * Aggregates per-source {@code *.scip} shards into a single {@link Index}: walks targetroots for
+ * shards, rewrites placeholder symbols via {@link SymbolRewriter}, deduplicates documents by {@code
+ * relative_path}, attaches inverse relationships, and emits everything through {@link ScipWriter}.
  */
 public final class ScipShardAggregator {
 
@@ -80,8 +70,7 @@ private void run() throws IOException {
     options.reporter.startProcessing(shards.size());
     writer.emitTyped(metadata());
 
-    // Deduplicate documents by relative_path. We rewrite symbols as we go so all merging happens
-    // on final-form strings.
+    // Rewrite symbols first so the dedup key is the final form.
     LinkedHashMap<String, Document.Builder> merged = new LinkedHashMap<>();
     InverseReferenceRelationships inverse = new InverseReferenceRelationships();
 
@@ -114,8 +103,6 @@ private void run() throws IOException {
     options.reporter.endProcessing();
   }
 
-  // --- shard parsing -------------------------------------------------------
-
   private static final PathMatcher JAR_PATTERN =
       FileSystems.getDefault().getPathMatcher("glob:**.jar");
 
@@ -145,8 +132,6 @@ private static Index parseBytes(byte[] bytes) throws IOException {
     return Index.parseFrom(in);
   }
 
-  // --- rewriting -----------------------------------------------------------
-
   private Document rewriteDocument(Document doc) {
     Document.Builder builder = doc.toBuilder().clearOccurrences().clearSymbols();
     for (Occurrence occ : doc.getOccurrencesList()) {
@@ -173,15 +158,14 @@ private SymbolInformation rewriteSymbol(SymbolInformation info) {
   }
 
   private void mergeInto(Document.Builder target, Document fresh) {
-    // Deduplicate occurrences by (range, symbol, roles) so that shards produced by repeated
-    // compilation rounds (or by multiple targetroots) don't introduce duplicate entries.
-    // Variants that differ only by enclosing_range are collapsed, preferring the one that has it.
+    // Dedup occurrences by (range, symbol, roles); collapse variants differing only in
+    // enclosing_range, preferring the one that has it.
     LinkedHashMap<OccurrenceKey, Occurrence> occurrences = new LinkedHashMap<>();
     for (Occurrence occ : target.getOccurrencesList()) putOccurrence(occurrences, occ);
     for (Occurrence occ : fresh.getOccurrencesList()) putOccurrence(occurrences, occ);
     target.clearOccurrences().addAllOccurrences(occurrences.values());
 
-    // Deduplicate symbols by symbol string; merge relationships of duplicates.
+    // Dedup symbols by symbol string; merge relationships of duplicates.
     LinkedHashMap<String, SymbolInformation> bySymbol = new LinkedHashMap<>();
     for (SymbolInformation info : target.getSymbolsList()) {
       bySymbol.put(info.getSymbol(), info);
@@ -242,8 +226,6 @@ private static SymbolInformation mergeSymbol(SymbolInformation a, SymbolInformat
     return builder.build();
   }
 
-  // --- metadata ------------------------------------------------------------
-
   private Index metadata() {
     return Index.newBuilder()
         .setMetadata(
@@ -259,12 +241,9 @@ private Index metadata() {
         .build();
   }
 
-  // --- inverse relationships ----------------------------------------------
-
   /**
-   * Collects symbols that override or implement other symbols, indexed by the overridden symbol.
-   * When a final document is emitted, the per-symbol entries are augmented with {@code
-   * is_implementation && is_reference} relationships pointing back to overriders.
+   * Collects overriders keyed by the overridden symbol, then augments each final document with
+   * {@code is_implementation && is_reference} relationships pointing back at the overriders.
    */
   private final class InverseReferenceRelationships {
 

scip-semanticdb/src/main/java/com/sourcegraph/scip_semanticdb/ScipShardWalker.java

@@ -12,10 +12,9 @@
 import java.util.List;
 
 /**
- * A file visitor that recursively collects per-source SCIP shard files ({@code *.scip}) emitted by
- * the compiler plug-ins. Only files under a {@code META-INF/scip/} directory are returned so we
- * don't accidentally re-ingest a previously-written aggregate {@code index.scip} that may live in
- * the same target tree.
+ * Collects per-source SCIP shards ({@code *.scip}) emitted by the compiler plug-ins. Restricted to
+ * {@code META-INF/scip/} so a previously-written aggregate {@code index.scip} in the same target
+ * tree is not re-ingested.
  */
 public class ScipShardWalker extends SimpleFileVisitor<Path> {
   private final ArrayList<Path> result;

scip-semanticdb/src/main/java/com/sourcegraph/scip_semanticdb/SymbolRewriter.java

@@ -1,24 +1,13 @@
 package com.sourcegraph.scip_semanticdb;
 
 /**
- * Rewrites placeholder SCIP global symbols emitted by the compiler plugin into their final form.
- *
- * <p>The compiler plugin does not know the Maven coordinates ({@code groupId:artifactId:version})
- * of the source it is compiling, so it emits global symbols with the sentinel prefix {@code ". . .
- * . "}. The aggregator resolves the appropriate coordinates via {@link PackageTable} and rewrites
- * those symbols to the final {@code "scip-java"} scheme.
- *
- * <p>{@code local N} symbols are passed through unchanged.
+ * Rewrites placeholder SCIP global symbols ({@link #PLACEHOLDER_PREFIX}) emitted by the compiler
+ * plug-ins into their final {@code "scip-java"} form by resolving Maven coordinates via {@link
+ * PackageTable}. Local symbols are passed through unchanged.
  */
 public final class SymbolRewriter {
 
-  /**
-   * Sentinel prefix used by the compiler plugin for global symbols whose coordinates are not yet
-   * known.
-   */
   public static final String PLACEHOLDER_PREFIX = ". . . . ";
-
-  /** SCIP scheme used for symbols produced by scip-java. */
   public static final String SCIP_JAVA_SCHEME = "scip-java";
 
   private final PackageTable packages;
@@ -27,25 +16,14 @@ public SymbolRewriter(PackageTable packages) {
     this.packages = packages;
   }
 
-  /**
-   * Returns {@code true} if {@code symbol} is a placeholder global emitted by the compiler plugin.
-   */
   public static boolean isPlaceholderGlobal(String symbol) {
     return symbol != null && symbol.startsWith(PLACEHOLDER_PREFIX);
   }
 
-  /**
-   * Returns {@code true} if {@code symbol} is a SCIP local symbol of the form {@code "local N"}.
-   */
   public static boolean isLocal(String symbol) {
     return symbol != null && symbol.startsWith("local ");
   }
 
-  /**
-   * Rewrites a placeholder global symbol into its final form using the appropriate package
-   * coordinates from {@link PackageTable}. Local symbols and already-rewritten symbols are returned
-   * unchanged.
-   */
   public String rewrite(String symbol) {
     if (symbol == null || symbol.isEmpty()) return symbol;
     if (isLocal(symbol)) return symbol;

semanticdb-javac/src/main/java/com/sourcegraph/semanticdb_javac/ScipOccurrences.java

@@ -9,22 +9,20 @@
 import java.util.Objects;
 
 /**
- * Helpers for deduplicating SCIP {@link Occurrence} entries by their {@code (symbol, range, roles)}
- * key. Variants that differ only in whether {@code enclosing_range} is set are collapsed,
- * preferring the one that carries the enclosing range.
+ * Deduplicates SCIP {@link Occurrence} entries by {@code (symbol, range, roles)}. Variants
+ * differing only in whether {@code enclosing_range} is set are collapsed, preferring the one that
+ * carries the enclosing range.
  */
 final class ScipOccurrences {
 
   private ScipOccurrences() {}
 
-  /** Returns a new list with duplicate occurrences collapsed in insertion order. */
   static List<Occurrence> deduplicate(List<Occurrence> occurrences) {
     LinkedHashMap<Key, Occurrence> out = new LinkedHashMap<>();
     for (Occurrence occ : occurrences) put(out, occ);
     return new ArrayList<>(out.values());
   }
 
-  /** Inserts {@code occ} into {@code out}, collapsing duplicates by {@link Key}. */
   static void put(Map<Key, Occurrence> out, Occurrence occ) {
     Key key = Key.of(occ);
     Occurrence existing = out.get(key);

semanticdb-javac/src/main/java/com/sourcegraph/semanticdb_javac/ScipRange.java

@@ -4,11 +4,8 @@
 import java.util.List;
 
 /**
- * Lightweight value type for source ranges produced by {@link ScipVisitor}.
- *
- * <p>Coordinates are zero-based and follow the SCIP convention (line + character, end-exclusive
- * character). Convertible directly to SCIP's {@code repeated int32 range} representation via {@link
- * #asScipRange()}.
+ * Zero-based source range produced by {@link ScipVisitor}, convertible to SCIP's compact {@code
+ * repeated int32 range} via {@link #asScipRange()}.
  */
 final class ScipRange {
   final int startLine;
@@ -23,15 +20,12 @@ final class ScipRange {
     this.endCharacter = endCharacter;
   }
 
-  /** Returns a copy with adjusted start/end characters (used to correct for tab-expansion). */
+  /** Returns a copy with adjusted start/end characters (used to correct tab-expansion). */
   ScipRange withCharacters(int startCharacter, int endCharacter) {
     return new ScipRange(startLine, startCharacter, endLine, endCharacter);
   }
 
-  /**
-   * Encodes the range using SCIP's compact {@code repeated int32 range} layout: 3 ints when the
-   * range fits on a single line, 4 ints otherwise.
-   */
+  /** 3 ints when the range fits on one line, 4 ints otherwise. */
   List<Integer> asScipRange() {
     if (startLine == endLine) {
       return Arrays.asList(startLine, startCharacter, endCharacter);

semanticdb-javac/src/main/java/com/sourcegraph/semanticdb_javac/ScipRole.java

@@ -1,9 +1,6 @@
 package com.sourcegraph.semanticdb_javac;
 
-/**
- * Minimal role enum used by {@link ScipVisitor} when emitting SCIP occurrences. Mirrors the subset
- * of {@code Semanticdb.SymbolOccurrence.Role} that the direct-to-SCIP visitor cares about.
- */
+/** Role of a SCIP occurrence as seen by {@link ScipVisitor}. */
 enum ScipRole {
   DEFINITION,
   REFERENCE,

semanticdb-javac/src/main/java/com/sourcegraph/semanticdb_javac/ScipShardWriter.java

@@ -15,21 +15,13 @@
 import java.util.List;
 
 /**
- * Writes and merges per-source SCIP shards produced by the compiler plugin.
- *
- * <p>Each source file produces a self-contained {@link Index} shard containing a single {@link
- * Document}. When a shard already exists on disk (e.g. during annotation processing rounds), the
- * new document is merged into the existing one, deduplicating occurrences, symbols and
- * relationships.
+ * Writes a per-source SCIP shard, merging it with any pre-existing shard at the same path (e.g. one
+ * written by a prior annotation-processing round).
  */
 public final class ScipShardWriter {
 
   private ScipShardWriter() {}
 
-  /**
-   * Writes the given {@code shard} to {@code output}, creating parent directories as needed. If
-   * {@code output} already exists, the existing shard is parsed and merged with the new one.
-   */
   public static void writeOrMerge(Path output, Index shard) throws IOException {
     Files.createDirectories(output.getParent());
     if (Files.exists(output)) {
@@ -42,10 +34,7 @@ public static void writeOrMerge(Path output, Index shard) throws IOException {
     Files.write(output, shard.toByteArray());
   }
 
-  /**
-   * Merges two SCIP shards by combining their document lists. Documents that share a {@code
-   * relative_path} have their occurrences, symbols and external symbols deduplicated.
-   */
+  /** Merges two shards: documents sharing a {@code relative_path} are dedup-merged. */
   static Index merge(Index a, Index b) {
     Index.Builder builder = Index.newBuilder();
     if (b.hasMetadata()) {
@@ -68,7 +57,7 @@ static Index merge(Index a, Index b) {
     }
     builder.addAllDocuments(byPath.values());
 
-    // External symbols: deduplicate by symbol string. Last writer wins to keep latest data.
+    // External symbols: last writer wins to keep the most recent data.
     LinkedHashMap<String, SymbolInformation> externals = new LinkedHashMap<>();
     for (SymbolInformation s : a.getExternalSymbolsList()) externals.put(s.getSymbol(), s);
     for (SymbolInformation s : b.getExternalSymbolsList()) externals.put(s.getSymbol(), s);
@@ -78,18 +67,15 @@ static Index merge(Index a, Index b) {
   }
 
   private static Document mergeDocuments(Document a, Document b) {
+    // b.toBuilder() carries forward the most recent language/relative_path/text/encoding.
     Document.Builder builder = b.toBuilder().clearOccurrences().clearSymbols();
-    // Use the most recent metadata for language/relative_path/text/encoding which already
-    // come from b via toBuilder().
 
-    // Deduplicate occurrences by (range, symbol, roles). Variants that differ only in
-    // enclosing_range get collapsed, preferring the one that carries the enclosing range.
     LinkedHashMap<ScipOccurrences.Key, Occurrence> occurrences = new LinkedHashMap<>();
     for (Occurrence occ : a.getOccurrencesList()) ScipOccurrences.put(occurrences, occ);
     for (Occurrence occ : b.getOccurrencesList()) ScipOccurrences.put(occurrences, occ);
     builder.addAllOccurrences(occurrences.values());
 
-    // Deduplicate symbols by symbol string; merge relationships and documentation.
+    // Dedup symbols by symbol string; merge relationships and documentation.
     LinkedHashMap<String, SymbolInformation> bySymbol = new LinkedHashMap<>();
     for (SymbolInformation info : a.getSymbolsList()) bySymbol.put(info.getSymbol(), info);
     for (SymbolInformation info : b.getSymbolsList()) {
@@ -103,13 +89,13 @@ private static Document mergeDocuments(Document a, Document b) {
 
   private static SymbolInformation mergeSymbol(SymbolInformation a, SymbolInformation b) {
     SymbolInformation.Builder builder = b.toBuilder();
-    // Merge relationships, deduplicating by structural equality with deterministic ordering.
+    // Dedup relationships by structural equality; preserve insertion order.
     LinkedHashMap<Relationship, Relationship> rels = new LinkedHashMap<>();
     for (Relationship r : a.getRelationshipsList()) rels.put(r, r);
     for (Relationship r : b.getRelationshipsList()) rels.put(r, r);
     builder.clearRelationships().addAllRelationships(rels.values());
 
-    // Merge documentation, preserving order and avoiding duplicates.
+    // Dedup documentation; preserve insertion order.
     List<String> docs = new ArrayList<>(a.getDocumentationList());
     for (String d : b.getDocumentationList()) {
       if (!docs.contains(d)) docs.add(d);