Consolidate Bazel-specific javacopt handling

--
MOS_MIGRATED_REVID=127957458

8 files changed, 162 insertions, 170 deletions

diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/BUILD b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/BUILD
index c9c6b24983..8dc934e8ac 100644
--- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/BUILD
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/BUILD
@@ -21,11 +21,17 @@ java_library(
 
 java_library(
     name = "javac",
-    srcs = glob([
-        "javac/*.java",
-    ]),
+    srcs = glob(
+        [
+            "javac/*.java",
+        ],
+        exclude = [
+            "javac/JavacOptions.java",
+        ],
+    ),
     deps = [
         ":invalid_command_line_exception",
+        ":javac_options",
         "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins",
         "//third_party:guava",
         "//third_party:jsr305",
@@ -45,6 +51,7 @@ java_library(
     deps = [
         ":invalid_command_line_exception",
         ":javac",
+        ":javac_options",
         "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar/jarhelper",
         "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins",
         "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins:classloader",
@@ -69,8 +76,13 @@ java_library(
     srcs = [
         "javac/JavacOptions.java",
     ],
-    visibility = ["//java/com/google/devtools/build/buildjar:__pkg__"],
+    visibility = [
+        "//java/com/google/devtools/build/buildjar:__pkg__",
+        "//src/java_tools/buildjar/java/com/google/devtools/build/java/bazel:__pkg__",
+        "//src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac:__pkg__",
+    ],
     deps = [
+        "//third_party:auto_value",
         "//third_party:guava",
     ],
 )
@@ -91,6 +103,7 @@ load("//tools/build_rules:java_rules_skylark.bzl", "bootstrap_java_library", "bo
 bootstrap_java_library(
     name = "skylark-deps",
     jars = [
+        "//third_party:auto_value-jars",
         "//third_party:error_prone-jars",
         "//third_party:guava-jars",
         "//third_party:jsr305-jars",
diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/JavacOptions.java b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/JavacOptions.java
index e4452a36a0..4f7e749c7e 100644
--- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/JavacOptions.java
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/JavacOptions.java
@@ -14,45 +14,85 @@
 
 package com.google.devtools.build.buildjar.javac;
 
+import com.google.auto.value.AutoValue;
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
-
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Set;
 
 /**
- * Preprocess javac -Xlint options. We
- * also need to make the different versions of javac treat -Xlint options
- * uniformly.
- *
- * <p>Some versions of javac now process the -Xlint options without allowing
- * later options to override earlier ones on the command line. For example,
- * {@code -Xlint:All -Xlint:None} results in all warnings being enabled.
+ * Preprocess javac -Xlint options. We also need to make the different versions of javac treat
+ * -Xlint options uniformly.
  *
- * <p>This class preprocesses the -Xlint options within the javac options to
- * achieve a command line that is sensitive to ordering. That is, with this
- * preprocessing step, {@code -Xlint:all -Xlint:none} results in no
- * warnings being enabled.
+ * <p>Some versions of javac now process the -Xlint options without allowing later options to
+ * override earlier ones on the command line. For example, {@code -Xlint:All -Xlint:None} results in
+ * all warnings being enabled.
  *
- * <p>TODO(bazel-team): Convince the current javac owners that the javac options
- * need to behave the way this class behaves. :-)
+ * <p>This class preprocesses the -Xlint options within the javac options to achieve a command line
+ * that is sensitive to ordering. That is, with this preprocessing step, {@code -Xlint:all
+ * -Xlint:none} results in no warnings being enabled.
  */
 public final class JavacOptions {
 
+  /** Returns an immutable list containing all the non-Bazel specific Javac flags. */
+  public static ImmutableList<String> removeBazelSpecificFlags(String[] javacopts) {
+    return removeBazelSpecificFlags(Arrays.asList(javacopts));
+  }
+
+  /** Returns an immutable list containing all the non-Bazel specific Javac flags. */
+  public static ImmutableList<String> removeBazelSpecificFlags(Iterable<String> javacopts) {
+    return filterJavacopts(javacopts).standardJavacopts();
+  }
+
+  /** A collection of javac flags, divided into Bazel-specific and standard options. */
+  @AutoValue
+  public abstract static class FilteredJavacopts {
+    /** Bazel-specific javac flags, e.g. Error Prone's -Xep: flags. */
+    public abstract ImmutableList<String> bazelJavacopts();
+
+    /** Standard javac flags. */
+    public abstract ImmutableList<String> standardJavacopts();
+
+    /** Creates a {@link FilteredJavacopts}. */
+    public static FilteredJavacopts create(
+        ImmutableList<String> bazelJavacopts, ImmutableList<String> standardJavacopts) {
+      return new AutoValue_JavacOptions_FilteredJavacopts(bazelJavacopts, standardJavacopts);
+    }
+  }
+
+  /** Filters a list of javac flags into Bazel-specific and standard flags. */
+  public static FilteredJavacopts filterJavacopts(Iterable<String> javacopts) {
+    ImmutableList.Builder<String> bazelJavacopts = ImmutableList.builder();
+    ImmutableList.Builder<String> standardJavacopts = ImmutableList.builder();
+    for (String opt : javacopts) {
+      if (isBazelSpecificFlag(opt)) {
+        bazelJavacopts.add(opt);
+      } else {
+        standardJavacopts.add(opt);
+      }
+    }
+    return FilteredJavacopts.create(bazelJavacopts.build(), standardJavacopts.build());
+  }
+
+  private static boolean isBazelSpecificFlag(String opt) {
+    return opt.startsWith("-Werror:") || opt.startsWith("-Xep");
+  }
+
   private static final XlintOptionNormalizer XLINT_OPTION_NORMALIZER = new XlintOptionNormalizer();
 
   /**
    * Interface to define an option normalizer. For instance, to group all -Xlint: option into one
    * place.
-   * 
-   * <p>All normalizers used by the JavacOptions class will be started by calling the
-   * {@link #start()} method when starting the parsing of a list of option. For each option, the
-   * first option normalized whose {@link #processOption(String)} method returns true stops its
-   * parsing and the option is supposed to be added at the end to the normalized list of option with
-   * the {@link #normalize(List)} method. Options not handled by a normalizer will be returned as
-   * such in the normalized option list.
+   *
+   * <p>All normalizers used by the JavacOptions class will be started by calling the {@link
+   * #start()} method when starting the parsing of a list of option. For each option, the first
+   * option normalized whose {@link #processOption(String)} method returns true stops its parsing
+   * and the option is supposed to be added at the end to the normalized list of option with the
+   * {@link #normalize(List)} method. Options not handled by a normalizer will be returned as such
+   * in the normalized option list.
    */
   public static interface JavacOptionNormalizer {
     /** Resets the state of the normalizer to start a new option parsing. */
diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/BUILD b/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/BUILD
index 5b2b0b7955..3ede677953 100644
--- a/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/BUILD
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/BUILD
@@ -91,7 +91,11 @@ java_library(
     name = "JavaBuilderConfig",
     srcs = ["JavaBuilderConfig.java"],
     data = ["@bazel_tools//tools/jdk:JavaBuilder_deploy.jar"],
-    deps = [":javabuilder-javacopts-lib"],
+    deps = [
+        ":javabuilder-javacopts-lib",
+        "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar:javac_options",
+        "//third_party:guava",
+    ],
 )
 
 # Provides programmatic access to a bazel compatible javac.
@@ -108,6 +112,7 @@ java_library(
         ":JavaBuilderConfig",
         ":JavaLangtools",
         ":JavacBootclasspath",
+        "//third_party:guava",
     ],
 )
 
diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/BazelJavaCompiler.java b/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/BazelJavaCompiler.java
index 6a4156ad9f..206d0b66e1 100644
--- a/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/BazelJavaCompiler.java
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/BazelJavaCompiler.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.java.bazel;
 
+import com.google.common.collect.ImmutableList;
 import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
@@ -23,8 +24,7 @@ import java.net.MalformedURLException;
 import java.net.URL;
 import java.net.URLClassLoader;
 import java.nio.charset.Charset;
-import java.util.ArrayList;
-import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 import java.util.Locale;
 import java.util.Set;
@@ -49,27 +49,18 @@ import javax.tools.StandardLocation;
 public class BazelJavaCompiler {
 
   // The default blessed javac options.
-  
-  private static final String DEFAULT_BOOTCLASSPATH = JavacBootclasspath.asString();
 
-  private static final String[] DEFAULT_JAVACOPTS;
-  static {
-    List<String> defaultJavacopts = new ArrayList<>();
-    for (String javacopt : JavaBuilderConfig.defaultJavacOpts()) {
-      if (javacopt.startsWith("-Xep")) {
-        // ignore Error Prone-specific flags accepted by JavaBuilder
-        continue;
-      }
-      defaultJavacopts.add(javacopt);
-    }
+  private static final String DEFAULT_BOOTCLASSPATH = JavacBootclasspath.asString();
 
-    // The bootclasspath must be specified both via an invocation option and
-    // via fileManager.setLocation(PLATFORM_CLASS_PATH), to work around what
-    // appears to be a bug in jdk[6,8] javac.
-    defaultJavacopts.addAll(Arrays.asList("-bootclasspath", DEFAULT_BOOTCLASSPATH));
-    
-    DEFAULT_JAVACOPTS = defaultJavacopts.toArray(new String[defaultJavacopts.size()]);
-  }
+  private static final ImmutableList<String> DEFAULT_JAVACOPTS =
+      ImmutableList.<String>builder()
+          .addAll(JavaBuilderConfig.defaultJavacOpts())
+          // The bootclasspath must be specified both via an invocation option and
+          // via fileManager.setLocation(PLATFORM_CLASS_PATH), to work around what
+          // appears to be a bug in jdk[6,8] javac.
+          .add("-bootclasspath")
+          .add(DEFAULT_BOOTCLASSPATH)
+          .build();
 
   private static final Class<? extends JavaCompiler> JAVA_COMPILER_CLASS = getJavaCompilerClass();
 
@@ -109,12 +100,10 @@ public class BazelJavaCompiler {
   public static File getLangtoolsJar() {
     return JavaLangtools.file();
   }
-  
-  /**
-   * Returns the default javacopts, including the blessed bootclasspath.
-   */
+
+  /** Returns the default javacopts, including the blessed bootclasspath. */
   public static List<String> getDefaultJavacopts() {
-    return new ArrayList<>(Arrays.asList(DEFAULT_JAVACOPTS));
+    return DEFAULT_JAVACOPTS;
   }
 
   /**
@@ -186,70 +175,62 @@ public class BazelJavaCompiler {
   private static JavaCompiler newInstance(final JavaCompiler delegate) {
     // We forward most operations to the JavaCompiler implementation in langtools.jar.
     return new JavaCompiler() {
-        @Override
-        public CompilationTask getTask(
-            Writer out,
-            JavaFileManager fileManager,
-            DiagnosticListener<? super JavaFileObject> diagnosticListener,
-            Iterable<String> options,
-            Iterable<String> classes,
-            Iterable<? extends JavaFileObject> compilationUnits) {
-          // We prepend bazel's default javacopts to user javacopts,
-          // so that the user can override them. javac supports this
-          // "last option wins" style of option override.
-          List<String> fullOptions = getDefaultJavacopts();
-          if (options != null) {
-            for (String option : options) {
-              fullOptions.add(option);
-            }
-          }
-          return delegate.getTask(out,
-                                  fileManager,
-                                  diagnosticListener,
-                                  fullOptions,
-                                  classes,
-                                  compilationUnits);
+      @Override
+      public CompilationTask getTask(
+          Writer out,
+          JavaFileManager fileManager,
+          DiagnosticListener<? super JavaFileObject> diagnosticListener,
+          Iterable<String> options,
+          Iterable<String> classes,
+          Iterable<? extends JavaFileObject> compilationUnits) {
+        // We prepend bazel's default javacopts to user javacopts,
+        // so that the user can override them. javac supports this
+        // "last option wins" style of option override.
+        ImmutableList.Builder<String> fullOptions = ImmutableList.builder();
+        fullOptions.addAll(getDefaultJavacopts());
+        if (options != null) {
+          fullOptions.addAll(options);
         }
+        return delegate.getTask(
+            out, fileManager, diagnosticListener, fullOptions.build(), classes, compilationUnits);
+      }
 
-        @Override
-        public StandardJavaFileManager getStandardFileManager(
-            DiagnosticListener<? super JavaFileObject> diagnosticListener,
-            Locale locale,
-            Charset charset) {
-          StandardJavaFileManager fileManager = delegate.getStandardFileManager(
-              diagnosticListener,
-              locale,
-              charset);
-
-          try {
-            fileManager.setLocation(
-                StandardLocation.PLATFORM_CLASS_PATH,  // bootclasspath
-                JavacBootclasspath.asFiles());
-          } catch (IOException e) {
-            // Should never happen, according to javadocs for setLocation
-            throw new RuntimeException(e);
-          }
-          return fileManager;
+      @Override
+      public StandardJavaFileManager getStandardFileManager(
+          DiagnosticListener<? super JavaFileObject> diagnosticListener,
+          Locale locale,
+          Charset charset) {
+        StandardJavaFileManager fileManager =
+            delegate.getStandardFileManager(diagnosticListener, locale, charset);
+
+        try {
+          fileManager.setLocation(
+              StandardLocation.PLATFORM_CLASS_PATH, // bootclasspath
+              JavacBootclasspath.asFiles());
+        } catch (IOException e) {
+          // Should never happen, according to javadocs for setLocation
+          throw new RuntimeException(e);
         }
+        return fileManager;
+      }
 
-        @Override
-        public int run(InputStream in, OutputStream out, OutputStream err,
-                       String... arguments) {
-          // prepend bazel's default javacopts to user arguments
-          List<String> args = getDefaultJavacopts();
-          args.addAll(Arrays.asList(arguments));
-          return delegate.run(in, out, err, args.toArray(new String[0]));
-        }
+      @Override
+      public int run(InputStream in, OutputStream out, OutputStream err, String... arguments) {
+        // prepend bazel's default javacopts to user arguments
+        List<String> args = getDefaultJavacopts();
+        Collections.addAll(args, arguments);
+        return delegate.run(in, out, err, args.toArray(new String[0]));
+      }
 
-        @Override
-        public Set<SourceVersion> getSourceVersions() {
-          return delegate.getSourceVersions();
-        }
+      @Override
+      public Set<SourceVersion> getSourceVersions() {
+        return delegate.getSourceVersions();
+      }
 
-        @Override
-        public int isSupportedOption(String option) {
-          return delegate.isSupportedOption(option);
-        }
-      };
+      @Override
+      public int isSupportedOption(String option) {
+        return delegate.isSupportedOption(option);
+      }
+    };
   }
 }
diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/JavaBuilderConfig.java b/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/JavaBuilderConfig.java
index 511dfa584f..c86291dd0b 100644
--- a/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/JavaBuilderConfig.java
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/JavaBuilderConfig.java
@@ -14,18 +14,18 @@
 
 package com.google.devtools.build.java.bazel;
 
-import java.util.ArrayList;
-import java.util.Collections;
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.buildjar.javac.JavacOptions;
 import java.util.List;
 
-/**
- * Utility class to provide java-level access to the blessed javabuilder javacopts.
- */
+/** Utility class to provide Java-level access to the blessed JavaBuilder javacopts. */
 public class JavaBuilderConfig {
-  
+
+  private static final ImmutableList<String> JAVACOPTS =
+      ImmutableList.copyOf(
+          JavacOptions.removeBazelSpecificFlags(JavaBuilderJavacOpts.DEFAULT_JAVACOPTS));
+
   public static List<String> defaultJavacOpts() {
-    List<String> result = new ArrayList<>();
-    Collections.addAll(result, JavaBuilderJavacOpts.DEFAULT_JAVACOPTS);
-    return result;
+    return JAVACOPTS;
   }
 }
diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/BUILD b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/BUILD
index 9e3e6cbeea..9ab49b9ca7 100644
--- a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/BUILD
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/BUILD
@@ -14,6 +14,7 @@ java_library(
         ":javac_turbine_compiler",
         ":zip_output_file_manager",
         ":zip_util",
+        "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar:javac_options",
         "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins:dependency",
         "//src/java_tools/buildjar/java/com/google/devtools/build/java/turbine:turbine_options",
         "//src/java_tools/buildjar/java/com/google/devtools/build/java/turbine:turbine_options_parser",
diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbine.java b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbine.java
index b294bd2a3c..8959d3613b 100644
--- a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbine.java
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbine.java
@@ -15,10 +15,10 @@
 package com.google.devtools.build.java.turbine.javac;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.CharMatcher;
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.buildjar.javac.JavacOptions;
 import com.google.devtools.build.buildjar.javac.plugins.dependency.DependencyModule;
 import com.google.devtools.build.buildjar.javac.plugins.dependency.DependencyModule.StrictJavaDeps;
 import com.google.devtools.build.buildjar.javac.plugins.dependency.StrictJavaDepsPlugin;
@@ -119,7 +119,7 @@ public class JavacTurbine implements AutoCloseable {
 
     ImmutableList.Builder<String> argbuilder = ImmutableList.builder();
 
-    filterJavacopts(argbuilder, turbineOptions.javacOpts());
+    argbuilder.addAll(JavacOptions.removeBazelSpecificFlags(turbineOptions.javacOpts()));
 
     // Disable compilation of implicit source files.
     // This is insurance: the sourcepath is empty, so we don't expect implicit sources.
@@ -321,31 +321,6 @@ public class JavacTurbine implements AutoCloseable {
     return result.build();
   }
 
-  @VisibleForTesting
-  static void filterJavacopts(
-      ImmutableList.Builder<String> javacArgs, Iterable<String> defaultJavacopts) {
-    for (String opt : defaultJavacopts) {
-
-      // TODO(cushon): temporary hack until 4149f08bcc8bd1318d4021cf372ec89240ee3dbb is released
-      opt = CharMatcher.is('\'').trimFrom(opt);
-
-      if (isErrorProneFlag(opt)) {
-        // drop Error Prone's fake javacopts
-        continue;
-      }
-      javacArgs.add(opt);
-    }
-  }
-
-  /**
-   * Returns true for flags that are specific to Error Prone.
-   *
-   * <p>WARNING: keep in sync with ErrorProneOptions#isSupportedOption
-   */
-  static boolean isErrorProneFlag(String opt) {
-    return opt.startsWith("-Xep");
-  }
-
   /** Extra sources in srcjars to disk. */
   private static List<String> extractSourceJars(TurbineOptions turbineOptions, Path tmpdir)
       throws IOException {
diff --git a/src/java_tools/buildjar/javatests/com/google/devtools/build/java/turbine/javac/JavacTurbineTest.java b/src/java_tools/buildjar/javatests/com/google/devtools/build/java/turbine/javac/JavacTurbineTest.java
index 0f20f16112..fb5f6249de 100644
--- a/src/java_tools/buildjar/javatests/com/google/devtools/build/java/turbine/javac/JavacTurbineTest.java
+++ b/src/java_tools/buildjar/javatests/com/google/devtools/build/java/turbine/javac/JavacTurbineTest.java
@@ -350,29 +350,6 @@ public class JavacTurbineTest {
       };
 
   @Test
-  public void filterJavacopts() {
-    ImmutableList.Builder<String> output = ImmutableList.builder();
-    JavacTurbine.filterJavacopts(
-        output,
-        Arrays.asList(
-            "-g",
-            "-source",
-            "6",
-            "-target",
-            "6",
-            "-Xlint:all",
-            "-source",
-            "7",
-            "-target",
-            "7",
-            "-Xep:GuardedBy:ERROR"));
-    assertThat(output.build())
-        .containsExactly(
-            "-g", "-source", "6", "-target", "6", "-Xlint:all", "-source", "7", "-target", "7")
-        .inOrder();
-  }
-
-  @Test
   public void jdeps() throws Exception {
 
     Path libC = temp.newFile("libc.jar").toPath();