Merge pull request #227 from olafurpg/jdk-first

Improve handling of moniker import/exports and the JDK

Author: olafurpg

lsif-java/src/main/scala/com/sourcegraph/lsif_java/buildtools/BuildTool.scala

@@ -13,6 +13,7 @@ abstract class BuildTool(val name: String, index: IndexCommand) {
   protected def defaultTargetroot: Path
 
   def isHidden: Boolean = false
+  def buildKind: String = ""
 
   final def sourceroot: Path = index.workingDirectory
   final def targetroot: Path =

lsif-java/src/main/scala/com/sourcegraph/lsif_java/buildtools/LsifBuildTool.scala

@@ -54,6 +54,8 @@ class LsifBuildTool(index: IndexCommand) extends BuildTool("LSIF", index) {
     index.workingDirectory.resolve(LsifBuildTool.ConfigFileName)
   def usedInCurrentDirectory(): Boolean = Files.isRegularFile(configFile)
   override def isHidden: Boolean = true
+  override def buildKind: String =
+    parsedConfig.fold(_.kind, _ => super.buildKind)
   def generateSemanticdb(): CommandResult = {
     parsedConfig match {
       case ValueResult(value) =>

lsif-java/src/main/scala/com/sourcegraph/lsif_java/commands/IndexCommand.scala

@@ -160,6 +160,7 @@ case class IndexCommand(
             output = finalOutput,
             targetroot = List(tool.targetroot),
             packagehub = packagehub,
+            buildKind = tool.buildKind,
             app = app
           ).run()
         }

lsif-java/src/main/scala/com/sourcegraph/lsif_java/commands/IndexSemanticdbCommand.scala

@@ -38,6 +38,9 @@ final case class IndexSemanticdbCommand(
     packagehub: Option[String] = None,
     @Description("Directories that contain SemanticDB files.")
     @PositionalArguments() targetroot: List[Path] = Nil,
+    @Description(
+      "The kind of this build, one of: empty string, jdk, maven"
+    ) buildKind: String = "",
     @Inline() app: Application = Application.default
 ) extends Command {
   def sourceroot: Path = AbsolutePath.of(app.env.workingDirectory)
@@ -72,7 +75,8 @@ final case class IndexSemanticdbCommand(
         "java",
         format,
         parallel,
-        packages.map(_.toPackageInformation).asJava
+        packages.map(_.toPackageInformation).asJava,
+        buildKind
       )
     LsifSemanticdb.run(options)
     postPackages(packages)

lsif-semanticdb/src/main/java/com/sourcegraph/lsif_semanticdb/LsifSemanticdb.java

@@ -97,7 +97,7 @@ private Integer processDocument(
             .collect(Collectors.toSet());
     doc.id = documentId;
     ResultSets results =
-        new ResultSets(writer, globals, isExportedSymbol, localDefinitions, packages);
+        new ResultSets(writer, globals, isExportedSymbol, localDefinitions, packages, options);
     Set<Integer> rangeIds = new LinkedHashSet<>();
 
     for (SymbolOccurrence occ : doc.sortedSymbolOccurrences()) {

lsif-semanticdb/src/main/java/com/sourcegraph/lsif_semanticdb/LsifSemanticdbOptions.java

@@ -16,6 +16,7 @@ public class LsifSemanticdbOptions {
   public final LsifOutputFormat format;
   public final boolean parallel;
   public final List<MavenPackage> packages;
+  public final String buildKind;
 
   public LsifSemanticdbOptions(
       List<Path> targetroots,
@@ -26,7 +27,8 @@ public LsifSemanticdbOptions(
       String language,
       LsifOutputFormat format,
       boolean parallel,
-      List<MavenPackage> packages) {
+      List<MavenPackage> packages,
+      String buildKind) {
     this.targetroots = targetroots;
     this.output = output;
     this.sourceroot = sourceroot;
@@ -36,5 +38,6 @@ public LsifSemanticdbOptions(
     this.format = format;
     this.parallel = parallel;
     this.packages = packages;
+    this.buildKind = buildKind;
   }
 }

lsif-semanticdb/src/main/java/com/sourcegraph/lsif_semanticdb/PackageTable.java

@@ -36,29 +36,26 @@ public class PackageTable implements Function<Package, Integer> {
   public PackageTable(LsifSemanticdbOptions options, LsifWriter writer) throws IOException {
     this.writer = writer;
     this.javaVersion = new JavaVersion();
+    // NOTE: it's important that we index the JDK before maven packages. Some maven packages
+    // redefine classes from the JDK and we want those maven packages to take precedence over
+    // the JDK. The motivation to prioritize maven packages over the JDK is that we only want
+    // to exports monikers against the JDK when indexing the JDK repo.
+    indexJdk();
     for (MavenPackage pkg : options.packages) {
       indexPackage(pkg);
     }
-    indexJdk();
   }
 
-  public void writeImportedSymbol(String symbol, int monikerId) {
-    packageForSymbol(symbol)
-        .ifPresent(
-            pkg -> {
-              int pkgId = lsif.computeIfAbsent(pkg, this);
-              writer.emitPackageInformationEdge(monikerId, pkgId);
-            });
+  public void writeMonikerPackage(int monikerId, Package pkg) {
+    int pkgId = lsif.computeIfAbsent(pkg, this);
+    writer.emitPackageInformationEdge(monikerId, pkgId);
   }
 
-  private Optional<Package> packageForClassfile(String classfile) {
-    Package result = byClassfile.get(classfile);
-    if (result != null) return Optional.of(result);
-    if (!javaVersion.isJava8 && isJrtClassfile(classfile)) return Optional.of(javaVersion.pkg);
-    return Optional.empty();
+  public void writeImportedSymbol(String symbol, int monikerId) {
+    packageForSymbol(symbol).ifPresent(pkg -> writeMonikerPackage(monikerId, pkg));
   }
 
-  private Optional<Package> packageForSymbol(String symbol) {
+  public Optional<Package> packageForSymbol(String symbol) {
     return SymbolDescriptor.toplevel(symbol)
         .flatMap(
             toplevel -> {
@@ -67,6 +64,13 @@ private Optional<Package> packageForSymbol(String symbol) {
             });
   }
 
+  private Optional<Package> packageForClassfile(String classfile) {
+    Package result = byClassfile.get(classfile);
+    if (result != null) return Optional.of(result);
+    if (!javaVersion.isJava8 && isJrtClassfile(classfile)) return Optional.of(javaVersion.pkg);
+    return Optional.empty();
+  }
+
   private void indexPackage(MavenPackage pkg) throws IOException {
     if (!JAR_PATTERN.matches(pkg.jar)) {
       return;

lsif-semanticdb/src/main/java/com/sourcegraph/lsif_semanticdb/ResultSets.java

@@ -3,6 +3,7 @@
 import com.sourcegraph.semanticdb_javac.SemanticdbSymbols;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import java.util.function.Function;
 
@@ -15,18 +16,21 @@ public class ResultSets implements Function<String, ResultIds> {
   private final Set<String> exportedSymbols;
   private final Set<String> localDefinitions;
   private final PackageTable packages;
+  private final boolean isJdkRepo;
 
   public ResultSets(
       LsifWriter writer,
       Map<String, ResultIds> globals,
       Set<String> exportedSymbols,
       Set<String> localDefinitions,
-      PackageTable packages) {
+      PackageTable packages,
+      LsifSemanticdbOptions options) {
     this.writer = writer;
     this.globals = globals;
     this.exportedSymbols = exportedSymbols;
     this.localDefinitions = localDefinitions;
     this.packages = packages;
+    this.isJdkRepo = options.buildKind.equals("jdk");
     locals = new HashMap<>();
   }
 
@@ -43,9 +47,20 @@ public ResultIds apply(String symbol) {
     int resultSet = writer.emitResultSet();
 
     // Moniker
+    Optional<Package> pkg = packages.packageForSymbol(symbol);
+    if (pkg.isPresent() && pkg.get() instanceof JdkPackage && !isJdkRepo) {
+      // Never export monikers for the JDK repo unless we're indexing the JDK repo.
+      // Some Maven packages contain sources that redefine symbols like `java/lang/String#`
+      // even if the the jar files don't contain `java/lang/String.class`. For example,
+      // see the package com.google.gwt:gwt-user:2.9.0.
+      // Related issue: https://github.com/sourcegraph/sourcegraph/issues/21058
+      isExportedSymbol = false;
+    }
     int monikerId = writer.emitMonikerVertex(symbol, hasDefinitionResult);
     writer.emitMonikerEdge(resultSet, monikerId);
-    packages.writeImportedSymbol(symbol, monikerId);
+    if (pkg.isPresent()) {
+      packages.writeMonikerPackage(monikerId, pkg.get());
+    }
 
     int definitionId = hasDefinitionResult ? writer.emitDefinitionResult(resultSet) : -1;