From 27f96dd1b3bfed561f7ba949847097ef1123bd04 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 22 Oct 2025 09:57:46 +0100 Subject: [PATCH 1/4] Chonky boi --- ...ibleErrorPronePluginIntegrationTest.groovy | 350 +++++++++++++++++- .../ReportedFixCache.java | 16 + .../VisitorStateModifications.java | 78 +++- 3 files changed, 435 insertions(+), 9 deletions(-) diff --git a/gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy b/gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy index 42ffc553..9f61b424 100644 --- a/gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy +++ b/gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy @@ -92,7 +92,6 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec errorprone 'com.palantir.suppressible-error-prone:test-error-prone-checks:' + project.findProperty("suppressibleErrorProneVersion") } - suppressibleErrorProne { configureEachErrorProneOptions { // These interfere with some tests, so disable them @@ -1611,6 +1610,355 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec !output.contains('[RemoveRolloutSuppressions]') } + def 'errorProneRemoveUnused removes only unused error-prone suppressions, and leaves unknown suppressions untouched'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + + @SuppressWarnings({"ArrayToString", "UnnecessaryFinal", "InlineTrivialConstant", "NotAnErrorProne", "checkstyle:Bla",}) + public final class App { + private static final String EMPTY_STRING = ""; + + public static void main(String[] args) { + new int[3].toString(); + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused=ArrayToString') + + then: + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + @SuppressWarnings({"ArrayToString", "InlineTrivialConstant", "NotAnErrorProne", "checkstyle:Bla"}) + public final class App { + private static final String EMPTY_STRING = ""; + + public static void main(String[] args) { + new int[3].toString(); + } + } + '''.stripIndent(true) + } + + def 'errorProneRemoveUnused only keeps the closest suppression to a violation'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + + @SuppressWarnings("InlineTrivialConstant") + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + @SuppressWarnings("InlineTrivialConstant") + class Inner { + @SuppressWarnings("InlineTrivialConstant") + class InnerInner { + @SuppressWarnings("InlineTrivialConstant") + class InnerInnerInner { + private static final String EMPTY = ""; + } + } + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused') + + then: + + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + class Inner { + class InnerInner { + @SuppressWarnings("InlineTrivialConstant") + class InnerInnerInner { + private static final String EMPTY = ""; + } + } + } + } + '''.stripIndent(true) + } + + def 'errorProneRemoveUnused handles multiple suppressions on different tree types gracefully'() { + // Here we test the three types of trees you can suppress — ClassTree, MethodTree, VariableTree + + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + // Doesn't move an already existing suppression, even if it could be closer to the violation + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + boolean truism = new int[3].equals(new int[3]); + + @SuppressWarnings("InlineTrivialConstant") + static class InnerInner { + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused') + + then: + + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + // Doesn't move an already existing suppression, even if it could be closer to the violation + @SuppressWarnings("ArrayEquals") + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + + boolean truism = new int[3].equals(new int[3]); + + static class InnerInner { + @SuppressWarnings("ArrayEquals") + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + } + + def 'errorProneRemoveUnused removes entire SuppressWarnings annotation when all suppressions are unused'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + + @SuppressWarnings({"UnusedVariable", "ArrayToString"}) + public final class App { + public static void main(String[] args) { + System.out.println("No violations here"); + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused') + + then: + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + public final class App { + public static void main(String[] args) { + System.out.println("No violations here"); + } + } + '''.stripIndent(true) + } + + def 'errorProneRemoveUnused and errorProneSuppress uses existing suppressions if possible'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + + @SuppressWarnings("InlineTrivialConstant") + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + @SuppressWarnings("InlineTrivialConstant") + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + boolean truism = new int[3].equals(new int[3]); + + @SuppressWarnings("InlineTrivialConstant") + static class InnerInner { + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused', '-PerrorProneSuppress') + + then: + + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + + @SuppressWarnings("for-rollout:ArrayEquals") + boolean truism = new int[3].equals(new int[3]); + + static class InnerInner { + @SuppressWarnings("ArrayEquals") + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + } + + def 'errorProneRemoveUnused and errorProneApply applies fixes on previously suppressed elements'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + boolean truism = new int[3].equals(new int[3]); + + @SuppressWarnings("InlineTrivialConstant") + static class InnerInner { + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused', '-PerrorProneApply=ArrayEquals') + + then: + + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + import java.util.Arrays; + + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + + boolean truism = Arrays.equals(new int[3], new int[3]); + + static class InnerInner { + void method() { + Arrays.equals(new int[3], new int[3]); + } + } + } + } + '''.stripIndent(true) + } + + def 'errorProneRemoveUnused + errorProneApply + errorProneSuppress applies fixes and suppressions on previously suppressed elements'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + + @SuppressWarnings("ArrayEquals") + public final class App { + private static final String EMPTY_STRING = ""; + + // Although InlineTrivialConstant can be placed lower in the AST hierarchy, + // we preserve existing suppressions whenever possible rather than move suppressions around. + // Also, note that we don't add for-rollout here. + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + static class Inner { + private static final String EMPTY = ""; + boolean truism = new int[3].equals(new int[3]); + + @SuppressWarnings("InlineTrivialConstant") + static class InnerInner { + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused', '-PerrorProneSuppress', '-PerrorProneApply=ArrayEquals') + + then: + + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + import java.util.Arrays; + + public final class App { + @SuppressWarnings("for-rollout:InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + // Although InlineTrivialConstant can be placed lower in the AST hierarchy, + // we preserve existing suppressions whenever possible rather than move suppressions around. + // Also, note that we don't add for-rollout here. + @SuppressWarnings("InlineTrivialConstant") + static class Inner { + private static final String EMPTY = ""; + boolean truism = Arrays.equals(new int[3], new int[3]); + + static class InnerInner { + void method() { + Arrays.equals(new int[3], new int[3]); + } + } + } + } + '''.stripIndent(true) + } + def 'error-prone dependencies have versions bound together by a virtual platform'() { setup: 'when an error-prone dependency is forced to certain version' // language=Gradle diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java index 754672bb..cd2cb19f 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java @@ -40,6 +40,12 @@ final class ReportedFixCache { public static final Predicate REMOVE_EVERYTHING = bc -> false; public static final Predicate KEEP_EVERYTHING = bc -> true; + public static final Predicate NOT_AN_ERRORPRONE = suppression -> { + String checkerName = suppression.startsWith(CommonConstants.AUTOMATICALLY_ADDED_PREFIX) + ? suppression.substring(CommonConstants.AUTOMATICALLY_ADDED_PREFIX.length()) + : suppression; + return !AllErrorprones.allBugcheckerNames().contains(checkerName); + }; ReportedFixCache() {} @@ -67,6 +73,16 @@ public LazySuppressionFix reportNew( return cache.put(declaration.getLeaf(), createAndReportFix(declaration, visitorState, filterExisting)); } + /** + * Gets an existing {@code LazySuppressionFix} on {@code declaration} + */ + public LazySuppressionFix getExisting(TreePath declaration) { + if (!cache.containsKey(declaration.getLeaf())) { + throw new IllegalArgumentException("A fix on this declaration must already exists"); + } + return cache.get(declaration.getLeaf()); + } + private LazySuppressionFix createAndReportFix( TreePath declaration, VisitorState state, Predicate filterExisting) { if (!SuppressWarningsUtils.suppressibleTreePath(declaration)) { diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java index 82a47850..76f1efe2 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java @@ -21,7 +21,11 @@ import com.google.errorprone.BugPattern.SeverityLevel; import com.google.errorprone.VisitorState; import com.google.errorprone.matchers.Description; +import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.CompilationUnitTree; import com.sun.source.util.TreePath; +import com.sun.source.util.TreePathScanner; +import java.util.HashSet; import java.util.Optional; import java.util.Set; import java.util.logging.Logger; @@ -29,12 +33,13 @@ // CHECKSTYLE:ON +@SuppressWarnings("RestrictedApi") public final class VisitorStateModifications { private static final Logger log = Logger.getLogger(VisitorStateModifications.class.getName()); - private static final ReportedFixCache FIXES = new ReportedFixCache(); + private static final Set SEEN = new HashSet<>(); - @SuppressWarnings("RestrictedApi") + @SuppressWarnings("CyclomaticComplexity") public static Description interceptDescription(VisitorState visitorState, Description description) { // Prevent infinite recursion on reported fixes if (description == Description.NO_MATCH @@ -42,8 +47,13 @@ public static Description interceptDescription(VisitorState visitorState, Descri return description; } - if (getModes(visitorState).contains("RemoveUnused")) { - throw new UnsupportedOperationException("RemoveUnused mode is not yet supported"); + CompilationUnitTree compilationUnit = visitorState.getPath().getCompilationUnit(); + + Set modes = getModes(visitorState); + + if (modes.contains("RemoveUnused") && !SEEN.contains(compilationUnit)) { + removeAllSuppressionsOnErrorpronesInCompilationUnit(compilationUnit, visitorState); + SEEN.add(compilationUnit); } // If both -PerrorProneSuppress and -PerrorProneApply are used at the same time, for the checks configured as @@ -74,6 +84,25 @@ public static Description interceptDescription(VisitorState visitorState, Descri TreePath pathToActualError = TreePath.getPath(visitorState.getPath().getCompilationUnit(), description.position.getTree()); + Optional firstSuppressibleWhichSuppressesDescription = Stream.iterate( + pathToActualError, treePath -> treePath.getParentPath() != null, TreePath::getParentPath) + .dropWhile(path -> !suppresses(path, description)) + .findFirst(); + + if (firstSuppressibleWhichSuppressesDescription.isPresent() && modes.contains("RemoveUnused")) { + FIXES.getExisting(firstSuppressibleWhichSuppressesDescription.get()).addSuppression(description.checkName); + return Description.NO_MATCH; + } + + if (!modes.contains("Suppress")) { + log.warning("No autofix was found for " + description.checkName + " at position " + + description.position.getStartPosition() + " in " + + visitorState.getPath().getCompilationUnit().getSourceFile() + "." + + " -PerrorProneSuppress was not passed either. " + + " SuppressibleErrorProne will not be able to add a suppression for this error."); + return Description.NO_MATCH; + } + Optional firstSuppressible = Stream.iterate( pathToActualError, treePath -> treePath.getParentPath() != null, TreePath::getParentPath) .dropWhile(path -> !SuppressWarningsUtils.suppressibleTreePath(path)) @@ -95,14 +124,47 @@ public static Description interceptDescription(VisitorState visitorState, Descri // this, we make sure we only make one fix per source element we put the suppression on by using a Map. This // way we have our own mutable Fix that we can add errors to, and only once the file has been visited by all // the error-prone checks it will then produce a replacement with all the checks suppressed. - LazySuppressionFix suppressingFix = FIXES.getOrReportNew( - firstSuppressible.get(), - visitorState, - bugchecker -> !bugchecker.startsWith(CommonConstants.AUTOMATICALLY_ADDED_PREFIX)); + LazySuppressionFix suppressingFix = + FIXES.getOrReportNew(firstSuppressible.get(), visitorState, ReportedFixCache.KEEP_EVERYTHING); suppressingFix.addSuppression(CommonConstants.AUTOMATICALLY_ADDED_PREFIX + description.checkName); return Description.NO_MATCH; } + private static boolean suppresses(TreePath declaration, Description description) { + if (!SuppressWarningsUtils.suppressibleTreePath(declaration)) { + return false; + } + + Optional suppressWarningsMaybe = + SuppressWarningsUtils.getSuppressWarnings(declaration); + if (suppressWarningsMaybe.isEmpty()) { + return false; + } + + String checkName = description.checkName; + return AnnotationUtils.annotationStringValues(suppressWarningsMaybe.get()) + .anyMatch(checkName::equals); + } + + private static void removeAllSuppressionsOnErrorpronesInCompilationUnit( + CompilationUnitTree compilationUnit, VisitorState visitorState) { + + new TreePathScanner() { + @Override + public Void visitAnnotation(AnnotationTree node, Void unused) { + if (AnnotationUtils.isSuppressWarningsAnnotation(node)) { + TreePath declaration = getCurrentPath().getParentPath().getParentPath(); + + // 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 + FIXES.getOrReportNew(declaration, visitorState, ReportedFixCache.NOT_AN_ERRORPRONE); + } + + return super.visitAnnotation(node, unused); + } + }.scan(compilationUnit, null); + } + private static Set getModes(VisitorState state) { return state.errorProneOptions().getFlags().getSetOrEmpty("SuppressibleErrorProne:Mode"); } From c642485fc15f75fd738df667e7b8c2c754efc355 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 22 Oct 2025 17:51:08 +0100 Subject: [PATCH 2/4] Move code out of VisitorStateModifications --- ...ibleErrorPronePluginIntegrationTest.groovy | 14 ++--- .../ReportedFixCache.java | 2 +- .../SuppressionRemover.java | 52 +++++++++++++++++++ .../VisitorStateModifications.java | 32 +++--------- 4 files changed, 66 insertions(+), 34 deletions(-) create mode 100644 suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressionRemover.java diff --git a/gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy b/gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy index 9f61b424..62d68a88 100644 --- a/gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy +++ b/gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy @@ -1641,7 +1641,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec new int[3].toString(); } } - '''.stripIndent(true) + '''.stripIndent(true) } def 'errorProneRemoveUnused only keeps the closest suppression to a violation'() { @@ -1689,7 +1689,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec } } } - '''.stripIndent(true) + '''.stripIndent(true) } def 'errorProneRemoveUnused handles multiple suppressions on different tree types gracefully'() { @@ -1751,7 +1751,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec } } } - '''.stripIndent(true) + '''.stripIndent(true) } def 'errorProneRemoveUnused removes entire SuppressWarnings annotation when all suppressions are unused'() { @@ -1780,7 +1780,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec System.out.println("No violations here"); } } - '''.stripIndent(true) + '''.stripIndent(true) } def 'errorProneRemoveUnused and errorProneSuppress uses existing suppressions if possible'() { @@ -1838,7 +1838,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec } } } - '''.stripIndent(true) + '''.stripIndent(true) } def 'errorProneRemoveUnused and errorProneApply applies fixes on previously suppressed elements'() { @@ -1895,7 +1895,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec } } } - '''.stripIndent(true) + '''.stripIndent(true) } def 'errorProneRemoveUnused + errorProneApply + errorProneSuppress applies fixes and suppressions on previously suppressed elements'() { @@ -1956,7 +1956,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec } } } - '''.stripIndent(true) + '''.stripIndent(true) } def 'error-prone dependencies have versions bound together by a virtual platform'() { diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java index cd2cb19f..8e20b909 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java @@ -31,7 +31,7 @@ import java.util.stream.Stream; import javax.lang.model.element.Name; -final class ReportedFixCache { +public final class ReportedFixCache { // Weak map so that we don't leak memory by keeping hold of references to the source element tree keys and our // mutable fixes values around forever, once error-prone has finished with the source element tree used as a key // here (once the file has been visited by all the error-prone checks), our SuppressingFixes can be safely diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressionRemover.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressionRemover.java new file mode 100644 index 00000000..ec808530 --- /dev/null +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressionRemover.java @@ -0,0 +1,52 @@ +/* + * (c) Copyright 2025 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.suppressibleerrorprone; + +import com.google.errorprone.VisitorState; +import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.CompilationUnitTree; +import com.sun.source.util.TreePath; +import com.sun.source.util.TreePathScanner; +import java.util.Collections; +import java.util.Set; +import java.util.WeakHashMap; + +public class SuppressionRemover { + // Weak set so that we don't leak memory by keeping hold of references to CompilationUnitTrees after error-prone + // has finished processing. + private static final Set attachedFixes = Collections.newSetFromMap(new WeakHashMap<>()); + + public static void removeAllSuppressionsOnErrorprones( + ReportedFixCache reportedFixes, CompilationUnitTree unit, VisitorState state) { + if (attachedFixes.add(unit)) { + new TreePathScanner() { + @Override + public Void visitAnnotation(AnnotationTree node, Void unused) { + if (AnnotationUtils.isSuppressWarningsAnnotation(node)) { + TreePath declaration = getCurrentPath().getParentPath().getParentPath(); + + reportedFixes.getOrReportNew(declaration, state, ReportedFixCache.NOT_AN_ERRORPRONE); + } + + return super.visitAnnotation(node, unused); + } + }.scan(unit, null); + } + } + + private SuppressionRemover() {} +} diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java index 76f1efe2..b2d7dcba 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java @@ -24,8 +24,6 @@ import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.CompilationUnitTree; import com.sun.source.util.TreePath; -import com.sun.source.util.TreePathScanner; -import java.util.HashSet; import java.util.Optional; import java.util.Set; import java.util.logging.Logger; @@ -37,7 +35,6 @@ public final class VisitorStateModifications { private static final Logger log = Logger.getLogger(VisitorStateModifications.class.getName()); private static final ReportedFixCache FIXES = new ReportedFixCache(); - private static final Set SEEN = new HashSet<>(); @SuppressWarnings("CyclomaticComplexity") public static Description interceptDescription(VisitorState visitorState, Description description) { @@ -48,12 +45,12 @@ public static Description interceptDescription(VisitorState visitorState, Descri } CompilationUnitTree compilationUnit = visitorState.getPath().getCompilationUnit(); - Set modes = getModes(visitorState); - if (modes.contains("RemoveUnused") && !SEEN.contains(compilationUnit)) { - removeAllSuppressionsOnErrorpronesInCompilationUnit(compilationUnit, visitorState); - SEEN.add(compilationUnit); + 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); } // If both -PerrorProneSuppress and -PerrorProneApply are used at the same time, for the checks configured as @@ -90,6 +87,8 @@ public static Description interceptDescription(VisitorState visitorState, Descri .findFirst(); if (firstSuppressibleWhichSuppressesDescription.isPresent() && modes.contains("RemoveUnused")) { + // In RemoveUnused mode, removeAllSuppressionsOnErrorprones guarantees that a fix must already exist on + // this suppressible. FIXES.getExisting(firstSuppressibleWhichSuppressesDescription.get()).addSuppression(description.checkName); return Description.NO_MATCH; } @@ -146,25 +145,6 @@ private static boolean suppresses(TreePath declaration, Description description) .anyMatch(checkName::equals); } - private static void removeAllSuppressionsOnErrorpronesInCompilationUnit( - CompilationUnitTree compilationUnit, VisitorState visitorState) { - - new TreePathScanner() { - @Override - public Void visitAnnotation(AnnotationTree node, Void unused) { - if (AnnotationUtils.isSuppressWarningsAnnotation(node)) { - TreePath declaration = getCurrentPath().getParentPath().getParentPath(); - - // 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 - FIXES.getOrReportNew(declaration, visitorState, ReportedFixCache.NOT_AN_ERRORPRONE); - } - - return super.visitAnnotation(node, unused); - } - }.scan(compilationUnit, null); - } - private static Set getModes(VisitorState state) { return state.errorProneOptions().getFlags().getSetOrEmpty("SuppressibleErrorProne:Mode"); } From 96c416ebae4ed2c16986d7fcf122bd1b82f4a3ea Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Thu, 23 Oct 2025 13:27:34 +0100 Subject: [PATCH 3/4] Add test against errorProneRemoveUnused applying fixes --- ...ibleErrorPronePluginIntegrationTest.groovy | 39 ++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy b/gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy index 62d68a88..44e10ab4 100644 --- a/gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy +++ b/gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy @@ -1644,6 +1644,43 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec '''.stripIndent(true) } + def 'errorProneRemoveUnused does not apply fixes'() { + given: + // language=Java + def initialSource = ''' + package app; + + public final class App { + class Inner { + class InnerInner {} + } + } + '''.stripIndent(true) + + writeJavaSourceFileToSourceSets initialSource + + when: 'errorProneRemoveUnused is run by itself' + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused') + + then: 'No checks are applied' + javaSourceIsSyntacticallyEqualTo initialSource + + when: 'ClassCanBeStatic is applied alongside errorProneRemoveUnused' + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused', '-PerrorProneApply=ClassCanBeStatic') + + then: 'ClassCanBeStatic is applied' + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + public final class App { + static class Inner { + static class InnerInner {} + } + } + '''.stripIndent(true) + } + def 'errorProneRemoveUnused only keeps the closest suppression to a violation'() { // language=Java writeJavaSourceFileToSourceSets ''' @@ -1668,7 +1705,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec '''.stripIndent(true) when: - runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused') + def result = runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused') then: From b57c2f7d49bdedcb38c3675bfd9e867b2227066c Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Thu, 23 Oct 2025 13:33:06 +0100 Subject: [PATCH 4/4] Add comment explaining --- .../suppressibleerrorprone/VisitorStateModifications.java | 1 + 1 file changed, 1 insertion(+) diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java index b2d7dcba..8f707147 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java @@ -99,6 +99,7 @@ public static Description interceptDescription(VisitorState visitorState, Descri + visitorState.getPath().getCompilationUnit().getSourceFile() + "." + " -PerrorProneSuppress was not passed either. " + " SuppressibleErrorProne will not be able to add a suppression for this error."); + // Returning description here could trigger an unintended autofix return Description.NO_MATCH; }