Revamp JavaToolchain:

- Rely on existing infrastructure to verify that attributes only contain a single artifact and that mandatory attributes are specified
- Add single artifact modifiers to attributes that need them
- Remove "final" modifiers from method variables (we don't use them in Blaze)
- Replace deprecated #add() with #addProvider()

Since this relies on existing infrastructure, I thought adding tests is redundant. I did test it manually, though, to be extra safe by testing what happens if these attributes contain an empty filegroup or a filegroup with multiple artifacts in it.

RELNOTES: None.
PiperOrigin-RevId: 158975017

2 files changed, 57 insertions, 64 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchain.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchain.java
index 5ed85eec9d..a2068f6375 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchain.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchain.java
@@ -17,12 +17,11 @@ import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
-import com.google.devtools.build.lib.analysis.FileProvider;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.LocationExpander;
+import com.google.devtools.build.lib.analysis.PrerequisiteArtifacts;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
 import com.google.devtools.build.lib.analysis.RuleContext;
@@ -46,44 +45,40 @@ public final class JavaToolchain implements RuleConfiguredTargetFactory {
 
   @Override
   public ConfiguredTarget create(RuleContext ruleContext) throws RuleErrorException {
-    final String source = ruleContext.attributes().get("source_version", Type.STRING);
-    final String target = ruleContext.attributes().get("target_version", Type.STRING);
-    final NestedSet<Artifact> bootclasspath = getArtifactList("bootclasspath", ruleContext);
-    final NestedSet<Artifact> extclasspath = getArtifactList("extclasspath", ruleContext);
-    final String encoding = ruleContext.attributes().get("encoding", Type.STRING);
-    final List<String> xlint = ruleContext.attributes().get("xlint", Type.STRING_LIST);
-    final List<String> misc = ruleContext.getTokenizedStringListAttr("misc");
-    final boolean javacSupportsWorkers =
+    String source = ruleContext.attributes().get("source_version", Type.STRING);
+    String target = ruleContext.attributes().get("target_version", Type.STRING);
+    NestedSet<Artifact> bootclasspath = PrerequisiteArtifacts.nestedSet(
+        ruleContext, "bootclasspath", Mode.HOST);
+    NestedSet<Artifact> extclasspath = PrerequisiteArtifacts.nestedSet(
+        ruleContext, "extclasspath", Mode.HOST);
+    String encoding = ruleContext.attributes().get("encoding", Type.STRING);
+    List<String> xlint = ruleContext.attributes().get("xlint", Type.STRING_LIST);
+    List<String> misc = ruleContext.getTokenizedStringListAttr("misc");
+    boolean javacSupportsWorkers =
         ruleContext.attributes().get("javac_supports_workers", Type.BOOLEAN);
-    TransitiveInfoCollection javacDep = ruleContext.getPrerequisite("javac", Mode.HOST);
-    Artifact javac = null;
-    NestedSet<Artifact> javacJars = javacDep.getProvider(FileProvider.class).getFilesToBuild();
-    if (Iterables.size(javacJars) == 1) {
-      javac = Iterables.getOnlyElement(javacJars);
-    } else {
-      ruleContext.attributeError("javac", javacDep.getLabel() + " expected a single artifact");
-      return null;
-    }
-    final List<String> jvmOpts =
-        getJvmOpts(
-            ruleContext,
-            ImmutableMap.<Label, ImmutableCollection<Artifact>>of(
-                javacDep.getLabel(), ImmutableList.of(javac)));
-    Artifact javabuilder = getArtifact("javabuilder", ruleContext);
-    Artifact headerCompiler = getArtifact("header_compiler", ruleContext);
+    Artifact javac = ruleContext.getPrerequisiteArtifact("javac", Mode.HOST);
+    Artifact javabuilder = ruleContext.getPrerequisiteArtifact("javabuilder", Mode.HOST);
+    Artifact headerCompiler = ruleContext.getPrerequisiteArtifact("header_compiler", Mode.HOST);
     boolean forciblyDisableHeaderCompilation =
         ruleContext.attributes().get("forcibly_disable_header_compilation", Type.BOOLEAN);
-    Artifact singleJar = getArtifact("singlejar", ruleContext);
-    Artifact oneVersion = getArtifact("oneversion", ruleContext);
-    Artifact oneVersionWhitelist = getArtifact("oneversion_whitelist", ruleContext);
-    Artifact genClass = getArtifact("genclass", ruleContext);
-    Artifact resourceJarBuilder = getArtifact("resourcejar", ruleContext);
-    Artifact timezoneData = getArtifact("timezone_data", ruleContext);
+    Artifact singleJar = ruleContext.getPrerequisiteArtifact("singlejar", Mode.HOST);
+    Artifact oneVersion = ruleContext.getPrerequisiteArtifact("oneversion", Mode.HOST);
+    Artifact oneVersionWhitelist = ruleContext
+        .getPrerequisiteArtifact("oneversion_whitelist", Mode.HOST);
+    Artifact genClass = ruleContext.getPrerequisiteArtifact("genclass", Mode.HOST);
+    Artifact resourceJarBuilder = ruleContext.getPrerequisiteArtifact("resourcejar", Mode.HOST);
+    Artifact timezoneData = ruleContext.getPrerequisiteArtifact("timezone_data", Mode.HOST);
     FilesToRunProvider ijar = ruleContext.getExecutablePrerequisite("ijar", Mode.HOST);
     ImmutableListMultimap<String, String> compatibleJavacOptions =
         getCompatibleJavacOptions(ruleContext);
 
-    final JavaToolchainData toolchainData =
+    TransitiveInfoCollection javacDep = ruleContext.getPrerequisite("javac", Mode.HOST);
+    List<String> jvmOpts = getJvmOpts(
+        ruleContext,
+        ImmutableMap.<Label, ImmutableCollection<Artifact>>of(
+            javacDep.getLabel(), ImmutableList.of(javac)));
+
+    JavaToolchainData toolchainData =
         new JavaToolchainData(
             source,
             target,
@@ -94,7 +89,7 @@ public final class JavaToolchain implements RuleConfiguredTargetFactory {
             misc,
             jvmOpts,
             javacSupportsWorkers ? SupportsWorkers.YES : SupportsWorkers.NO);
-    final JavaConfiguration configuration = ruleContext.getFragment(JavaConfiguration.class);
+    JavaConfiguration configuration = ruleContext.getFragment(JavaConfiguration.class);
     JavaToolchainProvider provider =
         JavaToolchainProvider.create(
             ruleContext.getLabel(),
@@ -118,9 +113,9 @@ public final class JavaToolchain implements RuleConfiguredTargetFactory {
         new RuleConfiguredTargetBuilder(ruleContext)
             .addSkylarkTransitiveInfo(
                 JavaToolchainSkylarkApiProvider.NAME, new JavaToolchainSkylarkApiProvider())
-            .add(JavaToolchainProvider.class, provider)
-            .setFilesToBuild(new NestedSetBuilder<Artifact>(Order.STABLE_ORDER).build())
-            .add(RunfilesProvider.class, RunfilesProvider.simple(Runfiles.EMPTY));
+            .addProvider(JavaToolchainProvider.class, provider)
+            .addProvider(RunfilesProvider.class, RunfilesProvider.simple(Runfiles.EMPTY))
+            .setFilesToBuild(new NestedSetBuilder<Artifact>(Order.STABLE_ORDER).build());
 
     return builder.build();
   }
@@ -135,29 +130,6 @@ public final class JavaToolchain implements RuleConfiguredTargetFactory {
     return result.build();
   }
 
-  private static Artifact getArtifact(String attributeName, RuleContext ruleContext) {
-    TransitiveInfoCollection prerequisite = ruleContext.getPrerequisite(attributeName, Mode.HOST);
-    if (prerequisite == null) {
-      return null;
-    }
-    Iterable<Artifact> artifacts = prerequisite.getProvider(FileProvider.class).getFilesToBuild();
-    if (Iterables.size(artifacts) != 1) {
-      ruleContext.attributeError(
-          attributeName, prerequisite.getLabel() + " expected a single artifact");
-      return null;
-    }
-    return Iterables.getOnlyElement(artifacts);
-  }
-
-  private static NestedSet<Artifact> getArtifactList(
-      String attributeName, RuleContext ruleContext) {
-    TransitiveInfoCollection prerequisite = ruleContext.getPrerequisite(attributeName, Mode.HOST);
-    if (prerequisite == null) {
-      return null;
-    }
-    return prerequisite.getProvider(FileProvider.class).getFilesToBuild();
-  }
-
   private static ImmutableList<String> getJvmOpts(
       RuleContext ruleContext, ImmutableMap<Label, ImmutableCollection<Artifact>> locations) {
     // LocationExpander is used directly instead of e.g. getExpandedStringListAttr because the
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainRule.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainRule.java
index a3115e3932..5fabbdf6bd 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainRule.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainRule.java
@@ -98,7 +98,11 @@ public final class JavaToolchainRule implements RuleDefinition {
         /* <!-- #BLAZE_RULE(java_toolchain).ATTRIBUTE(javac) -->
         Label of the javac jar.
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
-        .add(attr("javac", LABEL_LIST).mandatory().cfg(HOST).allowedFileTypes(FileTypeSet.ANY_FILE))
+        .add(attr("javac", LABEL_LIST)
+            .mandatory()
+            .cfg(HOST)
+            .singleArtifact()
+            .allowedFileTypes(FileTypeSet.ANY_FILE))
         /* <!-- #BLAZE_RULE(java_toolchain).ATTRIBUTE(javabuilder) -->
         Label of the JavaBuilder deploy jar.
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
@@ -106,6 +110,7 @@ public final class JavaToolchainRule implements RuleDefinition {
             attr("javabuilder", LABEL_LIST)
                 .mandatory()
                 .cfg(HOST)
+                .singleArtifact()
                 .allowedFileTypes(FileTypeSet.ANY_FILE)
                 .exec())
         /* <!-- #BLAZE_RULE(java_toolchain).ATTRIBUTE(singlejar) -->
@@ -115,6 +120,7 @@ public final class JavaToolchainRule implements RuleDefinition {
             attr("singlejar", LABEL_LIST)
                 .mandatory()
                 .cfg(HOST)
+                .singleArtifact()
                 .allowedFileTypes(FileTypeSet.ANY_FILE)
                 .exec())
         /* <!-- #BLAZE_RULE(java_toolchain).ATTRIBUTE(genclass) -->
@@ -123,6 +129,7 @@ public final class JavaToolchainRule implements RuleDefinition {
         .add(
             attr("genclass", LABEL_LIST)
                 .mandatory()
+                .singleArtifact()
                 .cfg(HOST)
                 .allowedFileTypes(FileTypeSet.ANY_FILE)
                 .exec())
@@ -130,12 +137,20 @@ public final class JavaToolchainRule implements RuleDefinition {
         Label of the resource jar builder executable.
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
         .add(
-            attr("resourcejar", LABEL_LIST).cfg(HOST).allowedFileTypes(FileTypeSet.ANY_FILE).exec())
+            attr("resourcejar", LABEL_LIST)
+                .cfg(HOST)
+                .singleArtifact()
+                .allowedFileTypes(FileTypeSet.ANY_FILE)
+                .exec())
         /* <!-- #BLAZE_RULE(java_toolchain).ATTRIBUTE(timezone_data) -->
         Label of a resource jar containing timezone data. If set, the timezone data is added as an
         implicitly runtime dependency of all java_binary rules.
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
-        .add(attr("timezone_data", LABEL).cfg(HOST).allowedFileTypes(FileTypeSet.ANY_FILE).exec())
+        .add(attr("timezone_data", LABEL)
+            .cfg(HOST)
+            .singleArtifact()
+            .allowedFileTypes(FileTypeSet.ANY_FILE)
+            .exec())
         /* <!-- #BLAZE_RULE(java_toolchain).ATTRIBUTE(ijar) -->
         Label of the ijar executable.
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
@@ -151,18 +166,24 @@ public final class JavaToolchainRule implements RuleDefinition {
         .add(
             attr("header_compiler", LABEL_LIST)
                 .cfg(HOST)
+                .singleArtifact()
                 .allowedFileTypes(FileTypeSet.ANY_FILE)
                 .exec())
         /* <!-- #BLAZE_RULE(java_toolchain).ATTRIBUTE(oneversion) -->
         Label of the one-version enforcement binary.
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
-        .add(attr("oneversion", LABEL).cfg(HOST).allowedFileTypes(FileTypeSet.ANY_FILE).exec())
+        .add(attr("oneversion", LABEL)
+            .cfg(HOST)
+            .singleArtifact()
+            .allowedFileTypes(FileTypeSet.ANY_FILE)
+            .exec())
         /* <!-- #BLAZE_RULE(java_toolchain).ATTRIBUTE(oneversion_whitelist) -->
         Label of the one-version whitelist.
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
         .add(
             attr("oneversion_whitelist", LABEL)
                 .cfg(HOST)
+                .singleArtifact()
                 .allowedFileTypes(FileTypeSet.ANY_FILE)
                 .exec())
         /* <!-- #BLAZE_RULE(java_toolchain).ATTRIBUTE(forcibly_disable_header_compilation) -->