Fix a regression where the SCIP index contained invalid symbols

Uploading an index with scip-java v0.8.17 results in the following error in the Sourcegraph UI ``` codeNavSvc.GetImplementations: lsifStore.MonikersByPosition: reached end of symbol while parsing <scheme>, expected a ' ' character minimized/Issue413Subclass#c. _____________________________^ ``` The problem is that this release introduced a regression where scip-java emitted invalid SCIP symbols. This PR fixes the issue, and adds new checks in the testing infrastructure to prevent this kind of regression from happening again.
sourcegraph · May 2, 2023 · 19bf935 · 19bf935
1 parent f36c68a
commit 19bf935
Show file tree

Hide file tree

Showing 27 changed files with 246 additions and 185 deletions.
diff --git a/scip-java/src/main/scala/com/sourcegraph/scip_java/ScipPrinters.scala b/scip-java/src/main/scala/com/sourcegraph/scip_java/ScipPrinters.scala
@@ -149,6 +149,10 @@ object ScipPrinters {
         caretCharacter * width
 
     val symbol = syntheticDefinition.fold(occ.getSymbol)(_.getSymbol)
+
+    // Fail the tests if the index contains symbols that don't parse as valid SCIP symbols.
+    val _ = ScipSymbol.parseOrThrowExceptionIfInvalid(symbol)
+
     out
       .append(comment)
       .append(indent)

diff --git a/scip-java/src/main/scala/com/sourcegraph/scip_java/ScipSymbol.scala b/scip-java/src/main/scala/com/sourcegraph/scip_java/ScipSymbol.scala
@@ -0,0 +1,55 @@
+package com.sourcegraph.scip_java
+
+import com.sourcegraph.scip_semanticdb.SymbolDescriptor
+import com.sourcegraph.semanticdb_javac.SemanticdbSymbols
+
+sealed abstract class ScipSymbol {}
+final case class LocalScipSymbol(identifier: String) extends ScipSymbol
+final case class GlobalScipSymbol(
+    scheme: String,
+    packageManager: String,
+    packageName: String,
+    packageVersion: String,
+    descriptors: List[SymbolDescriptor]
+) extends ScipSymbol
+
+object ScipSymbol {
+
+  def parseOrThrowExceptionIfInvalid(scipSymbol: String): ScipSymbol = {
+    if (scipSymbol.startsWith("local ")) {
+      LocalScipSymbol(scipSymbol.stripPrefix("local "))
+    } else {
+      scipSymbol.split(" ", 5) match {
+        case Array(
+              scheme,
+              packageManager,
+              packageName,
+              packageVersion,
+              descriptor
+            ) =>
+          GlobalScipSymbol(
+            scheme,
+            packageManager,
+            packageName,
+            packageVersion,
+            parseDescriptors(descriptor)
+          )
+        case _ =>
+          throw new IllegalArgumentException(
+            s"Invalid scip symbol: $scipSymbol"
+          )
+      }
+    }
+  }
+
+  private def parseDescriptors(
+      semanticdbSymbol: String
+  ): List[SymbolDescriptor] = {
+    val descriptor = SymbolDescriptor.parseFromSymbol(semanticdbSymbol)
+    if (descriptor.owner == SemanticdbSymbols.ROOT_PACKAGE)
+      Nil
+    else
+      descriptor :: parseDescriptors(descriptor.owner)
+  }
+
+}
diff --git a/scip-semanticdb/src/main/java/com/sourcegraph/scip_semanticdb/ScipSemanticdb.java b/scip-semanticdb/src/main/java/com/sourcegraph/scip_semanticdb/ScipSemanticdb.java
@@ -137,9 +137,11 @@ private void processTypedDocument(
         ArrayList<String> inverseReferences = references.map.get(info.getSymbol());
         if (inverseReferences != null) {
           for (String inverseReference : inverseReferences) {
+            Package inverseReferencePkg =
+                packages.packageForSymbol(inverseReference).orElse(Package.EMPTY);
             scipInfo.addRelationships(
                 Scip.Relationship.newBuilder()
-                    .setSymbol(inverseReference)
+                    .setSymbol(typedSymbol(inverseReference, inverseReferencePkg))
                     .setIsImplementation(true)
                     .setIsReference(true));
           }

diff --git a/tests/snapshots/src/main/generated/ByteParser.scala b/tests/snapshots/src/main/generated/ByteParser.scala
@@ -107,9 +107,9 @@ abstract class ByteParser[J] extends upickle.core.BufferingByteParser{
   protected[this] def close(): Unit
 //                    ^^^^^ definition semanticdb maven maven/com.lihaoyi/ujson_2.13 1.4.0 ujson/ByteParser#close().
 //                          documentation ```scala\ndef close(): Unit\n```
-//                          relationship is_reference is_implementation ujson/ByteArrayParser#close().
-//                          relationship is_reference is_implementation ujson/ByteBufferParser#close().
-//                          relationship is_reference is_implementation ujson/InputStreamParser#close().
+//                          relationship is_reference is_implementation semanticdb maven maven/com.lihaoyi/ujson_2.13 1.4.0 ujson/ByteArrayParser#close().
+//                          relationship is_reference is_implementation semanticdb maven maven/com.lihaoyi/ujson_2.13 1.4.0 ujson/ByteBufferParser#close().
+//                          relationship is_reference is_implementation semanticdb maven maven/com.lihaoyi/ujson_2.13 1.4.0 ujson/InputStreamParser#close().
 //                             ^^^^ reference semanticdb maven maven/org.scala-lang/scala-library 2.13.10 scala/Unit#
 
   /**

diff --git a/tests/snapshots/src/main/generated/CharParser.scala b/tests/snapshots/src/main/generated/CharParser.scala
@@ -107,8 +107,8 @@ abstract class CharParser[J] extends upickle.core.BufferingCharParser{
   protected[this] def close(): Unit
 //                    ^^^^^ definition semanticdb maven maven/com.lihaoyi/ujson_2.13 1.4.0 ujson/CharParser#close().
 //                          documentation ```scala\ndef close(): Unit\n```
-//                          relationship is_reference is_implementation ujson/CharSequenceParser#close().
-//                          relationship is_reference is_implementation ujson/StringParser#close().
+//                          relationship is_reference is_implementation semanticdb maven maven/com.lihaoyi/ujson_2.13 1.4.0 ujson/CharSequenceParser#close().
+//                          relationship is_reference is_implementation semanticdb maven maven/com.lihaoyi/ujson_2.13 1.4.0 ujson/StringParser#close().
 //                             ^^^^ reference semanticdb maven maven/org.scala-lang/scala-library 2.13.10 scala/Unit#
 
   /**

diff --git a/tests/snapshots/src/main/generated/com/airbnb/epoxy/AsyncEpoxyDiffer.java b/tests/snapshots/src/main/generated/com/airbnb/epoxy/AsyncEpoxyDiffer.java
@@ -60,7 +60,7 @@ interface ResultCallback {
     void onResult(@NonNull DiffResult result);
 //       ^^^^^^^^ definition semanticdb maven . . com/airbnb/epoxy/AsyncEpoxyDiffer#ResultCallback#onResult().
 //                documentation ```java\npublic abstract void onResult(DiffResult result)\n```
-//                relationship is_reference is_implementation com/airbnb/epoxy/EpoxyControllerAdapter#onResult().
+//                relationship is_reference is_implementation semanticdb maven . . com/airbnb/epoxy/EpoxyControllerAdapter#onResult().
 //                 ^^^^^^^ reference semanticdb maven maven/androidx.annotation/annotation 1.1.0 androidx/annotation/NonNull#
 //                         ^^^^^^^^^^ reference semanticdb maven . . com/airbnb/epoxy/DiffResult#
 //                                    ^^^^^^ definition local 0