Add the preferKeepingExistingSuppression extension property to shunt removeUnused mode for non-standard checks

gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorProneExtension.java

@@ -31,12 +31,27 @@ public abstract class SuppressibleErrorProneExtension {
 
     public SuppressibleErrorProneExtension(Project project) {
         this.project = project;
+
+        getPreferKeepingExistingSuppression()
+                .convention(Set.of(
+                        // NullAway does not respect -XepIgnoreSuppressionAnnotations, and it will take some work to
+                        // make it so. In the meantime, let's not touch any NullAway suppressions
+                        "NullAway",
+                        "CheckNullabilityTypes",
+                        // rawtypes is used by both the compiler and error-prone, let's not remove these
+                        "rawtypes",
+                        "RawTypes",
+                        // The autofix in SafeLoggingPropagation (adding an @Unsafe or @DoNotLog annotation) isn't
+                        // always desirable — there are legitimate reasons to suppress it.
+                        "SafeLoggingPropagation"));
     }
 
     public abstract SetProperty<String> getPatchChecks();
 
     public abstract ListProperty<ConditionalPatchCheck> getConditionalPatchChecks();
 
+    public abstract SetProperty<String> getPreferKeepingExistingSuppression();
+
     public final Set<String> patchChecksForCompilation(JavaCompile javaCompile) {
         return Stream.concat(
                         getPatchChecks().get().stream(),

gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/Modes.java

@@ -129,7 +129,17 @@ public final CommonOptions commonOptionsFor(JavaCompile javaCompile) {
                 .append(nonInterferingCommonOptions)
                 .toSet();
 
-        return CommonOptions.naivelyCombine(allCommonOptions);
+        CommonOptions combined = CommonOptions.naivelyCombine(allCommonOptions);
+
+        return combined.withExtraErrorProneCheckFlag(
+                "SuppressibleErrorProne:PreferKeepingExistingSuppression", () -> javaCompile
+                        .getProject()
+                        .getExtensions()
+                        .getByType(com.palantir.gradle.suppressibleerrorprone.SuppressibleErrorProneExtension.class)
+                        .getPreferKeepingExistingSuppression()
+                        .get()
+                        .stream()
+                        .collect(Collectors.joining(",")));
     }
 
     private Map<ModeName, Optional<String>> modesEnabledAndFlagValues() {

gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/CommonOptions.java

@@ -20,8 +20,10 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Set;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 import one.util.streamex.EntryStream;
 
 /**
@@ -46,6 +48,10 @@ default boolean ignoreSuppressionAnnotations() {
         return false;
     }
 
+    default Set<String> preferKeepingExistingSuppression() {
+        return Set.of();
+    }
+
     default CommonOptions naivelyCombinedWith(CommonOptions other) {
         return new CommonOptions() {
             @Override
@@ -70,6 +76,15 @@ public RemoveRolloutCheck removeRolloutCheck() {
             public boolean ignoreSuppressionAnnotations() {
                 return CommonOptions.this.ignoreSuppressionAnnotations() || other.ignoreSuppressionAnnotations();
             }
+
+            @Override
+            public Set<String> preferKeepingExistingSuppression() {
+                // Union the two sets when combining
+                return Stream.concat(
+                                CommonOptions.this.preferKeepingExistingSuppression().stream(),
+                                other.preferKeepingExistingSuppression().stream())
+                        .collect(Collectors.toSet());
+            }
         };
     }
 
@@ -100,6 +115,11 @@ public RemoveRolloutCheck removeRolloutCheck() {
             public boolean ignoreSuppressionAnnotations() {
                 return CommonOptions.this.ignoreSuppressionAnnotations();
             }
+
+            @Override
+            public Set<String> preferKeepingExistingSuppression() {
+                return CommonOptions.this.preferKeepingExistingSuppression();
+            }
         };
     }
 

gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy

@@ -1706,6 +1706,59 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec
         '''.stripIndent(true)
     }
 
+    def 'errorProneRemoveUnused leaves preferKeepingExistingSuppression checks untouched'() {
+        // language=Java
+        def initialSource = '''
+            package app;
+
+            @SuppressWarnings({"NullAway", "for-rollout:rawtypes"})
+            public final class App {}
+        '''.stripIndent(true)
+        writeJavaSourceFileToSourceSets initialSource
+        when:
+        runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused')
+        then:
+        // language=Java
+        javaSourceIsSyntacticallyEqualTo initialSource
+    }
+
+    def 'errorProneRemoveUnused + errorProneApply respects preferKeepingExistingSuppression'() {
+        given:
+        // language=Gradle
+        buildFile << '''
+            suppressibleErrorProne {
+                preferKeepingExistingSuppression = ["ShouldBePrivate"] as Set
+            }
+        '''.stripIndent(true)
+
+        writeJavaSourceFileToSourceSets '''
+            package app;
+
+            public final class App {
+                @SuppressWarnings("ShouldBePrivate")
+                void fixme(String s) {}
+
+                void fixme(Integer i) {}
+            }
+        '''.stripIndent(true)
+
+        when: 'ShouldBePrivate is in preferKeepingExistingSuppression'
+        runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused', '-PerrorProneApply=ShouldBePrivate')
+
+        then: 'removeUnused + apply does not fix suppressed node'
+        // language=Java
+        javaSourceIsSyntacticallyEqualTo '''
+            package app;
+
+            public final class App {
+                @SuppressWarnings("ShouldBePrivate")
+                void fixme(String s) {}
+
+                private void fixme(Integer i) {}
+            }
+        '''.stripIndent(true)
+    }
+
     def 'errorProneRemoveUnused only keeps the closest suppression to a violation'() {
         // language=Java
         writeJavaSourceFileToSourceSets '''

suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressionRemover.java

@@ -30,9 +30,13 @@ public final class SuppressionRemover {
     // has finished processing.
     private static final Set<CompilationUnitTree> attachedFixes = Collections.newSetFromMap(new WeakHashMap<>());
 
-    public static void removeAllSuppressionsOnErrorprones(
+    public static void removeAllKnownBugcheckerSuppressions(
             ReportedFixCache reportedFixes, CompilationUnitTree unit, VisitorState state) {
         if (attachedFixes.add(unit)) {
+            Set<String> preferKeepingExistingSuppression = state.errorProneOptions()
+                    .getFlags()
+                    .getSetOrEmpty("SuppressibleErrorProne:PreferKeepingExistingSuppression");
+
             new TreePathScanner<Void, Void>() {
                 @SuppressWarnings("for-rollout:VoidUsed")
                 @Override
@@ -41,8 +45,12 @@ public Void visitAnnotation(AnnotationTree node, Void unused) {
                         TreePath declaration = getCurrentPath().getParentPath().getParentPath();
 
                         reportedFixes.getOrReportNew(
-                                declaration, state, suppression -> !AllErrorprones.allBugcheckerNames(state)
-                                        .contains(suppression));
+                                declaration,
+                                state,
+                                suppression -> !AllErrorprones.allBugcheckerNames(state)
+                                                .contains(SuppressWarningsUtils.stripForRollout(suppression))
+                                        || preferKeepingExistingSuppression.contains(
+                                                SuppressWarningsUtils.stripForRollout(suppression)));
                     }
 
                     return super.visitAnnotation(node, unused);

suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java

@@ -29,7 +29,6 @@
 import java.util.Set;
 import java.util.logging.Logger;
 import java.util.stream.Stream;
-import one.util.streamex.MoreCollectors;
 
 // CHECKSTYLE:ON
 
@@ -52,7 +51,7 @@ public static Description interceptDescription(VisitorState visitorState, Descri
         if (modes.contains("RemoveUnused")) {
             // Start by removing all suppressions on error-prones, including rollout and human-made.
             // Then, as we encounter Descriptions without fixes, we add back the closest suppression
-            SuppressionRemover.removeAllSuppressionsOnErrorprones(FIXES, compilationUnit, visitorState);
+            SuppressionRemover.removeAllKnownBugcheckerSuppressions(FIXES, compilationUnit, visitorState);
         }
 
         // If both -PerrorProneSuppress and -PerrorProneApply are used at the same time, for the checks configured as
@@ -67,7 +66,28 @@ public static Description interceptDescription(VisitorState visitorState, Descri
         boolean checkHasSuggestedFixes = !description.fixes.isEmpty();
 
         if (shouldPreferDefaultSuggestedFixesForThisCheck && checkHasSuggestedFixes) {
-            return description;
+            // If this check is in preferKeepingExistingSuppression AND there's an existing suppression
+            // that will be kept, we should NOT apply the fix because the suppression will remain
+            Set<String> preferKeepingExistingSuppression = visitorState
+                    .errorProneOptions()
+                    .getFlags()
+                    .getSetOrEmpty("SuppressibleErrorProne:PreferKeepingExistingSuppression");
+
+            TreePath pathToActualError =
+                    TreePath.getPath(visitorState.getPath().getCompilationUnit(), description.position.getTree());
+
+            boolean hasExistingSuppressionThatWillBeKept =
+                    preferKeepingExistingSuppression.contains(description.checkName)
+                            && Stream.iterate(
+                                            pathToActualError,
+                                            treePath -> treePath.getParentPath() != null,
+                                            TreePath::getParentPath)
+                                    .anyMatch(path -> suppresses(path, description, visitorState));
+
+            if (!hasExistingSuppressionThatWillBeKept) {
+                return description;
+            }
+            // Fall through to treat it like a regular check (either suppress or return NO_MATCH)
         }
 
         // If the check is a suggestion, we don't want to auto-suppress it, so we return no match (such that it also
@@ -91,17 +111,11 @@ public static Description interceptDescription(VisitorState visitorState, Descri
         if (firstSuppressibleWhichSuppressesDescription.isPresent() && modes.contains("RemoveUnused")) {
             // In RemoveUnused mode, removeAllSuppressionsOnErrorprones guarantees that a fix must already exist on
             // this suppressible.
-            TreePath declaration = firstSuppressibleWhichSuppressesDescription.get();
-            Set<String> allNames =
-                    AllErrorprones.allNames(visitorState, description.checkName).get();
-            // Use the existing suppression, rather than changing it to the canonical suppression
-            String existingSuppression = AnnotationUtils.annotationStringValues(
-                            SuppressWarningsUtils.getSuppressWarnings(declaration)
-                                    .get())
-                    .filter(suppression -> allNames.contains(SuppressWarningsUtils.stripForRollout(suppression)))
-                    .collect(MoreCollectors.first())
-                    .get();
-            FIXES.getExisting(declaration).addSuppression(existingSuppression);
+            TreePath firstSuppressible = firstSuppressibleWhichSuppressesDescription.get();
+            String existingSuppression =
+                    suppressionsOn(firstSuppressible, description, visitorState).get(0);
+            FIXES.getOrReportNew(firstSuppressible, visitorState, suppression -> true)
+                    .addSuppression(existingSuppression);
             return Description.NO_MATCH;
         }