From e927532a03addbcdd1e716b8453b53d9c3eb98e1 Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Mon, 3 Aug 2015 23:57:23 +0000 Subject: Don't run plugins on compilations with errors. -- MOS_MIGRATED_REVID=99775681 --- .../build/buildjar/javac/BlazeJavaCompiler.java | 95 +++++++++++++++++----- .../build/buildjar/javac/BlazeJavacMain.java | 40 +++++++-- 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 plugins = new ArrayList<>(); - public BlazeJavaCompiler(Context context, Iterable plugins) { + private BlazeJavaCompiler(Context context, Iterable 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 plugins) { - context.put(compilerKey, new Context.Factory() { - @Override - public JavaCompiler make(Context c) { - return new BlazeJavaCompiler(c, plugins); - } - }); + public static void preRegister( + final Context context, + final Iterable plugins, + @Nullable final Function listener) { + context.put( + compilerKey, + new Context.Factory() { + 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 plugins) { + preRegister(context, plugins, null); } @Override public Env attribute(Env env) { Env 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 env, Queue> 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 plugins; private final PrintWriter errOutput; private final String compilerName; + private BlazeJavaCompiler compiler = null; public BlazeJavacMain(PrintWriter errOutput, List plugins) { this.compilerName = "blaze javac"; @@ -76,13 +82,22 @@ public class BlazeJavacMain { preRegister(context, plugins); } + private final Function compilerListener = + new Function() { + @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 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); + } } -- cgit v1.2.3