Don't run plugins on compilations with errors.

--
MOS_MIGRATED_REVID=99775681

2 files changed, 108 insertions, 27 deletions

diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/BlazeJavaCompiler.java b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/BlazeJavaCompiler.java
index 9494ce7f49..e362796ee7 100644
--- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/BlazeJavaCompiler.java
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/BlazeJavaCompiler.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.buildjar.javac;
 
+import com.google.common.base.Function;
 import com.google.devtools.build.buildjar.javac.plugins.BlazeJavaCompilerPlugin;
 
 import com.sun.tools.javac.comp.AttrContext;
@@ -26,21 +27,26 @@ import java.util.ArrayList;
 import java.util.List;
 import java.util.Queue;
 
+import javax.annotation.Nullable;
+
 /**
- * An extended version of the javac compiler, providing support for 
+ * An extended version of the javac compiler, providing support for
  * composable static analyses via a plugin mechanism. BlazeJavaCompiler
  * keeps a list of plugins and calls callback methods in those plugins
  * after certain compiler phases. The plugins perform the actual static
- * analyses. 
+ * analyses.
  */
 public class BlazeJavaCompiler extends JavaCompiler {
 
+  private int skippedFlowEvents = 0;
+  private int flowEvents = 0;
+
   /**
    * A list of plugins to run at particular points in the compile
    */
   private final List<BlazeJavaCompilerPlugin> plugins = new ArrayList<>();
 
-  public BlazeJavaCompiler(Context context, Iterable<BlazeJavaCompilerPlugin> plugins) {
+  private BlazeJavaCompiler(Context context, Iterable<BlazeJavaCompilerPlugin> plugins) {
     super(context);
 
     // initialize all plugins
@@ -51,28 +57,56 @@ public class BlazeJavaCompiler extends JavaCompiler {
   }
 
   /**
-   * Adds an initialization hook to the Context, such that each subsequent
-   * request for a JavaCompiler (i.e., a lookup for 'compilerKey' of our
-   * superclass, JavaCompiler) will actually construct and return our version
-   * of BlazeJavaCompiler. It's necessary since many new JavaCompilers may
-   * be requested for later stages of the compilation (annotation processing),
-   * within the same Context. And it's the preferred way for extending behavior
-   * within javac, per the documentation in {@link Context}.
+   * Adds an initialization hook to the Context, such that requests for a
+   * JavaCompiler (i.e., a lookup for 'compilerKey' of our superclass,
+   * JavaCompiler) will actually construct and return BlazeJavaCompiler.
+   *
+   * This is the preferred way for extending behavior within javac,
+   * per the documentation in {@link Context}.
+   *
+   * Prior to JDK-8038455 additional JavaCompilers were created for
+   * annotation processing rounds, but we now expect a single
+   * compiler instance per compilation. The factory is still seems
+   * to be necessary to avoid context-ordering issues, but we
+   * assert that the factory is only called once, and save the
+   * output after its call for introspection.
    */
-  public static void preRegister(final Context context,
-      final Iterable<BlazeJavaCompilerPlugin> plugins) {
-    context.put(compilerKey, new Context.Factory<JavaCompiler>() {
-      @Override
-      public JavaCompiler make(Context c) {
-        return new BlazeJavaCompiler(c, plugins);
-      }
-    });
+  public static void preRegister(
+      final Context context,
+      final Iterable<BlazeJavaCompilerPlugin> plugins,
+      @Nullable final Function<BlazeJavaCompiler, Void> listener) {
+    context.put(
+        compilerKey,
+        new Context.Factory<JavaCompiler>() {
+          boolean first = true;
+
+          @Override
+          public JavaCompiler make(Context c) {
+            if (!first) {
+              throw new AssertionError("Expected a single creation of BlazeJavaCompiler.");
+            }
+            first = false;
+            BlazeJavaCompiler compiler = new BlazeJavaCompiler(c, plugins);
+            if (listener != null) {
+              listener.apply(compiler);
+            }
+            return compiler;
+          }
+        });
+  }
+
+  public static void preRegister(
+      final Context context, final Iterable<BlazeJavaCompilerPlugin> plugins) {
+    preRegister(context, plugins, null);
   }
 
   @Override
   public Env<AttrContext> attribute(Env<AttrContext> env) {
     Env<AttrContext> result = super.attribute(env);
-
+    // don't run plugins if there were compilation errors
+    if (errorCount() > 0) {
+      return result;
+    }
     // Iterate over all plugins, calling their postAttribute methods
     for (BlazeJavaCompilerPlugin plugin : plugins) {
       plugin.postAttribute(result);
@@ -83,11 +117,18 @@ public class BlazeJavaCompiler extends JavaCompiler {
 
   @Override
   protected void flow(Env<AttrContext> env, Queue<Env<AttrContext>> results) {
-    if (compileStates.isDone(env, CompileState.FLOW)) {
-      super.flow(env, results);
+    boolean isDone = compileStates.isDone(env, CompileState.FLOW);
+    super.flow(env, results);
+    if (isDone) {
       return;
     }
-    super.flow(env, results);
+    // don't run plugins if there were compilation errors
+    if (errorCount() > 0) {
+      skippedFlowEvents++;
+      return;
+    }
+    flowEvents++;
+
     // Iterate over all plugins, calling their postFlow methods
     for (BlazeJavaCompilerPlugin plugin : plugins) {
       plugin.postFlow(env);
@@ -103,6 +144,16 @@ public class BlazeJavaCompiler extends JavaCompiler {
     super.close();
   }
 
+  /** The number of flow events that were skipped. */
+  public int skippedFlowEvents() {
+    return skippedFlowEvents;
+  }
+
+  /** The number of error-free flow events that were passed to plugins. */
+  public int flowEvents() {
+    return flowEvents;
+  }
+
   /**
    * Testing purposes only.  Returns true if the collection of plugins in
    * this instance contains one of the provided type.
diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/BlazeJavacMain.java b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/BlazeJavacMain.java
index 057c145a74..2ac756670f 100644
--- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/BlazeJavacMain.java
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/BlazeJavacMain.java
@@ -14,7 +14,12 @@
 
 package com.google.devtools.build.buildjar.javac;
 
+import static com.google.common.base.Verify.verifyNotNull;
+
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Function;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Verify;
 import com.google.devtools.build.buildjar.InvalidCommandLineException;
 import com.google.devtools.build.buildjar.javac.plugins.BlazeJavaCompilerPlugin;
 import com.google.devtools.build.buildjar.javac.plugins.BlazeJavaCompilerPlugin.PluginException;
@@ -59,6 +64,7 @@ public class BlazeJavacMain {
   private List<BlazeJavaCompilerPlugin> plugins;
   private final PrintWriter errOutput;
   private final String compilerName;
+  private BlazeJavaCompiler compiler = null;
 
   public BlazeJavacMain(PrintWriter errOutput, List<BlazeJavaCompilerPlugin> plugins) {
     this.compilerName = "blaze javac";
@@ -76,13 +82,22 @@ public class BlazeJavacMain {
     preRegister(context, plugins);
   }
 
+  private final Function<BlazeJavaCompiler, Void> compilerListener =
+      new Function<BlazeJavaCompiler, Void>() {
+        @Override
+        public Void apply(BlazeJavaCompiler compiler) {
+          Verify.verify(BlazeJavacMain.this.compiler == null);
+          BlazeJavacMain.this.compiler = Preconditions.checkNotNull(compiler);
+          return null;
+        }
+      };
+
   public void preRegister(Context context, List<BlazeJavaCompilerPlugin> plugins) {
     this.plugins = plugins;
     for (BlazeJavaCompilerPlugin plugin : plugins) {
       plugin.initializeContext(context);
     }
-
-    BlazeJavaCompiler.preRegister(context, plugins);
+    BlazeJavaCompiler.preRegister(context, plugins, compilerListener);
   }
 
   public Result compile(String[] argv) {
@@ -121,8 +136,9 @@ public class BlazeJavacMain {
   @VisibleForTesting
   public Result compile(String[] argv, Context context) {
     enableEndPositions(context);
+    Result result = Result.ABNORMAL;
     try {
-      return new Main(compilerName, errOutput).compile(argv, context);
+      result = new Main(compilerName, errOutput).compile(argv, context);
     } catch (PropagatedException e) {
       if (e.getCause() instanceof PluginException) {
         PluginException pluginException = (PluginException) e.getCause();
@@ -130,8 +146,17 @@ public class BlazeJavacMain {
         return pluginException.getResult();
       }
       e.printStackTrace(errOutput);
-      return Result.ABNORMAL;
+      result = Result.ABNORMAL;
+    } finally {
+      if (result.isOK()) {
+        verifyNotNull(compiler);
+        if (compiler.flowEvents() == 0) {
+          errOutput.println("Expected at least one FLOW event");
+          result = Result.ABNORMAL;
+        }
+      }
     }
+    return result;
   }
 
   // javac9 removes the ability to pass lists of {@link JavaFileObject}s or {@link Processors}s to
@@ -140,7 +165,7 @@ public class BlazeJavacMain {
   // behaviour closer to stock javac, but it makes it harder to write integration tests. This class
   // provides a compile method that accepts file objects and processors, but it isn't
   // guaranteed to behave exactly the same way as JavaBuilder does when used from the command-line.
-  // TODO(bazel-team): either stop using Main and commit to using the the API for everything, or
+  // TODO(cushon): either stop using Main and commit to using the the API for everything, or
   // re-write integration tests for JavaBuilder to use the real compile() method.
   @VisibleForTesting
   @Deprecated
@@ -197,4 +222,9 @@ public class BlazeJavacMain {
     }
     return processedArgs.toArray(new String[processedArgs.size()]);
   }
+
+  @VisibleForTesting
+  BlazeJavaCompiler getCompiler() {
+    return verifyNotNull(compiler);
+  }
 }