Make java_common.compile's javacopt handling consistent with native Java rules

Compilations performed by java_common.compile now use the javacopts in the
java_toolchain by default, instead of requiring them to be explicitly retrived
using java_common.default_javac_opts, for consistency with the native rules.

java_common.compile(javac_opts=...) can still be used to pass additional javacopts.

RELNOTES: Make java_common.compile now uses java_toolchain javacopts by default; explicitly retrieving them using java_common.default_javac_opts is unnecessary.
PiperOrigin-RevId: 194254098

11 files changed, 76 insertions, 54 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidSemantics.java
index 4d957a9e19..cf7ba3c48e 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidSemantics.java
@@ -43,13 +43,11 @@ public class BazelAndroidSemantics implements AndroidSemantics {
   }
 
   @Override
-  public ImmutableList<String> getJavacArguments(RuleContext ruleContext) {
+  public ImmutableList<String> getCompatibleJavacOptions(RuleContext ruleContext) {
     ImmutableList.Builder<String> javacArgs = new ImmutableList.Builder<>();
-
     if (!ruleContext.getFragment(AndroidConfiguration.class).desugarJava8()) {
       javacArgs.add("-source", "7", "-target", "7");
     }
-
     return javacArgs.build();
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java
index 4efc58365b..ecb65b4b7a 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java
@@ -62,6 +62,7 @@ import com.google.devtools.build.lib.rules.java.JavaRuleOutputJarsProvider;
 import com.google.devtools.build.lib.rules.java.JavaSemantics;
 import com.google.devtools.build.lib.rules.java.JavaSourceJarsProvider;
 import com.google.devtools.build.lib.rules.java.JavaTargetAttributes;
+import com.google.devtools.build.lib.rules.java.JavaToolchainProvider;
 import com.google.devtools.build.lib.rules.java.JavaUtil;
 import com.google.devtools.build.lib.rules.java.proto.GeneratedExtensionRegistryProvider;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
@@ -553,8 +554,9 @@ public class BazelJavaSemantics implements JavaSemantics {
   }
 
   @Override
-  public Iterable<String> getExtraJavacOpts(RuleContext ruleContext) {
-    return ImmutableList.<String>of();
+  public ImmutableList<String> getCompatibleJavacOptions(
+      RuleContext ruleContext, JavaToolchainProvider toolchain) {
+    return ImmutableList.of();
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java
index 7240e132c3..a89910839d 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java
@@ -16,7 +16,6 @@ package com.google.devtools.build.lib.rules.android;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ResourceSet;
 import com.google.devtools.build.lib.analysis.AnalysisUtils;
@@ -466,13 +465,14 @@ public class AndroidCommon {
       bootclasspath =
           ImmutableList.of(AndroidSdkProvider.fromRuleContext(ruleContext).getAndroidJar());
     }
-    Iterable<String> javacopts = androidSemantics.getJavacArguments(ruleContext);
+    ImmutableList.Builder<String> javacopts = ImmutableList.builder();
+    javacopts.addAll(androidSemantics.getCompatibleJavacOptions(ruleContext));
     if (DataBinding.isEnabled(ruleContext)) {
-      javacopts = Iterables.concat(javacopts, DataBinding.getJavacopts(ruleContext, isBinary));
+      javacopts.addAll(DataBinding.getJavacOpts(ruleContext, isBinary));
     }
     JavaTargetAttributes.Builder attributes =
         javaCommon
-            .initCommon(idlHelper.getIdlGeneratedJavaSources(), javacopts)
+            .initCommon(idlHelper.getIdlGeneratedJavaSources(), javacopts.build())
             .setBootClassPath(bootclasspath);
     if (DataBinding.isEnabled(ruleContext)) {
       DataBinding.addAnnotationProcessor(ruleContext, attributes);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidSemantics.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidSemantics.java
index 87130b7351..3726808cb5 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidSemantics.java
@@ -69,10 +69,10 @@ public interface AndroidSemantics {
    * Returns the command line options to be used when compiling Java code for {@code android_*}
    * rules.
    *
-   * <p>These will come after the default options specified by the toolchain and the ones in the
-   * {@code javacopts} attribute.
+   * <p>These will come after the default options specified by the toolchain, and before the ones in
+   * the {@code javacopts} attribute.
    */
-  ImmutableList<String> getJavacArguments(RuleContext ruleContext);
+  ImmutableList<String> getCompatibleJavacOptions(RuleContext ruleContext);
 
   /**
    * Configures the builder for generating the output jar used to configure the main dex file.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/DataBinding.java b/src/main/java/com/google/devtools/build/lib/rules/android/DataBinding.java
index 338494eb4f..9f6ac28ea6 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/DataBinding.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/DataBinding.java
@@ -169,7 +169,7 @@ public final class DataBinding {
   }
 
   /** The javac flags that are needed to configure data binding's annotation processor. */
-  static ImmutableList<String> getJavacopts(RuleContext ruleContext, boolean isBinary) {
+  static ImmutableList<String> getJavacOpts(RuleContext ruleContext, boolean isBinary) {
     ImmutableList.Builder<String> flags = ImmutableList.builder();
     String metadataOutputDir = getDataBindingExecPath(ruleContext).getPathString();
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java
index 2545d99b90..26ba4441ea 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java
@@ -61,7 +61,6 @@ import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Set;
-import java.util.stream.Stream;
 import javax.annotation.Nullable;
 
 /**
@@ -108,6 +107,7 @@ public class JavaCommon {
 
   private final RuleContext ruleContext;
   private final JavaSemantics semantics;
+  private final JavaToolchainProvider javaToolchain;
   private JavaCompilationHelper javaCompilationHelper;
 
   public JavaCommon(RuleContext ruleContext, JavaSemantics semantics) {
@@ -144,6 +144,7 @@ public class JavaCommon {
       ImmutableList<TransitiveInfoCollection> runtimeDeps,
       ImmutableList<TransitiveInfoCollection> bothDeps) {
     this.ruleContext = ruleContext;
+    this.javaToolchain = JavaToolchainProvider.from(ruleContext);
     this.semantics = semantics;
     this.sources = sources;
     this.targetsTreatedAsDeps = ImmutableMap.of(
@@ -451,39 +452,34 @@ public class JavaCommon {
 
   public final void initializeJavacOpts() {
     Preconditions.checkState(javacOpts == null);
-    javacOpts = computeJavacOpts(semantics.getExtraJavacOpts(ruleContext));
+    javacOpts = computeJavacOpts(getCompatibleJavacOptions());
   }
 
-  /**
-   * For backwards compatibility, this method allows multiple calls to set the Javac opts. Do not
-   * use this.
-   */
-  @Deprecated
-  public final void initializeJavacOpts(Iterable<String> extraJavacOpts) {
-    javacOpts = computeJavacOpts(extraJavacOpts);
+  /** Computes javacopts for the current rule. */
+  private ImmutableList<String> computeJavacOpts(Collection<String> extraRuleJavacOpts) {
+    return ImmutableList.<String>builder()
+        .addAll(computeToolchainJavacOpts(ruleContext, javaToolchain))
+        .addAll(extraRuleJavacOpts)
+        .addAll(ruleContext.getExpander().withDataLocations().tokenized("javacopts"))
+        .build();
   }
 
-  private ImmutableList<String> computeJavacOpts(Iterable<String> extraJavacOpts) {
+  /**
+   * Returns the toolchain javacopts for the current rule, including global defaults as well as any
+   * per-package configuration.
+   */
+  public static ImmutableList<String> computeToolchainJavacOpts(
+      RuleContext ruleContext, JavaToolchainProvider toolchain) {
     return Streams.concat(
-            toolchainJavacOpts(ruleContext),
-            Streams.stream(extraJavacOpts),
-            ruleContext.getExpander().withDataLocations().tokenized("javacopts").stream())
+            toolchain.getJavacOptions().stream(),
+            toolchain
+                .packageConfiguration()
+                .stream()
+                .filter(p -> p.matches(ruleContext.getLabel()))
+                .flatMap(p -> p.javacopts().stream()))
         .collect(toImmutableList());
   }
 
-  private Stream<String> toolchainJavacOpts(RuleContext ruleContext) {
-    JavaToolchainProvider toolchain = JavaToolchainProvider.from(ruleContext);
-    return Stream.concat(
-        toolchain.getJavacOptions().stream(),
-        // Enable any javacopts from java_toolchain.packages that are configured for the current
-        // package.
-        toolchain
-            .packageConfiguration()
-            .stream()
-            .filter(p -> p.matches(ruleContext.getLabel()))
-            .flatMap(p -> p.javacopts().stream()));
-  }
-
   public static PathFragment getHostJavaExecutable(RuleContext ruleContext) {
     return JavaRuntimeInfo.forHost(ruleContext).javaBinaryExecPath();
   }
@@ -600,7 +596,11 @@ public class JavaCommon {
   }
 
   public JavaTargetAttributes.Builder initCommon() {
-    return initCommon(ImmutableList.<Artifact>of(), semantics.getExtraJavacOpts(ruleContext));
+    return initCommon(ImmutableList.<Artifact>of(), getCompatibleJavacOptions());
+  }
+
+  private ImmutableList<String> getCompatibleJavacOptions() {
+    return semantics.getCompatibleJavacOptions(ruleContext, javaToolchain);
   }
 
   /**
@@ -614,7 +614,7 @@ public class JavaCommon {
   public JavaTargetAttributes.Builder initCommon(
       Collection<Artifact> extraSrcs, Iterable<String> extraJavacOpts) {
     Preconditions.checkState(javacOpts == null);
-    javacOpts = computeJavacOpts(extraJavacOpts);
+    javacOpts = computeJavacOpts(ImmutableList.copyOf(extraJavacOpts));
     activePlugins = collectPlugins();
 
     JavaTargetAttributes.Builder javaTargetAttributes = new JavaTargetAttributes.Builder(semantics);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaInfoBuildHelper.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaInfoBuildHelper.java
index faede121b7..9c98907be2 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaInfoBuildHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaInfoBuildHelper.java
@@ -451,6 +451,8 @@ final class JavaInfoBuildHelper {
       throw new EvalException(null, "'host_javabase' must point to a Java runtime");
     }
 
+    JavaToolchainProvider toolchainProvider = getJavaToolchainProvider(javaToolchain);
+
     JavaLibraryHelper helper =
         new JavaLibraryHelper(skylarkRuleContext.getRuleContext())
             .setOutput(outputJar)
@@ -458,7 +460,16 @@ final class JavaInfoBuildHelper {
             .addSourceFiles(sourceFiles)
             .addResources(resources)
             .setSourcePathEntries(sourcepathEntries)
-            .setJavacOpts(javacOpts);
+            .setJavacOpts(
+                ImmutableList.<String>builder()
+                    .addAll(
+                        JavaCommon.computeToolchainJavacOpts(
+                            skylarkRuleContext.getRuleContext(), toolchainProvider))
+                    .addAll(
+                        javaSemantics.getCompatibleJavacOptions(
+                            skylarkRuleContext.getRuleContext(), toolchainProvider))
+                    .addAll(javacOpts)
+                    .build());
 
     List<JavaCompilationArgsProvider> depsCompilationArgsProviders =
         JavaInfo.fetchProvidersFromList(deps, JavaCompilationArgsProvider.class);
@@ -486,7 +497,7 @@ final class JavaInfoBuildHelper {
     JavaCompilationArtifacts artifacts =
         helper.build(
             javaSemantics,
-            getJavaToolchainProvider(javaToolchain),
+            toolchainProvider,
             javaRuntimeInfo,
             SkylarkList.createImmutable(ImmutableList.of()),
             outputJarsBuilder,
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java
index 5ee66560c1..d6b7d60527 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java
@@ -364,9 +364,13 @@ public interface JavaSemantics {
   void addRunfilesForLibrary(RuleContext ruleContext, Runfiles.Builder runfilesBuilder);
 
   /**
-   * Returns the additional options to be passed to javac.
+   * Returns the command line options to be used when compiling Java code for {@code java_*} rules.
+   *
+   * <p>These will come after the default options specified by the toolchain, and before the ones in
+   * the {@code javacopts} attribute.
    */
-  Iterable<String> getExtraJavacOpts(RuleContext ruleContext);
+  ImmutableList<String> getCompatibleJavacOptions(
+      RuleContext ruleContext, JavaToolchainProvider toolchain);
 
   /**
    * Add additional targets to be treated as direct dependencies.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaSkylarkCommon.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaSkylarkCommon.java
index 9d46f886ab..6570c5e771 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaSkylarkCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaSkylarkCommon.java
@@ -524,8 +524,9 @@ public class JavaSkylarkCommon {
       mandatoryPositionals = 1,
       parameters = {
         @Param(name = "java_toolchain_attr", positional = false, named = true, type = String.class)
-      }
-  )
+      })
+  // TODO(b/78512644): migrate callers to passing explicit javacopts or using custom toolchains, and
+  // delete
   public static ImmutableList<String> getDefaultJavacOpts(
       SkylarkRuleContext skylarkRuleContext, String javaToolchainAttr) throws EvalException {
     RuleContext ruleContext = skylarkRuleContext.getRuleContext();
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoSkylarkCommon.java b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoSkylarkCommon.java
index 08482be9e2..e74d5a005f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoSkylarkCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoSkylarkCommon.java
@@ -23,7 +23,6 @@ import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.skylark.SkylarkRuleContext;
 import com.google.devtools.build.lib.rules.java.JavaCompilationArgsProvider;
 import com.google.devtools.build.lib.rules.java.JavaInfo;
-import com.google.devtools.build.lib.rules.java.JavaSemantics;
 import com.google.devtools.build.lib.rules.java.JavaToolchainProvider;
 import com.google.devtools.build.lib.rules.proto.ProtoCompileActionBuilder;
 import com.google.devtools.build.lib.rules.proto.ProtoLangToolchainProvider;
@@ -136,7 +135,8 @@ public class JavaProtoSkylarkCommon {
       @Param(name = "java_toolchain_attr", positional = false, named = true, type = String.class)
     }
   )
-  // TODO(elenairina): Consider a nicer way of returning this, taking in a JavaToolchainProvider.
+  // TODO(b/78512644): migrate callers to passing explicit proto javacopts or using custom
+  // toolchains, and delete
   public static ImmutableList<String> getJavacOpts(
       SkylarkRuleContext skylarkRuleContext, String javaToolchainAttr) throws EvalException {
     ConfiguredTarget javaToolchainConfigTarget =
@@ -144,10 +144,7 @@ public class JavaProtoSkylarkCommon {
     JavaToolchainProvider toolchain =
         checkNotNull(JavaToolchainProvider.from(javaToolchainConfigTarget));
 
-    return ImmutableList.<String>builder()
-        .addAll(toolchain.getJavacOptions())
-        .addAll(toolchain.getCompatibleJavacOptions(JavaSemantics.PROTO_JAVACOPTS_KEY))
-        .build();
+    return ProtoJavacOpts.constructJavacOpts(skylarkRuleContext.getRuleContext(), toolchain);
   }
 
   private static ProtoLangToolchainProvider getProtoToolchainProvider(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/proto/ProtoJavacOpts.java b/src/main/java/com/google/devtools/build/lib/rules/java/proto/ProtoJavacOpts.java
index 5717828c98..8aea8daa4f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/proto/ProtoJavacOpts.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/proto/ProtoJavacOpts.java
@@ -30,7 +30,16 @@ public class ProtoJavacOpts {
    * <p>See java_toolchain.compatible_javacopts for the javacopts required for protos.
    */
   public static ImmutableList<String> constructJavacOpts(RuleContext ruleContext) {
-    JavaToolchainProvider toolchain = JavaToolchainProvider.from(ruleContext);
+    return constructJavacOpts(ruleContext, JavaToolchainProvider.from(ruleContext));
+  }
+
+  /**
+   * Returns javacopts for compiling the Java source files generated by the proto compiler.
+   *
+   * <p>See java_toolchain.compatible_javacopts for the javacopts required for protos.
+   */
+  public static ImmutableList<String> constructJavacOpts(
+      RuleContext ruleContext, JavaToolchainProvider toolchain) {
     return ImmutableList.<String>builder()
         .addAll(toolchain.getJavacOptions())
         .addAll(toolchain.getCompatibleJavacOptions(JavaSemantics.PROTO_JAVACOPTS_KEY))