From 5828416d06cd4247b2dc575a593621f07a35f0b5 Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Tue, 15 Oct 2024 07:48:38 -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: 686102738 --- .../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 5c90bdc0001..6778d2d4190 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 824104ffbc6..ef52380e3be 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"); + } }