From 77d732ee2579399abc2508f901df22bf88ac3168 Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Tue, 15 Oct 2024 07:25:45 -0700 Subject: [PATCH] Check that `-XDshould-stop.ifError=FLOW` is set when running Error Prone This is necessary to ensure javac compiles the 'flow' phase for subsequent compilation units after Error Prone reports diagnostics. The 'flow' phase is necessary among other things to set `EFFECTIVELY_FINAL`. Configuring a later stop policy also causes more diagnostics to be shown at once for failing builds when using `-XDcompilePolicy=simple`, which is helpful. https://github.com/google/error-prone/issues/4595 RELNOTES=The javac flag `-XDshould-stop.ifError=FLOW` is now required when running Error Prone, see #4595 PiperOrigin-RevId: 686096407 --- .../BaseErrorProneJavaCompiler.java | 25 ++++++ .../ErrorProneCompilerIntegrationTest.java | 81 +++++++++++++++++++ 2 files changed, 106 insertions(+) diff --git a/check_api/src/main/java/com/google/errorprone/BaseErrorProneJavaCompiler.java b/check_api/src/main/java/com/google/errorprone/BaseErrorProneJavaCompiler.java index 5c90bdc00016..6778d2d41909 100644 --- a/check_api/src/main/java/com/google/errorprone/BaseErrorProneJavaCompiler.java +++ b/check_api/src/main/java/com/google/errorprone/BaseErrorProneJavaCompiler.java @@ -23,6 +23,7 @@ import com.sun.source.util.JavacTask; import com.sun.tools.javac.api.BasicJavacTask; import com.sun.tools.javac.api.JavacTool; +import com.sun.tools.javac.comp.CompileStates.CompileState; import com.sun.tools.javac.util.Context; import com.sun.tools.javac.util.JavacMessages; import com.sun.tools.javac.util.Options; @@ -70,6 +71,7 @@ public CompilationTask getTask( ImmutableList javacOpts = errorProneOptions.getRemainingArgs(); javacOpts = defaultToLatestSupportedLanguageLevel(javacOpts); javacOpts = setCompilePolicyToByFile(javacOpts); + javacOpts = setShouldStopIfErrorPolicyToFlow(javacOpts); JavacTask task = (JavacTask) javacTool.getTask( @@ -194,6 +196,29 @@ private static ImmutableList setCompilePolicyToByFile(ImmutableListbuilder().addAll(args).add("-XDcompilePolicy=simple").build(); } + private static void checkShouldStopIfErrorPolicy(String value) { + CompileState state = CompileState.valueOf(value); + if (CompileState.FLOW.isAfter(state)) { + throw new InvalidCommandLineOptionException( + String.format( + "-XDshould-stop.ifError=%s is not supported by Error Prone, pass" + + " -XDshould-stop.ifError=FLOW instead", + state)); + } + } + + private static ImmutableList setShouldStopIfErrorPolicyToFlow( + ImmutableList args) { + for (String arg : args) { + if (arg.startsWith("-XDshould-stop.ifError")) { + String value = arg.substring(arg.indexOf('=') + 1); + checkShouldStopIfErrorPolicy(value); + return args; // don't do anything if a valid policy is already set + } + } + return ImmutableList.builder().addAll(args).add("-XDshould-stop.ifError=FLOW").build(); + } + /** Registers our message bundle. */ public static void setupMessageBundle(Context context) { ResourceBundle bundle = ResourceBundle.getBundle("com.google.errorprone.errors"); diff --git a/core/src/test/java/com/google/errorprone/ErrorProneCompilerIntegrationTest.java b/core/src/test/java/com/google/errorprone/ErrorProneCompilerIntegrationTest.java index 824104ffbc6f..ef52380e3be2 100644 --- a/core/src/test/java/com/google/errorprone/ErrorProneCompilerIntegrationTest.java +++ b/core/src/test/java/com/google/errorprone/ErrorProneCompilerIntegrationTest.java @@ -39,10 +39,12 @@ import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.ReturnTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; import com.google.errorprone.bugpatterns.NonAtomicVolatileUpdate; import com.google.errorprone.matchers.Description; import com.google.errorprone.scanner.BuiltInCheckerSuppliers; import com.google.errorprone.scanner.ScannerSupplier; +import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ExpressionStatementTree; import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.MemberSelectTree; @@ -50,6 +52,7 @@ import com.sun.source.tree.MethodTree; import com.sun.source.tree.ReturnTree; import com.sun.source.tree.Tree; +import com.sun.source.tree.VariableTree; import com.sun.tools.javac.main.Main.Result; import java.io.PrintWriter; import java.io.StringWriter; @@ -669,4 +672,82 @@ public class Test { outputStream.flush(); assertWithMessage(outputStream.toString()).that(exitCode).isEqualTo(Result.OK); } + + @BugPattern(summary = "All variables should be effectively final", severity = ERROR) + public static class EffectivelyFinalChecker extends BugChecker implements VariableTreeMatcher { + @Override + public Description matchVariable(VariableTree tree, VisitorState state) { + if (ASTHelpers.isConsideredFinal(ASTHelpers.getSymbol(tree))) { + return NO_MATCH; + } + return describeMatch(tree); + } + } + + @Test + public void stopPolicy_effectivelyFinal() { + compilerBuilder.report( + ScannerSupplier.fromBugCheckerClasses(EffectivelyFinalChecker.class, CPSChecker.class)); + compiler = compilerBuilder.build(); + // Without -XDshould-stop.ifError=FLOW, the errors reported by CPSChecker will cause javac to + // stop processing B after an error is reported in A. Error Prone will still analyze B without + // it having gone through 'flow', and the EFFECTIVELY_FINAL analysis will not have happened. + // see https://github.com/google/error-prone/issues/4595 + Result exitCode = + compiler.compile( + ImmutableList.of( + forSourceLines( + "A.java", + """ + class A { + int f(int x) { + return x; + } + } + """), + forSourceLines( + "B.java", + """ + class B { + int f(int x) { + return x; + } + } + """))); + + outputStream.flush(); + assertWithMessage(outputStream.toString()).that(exitCode).isEqualTo(Result.ERROR); + + assertThat(diagnosticHelper.getDiagnostics()).hasSize(2); + assertWithMessage("Error should be found. " + diagnosticHelper.describe()) + .that(diagnosticHelper.getDiagnostics()) + .comparingElementsUsing(DIAGNOSTIC_CONTAINING) + .containsExactly("[CPSChecker]", "[CPSChecker]"); + } + + @Test + public void stopPolicy_flow() { + Result exitCode = + compiler.compile( + new String[] {"-XDshould-stop.ifError=FLOW"}, + ImmutableList.of( + forSourceLines( + "Test.java", + """ + package test; + class Test {} + """))); + outputStream.flush(); + assertWithMessage(outputStream.toString()).that(exitCode).isEqualTo(Result.OK); + } + + @Test + public void stopPolicy_init() { + InvalidCommandLineOptionException e = + assertThrows( + InvalidCommandLineOptionException.class, + () -> + compiler.compile(new String[] {"-XDshould-stop.ifError=INIT"}, ImmutableList.of())); + assertThat(e).hasMessageThat().contains("-XDshould-stop.ifError=INIT is not supported"); + } }