From 089aa9b128b87d41407432634ed51a2efa794d19 Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Thu, 5 Dec 2024 10:42:29 -0800 Subject: [PATCH] Fix a bug in the logic that prevents Error Prone from running on compilations with underlying errors PiperOrigin-RevId: 703169425 --- .../JavacErrorDescriptionListener.java | 23 +++---- .../ErrorProneCompilerIntegrationTest.java | 63 +++++++++++++++++++ 2 files changed, 71 insertions(+), 15 deletions(-) diff --git a/check_api/src/main/java/com/google/errorprone/JavacErrorDescriptionListener.java b/check_api/src/main/java/com/google/errorprone/JavacErrorDescriptionListener.java index 8cbbe8203b6..3838b02f987 100644 --- a/check_api/src/main/java/com/google/errorprone/JavacErrorDescriptionListener.java +++ b/check_api/src/main/java/com/google/errorprone/JavacErrorDescriptionListener.java @@ -19,9 +19,8 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.collect.ImmutableList.toImmutableList; -import com.google.common.base.Supplier; -import com.google.common.base.Suppliers; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.errorprone.fixes.AppliedFix; import com.google.errorprone.fixes.Fix; import com.google.errorprone.matchers.Description; @@ -33,7 +32,6 @@ import com.sun.tools.javac.util.Log; import java.io.IOException; import java.io.UncheckedIOException; -import java.util.EnumSet; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -60,17 +58,12 @@ public class JavacErrorDescriptionListener implements DescriptionListener { // The suffix for properties in src/main/resources/com/google/errorprone/errors.properties private static final String MESSAGE_BUNDLE_KEY = "error.prone"; - // DiagnosticFlag.MULTIPLE went away in JDK13, so we want to load it if it's available. - private static final Supplier> diagnosticFlags = - Suppliers.memoize( - () -> { - try { - return EnumSet.of(JCDiagnostic.DiagnosticFlag.valueOf("MULTIPLE")); - } catch (IllegalArgumentException iae) { - // JDK 13 and above - return EnumSet.noneOf(JCDiagnostic.DiagnosticFlag.class); - } - }); + // DiagnosticFlag.API ensures that errors are always reported, bypassing 'shouldReport' logic + // that filters out duplicate diagnostics at the same position, and ensures that + // ErrorProneAnalyzer can compare the counts of errors reported by Error Prone with the total + // number of errors reported. + private static final ImmutableSet DIAGNOSTIC_FLAGS = + ImmutableSet.of(JCDiagnostic.DiagnosticFlag.API); private JavacErrorDescriptionListener( Log log, @@ -125,7 +118,7 @@ public void onDescribed(Description description) { factory.create( type, /* lintCategory */ null, - diagnosticFlags.get(), + DIAGNOSTIC_FLAGS, log.currentSource(), pos, MESSAGE_BUNDLE_KEY, diff --git a/core/src/test/java/com/google/errorprone/ErrorProneCompilerIntegrationTest.java b/core/src/test/java/com/google/errorprone/ErrorProneCompilerIntegrationTest.java index c8783cf8352..ce50e38b3c3 100644 --- a/core/src/test/java/com/google/errorprone/ErrorProneCompilerIntegrationTest.java +++ b/core/src/test/java/com/google/errorprone/ErrorProneCompilerIntegrationTest.java @@ -16,6 +16,7 @@ package com.google.errorprone; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; @@ -25,6 +26,7 @@ import static com.google.errorprone.matchers.Description.NO_MATCH; import static com.google.errorprone.util.ASTHelpers.constValue; import static java.util.Locale.ENGLISH; +import static java.util.Objects.requireNonNull; import static org.junit.Assert.assertThrows; import static org.junit.Assume.assumeTrue; @@ -760,4 +762,65 @@ public void stopPolicy_init_xD() { compiler.compile(new String[] {"-XDshould-stop.ifError=INIT"}, ImmutableList.of())); assertThat(e).hasMessageThat().contains("--should-stop=ifError=INIT is not supported"); } + + @BugPattern(summary = "Checks that symbols are non-null", severity = ERROR) + public static class GetSymbolChecker extends BugChecker implements MethodInvocationTreeMatcher { + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + requireNonNull(ASTHelpers.getSymbol(tree)); + return NO_MATCH; + } + } + + @BugPattern(summary = "Duplicates CPSChecker", severity = ERROR) + public static class CPSChecker2 extends CPSChecker {} + + // This is a regression test for a crash if two Error Prone diagnostics are reported at the same + // position. javac has logic to filter duplicate diagnostics, which can result in counts of + // Error Prone and other diagnostics getting out of sync with the counters kept in + // ErrorProneAnalyzer, which are used to skip Error Prone checks on compilation units with + // underlying errors. If the checks run on compilations with underlying errors they may see + // null symbols. + @Test + public void processingError() { + compilerBuilder.report( + ScannerSupplier.fromBugCheckerClasses( + CPSChecker.class, CPSChecker2.class, GetSymbolChecker.class)); + compiler = compilerBuilder.build(); + Result exitCode = + compiler.compile( + new String[] {"--should-stop=ifError=FLOW", "-XDcompilePolicy=byfile"}, + ImmutableList.of( + forSourceLines( + "A.java", + """ + class A { + int f(int x) { + return x; + } + } + """), + forSourceLines( + "B.java", + """ + package test; + import java.util.HashSet; + import java.util.Set; + class B { + enum E { ONE } + E f(int s) { + return E.valueOf(s); + } + } + """)), + ImmutableList.of()); + assertWithMessage(outputStream.toString()).that(exitCode).isEqualTo(Result.ERROR); + ImmutableList diagnostics = + diagnosticHelper.getDiagnostics().stream() + .filter(d -> d.getKind().equals(Diagnostic.Kind.ERROR)) + .map(d -> d.getCode()) + .collect(toImmutableList()); + assertThat(diagnostics).doesNotContain("compiler.err.error.prone.crash"); + assertThat(diagnostics).hasSize(3); + } }