Emit SJD errors even if we don't know the label of a dependency

Fixes #4846

PiperOrigin-RevId: 189123353

Author: cushon

src/java_tools/buildjar/java/com/google/devtools/build/buildjar/JarOwner.java

@@ -15,8 +15,8 @@
 package com.google.devtools.build.buildjar;
 
 import com.google.auto.value.AutoValue;
-import com.google.common.base.Function;
-import javax.annotation.Nullable;
+import java.nio.file.Path;
+import java.util.Optional;
 
 /**
  * Holds information about the Bazel rule that created a certain jar.
@@ -27,26 +27,21 @@
 @AutoValue
 public abstract class JarOwner {
 
-  /** A long way to say 'JarOwner::label'. */
-  public static final Function<JarOwner, String> LABEL =
-      new Function<JarOwner, String>() {
-        @Nullable
-        @Override
-        public String apply(@Nullable JarOwner jarOwner) {
-          return jarOwner == null ? null : jarOwner.label();
-        }
-      };
+  public abstract Path jar();
 
-  public abstract String label();
+  public abstract Optional<String> label();
 
-  @Nullable
-  public abstract String aspect();
+  public abstract Optional<String> aspect();
 
-  public static JarOwner create(String label) {
-    return new AutoValue_JarOwner(label, null);
+  public static JarOwner create(Path jar) {
+    return new AutoValue_JarOwner(jar, Optional.empty(), Optional.empty());
   }
 
-  public static JarOwner create(String label, String aspect) {
-    return new AutoValue_JarOwner(label, aspect);
+  public static JarOwner create(Path jar, String label, Optional<String> aspect) {
+    return new AutoValue_JarOwner(jar, Optional.of(label), aspect);
+  }
+
+  public JarOwner withLabel(Optional<String> label) {
+    return new AutoValue_JarOwner(jar(), label, aspect());
   }
 }

src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/DependencyModule.java

@@ -15,10 +15,12 @@
 package com.google.devtools.build.buildjar.javac.plugins.dependency;
 
 import static com.google.common.collect.ImmutableList.toImmutableList;
+import static java.util.stream.Collectors.joining;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Streams;
 import com.google.devtools.build.buildjar.JarOwner;
 import com.google.devtools.build.buildjar.javac.plugins.BlazeJavaCompilerPlugin;
 import com.google.devtools.build.lib.view.proto.Deps.Dependencies;
@@ -37,6 +39,7 @@
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
+import java.util.stream.Stream;
 import javax.tools.Diagnostic;
 import javax.tools.JavaFileObject;
 
@@ -340,12 +343,11 @@ public static class Builder {
     private static class DefaultFixMessage implements FixMessage {
       @Override
       public String get(Iterable<JarOwner> missing, String recipient, DependencyModule depModule) {
-        StringBuilder missingTargetsStr = new StringBuilder();
-        for (JarOwner owner : missing) {
-          missingTargetsStr.append(owner.label());
-          missingTargetsStr.append(" ");
-        }
-
+        // TODO(cushon): remove the extra whitespace at the end, and fix local_repository_test_jdk8
+        String missingTargetsStr =
+            Streams.stream(missing)
+                .flatMap(owner -> owner.label().map(Stream::of).orElse(Stream.empty()))
+                .collect(joining(" ", "", " "));
         return String.format(
             "%1$s ** Please add the following dependencies:%2$s \n  %3$s to %4$s \n"
                 + "%1$s ** You can use the following buildozer command:%2$s "

src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/StrictJavaDepsPlugin.java

@@ -14,13 +14,13 @@
 
 package com.google.devtools.build.buildjar.javac.plugins.dependency;
 
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.collect.Iterables.getOnlyElement;
 import static com.google.devtools.build.buildjar.javac.plugins.dependency.DependencyModule.StrictJavaDeps.ERROR;
 
 import com.google.auto.value.AutoValue;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Ordering;
 import com.google.devtools.build.buildjar.JarOwner;
 import com.google.devtools.build.buildjar.javac.plugins.BlazeJavaCompilerPlugin;
 import com.google.devtools.build.buildjar.javac.plugins.dependency.DependencyModule.StrictJavaDeps;
@@ -45,12 +45,12 @@
 import java.io.PrintWriter;
 import java.io.UncheckedIOException;
 import java.nio.file.Path;
-import java.text.MessageFormat;
 import java.util.ArrayList;
+import java.util.Comparator;
 import java.util.HashSet;
-import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Properties;
 import java.util.Set;
 import java.util.jar.Attributes;
@@ -209,14 +209,18 @@ public void finish() {
               ? null
               // we don't use the target mapping for the target, just the missing deps
               : canonicalizeTarget(dependencyModule.getTargetLabel());
-      LinkedHashSet<JarOwner> canonicalizedMissing = new LinkedHashSet<>();
-      for (JarOwner owner :
-          Ordering.natural().onResultOf(JarOwner.LABEL).immutableSortedCopy(missingTargets)) {
-        // for dependencies that are missing we canonicalize and remap the target so we don't
-        // suggest private build labels.
-        String actualTarget = canonicalizeTarget(remapTarget(owner.label()));
-        canonicalizedMissing.add(JarOwner.create(actualTarget, owner.aspect()));
-      }
+      Set<JarOwner> canonicalizedMissing =
+          missingTargets
+              .stream()
+              .filter(owner -> owner.label().isPresent())
+              .sorted(Comparator.comparing((JarOwner owner) -> owner.label().get()))
+              // for dependencies that are missing we canonicalize and remap the target so we don't
+              // suggest private build labels.
+              .map(
+                  owner ->
+                      owner.withLabel(
+                          owner.label().map(label -> canonicalizeTarget(remapTarget(label)))))
+              .collect(toImmutableSet());
       errWriter.print(
           dependencyModule
               .getFixMessage()
@@ -231,10 +235,6 @@ public void finish() {
    */
   private static class CheckingTreeScanner extends TreeScanner {
 
-    private static final String TRANSITIVE_DEP_MESSAGE =
-        "[strict] Using {0} from an indirect dependency (TOOL_INFO: \"{1}\"). "
-            + "See command below **";
-
     private final ImmutableSet<Path> directJars;
 
     /** Strict deps diagnostics. */
@@ -308,20 +308,27 @@ private void collectExplicitDependency(Path jarPath, JCTree node, Symbol sym) {
           // IO cost here is fine because we only hit this path for an explicit dependency
           // _not_ in the direct jars, i.e. an error
           JarOwner owner = readJarOwnerFromManifest(jarPath);
-          if (owner != null && seenTargets.add(owner)) {
+          if (seenTargets.add(owner)) {
             // owner is of the form "//label/of:rule <Aspect name>" where <Aspect name> is
             // optional.
-            String canonicalTargetName = canonicalizeTarget(remapTarget(owner.label()));
+            Optional<String> canonicalTargetName =
+                owner.label().map(label -> canonicalizeTarget(remapTarget(label)));
             missingTargets.add(owner);
             String toolInfo =
-                owner.aspect() == null
-                    ? canonicalTargetName
-                    : String.format("%s wrapped in %s", canonicalTargetName, owner.aspect());
+                owner.aspect().isPresent()
+                    ? String.format(
+                        "%s wrapped in %s", canonicalTargetName.get(), owner.aspect().get())
+                    : canonicalTargetName.isPresent()
+                        ? canonicalTargetName.get()
+                        : owner.jar().toString();
             String used =
                 sym.getSimpleName().contentEquals("package-info")
                     ? "package " + sym.getEnclosingElement()
                     : "type " + sym;
-            String message = MessageFormat.format(TRANSITIVE_DEP_MESSAGE, used, toolInfo);
+            String message =
+                String.format(
+                    "[strict] Using %s from an indirect dependency (TOOL_INFO: \"%s\").%s",
+                    used, toolInfo, (owner.label().isPresent() ? " See command below **" : ""));
             diagnostics.add(SjdDiagnostic.create(node.pos, message, source));
           }
         }
@@ -346,15 +353,15 @@ private static JarOwner readJarOwnerFromManifest(Path jarPath) {
       try (JarFile jarFile = new JarFile(jarPath.toFile())) {
         Manifest manifest = jarFile.getManifest();
         if (manifest == null) {
-          return null;
+          return JarOwner.create(jarPath);
         }
         Attributes attributes = manifest.getMainAttributes();
         String label = (String) attributes.get(TARGET_LABEL);
         if (label == null) {
-          return null;
+          return JarOwner.create(jarPath);
         }
         String injectingRuleKind = (String) attributes.get(INJECTING_RULE_KIND);
-        return JarOwner.create(label, injectingRuleKind);
+        return JarOwner.create(jarPath, label, Optional.ofNullable(injectingRuleKind));
       } catch (IOException e) {
         // This jar file pretty much has to exist, we just used it in the compiler. Throw unchecked.
         throw new UncheckedIOException(e);