From e6808d4f8b5a2eb7c4efad85aef888f13c8e62c8 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Mon, 22 Sep 2025 11:13:19 +0100 Subject: [PATCH 01/12] Add tests --- ...ibleErrorPronePluginIntegrationTest.groovy | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) 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 81fef071..70c21460 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 @@ -1308,6 +1308,64 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec !output.contains('[RemoveRolloutSuppressions]') } + def 'can remove unused suppressions with -PerrorProneRemoveUnused'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + @SuppressWarnings({"ArrayToString", "UnusedVariable"}) + public final class App { + public static void main(String[] args) { + // Only ArrayToString is actually triggered + new int[3].toString(); + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneSuppress') + + then: + // language=Java + appJavaTextEquals ''' + package app; + @SuppressWarnings("ArrayToString") + public final class App { + public static void main(String[] args) { + // Only ArrayToString is actually triggered + new int[3].toString(); + } + } + '''.stripIndent(true) + } + + def 'removes entire SuppressWarnings annotation when all suppressions are unused'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + @SuppressWarnings({"UnusedVariable", "SomeOtherCheck"}) + public final class App { + public static void main(String[] args) { + System.out.println("No violations here"); + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-') + + then: + // language=Java + appJavaTextEquals ''' + package app; + public final class App { + public static void main(String[] args) { + System.out.println("No violations here"); + } + } + '''.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 From 83a9b16c0b6dac0226ff78eaf4b6700eb60a1743 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 24 Sep 2025 19:33:13 +0100 Subject: [PATCH 02/12] wip --- .../SuppressibleErrorPronePlugin.java | 6 + .../suppressibleerrorprone/modes/Modes.java | 2 + .../modes/common/CommonOptions.java | 9 + .../modes/common/ModeName.java | 1 + .../modes/common/RemoveUnusedCheck.java | 38 ++ .../modes/modes/RemoveUnusedMode.java | 58 ++ .../transform/ModifyErrorProneCheckApi.java | 6 +- ...ppressibleTreePathScannerClassVisitor.java | 75 +++ ...ibleErrorPronePluginIntegrationTest.groovy | 81 ++- suppressible-error-prone/build.gradle | 1 + .../AnnotationUtils.java | 5 + .../CheckerRegistry.java | 130 +++++ .../RemoveUnusedSuppressions.java | 526 ++++++++++++++++++ ...pressibleTreePathScannerModifications.java | 26 + .../SuppressionUsageTree.java | 164 ++++++ 15 files changed, 1108 insertions(+), 20 deletions(-) create mode 100644 gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/RemoveUnusedCheck.java create mode 100644 gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/RemoveUnusedMode.java create mode 100644 gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/transform/SuppressibleTreePathScannerClassVisitor.java create mode 100644 suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/CheckerRegistry.java create mode 100644 suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveUnusedSuppressions.java create mode 100644 suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleTreePathScannerModifications.java create mode 100644 suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressionUsageTree.java diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePlugin.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePlugin.java index 6a5bd658..4d14f776 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePlugin.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePlugin.java @@ -209,6 +209,12 @@ private void setupErrorProneOptions(CommonOptions commonOptions, ErrorProneOptio .getCheckOptions() .putAll(getProviderFactory().provider(commonOptions::extraErrorProneCheckOptions)); + errorProneOptions + .getChecks() + .put("RemoveUnusedSuppressions", getProviderFactory().provider(() -> commonOptions + .removeUnusedCheck() + .toCheckSeverity())); + // We disable this to avoid having `Note: [RemoveRolloutSuppressions]` in // unrelated error messages as it's a suggestion level check. If the remove rollout mode is enabled, // this check will be explicitly patched, which will enable it by default. diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/Modes.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/Modes.java index 32135d9c..9ebebb2b 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/Modes.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/Modes.java @@ -33,6 +33,7 @@ import com.palantir.gradle.suppressibleerrorprone.modes.modes.ApplyMode; import com.palantir.gradle.suppressibleerrorprone.modes.modes.DisableMode; import com.palantir.gradle.suppressibleerrorprone.modes.modes.RemoveRolloutMode; +import com.palantir.gradle.suppressibleerrorprone.modes.modes.RemoveUnusedMode; import com.palantir.gradle.suppressibleerrorprone.modes.modes.SuppressMode; import com.palantir.gradle.suppressibleerrorprone.modes.modes.TimingsMode; import java.util.List; @@ -60,6 +61,7 @@ public abstract class Modes { ModeName.APPLY, new ApplyMode(), ModeName.DISABLE, new DisableMode(), ModeName.REMOVE_ROLLOUT, new RemoveRolloutMode(), + ModeName.REMOVE_UNUSED, new RemoveUnusedMode(), ModeName.SUPPRESS, new SuppressMode(), ModeName.TIMINGS, new TimingsMode()); diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/CommonOptions.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/CommonOptions.java index a6adcfeb..d7445784 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/CommonOptions.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/CommonOptions.java @@ -41,6 +41,10 @@ default RemoveRolloutCheck removeRolloutCheck() { return RemoveRolloutCheck.DISABLE; } + default RemoveUnusedCheck removeUnusedCheck() { + return RemoveUnusedCheck.DISABLE; + } + default CommonOptions naivelyCombinedWith(CommonOptions other) { return new CommonOptions() { @Override @@ -59,6 +63,11 @@ public Map extraErrorProneCheckOptions() { public RemoveRolloutCheck removeRolloutCheck() { return CommonOptions.this.removeRolloutCheck().or(other.removeRolloutCheck()); } + + @Override + public RemoveUnusedCheck removeUnusedCheck() { + return CommonOptions.this.removeUnusedCheck().or(other.removeUnusedCheck()); + } }; } diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModeName.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModeName.java index d4863897..8e7f3c4c 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModeName.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModeName.java @@ -26,6 +26,7 @@ public enum ModeName { APPLY("errorProneApply"), SUPPRESS("errorProneSuppress"), REMOVE_ROLLOUT("errorProneRemoveRollout"), + REMOVE_UNUSED("errorProneRemoveUnused"), TIMINGS("errorProneTimings"), // Historically, the logic of this plugin lived in baseline, so we need to support the old disable flag. DISABLE("errorProneDisable", "com.palantir.baseline-error-prone.disable"), diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/RemoveUnusedCheck.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/RemoveUnusedCheck.java new file mode 100644 index 00000000..d3ba4bf4 --- /dev/null +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/RemoveUnusedCheck.java @@ -0,0 +1,38 @@ +/* + * (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.gradle.suppressibleerrorprone.modes.common; + +import net.ltgt.gradle.errorprone.CheckSeverity; + +public enum RemoveUnusedCheck { + DISABLE, + ENABLED; + + public CheckSeverity toCheckSeverity() { + return switch (this) { + case DISABLE -> CheckSeverity.OFF; + case ENABLED -> CheckSeverity.DEFAULT; + }; + } + + public RemoveUnusedCheck or(RemoveUnusedCheck other) { + return switch (this) { + case DISABLE -> other; + case ENABLED -> this; + }; + } +} diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/RemoveUnusedMode.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/RemoveUnusedMode.java new file mode 100644 index 00000000..48c29bf0 --- /dev/null +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/RemoveUnusedMode.java @@ -0,0 +1,58 @@ +/* + * (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.gradle.suppressibleerrorprone.modes.modes; + +import com.palantir.gradle.suppressibleerrorprone.modes.common.CommonOptions; +import com.palantir.gradle.suppressibleerrorprone.modes.common.Mode; +import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption; +import com.palantir.gradle.suppressibleerrorprone.modes.common.PatchChecksOption; +import com.palantir.gradle.suppressibleerrorprone.modes.common.RemoveUnusedCheck; +import java.util.Map; + +public class RemoveUnusedMode implements Mode { + private static final String ALL_CHECKS = ""; + + public ModifyCheckApiOption modifyCheckApi() { + return ModifyCheckApiOption.mustModify(); + } + + @Override + public CommonOptions configureAndReturnCommonOptions(ModeOptionContext context) { + return new CommonOptions() { + @Override + public PatchChecksOption patchChecks() { + return PatchChecksOption.someChecks("RemoveUnusedSuppressions"); + } + + @Override + public Map extraErrorProneCheckOptions() { + // For the suppressions to remove, if no specific check is enabled, we need to just remove everything + // We can't explicitly list all possible checks, because some might not exist anymore + // The logic itself needs to consider an empty list as "remove all" + + return Map.of( + "SuppressibleErrorProne:RemoveUnusedSuppressions", + context.flagValue().orElse(ALL_CHECKS)); + } + + @Override + public RemoveUnusedCheck removeUnusedCheck() { + return RemoveUnusedCheck.ENABLED; + } + }; + } +} diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/transform/ModifyErrorProneCheckApi.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/transform/ModifyErrorProneCheckApi.java index 420b50bc..95013a71 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/transform/ModifyErrorProneCheckApi.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/transform/ModifyErrorProneCheckApi.java @@ -67,7 +67,7 @@ private void suppressCheckApi(File output) { visitJar(output, (classJarPath, inputStream) -> classVisitorFor(classJarPath) .map(classVisitorFactory -> { ClassReader classReader = newClassReader(inputStream); - ClassWriter classWriter = new ClassWriter(classReader, 0); + ClassWriter classWriter = new ClassWriter(classReader, ClassWriter.COMPUTE_FRAMES); ClassVisitor classVisitor = classVisitorFactory.apply(classWriter); classReader.accept(classVisitor, 0); @@ -91,6 +91,10 @@ && getParameters().getModifyVisitorState().get()) { return Optional.of(VisitorStateClassVisitor::new); } + if (classJarPath.equals("com/google/errorprone/bugpatterns/BugChecker$SuppressibleTreePathScanner.class")) { + return Optional.of(SuppressibleTreePathScannerClassVisitor::new); + } + return Optional.empty(); } diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/transform/SuppressibleTreePathScannerClassVisitor.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/transform/SuppressibleTreePathScannerClassVisitor.java new file mode 100644 index 00000000..12643aae --- /dev/null +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/transform/SuppressibleTreePathScannerClassVisitor.java @@ -0,0 +1,75 @@ +/* + * (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.gradle.suppressibleerrorprone.transform; + +import org.objectweb.asm.ClassVisitor; +import org.objectweb.asm.Label; +import org.objectweb.asm.MethodVisitor; +import org.objectweb.asm.Opcodes; + +final class SuppressibleTreePathScannerClassVisitor extends ClassVisitor { + SuppressibleTreePathScannerClassVisitor(ClassVisitor classVisitor) { + super(Opcodes.ASM9, classVisitor); + } + + @Override + public MethodVisitor visitMethod( + int access, String name, String descriptor, String signature, String[] exceptions) { + MethodVisitor methodVisitor = super.visitMethod(access, name, descriptor, signature, exceptions); + + if (name.equals("suppressed") && descriptor.equals("(Lcom/sun/source/tree/Tree;)Z")) { + return new SuppressedMethodVisitor(methodVisitor); + } + + return methodVisitor; + } + + private static final class SuppressedMethodVisitor extends MethodVisitor { + SuppressedMethodVisitor(MethodVisitor methodVisitor) { + super(Opcodes.ASM9, methodVisitor); + } + + @Override + public void visitCode() { + super.visitCode(); + // Load this (SuppressibleTreePathScanner instance) + mv.visitVarInsn(Opcodes.ALOAD, 0); + // Get the state field + mv.visitFieldInsn( + Opcodes.GETFIELD, + "com/google/errorprone/bugpatterns/BugChecker$SuppressibleTreePathScanner", + "state", + "Lcom/google/errorprone/VisitorState;"); + // Check condition and potentially return false early + mv.visitMethodInsn( + Opcodes.INVOKESTATIC, + "com/palantir/suppressibleerrorprone/SuppressibleTreePathScannerModifications", + "shouldBypassSuppressions", + "(Lcom/google/errorprone/VisitorState;)Z", + false); + + // If condition is true, return false immediately + Label continueLabel = new Label(); + mv.visitJumpInsn(Opcodes.IFEQ, continueLabel); + mv.visitInsn(Opcodes.ICONST_0); // push false + mv.visitInsn(Opcodes.IRETURN); + mv.visitLabel(continueLabel); + // Add a frame here to fix verification + mv.visitFrame(Opcodes.F_SAME, 0, null, 0, null); + } + } +} 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 70c21460..49cbf10f 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 @@ -1308,41 +1308,84 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec !output.contains('[RemoveRolloutSuppressions]') } - def 'can remove unused suppressions with -PerrorProneRemoveUnused'() { + def 'errorProneRemoveUnused removes unused suppressions, and only unused suppressions'() { // language=Java writeJavaSourceFileToSourceSets ''' - package app; - @SuppressWarnings({"ArrayToString", "UnusedVariable"}) - public final class App { - public static void main(String[] args) { - // Only ArrayToString is actually triggered - new int[3].toString(); + package app; + @SuppressWarnings({"ArrayToString", "UnnecessaryFinal", "InlineTrivialConstant"}) + 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') + + then: + // language=Java + appJavaTextEquals ''' + package app; + @SuppressWarnings({"ArrayToString", "InlineTrivialConstant"}) + 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 removes suppressions not directly connected to a report'() { + // 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 { + private static final String EMPTY = ""; + } + } + } + '''.stripIndent(true) when: - runTasksSuccessfully('compileAllErrorProne', '-PerrorProneSuppress') + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused') then: + // language=Java appJavaTextEquals ''' - package app; - @SuppressWarnings("ArrayToString") - public final class App { - public static void main(String[] args) { - // Only ArrayToString is actually triggered - new int[3].toString(); + package app; + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + class Inner { + @SuppressWarnings("InlineTrivialConstant") + class InnerInner { + private static final String EMPTY = ""; + } + } } - } '''.stripIndent(true) } - def 'removes entire SuppressWarnings annotation when all suppressions are unused'() { + def 'errorProneRemoveUnused removes entire SuppressWarnings annotation when all suppressions are unused'() { // language=Java writeJavaSourceFileToSourceSets ''' package app; - @SuppressWarnings({"UnusedVariable", "SomeOtherCheck"}) + @SuppressWarnings({"UnusedVariable", "ArrayToString"}) public final class App { public static void main(String[] args) { System.out.println("No violations here"); @@ -1351,7 +1394,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec '''.stripIndent(true) when: - runTasksSuccessfully('compileAllErrorProne', '-') + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused') then: // language=Java diff --git a/suppressible-error-prone/build.gradle b/suppressible-error-prone/build.gradle index ece60e95..c6aa36a7 100644 --- a/suppressible-error-prone/build.gradle +++ b/suppressible-error-prone/build.gradle @@ -4,6 +4,7 @@ apply plugin: 'com.palantir.external-publish-jar' dependencies { implementation 'com.google.errorprone:error_prone_annotation' implementation 'com.google.errorprone:error_prone_check_api' + implementation 'com.google.errorprone:error_prone_core' annotationProcessor 'com.google.auto.service:auto-service' compileOnly 'com.google.auto.service:auto-service' diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AnnotationUtils.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AnnotationUtils.java index 12f7ef9b..271de804 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AnnotationUtils.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AnnotationUtils.java @@ -24,6 +24,7 @@ import com.sun.source.tree.MemberSelectTree; import com.sun.source.tree.NewArrayTree; import com.sun.source.tree.Tree; +import com.sun.source.util.TreePath; import java.util.stream.Stream; import javax.lang.model.element.Name; @@ -67,5 +68,9 @@ static Name annotationName(Tree annotationType) { "Unsupported annotation type: " + annotationType.getClass().getCanonicalName()); } + static Tree getAnnotatedTree(TreePath pathToAnnotationTree) { + return pathToAnnotationTree.getParentPath().getParentPath().getLeaf(); + } + private AnnotationUtils() {} } diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/CheckerRegistry.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/CheckerRegistry.java new file mode 100644 index 00000000..2bd69673 --- /dev/null +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/CheckerRegistry.java @@ -0,0 +1,130 @@ +/* + * (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.common.collect.ImmutableMap; +import com.google.errorprone.BugCheckerInfo; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.ErrorProneFlags; +import com.google.errorprone.ErrorPronePlugins; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.scanner.BuiltInCheckerSuppliers; +import com.google.errorprone.scanner.ErrorProneInjector; +import com.google.errorprone.scanner.ScannerSupplier; +import com.sun.tools.javac.util.Context; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import java.util.logging.Level; +import java.util.logging.Logger; + +/** + * Registry that maps checker names to their BugChecker instances. + * This includes both built-in Error Prone checkers and custom plugin checkers. + */ +public final class CheckerRegistry { + + private static final Logger logger = Logger.getLogger(CheckerRegistry.class.getName()); + + // Cache of checker name to BugChecker instance + private final Map checkersByName; + + private CheckerRegistry(Map checkersByName) { + this.checkersByName = ImmutableMap.copyOf(checkersByName); + } + + /** + * Creates a CheckerRegistry from enabled checkers only. + * This is more efficient if you only need to check suppressions for active checkers. + */ + public static CheckerRegistry createFromEnabledCheckers(VisitorState state) { + Context context = state.context; + + // Get only enabled checkers (errors and warnings) + ScannerSupplier enabledSupplier = BuiltInCheckerSuppliers.defaultChecks(); + + // Load plugin checkers and filter to enabled ones + ScannerSupplier allSuppliers = ErrorPronePlugins.loadPlugins(enabledSupplier, context); + + // Build the registry with only enabled checkers + Map checkersByName = new HashMap<>(); + + // Get the severity map to check if checkers are enabled + Map severityMap = state.severityMap(); + + for (BugCheckerInfo info : allSuppliers.getAllChecks().values()) { + // Check if this checker is enabled (not OFF) + com.google.errorprone.BugPattern.SeverityLevel severity = info.severity(severityMap); + + if (severity != SeverityLevel.SUGGESTION) { + try { + BugChecker checker = instantiateChecker(info.checkerClass()); + + // Register by canonical name + checkersByName.put(info.canonicalName(), checker); + + // Register by all alternative names + for (String name : info.allNames()) { + checkersByName.put(name, checker); + } + + } catch (Exception e) { + logger.log(Level.WARNING, "Failed to instantiate checker: " + info.canonicalName(), e); + } + } + } + + System.err.println("Initialized checkers: " + checkersByName.keySet()); + + return new CheckerRegistry(checkersByName); + } + + /** + * Gets the BugChecker instance for the given checker name. + * + * @param checkerName the name of the checker (canonical name or alternative name) + * @return Optional containing the BugChecker if found, empty otherwise + */ + public Optional getCheckerForName(String checkerName) { + return Optional.ofNullable(checkersByName.get(checkerName)); + } + + /** + * Convenience method for the RemoveUnusedSuppressions class. + */ + public static BugChecker getCheckerForSuppression(String suppression, VisitorState state) { + // Create registry lazily - you might want to cache this per compilation unit + CheckerRegistry registry = createFromEnabledCheckers(state); + return registry.getCheckerForName(suppression).orElse(null); + } + + private static BugChecker instantiateChecker(Class checkerClass) { + // Create an injector with empty flags, similar to ScannerSupplierImpl + ErrorProneInjector injector = + ErrorProneInjector.create().addBinding(ErrorProneFlags.class, ErrorProneFlags.empty()); + + return injector.getInstance(checkerClass); + } + + /** + * Returns the number of registered checkers. + */ + public int size() { + return checkersByName.size(); + } +} diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveUnusedSuppressions.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveUnusedSuppressions.java new file mode 100644 index 00000000..b43474b7 --- /dev/null +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveUnusedSuppressions.java @@ -0,0 +1,526 @@ +/* + * (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.auto.service.AutoService; +import com.google.common.collect.ImmutableSet; +import com.google.errorprone.BugPattern; +import com.google.errorprone.ErrorProneOptions; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.fixes.Fix; +import com.google.errorprone.fixes.Replacement; +import com.google.errorprone.fixes.Replacements.CoalescePolicy; +import com.google.errorprone.matchers.Description; +import com.palantir.suppressibleerrorprone.SuppressionUsageTree.TreeWithUnusedSuppressions; +import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.ArrayAccessTree; +import com.sun.source.tree.ArrayTypeTree; +import com.sun.source.tree.AssertTree; +import com.sun.source.tree.AssignmentTree; +import com.sun.source.tree.BinaryTree; +import com.sun.source.tree.BlockTree; +import com.sun.source.tree.BreakTree; +import com.sun.source.tree.CaseTree; +import com.sun.source.tree.CatchTree; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.CompilationUnitTree; +import com.sun.source.tree.CompoundAssignmentTree; +import com.sun.source.tree.ConditionalExpressionTree; +import com.sun.source.tree.ContinueTree; +import com.sun.source.tree.DoWhileLoopTree; +import com.sun.source.tree.EmptyStatementTree; +import com.sun.source.tree.EnhancedForLoopTree; +import com.sun.source.tree.ExpressionStatementTree; +import com.sun.source.tree.ForLoopTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.IfTree; +import com.sun.source.tree.ImportTree; +import com.sun.source.tree.InstanceOfTree; +import com.sun.source.tree.LabeledStatementTree; +import com.sun.source.tree.LambdaExpressionTree; +import com.sun.source.tree.LiteralTree; +import com.sun.source.tree.MemberReferenceTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.ModifiersTree; +import com.sun.source.tree.NewArrayTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.ParenthesizedTree; +import com.sun.source.tree.ReturnTree; +import com.sun.source.tree.SwitchTree; +import com.sun.source.tree.SynchronizedTree; +import com.sun.source.tree.ThrowTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.TryTree; +import com.sun.source.tree.TypeCastTree; +import com.sun.source.tree.UnaryTree; +import com.sun.source.tree.VariableTree; +import com.sun.source.tree.WhileLoopTree; +import com.sun.source.util.TreeScanner; +import com.sun.tools.javac.tree.EndPosTable; +import com.sun.tools.javac.tree.JCTree.JCCompilationUnit; +import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition; +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; + +/** + * This error-prone check identifies and removes unused @SuppressWarnings annotations. + * It works by checking each suppression warning against its associated BugChecker to determine + * if the suppression is actually needed. + */ +@AutoService(BugChecker.class) +@BugPattern( + link = "https://github.com/palantir/suppressible-error-prone", + linkType = BugPattern.LinkType.CUSTOM, + // This needs to be SUGGESTION so that error prone won't try to apply the check in normal operations + // When requested, we will directly enable it in the command line arguments + severity = BugPattern.SeverityLevel.SUGGESTION, + summary = "Remove unused @SuppressWarnings annotations") +@SuppressWarnings("TreeToString") +public final class RemoveUnusedSuppressions extends BugChecker implements BugChecker.CompilationUnitTreeMatcher { + private static final ThreadLocal SUPPRESSION_TREE = new ThreadLocal<>(); + + // Cache the registry per compilation to avoid repeated instantiation + private volatile CheckerRegistry cachedRegistry; + + @Override + public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) { + // build SuppressionUsageTree + SUPPRESSION_TREE.set(SuppressionUsageTree.constructSuppressions(tree, state)); + SuppressionUsageTree suppressionUsageTree = SUPPRESSION_TREE.get(); + System.err.println("All suppressions found in compilation unit: " + suppressionUsageTree.allSuppressionNames()); + + if (suppressionUsageTree.allSuppressionNames().isEmpty()) { + return Description.NO_MATCH; + } + + // for each suppressed bugchecker, scan the whole compilation unit, bypassing any suppressions. + for (String suppression : suppressionUsageTree.allSuppressionNames()) { + Optional bugCheckerMaybe = getBugCheckerForSuppression(suppression, state); + if (bugCheckerMaybe.isEmpty()) { + System.err.println(suppression + ": not found in registry, so we conservatively assume they are used"); + suppressionUsageTree.markAllSuppressionsAsUsed(suppression); + continue; + } + + BugChecker bugChecker = bugCheckerMaybe.get(); + VisitorState customState = VisitorState.createConfiguredForCompilation( + state.context, + (description) -> { + if (description != Description.NO_MATCH) { + SUPPRESSION_TREE + .get() + .flagFirstParentSuppressionAsUsed( + description.position.getTree(), description.checkName); + } + }, + state.severityMap(), + ignoreSuppressions(state.errorProneOptions())) + .withPath(state.getPath()); + System.err.println(suppression + ": scanning to find usages"); + + new SuppressionCheckingScanner(bugChecker, suppressionUsageTree).scan(tree, customState); + } + + for (TreeWithUnusedSuppressions treeWithUnusedSuppressions : suppressionUsageTree.unusedSuppressions()) { + Set unusedSuppressions = treeWithUnusedSuppressions.unusedSuppressions(); + System.err.println("========================================"); + System.err.println("Unused suppressions found in tree : " + unusedSuppressions); + System.err.println(treeWithUnusedSuppressions.tree()); + System.err.println("========================================\n"); + + // Get modifiers tree based on tree type + ModifiersTree modifiers; + Tree declarationTree = treeWithUnusedSuppressions.tree(); + if (declarationTree instanceof MethodTree methodTree) { + modifiers = methodTree.getModifiers(); + } else if (declarationTree instanceof ClassTree classTree) { + modifiers = classTree.getModifiers(); + } else if (declarationTree instanceof VariableTree variableTree) { + modifiers = variableTree.getModifiers(); + } else { + throw new IllegalStateException("Unexpected tree type: " + declarationTree.getClass()); + } + + // Find @SuppressWarnings annotation + for (AnnotationTree annotation : modifiers.getAnnotations()) { + if (isSuppressWarningsAnnotation(annotation)) { + Fix fix = createSuppressionFix(annotation, unusedSuppressions, state); + state.reportMatch(buildDescription(tree) + .setMessage("Remove unused @SuppressWarnings: " + unusedSuppressions) + .addFix(fix) + .build()); + } + } + } + + return Description.NO_MATCH; + } + + private static ErrorProneOptions ignoreSuppressions(ErrorProneOptions originalOptions) { + List args = new ArrayList<>(); + args.add("-XepIgnoreSuppressionAnnotations"); + + // Reconstruct severity mappings + originalOptions.getSeverityMap().forEach((check, severity) -> { + args.add("-Xep:" + check + ":" + severity); + }); + + // Reconstruct boolean flags + if (originalOptions.ignoreUnknownChecks()) { + args.add("-XepIgnoreUnknownCheckNames"); + } + if (originalOptions.disableWarningsInGeneratedCode()) { + args.add("-XepDisableWarningsInGeneratedCode"); + } + if (originalOptions.isDisableAllWarnings()) { + args.add("-XepDisableAllWarnings"); + } + if (originalOptions.isDropErrorsToWarnings()) { + args.add("-XepAllErrorsAsWarnings"); + } + if (originalOptions.isSuggestionsAsWarnings()) { + args.add("-XepAllSuggestionsAsWarnings"); + } + if (originalOptions.isEnableAllChecksAsWarnings()) { + args.add("-XepAllDisabledChecksAsWarnings"); + } + if (originalOptions.isDisableAllChecks()) { + args.add("-XepDisableAllChecks"); + } + if (originalOptions.isTestOnlyTarget()) { + args.add("-XepCompilingTestOnlyCode"); + } + if (originalOptions.isPubliclyVisibleTarget()) { + args.add("-XepCompilingPubliclyVisibleCode"); + } + + // Reconstruct excluded paths pattern + if (originalOptions.getExcludedPattern() != null) { + args.add("-XepExcludedPaths:" + originalOptions.getExcludedPattern().pattern()); + } + + return ErrorProneOptions.processArgs(args); + } + + /** + * Gets the BugChecker associated with a suppression name using the registry. + */ + private Optional getBugCheckerForSuppression(String suppression, VisitorState state) { + // Use cached registry or create new one + if (cachedRegistry == null) { + synchronized (this) { + if (cachedRegistry == null) { + cachedRegistry = CheckerRegistry.createFromEnabledCheckers(state); + } + } + } + return cachedRegistry.getCheckerForName(suppression); + } + + private class SuppressionCheckingScanner extends TreeScanner { + private final SuppressionUsageTree suppressionUsageTree; + private final BugChecker checker; + + public SuppressionCheckingScanner(BugChecker checker, SuppressionUsageTree suppressionUsageTree) { + super(); + this.checker = checker; + this.suppressionUsageTree = suppressionUsageTree; + } + + @Override + public Void scan(Tree tree, VisitorState state) { + if (tree == null) { + return null; + } + + VisitorState newState = state.withPath(state.getPath()); + + Description description = checkTreeAgainstChecker(tree, newState); + state.reportMatch(description); + + return super.scan(tree, newState); + } + + /** + * Checks a tree against all the matcher interfaces implemented by the BugChecker. + */ + private Description checkTreeAgainstChecker(Tree tree, VisitorState state) { + // Check each matcher interface that the checker implements + if (checker instanceof BugChecker.AnnotationTreeMatcher && tree instanceof AnnotationTree) { + return ((BugChecker.AnnotationTreeMatcher) checker).matchAnnotation((AnnotationTree) tree, state); + } + if (checker instanceof BugChecker.ArrayAccessTreeMatcher && tree instanceof ArrayAccessTree) { + return ((BugChecker.ArrayAccessTreeMatcher) checker).matchArrayAccess((ArrayAccessTree) tree, state); + } + if (checker instanceof BugChecker.ArrayTypeTreeMatcher && tree instanceof ArrayTypeTree) { + return ((BugChecker.ArrayTypeTreeMatcher) checker).matchArrayType((ArrayTypeTree) tree, state); + } + if (checker instanceof BugChecker.AssertTreeMatcher && tree instanceof AssertTree) { + return ((BugChecker.AssertTreeMatcher) checker).matchAssert((AssertTree) tree, state); + } + if (checker instanceof BugChecker.AssignmentTreeMatcher && tree instanceof AssignmentTree) { + return ((BugChecker.AssignmentTreeMatcher) checker).matchAssignment((AssignmentTree) tree, state); + } + if (checker instanceof BugChecker.BinaryTreeMatcher && tree instanceof BinaryTree) { + return ((BugChecker.BinaryTreeMatcher) checker).matchBinary((BinaryTree) tree, state); + } + if (checker instanceof BugChecker.BlockTreeMatcher && tree instanceof BlockTree) { + return ((BugChecker.BlockTreeMatcher) checker).matchBlock((BlockTree) tree, state); + } + if (checker instanceof BugChecker.BreakTreeMatcher && tree instanceof BreakTree) { + return ((BugChecker.BreakTreeMatcher) checker).matchBreak((BreakTree) tree, state); + } + if (checker instanceof BugChecker.CaseTreeMatcher && tree instanceof CaseTree) { + return ((BugChecker.CaseTreeMatcher) checker).matchCase((CaseTree) tree, state); + } + if (checker instanceof BugChecker.CatchTreeMatcher && tree instanceof CatchTree) { + return ((BugChecker.CatchTreeMatcher) checker).matchCatch((CatchTree) tree, state); + } + if (checker instanceof BugChecker.ClassTreeMatcher && tree instanceof ClassTree) { + return ((BugChecker.ClassTreeMatcher) checker).matchClass((ClassTree) tree, state); + } + if (checker instanceof BugChecker.CompilationUnitTreeMatcher && tree instanceof CompilationUnitTree) { + return ((BugChecker.CompilationUnitTreeMatcher) checker) + .matchCompilationUnit((CompilationUnitTree) tree, state); + } + if (checker instanceof BugChecker.CompoundAssignmentTreeMatcher && tree instanceof CompoundAssignmentTree) { + return ((BugChecker.CompoundAssignmentTreeMatcher) checker) + .matchCompoundAssignment((CompoundAssignmentTree) tree, state); + } + if (checker instanceof BugChecker.ConditionalExpressionTreeMatcher + && tree instanceof ConditionalExpressionTree) { + return ((BugChecker.ConditionalExpressionTreeMatcher) checker) + .matchConditionalExpression((ConditionalExpressionTree) tree, state); + } + if (checker instanceof BugChecker.ContinueTreeMatcher && tree instanceof ContinueTree) { + return ((BugChecker.ContinueTreeMatcher) checker).matchContinue((ContinueTree) tree, state); + } + if (checker instanceof BugChecker.DoWhileLoopTreeMatcher && tree instanceof DoWhileLoopTree) { + return ((BugChecker.DoWhileLoopTreeMatcher) checker).matchDoWhileLoop((DoWhileLoopTree) tree, state); + } + if (checker instanceof BugChecker.EmptyStatementTreeMatcher && tree instanceof EmptyStatementTree) { + return ((BugChecker.EmptyStatementTreeMatcher) checker) + .matchEmptyStatement((EmptyStatementTree) tree, state); + } + if (checker instanceof BugChecker.EnhancedForLoopTreeMatcher && tree instanceof EnhancedForLoopTree) { + return ((BugChecker.EnhancedForLoopTreeMatcher) checker) + .matchEnhancedForLoop((EnhancedForLoopTree) tree, state); + } + if (checker instanceof BugChecker.ExpressionStatementTreeMatcher + && tree instanceof ExpressionStatementTree) { + return ((BugChecker.ExpressionStatementTreeMatcher) checker) + .matchExpressionStatement((ExpressionStatementTree) tree, state); + } + if (checker instanceof BugChecker.ForLoopTreeMatcher && tree instanceof ForLoopTree) { + return ((BugChecker.ForLoopTreeMatcher) checker).matchForLoop((ForLoopTree) tree, state); + } + if (checker instanceof BugChecker.IdentifierTreeMatcher && tree instanceof IdentifierTree) { + return ((BugChecker.IdentifierTreeMatcher) checker).matchIdentifier((IdentifierTree) tree, state); + } + if (checker instanceof BugChecker.IfTreeMatcher && tree instanceof IfTree) { + return ((BugChecker.IfTreeMatcher) checker).matchIf((IfTree) tree, state); + } + if (checker instanceof BugChecker.ImportTreeMatcher && tree instanceof ImportTree) { + return ((BugChecker.ImportTreeMatcher) checker).matchImport((ImportTree) tree, state); + } + if (checker instanceof BugChecker.InstanceOfTreeMatcher && tree instanceof InstanceOfTree) { + return ((BugChecker.InstanceOfTreeMatcher) checker).matchInstanceOf((InstanceOfTree) tree, state); + } + if (checker instanceof BugChecker.LabeledStatementTreeMatcher && tree instanceof LabeledStatementTree) { + return ((BugChecker.LabeledStatementTreeMatcher) checker) + .matchLabeledStatement((LabeledStatementTree) tree, state); + } + if (checker instanceof BugChecker.LambdaExpressionTreeMatcher && tree instanceof LambdaExpressionTree) { + return ((BugChecker.LambdaExpressionTreeMatcher) checker) + .matchLambdaExpression((LambdaExpressionTree) tree, state); + } + if (checker instanceof BugChecker.LiteralTreeMatcher && tree instanceof LiteralTree) { + return ((BugChecker.LiteralTreeMatcher) checker).matchLiteral((LiteralTree) tree, state); + } + if (checker instanceof BugChecker.MemberReferenceTreeMatcher && tree instanceof MemberReferenceTree) { + return ((BugChecker.MemberReferenceTreeMatcher) checker) + .matchMemberReference((MemberReferenceTree) tree, state); + } + if (checker instanceof BugChecker.MemberSelectTreeMatcher && tree instanceof MemberSelectTree) { + return ((BugChecker.MemberSelectTreeMatcher) checker).matchMemberSelect((MemberSelectTree) tree, state); + } + if (checker instanceof BugChecker.MethodTreeMatcher && tree instanceof MethodTree) { + return ((BugChecker.MethodTreeMatcher) checker).matchMethod((MethodTree) tree, state); + } + if (checker instanceof BugChecker.MethodInvocationTreeMatcher && tree instanceof MethodInvocationTree) { + return ((BugChecker.MethodInvocationTreeMatcher) checker) + .matchMethodInvocation((MethodInvocationTree) tree, state); + } + if (checker instanceof BugChecker.ModifiersTreeMatcher && tree instanceof ModifiersTree) { + return ((BugChecker.ModifiersTreeMatcher) checker).matchModifiers((ModifiersTree) tree, state); + } + if (checker instanceof BugChecker.NewArrayTreeMatcher && tree instanceof NewArrayTree) { + return ((BugChecker.NewArrayTreeMatcher) checker).matchNewArray((NewArrayTree) tree, state); + } + if (checker instanceof BugChecker.NewClassTreeMatcher && tree instanceof NewClassTree) { + return ((BugChecker.NewClassTreeMatcher) checker).matchNewClass((NewClassTree) tree, state); + } + if (checker instanceof BugChecker.ParenthesizedTreeMatcher && tree instanceof ParenthesizedTree) { + return ((BugChecker.ParenthesizedTreeMatcher) checker) + .matchParenthesized((ParenthesizedTree) tree, state); + } + if (checker instanceof BugChecker.ReturnTreeMatcher && tree instanceof ReturnTree) { + return ((BugChecker.ReturnTreeMatcher) checker).matchReturn((ReturnTree) tree, state); + } + if (checker instanceof BugChecker.SwitchTreeMatcher && tree instanceof SwitchTree) { + return ((BugChecker.SwitchTreeMatcher) checker).matchSwitch((SwitchTree) tree, state); + } + if (checker instanceof BugChecker.SynchronizedTreeMatcher && tree instanceof SynchronizedTree) { + return ((BugChecker.SynchronizedTreeMatcher) checker).matchSynchronized((SynchronizedTree) tree, state); + } + if (checker instanceof BugChecker.ThrowTreeMatcher && tree instanceof ThrowTree) { + return ((BugChecker.ThrowTreeMatcher) checker).matchThrow((ThrowTree) tree, state); + } + if (checker instanceof BugChecker.TryTreeMatcher && tree instanceof TryTree) { + return ((BugChecker.TryTreeMatcher) checker).matchTry((TryTree) tree, state); + } + if (checker instanceof BugChecker.TypeCastTreeMatcher && tree instanceof TypeCastTree) { + return ((BugChecker.TypeCastTreeMatcher) checker).matchTypeCast((TypeCastTree) tree, state); + } + if (checker instanceof BugChecker.UnaryTreeMatcher && tree instanceof UnaryTree) { + return ((BugChecker.UnaryTreeMatcher) checker).matchUnary((UnaryTree) tree, state); + } + if (checker instanceof BugChecker.VariableTreeMatcher && tree instanceof VariableTree) { + return ((BugChecker.VariableTreeMatcher) checker).matchVariable((VariableTree) tree, state); + } + if (checker instanceof BugChecker.WhileLoopTreeMatcher && tree instanceof WhileLoopTree) { + return ((BugChecker.WhileLoopTreeMatcher) checker).matchWhileLoop((WhileLoopTree) tree, state); + } + + return Description.NO_MATCH; + } + } + + /** + * Hook to be called from VisitorState.reportMatch to track when matches are reported. + */ + public static void onMatchReported(Description description) { + System.err.println("reportMatch called"); + + if (description != Description.NO_MATCH) { + SUPPRESSION_TREE + .get() + .flagFirstParentSuppressionAsUsed(description.position.getTree(), description.checkName); + } + } + + private static boolean isSuppressWarningsAnnotation(AnnotationTree annotation) { + return AnnotationUtils.annotationName(annotation.getAnnotationType()).contentEquals("SuppressWarnings"); + } + + private static Fix createSuppressionFix( + AnnotationTree annotation, Set unusedSuppressions, VisitorState state) { + // Get current suppression values using existing utility + List currentSuppressions = + AnnotationUtils.annotationStringValues(annotation).toList(); + + // Remove unused suppressions + List remainingSuppressions = currentSuppressions.stream() + .filter(s -> !unusedSuppressions.contains(s)) + .collect(Collectors.toList()); + + if (remainingSuppressions.isEmpty()) { + // Remove entire annotation + return new LineRemovingReplacementFix(state.getSourceCode(), (DiagnosticPosition) annotation, ""); + } else { + // Update annotation with remaining suppressions + String newAnnotation = buildSuppressWarningsAnnotation(remainingSuppressions); + return new LineRemovingReplacementFix( + state.getSourceCode(), (DiagnosticPosition) annotation, newAnnotation); + } + } + + private static String buildSuppressWarningsAnnotation(List suppressions) { + if (suppressions.size() == 1) { + return "@SuppressWarnings(\"" + suppressions.get(0) + "\")"; + } else { + String values = suppressions.stream().map(s -> "\"" + s + "\"").collect(Collectors.joining(", ")); + return "@SuppressWarnings({" + values + "})"; + } + } + + /** + * This class handles replacement with optional line removal for empty suppressions. + */ + private static final class LineRemovingReplacementFix implements Fix { + private final CharSequence sourceCode; + private final DiagnosticPosition position; + private final String replacementText; + + private LineRemovingReplacementFix( + CharSequence sourceCode, DiagnosticPosition position, String replacementText) { + this.sourceCode = sourceCode; + this.position = position; + this.replacementText = replacementText; + } + + @Override + public String toString(JCCompilationUnit compilationUnit) { + return "LineRemovingReplacementFix"; + } + + @Override + public String getShortDescription() { + return "Replace text at the position with the provided text, " + + "or remove the text and all preceding whitespace"; + } + + @Override + public CoalescePolicy getCoalescePolicy() { + return CoalescePolicy.REJECT; + } + + @Override + public ImmutableSet getReplacements(EndPosTable endPositions) { + // If we are looking to delete the entire element, we should also remove whitespace before it, + // up to and including the newline + if (replacementText.isEmpty() && sourceCode != null) { + int start = SourceCodeUtils.startPositionWithWhitespaceIncludingNewLine( + sourceCode, position.getStartPosition()); + return ImmutableSet.of(Replacement.create(start, position.getEndPosition(endPositions), "")); + } + return ImmutableSet.of(Replacement.create( + position.getStartPosition(), position.getEndPosition(endPositions), replacementText)); + } + + @Override + public ImmutableSet getImportsToAdd() { + return ImmutableSet.of(); + } + + @Override + public ImmutableSet getImportsToRemove() { + return ImmutableSet.of(); + } + + @Override + public boolean isEmpty() { + return false; + } + } +} diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleTreePathScannerModifications.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleTreePathScannerModifications.java new file mode 100644 index 00000000..2bbe2b94 --- /dev/null +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleTreePathScannerModifications.java @@ -0,0 +1,26 @@ +/* + * (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; + +public class SuppressibleTreePathScannerModifications { + + public static boolean shouldBypassSuppressions(VisitorState state) { + return state.errorProneOptions().isIgnoreSuppressionAnnotations(); + } +} diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressionUsageTree.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressionUsageTree.java new file mode 100644 index 00000000..bc64e31b --- /dev/null +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressionUsageTree.java @@ -0,0 +1,164 @@ +/* + * (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.ClassTree; +import com.sun.source.tree.CompilationUnitTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.ModifiersTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.VariableTree; +import com.sun.source.util.TreePath; +import com.sun.source.util.TreePathScanner; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.stream.Collectors; + +@SuppressWarnings("BadAssert") +public class SuppressionUsageTree { + private final Map> treeToSuppressions; + private final Map> usedSuppressions; + private final Map treeToPath; + + private SuppressionUsageTree(Map> treeToSuppressions, Map treeToPath) { + this.treeToSuppressions = Map.copyOf(treeToSuppressions); + this.treeToPath = Map.copyOf(treeToPath); + this.usedSuppressions = new ConcurrentHashMap<>(); + treeToSuppressions.keySet().forEach(tree -> usedSuppressions.put(tree, ConcurrentHashMap.newKeySet())); + } + + public static SuppressionUsageTree constructSuppressions(CompilationUnitTree tree, VisitorState state) { + Map> treeToSuppressions = new HashMap<>(); + Map treeToPath = new HashMap<>(); + + new TreePathScanner() { + @Override + public Void scan(Tree tree, Void p) { + if (tree != null) { + treeToPath.put(tree, new TreePath(getCurrentPath(), tree)); + } + return super.scan(tree, p); + } + + @Override + public Void visitMethod(MethodTree node, Void p) { + collectSuppressions(node, node.getModifiers()); + return super.visitMethod(node, p); + } + + @Override + public Void visitClass(ClassTree node, Void p) { + collectSuppressions(node, node.getModifiers()); + return super.visitClass(node, p); + } + + @Override + public Void visitVariable(VariableTree node, Void p) { + collectSuppressions(node, node.getModifiers()); + return super.visitVariable(node, p); + } + + private void collectSuppressions(Tree tree, ModifiersTree modifiers) { + Set suppressions = new HashSet<>(); + for (AnnotationTree annotation : modifiers.getAnnotations()) { + if (isSuppressWarningsAnnotation(annotation)) { + AnnotationUtils.annotationStringValues(annotation).forEach(suppressions::add); + } + } + if (!suppressions.isEmpty()) { + treeToSuppressions.put(tree, suppressions); + } + } + + private boolean isSuppressWarningsAnnotation(AnnotationTree annotation) { + return AnnotationUtils.annotationName(annotation.getAnnotationType()) + .contentEquals("SuppressWarnings"); + } + }.scan(new TreePath(tree), null); + + return new SuppressionUsageTree(treeToSuppressions, treeToPath); + } + + public Set allSuppressionNames() { + return treeToSuppressions.values().stream().flatMap(Set::stream).collect(Collectors.toSet()); + } + + public void flagFirstParentSuppressionAsUsed(Tree tree, String suppressionName) { + System.err.println("========================================"); + System.err.println("Flagging suppressions as used: " + suppressionName); + System.err.println("tree: " + tree); + TreePath treePath = treeToPath.get(tree); + if (treePath == null) { + System.err.println("No TreePath found for tree - tree not in our suppression map"); + System.err.println("========================================\n"); + return; // Tree not found in our map + } + System.err.println("leaf of path: " + treePath.getLeaf()); + assert treePath.getLeaf().equals(tree); + + for (TreePath path = treePath; path != null; path = path.getParentPath()) { + Tree curr = path.getLeaf(); + Set suppressions = treeToSuppressions.get(curr); + if (suppressions != null && suppressions.contains(suppressionName)) { + usedSuppressions.get(curr).add(suppressionName); + System.err.println("Flagged suppression '" + suppressionName + "' as used: " + curr); + System.err.println("========================================\n"); + + return; + } + } + + System.err.println("No parent suppression found for '" + suppressionName + "' - walked entire parent chain"); + System.err.println("========================================\n"); + } + + public void markAllSuppressionsAsUsed(String suppressionName) { + treeToSuppressions.entrySet().stream() + .filter(entry -> entry.getValue().contains(suppressionName)) + .forEach(entry -> usedSuppressions.get(entry.getKey()).add(suppressionName)); + } + + public Set unusedSuppressions() { + return treeToSuppressions.entrySet().stream() + .map(entry -> { + Tree tree = entry.getKey(); + Set allSuppressions = entry.getValue(); + Set used = usedSuppressions.get(tree); + Set unused = allSuppressions.stream() + .filter(s -> !used.contains(s)) + .collect(Collectors.toSet()); + return unused.isEmpty() ? null : new TreeWithUnusedSuppressions(tree, unused); + }) + .filter(Objects::nonNull) + .collect(Collectors.toSet()); + } + + public record TreeWithUnusedSuppressions(Tree tree, Set unusedSuppressions) { + public TreeWithUnusedSuppressions { + if (!(tree instanceof MethodTree || tree instanceof ClassTree || tree instanceof VariableTree)) { + throw new IllegalArgumentException("Tree must be MethodTree, ClassTree, or VariableTree"); + } + unusedSuppressions = Set.copyOf(unusedSuppressions); + } + } +} From 193a715df3d2ec59b0a2a0bcbd7399ff2480f466 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 24 Sep 2025 19:35:42 +0100 Subject: [PATCH 03/12] dramatic lil bro --- .../SuppressibleErrorPronePluginIntegrationTest.groovy | 2 +- .../com/palantir/suppressibleerrorprone/AnnotationUtils.java | 5 ----- 2 files changed, 1 insertion(+), 6 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 49cbf10f..7dfe3014 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 @@ -1308,7 +1308,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec !output.contains('[RemoveRolloutSuppressions]') } - def 'errorProneRemoveUnused removes unused suppressions, and only unused suppressions'() { + def 'errorProneRemoveUnused removes only unused suppressions'() { // language=Java writeJavaSourceFileToSourceSets ''' package app; diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AnnotationUtils.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AnnotationUtils.java index 271de804..12f7ef9b 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AnnotationUtils.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AnnotationUtils.java @@ -24,7 +24,6 @@ import com.sun.source.tree.MemberSelectTree; import com.sun.source.tree.NewArrayTree; import com.sun.source.tree.Tree; -import com.sun.source.util.TreePath; import java.util.stream.Stream; import javax.lang.model.element.Name; @@ -68,9 +67,5 @@ static Name annotationName(Tree annotationType) { "Unsupported annotation type: " + annotationType.getClass().getCanonicalName()); } - static Tree getAnnotatedTree(TreePath pathToAnnotationTree) { - return pathToAnnotationTree.getParentPath().getParentPath().getLeaf(); - } - private AnnotationUtils() {} } From 13e34d135e19249f6652f7c6424077c95bd1f164 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 24 Sep 2025 20:43:28 +0100 Subject: [PATCH 04/12] clean stuff up --- .../AnnotationUtils.java | 4 + .../BugCheckerRegistry.java | 75 ++++++ .../CheckerRegistry.java | 130 ----------- .../LineRemovingReplacementFix.java | 93 ++++++++ .../RemoveRolloutSuppressions.java | 75 ------ .../RemoveUnusedSuppressions.java | 218 +++++------------- ...eTree.java => UnusedSuppressionsTree.java} | 23 +- 7 files changed, 238 insertions(+), 380 deletions(-) create mode 100644 suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/BugCheckerRegistry.java delete mode 100644 suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/CheckerRegistry.java create mode 100644 suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/LineRemovingReplacementFix.java rename suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/{SuppressionUsageTree.java => UnusedSuppressionsTree.java} (88%) diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AnnotationUtils.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AnnotationUtils.java index 12f7ef9b..5026aade 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AnnotationUtils.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AnnotationUtils.java @@ -67,5 +67,9 @@ static Name annotationName(Tree annotationType) { "Unsupported annotation type: " + annotationType.getClass().getCanonicalName()); } + static boolean isSuppressWarningsAnnotation(AnnotationTree annotation) { + return AnnotationUtils.annotationName(annotation.getAnnotationType()).contentEquals("SuppressWarnings"); + } + private AnnotationUtils() {} } diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/BugCheckerRegistry.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/BugCheckerRegistry.java new file mode 100644 index 00000000..5f6543a5 --- /dev/null +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/BugCheckerRegistry.java @@ -0,0 +1,75 @@ +/* + * (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.common.collect.ImmutableMap; +import com.google.errorprone.BugCheckerInfo; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.ErrorProneFlags; +import com.google.errorprone.ErrorPronePlugins; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.scanner.BuiltInCheckerSuppliers; +import com.google.errorprone.scanner.ErrorProneInjector; +import com.google.errorprone.scanner.ScannerSupplier; +import java.util.Map; +import java.util.Optional; +import java.util.stream.Collectors; + +/** + * Registry that maps checker names to their BugChecker instances. + * This includes both built-in Error Prone checkers and custom plugin checkers. + */ +public final class BugCheckerRegistry { + private final Map checkersByName; + + private BugCheckerRegistry(Map checkersByName) { + this.checkersByName = ImmutableMap.copyOf(checkersByName); + } + + /** + * Creates a BugCheckerRegistry from enabled checkers only. + */ + public static BugCheckerRegistry constructFromEnabledCheckers(VisitorState state) { + ScannerSupplier defaultBugCheckers = BuiltInCheckerSuppliers.defaultChecks(); + ScannerSupplier defaultAndPluginBugCheckers = ErrorPronePlugins.loadPlugins(defaultBugCheckers, state.context); + + // Use a injector with empty flags, similar to ScannerSupplierImpl + ErrorProneInjector injector = + ErrorProneInjector.create().addBinding(ErrorProneFlags.class, ErrorProneFlags.empty()); + Map severityMap = state.severityMap(); + + Map enabledBugCheckers = defaultAndPluginBugCheckers.getAllChecks().values().stream() + .filter(info -> info.severity(severityMap) != SeverityLevel.SUGGESTION) + .collect(Collectors.toMap( + BugCheckerInfo::canonicalName, + info -> injector.getInstance(info.checkerClass()) + )); + + return new BugCheckerRegistry(enabledBugCheckers); + } + + /** + * Gets the BugChecker instance for the given checker name. + * + * @param checkerName the name of the checker (canonical name or alternative name) + * @return Optional containing the BugChecker if found, empty otherwise + */ + public Optional get(String checkerName) { + return Optional.ofNullable(checkersByName.get(checkerName)); + } +} diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/CheckerRegistry.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/CheckerRegistry.java deleted file mode 100644 index 2bd69673..00000000 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/CheckerRegistry.java +++ /dev/null @@ -1,130 +0,0 @@ -/* - * (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.common.collect.ImmutableMap; -import com.google.errorprone.BugCheckerInfo; -import com.google.errorprone.BugPattern.SeverityLevel; -import com.google.errorprone.ErrorProneFlags; -import com.google.errorprone.ErrorPronePlugins; -import com.google.errorprone.VisitorState; -import com.google.errorprone.bugpatterns.BugChecker; -import com.google.errorprone.scanner.BuiltInCheckerSuppliers; -import com.google.errorprone.scanner.ErrorProneInjector; -import com.google.errorprone.scanner.ScannerSupplier; -import com.sun.tools.javac.util.Context; -import java.util.HashMap; -import java.util.Map; -import java.util.Optional; -import java.util.logging.Level; -import java.util.logging.Logger; - -/** - * Registry that maps checker names to their BugChecker instances. - * This includes both built-in Error Prone checkers and custom plugin checkers. - */ -public final class CheckerRegistry { - - private static final Logger logger = Logger.getLogger(CheckerRegistry.class.getName()); - - // Cache of checker name to BugChecker instance - private final Map checkersByName; - - private CheckerRegistry(Map checkersByName) { - this.checkersByName = ImmutableMap.copyOf(checkersByName); - } - - /** - * Creates a CheckerRegistry from enabled checkers only. - * This is more efficient if you only need to check suppressions for active checkers. - */ - public static CheckerRegistry createFromEnabledCheckers(VisitorState state) { - Context context = state.context; - - // Get only enabled checkers (errors and warnings) - ScannerSupplier enabledSupplier = BuiltInCheckerSuppliers.defaultChecks(); - - // Load plugin checkers and filter to enabled ones - ScannerSupplier allSuppliers = ErrorPronePlugins.loadPlugins(enabledSupplier, context); - - // Build the registry with only enabled checkers - Map checkersByName = new HashMap<>(); - - // Get the severity map to check if checkers are enabled - Map severityMap = state.severityMap(); - - for (BugCheckerInfo info : allSuppliers.getAllChecks().values()) { - // Check if this checker is enabled (not OFF) - com.google.errorprone.BugPattern.SeverityLevel severity = info.severity(severityMap); - - if (severity != SeverityLevel.SUGGESTION) { - try { - BugChecker checker = instantiateChecker(info.checkerClass()); - - // Register by canonical name - checkersByName.put(info.canonicalName(), checker); - - // Register by all alternative names - for (String name : info.allNames()) { - checkersByName.put(name, checker); - } - - } catch (Exception e) { - logger.log(Level.WARNING, "Failed to instantiate checker: " + info.canonicalName(), e); - } - } - } - - System.err.println("Initialized checkers: " + checkersByName.keySet()); - - return new CheckerRegistry(checkersByName); - } - - /** - * Gets the BugChecker instance for the given checker name. - * - * @param checkerName the name of the checker (canonical name or alternative name) - * @return Optional containing the BugChecker if found, empty otherwise - */ - public Optional getCheckerForName(String checkerName) { - return Optional.ofNullable(checkersByName.get(checkerName)); - } - - /** - * Convenience method for the RemoveUnusedSuppressions class. - */ - public static BugChecker getCheckerForSuppression(String suppression, VisitorState state) { - // Create registry lazily - you might want to cache this per compilation unit - CheckerRegistry registry = createFromEnabledCheckers(state); - return registry.getCheckerForName(suppression).orElse(null); - } - - private static BugChecker instantiateChecker(Class checkerClass) { - // Create an injector with empty flags, similar to ScannerSupplierImpl - ErrorProneInjector injector = - ErrorProneInjector.create().addBinding(ErrorProneFlags.class, ErrorProneFlags.empty()); - - return injector.getInstance(checkerClass); - } - - /** - * Returns the number of registered checkers. - */ - public int size() { - return checkersByName.size(); - } -} diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/LineRemovingReplacementFix.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/LineRemovingReplacementFix.java new file mode 100644 index 00000000..9d5eb445 --- /dev/null +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/LineRemovingReplacementFix.java @@ -0,0 +1,93 @@ +/* + * (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.common.collect.ImmutableSet; +import com.google.errorprone.fixes.Fix; +import com.google.errorprone.fixes.Replacement; +import com.google.errorprone.fixes.Replacements.CoalescePolicy; +import com.sun.tools.javac.tree.EndPosTable; +import com.sun.tools.javac.tree.JCTree.JCCompilationUnit; +import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition; + +/** + * This class has been introduced because the normal {@link com.google.errorprone.fixes.SuggestedFix} does not + * allow us to introduce a Replacement which has a different start position than the tree's normal start position. + * Here we want to: + * - replace the element defined by the provided position + * - also replace the whitespace before the element, up to and including the newline + * This way, if e.g. @SuppressWarnings("foo") must be removed entirely, we can remove the entire line, rather than + * just the annotation, leaving us with an empty line. + * + * Note that this will only delete the whitespace before the element if the entire element is removed + * (i.e. if the replacement text is null or empty). + */ +public final class LineRemovingReplacementFix implements Fix { + private final CharSequence sourceCode; + private final DiagnosticPosition position; + private final String replacementText; + + LineRemovingReplacementFix(CharSequence sourceCode, DiagnosticPosition position, String replacementText) { + this.sourceCode = sourceCode; + this.position = position; + this.replacementText = replacementText; + } + + @Override + public String toString(JCCompilationUnit compilationUnit) { + return "LineRemovingReplacementFix"; + } + + @Override + public String getShortDescription() { + return "Replace text at the position with the provided text, " + + "or remove the text and all preceding whitespace"; + } + + @Override + public CoalescePolicy getCoalescePolicy() { + return CoalescePolicy.REJECT; + } + + @Override + public ImmutableSet getReplacements(EndPosTable endPositions) { + // If we are looking to delete the entire element, we should also remove whitespace before it, + // up to and including the newline + if (replacementText.isEmpty() && sourceCode != null) { + int start = SourceCodeUtils.startPositionWithWhitespaceIncludingNewLine( + sourceCode, position.getStartPosition()); + return ImmutableSet.of(Replacement.create(start, position.getEndPosition(endPositions), "")); + } + return ImmutableSet.of(Replacement.create( + position.getStartPosition(), position.getEndPosition(endPositions), replacementText)); + } + + @Override + public ImmutableSet getImportsToAdd() { + return ImmutableSet.of(); + } + + @Override + public ImmutableSet getImportsToRemove() { + return ImmutableSet.of(); + } + + @Override + public boolean isEmpty() { + return false; + } +} diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveRolloutSuppressions.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveRolloutSuppressions.java index 41fa2e8a..2ffc8ce0 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveRolloutSuppressions.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveRolloutSuppressions.java @@ -17,17 +17,11 @@ package com.palantir.suppressibleerrorprone; import com.google.auto.service.AutoService; -import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; -import com.google.errorprone.fixes.Fix; -import com.google.errorprone.fixes.Replacement; -import com.google.errorprone.fixes.Replacements.CoalescePolicy; import com.google.errorprone.matchers.Description; import com.sun.source.tree.AnnotationTree; -import com.sun.tools.javac.tree.EndPosTable; -import com.sun.tools.javac.tree.JCTree.JCCompilationUnit; import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition; import java.util.List; import java.util.Set; @@ -93,73 +87,4 @@ public Description matchAnnotation(AnnotationTree tree, VisitorState state) { .addFix(new LineRemovingReplacementFix(state.getSourceCode(), (DiagnosticPosition) tree, updatedText)) .build(); } - - /** - * This class has been introduced because the normal {@link com.google.errorprone.fixes.SuggestedFix} does not - * allow us to introduce a Replacement which has a different start position than the tree's normal start position. - * Here we want to: - * - replace the element defined by the provided position - * - also replace the whitespace before the element, up to and including the newline - * This way, if e.g. @SuppressWarnings("foo") must be removed entirely, we can remove the entire line, rather than - * just the annotation, leaving us with an empty line. - * - * Note that this will only delete the whitespace before the element if the entire element is removed - * (i.e. if the replacement text is null or empty). - */ - private static final class LineRemovingReplacementFix implements Fix { - private final CharSequence sourceCode; - private final DiagnosticPosition position; - private final String replacementText; - - private LineRemovingReplacementFix( - CharSequence sourceCode, DiagnosticPosition position, String replacementText) { - this.sourceCode = sourceCode; - this.position = position; - this.replacementText = replacementText; - } - - @Override - public String toString(JCCompilationUnit compilationUnit) { - return "LineRemovingReplacementFix"; - } - - @Override - public String getShortDescription() { - return "Replace text at the position with the provided text, " - + "or remove the text and all preceding whitespace"; - } - - @Override - public CoalescePolicy getCoalescePolicy() { - return CoalescePolicy.REJECT; - } - - @Override - public ImmutableSet getReplacements(EndPosTable endPositions) { - // If we are looking to delete the entire element, we should also remove whitespace before it, - // up to and including the newline - if (replacementText.isEmpty() && sourceCode != null) { - int start = SourceCodeUtils.startPositionWithWhitespaceIncludingNewLine( - sourceCode, position.getStartPosition()); - return ImmutableSet.of(Replacement.create(start, position.getEndPosition(endPositions), "")); - } - return ImmutableSet.of(Replacement.create( - position.getStartPosition(), position.getEndPosition(endPositions), replacementText)); - } - - @Override - public ImmutableSet getImportsToAdd() { - return ImmutableSet.of(); - } - - @Override - public ImmutableSet getImportsToRemove() { - return ImmutableSet.of(); - } - - @Override - public boolean isEmpty() { - return false; - } - } } diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveUnusedSuppressions.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveUnusedSuppressions.java index b43474b7..0870ee87 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveUnusedSuppressions.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveUnusedSuppressions.java @@ -17,16 +17,14 @@ package com.palantir.suppressibleerrorprone; import com.google.auto.service.AutoService; -import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern; import com.google.errorprone.ErrorProneOptions; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.fixes.Fix; -import com.google.errorprone.fixes.Replacement; -import com.google.errorprone.fixes.Replacements.CoalescePolicy; import com.google.errorprone.matchers.Description; -import com.palantir.suppressibleerrorprone.SuppressionUsageTree.TreeWithUnusedSuppressions; +import com.google.errorprone.suppliers.Supplier; +import com.palantir.suppressibleerrorprone.UnusedSuppressionsTree.TreeWithUnusedSuppressions; import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.ArrayAccessTree; import com.sun.source.tree.ArrayTypeTree; @@ -73,8 +71,6 @@ import com.sun.source.tree.VariableTree; import com.sun.source.tree.WhileLoopTree; import com.sun.source.util.TreeScanner; -import com.sun.tools.javac.tree.EndPosTable; -import com.sun.tools.javac.tree.JCTree.JCCompilationUnit; import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition; import java.util.ArrayList; import java.util.List; @@ -97,51 +93,47 @@ summary = "Remove unused @SuppressWarnings annotations") @SuppressWarnings("TreeToString") public final class RemoveUnusedSuppressions extends BugChecker implements BugChecker.CompilationUnitTreeMatcher { - private static final ThreadLocal SUPPRESSION_TREE = new ThreadLocal<>(); - - // Cache the registry per compilation to avoid repeated instantiation - private volatile CheckerRegistry cachedRegistry; + private static final Supplier enabledBugCheckers = + VisitorState.memoize(BugCheckerRegistry::constructFromEnabledCheckers); @Override public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) { - // build SuppressionUsageTree - SUPPRESSION_TREE.set(SuppressionUsageTree.constructSuppressions(tree, state)); - SuppressionUsageTree suppressionUsageTree = SUPPRESSION_TREE.get(); - System.err.println("All suppressions found in compilation unit: " + suppressionUsageTree.allSuppressionNames()); + UnusedSuppressionsTree unusedSuppressionsTree = UnusedSuppressionsTree.initializeWithSuppressions(tree); - if (suppressionUsageTree.allSuppressionNames().isEmpty()) { + if (unusedSuppressionsTree.allSuppressionNames().isEmpty()) { return Description.NO_MATCH; } - // for each suppressed bugchecker, scan the whole compilation unit, bypassing any suppressions. - for (String suppression : suppressionUsageTree.allSuppressionNames()) { - Optional bugCheckerMaybe = getBugCheckerForSuppression(suppression, state); + for (String suppression : unusedSuppressionsTree.allSuppressionNames()) { + Optional bugCheckerMaybe = enabledBugCheckers.get(state).get(suppression); if (bugCheckerMaybe.isEmpty()) { System.err.println(suppression + ": not found in registry, so we conservatively assume they are used"); - suppressionUsageTree.markAllSuppressionsAsUsed(suppression); + unusedSuppressionsTree.markAllSuppressionsAsUsed(suppression); continue; } - BugChecker bugChecker = bugCheckerMaybe.get(); + // customState uses the same compilation context and severity map (which tells us which bugcheckers are + // enabled) as the main compilation, but with two tweaks: + // 1. The DescriptionListener usually takes your reported Descriptions and reports them to javac and makes + // changes to source. We use a custom listener which does none of that, and just reports any Descriptions to + // unusedSuppressionTree + // 2. Turn on XepIgnoreSuppressionAnnotations in ErrorProneOptions. VisitorState customState = VisitorState.createConfiguredForCompilation( state.context, (description) -> { if (description != Description.NO_MATCH) { - SUPPRESSION_TREE - .get() - .flagFirstParentSuppressionAsUsed( - description.position.getTree(), description.checkName); + unusedSuppressionsTree.flagFirstParentSuppressionAsUsed( + description.position.getTree(), description.checkName); } }, state.severityMap(), ignoreSuppressions(state.errorProneOptions())) .withPath(state.getPath()); System.err.println(suppression + ": scanning to find usages"); - - new SuppressionCheckingScanner(bugChecker, suppressionUsageTree).scan(tree, customState); + new SuppressionCheckingScanner(bugCheckerMaybe.get()).scan(tree, customState); } - for (TreeWithUnusedSuppressions treeWithUnusedSuppressions : suppressionUsageTree.unusedSuppressions()) { + for (TreeWithUnusedSuppressions treeWithUnusedSuppressions : unusedSuppressionsTree.unused()) { Set unusedSuppressions = treeWithUnusedSuppressions.unusedSuppressions(); System.err.println("========================================"); System.err.println("Unused suppressions found in tree : " + unusedSuppressions); @@ -149,33 +141,41 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s System.err.println("========================================\n"); // Get modifiers tree based on tree type - ModifiersTree modifiers; - Tree declarationTree = treeWithUnusedSuppressions.tree(); - if (declarationTree instanceof MethodTree methodTree) { - modifiers = methodTree.getModifiers(); - } else if (declarationTree instanceof ClassTree classTree) { - modifiers = classTree.getModifiers(); - } else if (declarationTree instanceof VariableTree variableTree) { - modifiers = variableTree.getModifiers(); - } else { - throw new IllegalStateException("Unexpected tree type: " + declarationTree.getClass()); - } + List suppressions = + getModifiersTree(treeWithUnusedSuppressions).getAnnotations().stream() + .filter(AnnotationUtils::isSuppressWarningsAnnotation) + .toList(); // Find @SuppressWarnings annotation - for (AnnotationTree annotation : modifiers.getAnnotations()) { - if (isSuppressWarningsAnnotation(annotation)) { - Fix fix = createSuppressionFix(annotation, unusedSuppressions, state); - state.reportMatch(buildDescription(tree) - .setMessage("Remove unused @SuppressWarnings: " + unusedSuppressions) - .addFix(fix) - .build()); - } + for (AnnotationTree suppression : suppressions) { + Fix fix = createSuppressionFix(suppression, unusedSuppressions, state); + state.reportMatch(buildDescription(tree) + .setMessage("Remove unused @SuppressWarnings: " + unusedSuppressions) + .addFix(fix) + .build()); } } return Description.NO_MATCH; } + private static ModifiersTree getModifiersTree(TreeWithUnusedSuppressions treeWithUnusedSuppressions) { + ModifiersTree modifiers; + Tree declarationTree = treeWithUnusedSuppressions.tree(); + if (declarationTree instanceof MethodTree methodTree) { + modifiers = methodTree.getModifiers(); + } else if (declarationTree instanceof ClassTree classTree) { + modifiers = classTree.getModifiers(); + } else if (declarationTree instanceof VariableTree variableTree) { + modifiers = variableTree.getModifiers(); + } else { + throw new IllegalStateException("Unexpected tree type: " + declarationTree.getClass()); + } + return modifiers; + } + + // Annoyingly, we have to construct a fresh ErrorProneOptions and copy the rest of the flags manually, + // before turning on XepIgnoreSuppressionAnnotations. This is so fragile :| private static ErrorProneOptions ignoreSuppressions(ErrorProneOptions originalOptions) { List args = new ArrayList<>(); args.add("-XepIgnoreSuppressionAnnotations"); @@ -222,29 +222,12 @@ private static ErrorProneOptions ignoreSuppressions(ErrorProneOptions originalOp return ErrorProneOptions.processArgs(args); } - /** - * Gets the BugChecker associated with a suppression name using the registry. - */ - private Optional getBugCheckerForSuppression(String suppression, VisitorState state) { - // Use cached registry or create new one - if (cachedRegistry == null) { - synchronized (this) { - if (cachedRegistry == null) { - cachedRegistry = CheckerRegistry.createFromEnabledCheckers(state); - } - } - } - return cachedRegistry.getCheckerForName(suppression); - } - private class SuppressionCheckingScanner extends TreeScanner { - private final SuppressionUsageTree suppressionUsageTree; private final BugChecker checker; - public SuppressionCheckingScanner(BugChecker checker, SuppressionUsageTree suppressionUsageTree) { + public SuppressionCheckingScanner(BugChecker checker) { super(); this.checker = checker; - this.suppressionUsageTree = suppressionUsageTree; } @Override @@ -254,7 +237,6 @@ public Void scan(Tree tree, VisitorState state) { } VisitorState newState = state.withPath(state.getPath()); - Description description = checkTreeAgainstChecker(tree, newState); state.reportMatch(description); @@ -416,111 +398,15 @@ private Description checkTreeAgainstChecker(Tree tree, VisitorState state) { } } - /** - * Hook to be called from VisitorState.reportMatch to track when matches are reported. - */ - public static void onMatchReported(Description description) { - System.err.println("reportMatch called"); - - if (description != Description.NO_MATCH) { - SUPPRESSION_TREE - .get() - .flagFirstParentSuppressionAsUsed(description.position.getTree(), description.checkName); - } - } - - private static boolean isSuppressWarningsAnnotation(AnnotationTree annotation) { - return AnnotationUtils.annotationName(annotation.getAnnotationType()).contentEquals("SuppressWarnings"); - } - private static Fix createSuppressionFix( - AnnotationTree annotation, Set unusedSuppressions, VisitorState state) { - // Get current suppression values using existing utility + AnnotationTree suppressWarnings, Set unusedSuppressions, VisitorState state) { List currentSuppressions = - AnnotationUtils.annotationStringValues(annotation).toList(); - - // Remove unused suppressions + AnnotationUtils.annotationStringValues(suppressWarnings).toList(); List remainingSuppressions = currentSuppressions.stream() .filter(s -> !unusedSuppressions.contains(s)) .collect(Collectors.toList()); - - if (remainingSuppressions.isEmpty()) { - // Remove entire annotation - return new LineRemovingReplacementFix(state.getSourceCode(), (DiagnosticPosition) annotation, ""); - } else { - // Update annotation with remaining suppressions - String newAnnotation = buildSuppressWarningsAnnotation(remainingSuppressions); - return new LineRemovingReplacementFix( - state.getSourceCode(), (DiagnosticPosition) annotation, newAnnotation); - } - } - - private static String buildSuppressWarningsAnnotation(List suppressions) { - if (suppressions.size() == 1) { - return "@SuppressWarnings(\"" + suppressions.get(0) + "\")"; - } else { - String values = suppressions.stream().map(s -> "\"" + s + "\"").collect(Collectors.joining(", ")); - return "@SuppressWarnings({" + values + "})"; - } - } - - /** - * This class handles replacement with optional line removal for empty suppressions. - */ - private static final class LineRemovingReplacementFix implements Fix { - private final CharSequence sourceCode; - private final DiagnosticPosition position; - private final String replacementText; - - private LineRemovingReplacementFix( - CharSequence sourceCode, DiagnosticPosition position, String replacementText) { - this.sourceCode = sourceCode; - this.position = position; - this.replacementText = replacementText; - } - - @Override - public String toString(JCCompilationUnit compilationUnit) { - return "LineRemovingReplacementFix"; - } - - @Override - public String getShortDescription() { - return "Replace text at the position with the provided text, " - + "or remove the text and all preceding whitespace"; - } - - @Override - public CoalescePolicy getCoalescePolicy() { - return CoalescePolicy.REJECT; - } - - @Override - public ImmutableSet getReplacements(EndPosTable endPositions) { - // If we are looking to delete the entire element, we should also remove whitespace before it, - // up to and including the newline - if (replacementText.isEmpty() && sourceCode != null) { - int start = SourceCodeUtils.startPositionWithWhitespaceIncludingNewLine( - sourceCode, position.getStartPosition()); - return ImmutableSet.of(Replacement.create(start, position.getEndPosition(endPositions), "")); - } - return ImmutableSet.of(Replacement.create( - position.getStartPosition(), position.getEndPosition(endPositions), replacementText)); - } - - @Override - public ImmutableSet getImportsToAdd() { - return ImmutableSet.of(); - } - - @Override - public ImmutableSet getImportsToRemove() { - return ImmutableSet.of(); - } - - @Override - public boolean isEmpty() { - return false; - } + String newSuppressWarnings = SuppressWarningsUtils.suppressWarningsString(remainingSuppressions); + return new LineRemovingReplacementFix( + state.getSourceCode(), (DiagnosticPosition) suppressWarnings, newSuppressWarnings); } } diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressionUsageTree.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/UnusedSuppressionsTree.java similarity index 88% rename from suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressionUsageTree.java rename to suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/UnusedSuppressionsTree.java index bc64e31b..8bcd2438 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressionUsageTree.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/UnusedSuppressionsTree.java @@ -16,7 +16,6 @@ package com.palantir.suppressibleerrorprone; -import com.google.errorprone.VisitorState; import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.ClassTree; import com.sun.source.tree.CompilationUnitTree; @@ -35,19 +34,19 @@ import java.util.stream.Collectors; @SuppressWarnings("BadAssert") -public class SuppressionUsageTree { +public class UnusedSuppressionsTree { private final Map> treeToSuppressions; private final Map> usedSuppressions; private final Map treeToPath; - private SuppressionUsageTree(Map> treeToSuppressions, Map treeToPath) { + private UnusedSuppressionsTree(Map> treeToSuppressions, Map treeToPath) { this.treeToSuppressions = Map.copyOf(treeToSuppressions); this.treeToPath = Map.copyOf(treeToPath); - this.usedSuppressions = new ConcurrentHashMap<>(); + this.usedSuppressions = new HashMap<>(); treeToSuppressions.keySet().forEach(tree -> usedSuppressions.put(tree, ConcurrentHashMap.newKeySet())); } - public static SuppressionUsageTree constructSuppressions(CompilationUnitTree tree, VisitorState state) { + public static UnusedSuppressionsTree initializeWithSuppressions(CompilationUnitTree tree) { Map> treeToSuppressions = new HashMap<>(); Map treeToPath = new HashMap<>(); @@ -96,15 +95,22 @@ private boolean isSuppressWarningsAnnotation(AnnotationTree annotation) { } }.scan(new TreePath(tree), null); - return new SuppressionUsageTree(treeToSuppressions, treeToPath); + return new UnusedSuppressionsTree(treeToSuppressions, treeToPath); } public Set allSuppressionNames() { return treeToSuppressions.values().stream().flatMap(Set::stream).collect(Collectors.toSet()); } + /** + * Starting from {@code tree}, look for the first tree along the path which has a suppression on + * {@code suppressionName}, and mark that suppression as used. + * + * This method is forced to take in a {@code Tree} rather than a {@code TreePath}, because it is called from + * {@code description.position.getTree()}. To avoid doing a tree walk, we cache the tree->path mapping during + * construction. + */ public void flagFirstParentSuppressionAsUsed(Tree tree, String suppressionName) { - System.err.println("========================================"); System.err.println("Flagging suppressions as used: " + suppressionName); System.err.println("tree: " + tree); TreePath treePath = treeToPath.get(tree); @@ -114,7 +120,6 @@ public void flagFirstParentSuppressionAsUsed(Tree tree, String suppressionName) return; // Tree not found in our map } System.err.println("leaf of path: " + treePath.getLeaf()); - assert treePath.getLeaf().equals(tree); for (TreePath path = treePath; path != null; path = path.getParentPath()) { Tree curr = path.getLeaf(); @@ -138,7 +143,7 @@ public void markAllSuppressionsAsUsed(String suppressionName) { .forEach(entry -> usedSuppressions.get(entry.getKey()).add(suppressionName)); } - public Set unusedSuppressions() { + public Set unused() { return treeToSuppressions.entrySet().stream() .map(entry -> { Tree tree = entry.getKey(); From 88c93b19793a99a0fb58dbd90c3e3b1905995261 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 24 Sep 2025 20:44:58 +0100 Subject: [PATCH 05/12] terrible naming!! --- .../SuppressibleErrorPronePluginIntegrationTest.groovy | 2 +- 1 file changed, 1 insertion(+), 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 7dfe3014..2996e0ee 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 @@ -1340,7 +1340,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec '''.stripIndent(true) } - def 'errorProneRemoveUnused only removes suppressions not directly connected to a report'() { + def 'errorProneRemoveUnused only removes the first suppression in the path of a violation'() { // language=Java writeJavaSourceFileToSourceSets ''' package app; From 6bb24c8c62ff8546ba6a47f54cc17f2c6aedc2a3 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 24 Sep 2025 20:58:43 +0100 Subject: [PATCH 06/12] also prevent interferences --- .../suppressibleerrorprone/modes/Modes.java | 2 + .../RemoveUnusedModeInterference.java | 46 +++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedModeInterference.java diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/Modes.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/Modes.java index 9ebebb2b..6ab78eca 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/Modes.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/Modes.java @@ -28,6 +28,7 @@ import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption; import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.CombinedValue; import com.palantir.gradle.suppressibleerrorprone.modes.interferences.DisableModeInterference; +import com.palantir.gradle.suppressibleerrorprone.modes.interferences.RemoveUnusedModeInterference; import com.palantir.gradle.suppressibleerrorprone.modes.interferences.RemovingAndSuppressingInterference; import com.palantir.gradle.suppressibleerrorprone.modes.interferences.SuppressingAndApplyingInterference; import com.palantir.gradle.suppressibleerrorprone.modes.modes.ApplyMode; @@ -67,6 +68,7 @@ ModeName.SUPPRESS, new SuppressMode(), private final Set interferences = Set.of( new DisableModeInterference(), + new RemoveUnusedModeInterference(), new RemovingAndSuppressingInterference(), new SuppressingAndApplyingInterference()); diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedModeInterference.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedModeInterference.java new file mode 100644 index 00000000..595a9158 --- /dev/null +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedModeInterference.java @@ -0,0 +1,46 @@ +/* + * (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.gradle.suppressibleerrorprone.modes.interferences; + +import com.palantir.gradle.suppressibleerrorprone.modes.common.ModeInterference; +import com.palantir.gradle.suppressibleerrorprone.modes.common.ModeInterferenceResult; +import com.palantir.gradle.suppressibleerrorprone.modes.common.ModeName; +import java.util.Set; +import java.util.function.Predicate; +import java.util.stream.Collectors; + +/** + * The end goal is to run this with Apply, so we can bring in a new fix added to an existing errorprone in one + * compilation. This will be introduced in a future PR. For the time being, disallow any other commands to be run with + * RemoveUnused. + */ +public class RemoveUnusedModeInterference implements ModeInterference { + @Override + public ModeInterferenceResult interferesWith(Set modeNames) { + if (modeNames.contains(ModeName.REMOVE_UNUSED) && modeNames.size() > 1) { + return ModeInterferenceResult.notCompatible("%s cannot be used at the same time as any of %s" + .formatted( + ModeName.REMOVE_UNUSED.asGradlePropertyArgument(), + modeNames.stream() + .filter(Predicate.not(Predicate.isEqual(ModeName.REMOVE_UNUSED))) + .map(ModeName::asGradlePropertyArgument) + .collect(Collectors.joining(", ")))); + } + + return ModeInterferenceResult.noInterference(); + } +} From d4a4b3ff02efefe1143a764c64946542f3563355 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 24 Sep 2025 21:13:19 +0100 Subject: [PATCH 07/12] Remove my precious printlns --- .../RemoveUnusedModeInterference.java | 2 +- .../modes/modes/RemoveUnusedMode.java | 6 +- .../RemoveUnusedSuppressions.java | 227 +----------------- ...pressibleTreePathScannerModifications.java | 2 + .../UnusedSuppressionsTree.java | 27 +-- 5 files changed, 17 insertions(+), 247 deletions(-) diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedModeInterference.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedModeInterference.java index 595a9158..24206a39 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedModeInterference.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedModeInterference.java @@ -28,7 +28,7 @@ * compilation. This will be introduced in a future PR. For the time being, disallow any other commands to be run with * RemoveUnused. */ -public class RemoveUnusedModeInterference implements ModeInterference { +public final class RemoveUnusedModeInterference implements ModeInterference { @Override public ModeInterferenceResult interferesWith(Set modeNames) { if (modeNames.contains(ModeName.REMOVE_UNUSED) && modeNames.size() > 1) { diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/RemoveUnusedMode.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/RemoveUnusedMode.java index 48c29bf0..0d661e42 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/RemoveUnusedMode.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/RemoveUnusedMode.java @@ -23,7 +23,7 @@ import com.palantir.gradle.suppressibleerrorprone.modes.common.RemoveUnusedCheck; import java.util.Map; -public class RemoveUnusedMode implements Mode { +public final class RemoveUnusedMode implements Mode { private static final String ALL_CHECKS = ""; public ModifyCheckApiOption modifyCheckApi() { @@ -44,9 +44,7 @@ public Map extraErrorProneCheckOptions() { // We can't explicitly list all possible checks, because some might not exist anymore // The logic itself needs to consider an empty list as "remove all" - return Map.of( - "SuppressibleErrorProne:RemoveUnusedSuppressions", - context.flagValue().orElse(ALL_CHECKS)); + return Map.of("SuppressibleErrorProne:RemoveUnusedSuppressions", ALL_CHECKS); } @Override diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveUnusedSuppressions.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveUnusedSuppressions.java index 0870ee87..e47193ae 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveUnusedSuppressions.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveUnusedSuppressions.java @@ -23,54 +23,16 @@ import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.fixes.Fix; import com.google.errorprone.matchers.Description; +import com.google.errorprone.scanner.ErrorProneScanner; import com.google.errorprone.suppliers.Supplier; import com.palantir.suppressibleerrorprone.UnusedSuppressionsTree.TreeWithUnusedSuppressions; import com.sun.source.tree.AnnotationTree; -import com.sun.source.tree.ArrayAccessTree; -import com.sun.source.tree.ArrayTypeTree; -import com.sun.source.tree.AssertTree; -import com.sun.source.tree.AssignmentTree; -import com.sun.source.tree.BinaryTree; -import com.sun.source.tree.BlockTree; -import com.sun.source.tree.BreakTree; -import com.sun.source.tree.CaseTree; -import com.sun.source.tree.CatchTree; import com.sun.source.tree.ClassTree; import com.sun.source.tree.CompilationUnitTree; -import com.sun.source.tree.CompoundAssignmentTree; -import com.sun.source.tree.ConditionalExpressionTree; -import com.sun.source.tree.ContinueTree; -import com.sun.source.tree.DoWhileLoopTree; -import com.sun.source.tree.EmptyStatementTree; -import com.sun.source.tree.EnhancedForLoopTree; -import com.sun.source.tree.ExpressionStatementTree; -import com.sun.source.tree.ForLoopTree; -import com.sun.source.tree.IdentifierTree; -import com.sun.source.tree.IfTree; -import com.sun.source.tree.ImportTree; -import com.sun.source.tree.InstanceOfTree; -import com.sun.source.tree.LabeledStatementTree; -import com.sun.source.tree.LambdaExpressionTree; -import com.sun.source.tree.LiteralTree; -import com.sun.source.tree.MemberReferenceTree; -import com.sun.source.tree.MemberSelectTree; -import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.MethodTree; import com.sun.source.tree.ModifiersTree; -import com.sun.source.tree.NewArrayTree; -import com.sun.source.tree.NewClassTree; -import com.sun.source.tree.ParenthesizedTree; -import com.sun.source.tree.ReturnTree; -import com.sun.source.tree.SwitchTree; -import com.sun.source.tree.SynchronizedTree; -import com.sun.source.tree.ThrowTree; import com.sun.source.tree.Tree; -import com.sun.source.tree.TryTree; -import com.sun.source.tree.TypeCastTree; -import com.sun.source.tree.UnaryTree; import com.sun.source.tree.VariableTree; -import com.sun.source.tree.WhileLoopTree; -import com.sun.source.util.TreeScanner; import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition; import java.util.ArrayList; import java.util.List; @@ -107,7 +69,6 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s for (String suppression : unusedSuppressionsTree.allSuppressionNames()) { Optional bugCheckerMaybe = enabledBugCheckers.get(state).get(suppression); if (bugCheckerMaybe.isEmpty()) { - System.err.println(suppression + ": not found in registry, so we conservatively assume they are used"); unusedSuppressionsTree.markAllSuppressionsAsUsed(suppression); continue; } @@ -120,7 +81,7 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s // 2. Turn on XepIgnoreSuppressionAnnotations in ErrorProneOptions. VisitorState customState = VisitorState.createConfiguredForCompilation( state.context, - (description) -> { + description -> { if (description != Description.NO_MATCH) { unusedSuppressionsTree.flagFirstParentSuppressionAsUsed( description.position.getTree(), description.checkName); @@ -129,16 +90,11 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s state.severityMap(), ignoreSuppressions(state.errorProneOptions())) .withPath(state.getPath()); - System.err.println(suppression + ": scanning to find usages"); - new SuppressionCheckingScanner(bugCheckerMaybe.get()).scan(tree, customState); + new ErrorProneScanner(bugCheckerMaybe.get()).scan(tree, customState); } for (TreeWithUnusedSuppressions treeWithUnusedSuppressions : unusedSuppressionsTree.unused()) { Set unusedSuppressions = treeWithUnusedSuppressions.unusedSuppressions(); - System.err.println("========================================"); - System.err.println("Unused suppressions found in tree : " + unusedSuppressions); - System.err.println(treeWithUnusedSuppressions.tree()); - System.err.println("========================================\n"); // Get modifiers tree based on tree type List suppressions = @@ -176,6 +132,7 @@ private static ModifiersTree getModifiersTree(TreeWithUnusedSuppressions treeWit // Annoyingly, we have to construct a fresh ErrorProneOptions and copy the rest of the flags manually, // before turning on XepIgnoreSuppressionAnnotations. This is so fragile :| + @SuppressWarnings("CyclomaticComplexity") // mostly just copying options private static ErrorProneOptions ignoreSuppressions(ErrorProneOptions originalOptions) { List args = new ArrayList<>(); args.add("-XepIgnoreSuppressionAnnotations"); @@ -222,182 +179,6 @@ private static ErrorProneOptions ignoreSuppressions(ErrorProneOptions originalOp return ErrorProneOptions.processArgs(args); } - private class SuppressionCheckingScanner extends TreeScanner { - private final BugChecker checker; - - public SuppressionCheckingScanner(BugChecker checker) { - super(); - this.checker = checker; - } - - @Override - public Void scan(Tree tree, VisitorState state) { - if (tree == null) { - return null; - } - - VisitorState newState = state.withPath(state.getPath()); - Description description = checkTreeAgainstChecker(tree, newState); - state.reportMatch(description); - - return super.scan(tree, newState); - } - - /** - * Checks a tree against all the matcher interfaces implemented by the BugChecker. - */ - private Description checkTreeAgainstChecker(Tree tree, VisitorState state) { - // Check each matcher interface that the checker implements - if (checker instanceof BugChecker.AnnotationTreeMatcher && tree instanceof AnnotationTree) { - return ((BugChecker.AnnotationTreeMatcher) checker).matchAnnotation((AnnotationTree) tree, state); - } - if (checker instanceof BugChecker.ArrayAccessTreeMatcher && tree instanceof ArrayAccessTree) { - return ((BugChecker.ArrayAccessTreeMatcher) checker).matchArrayAccess((ArrayAccessTree) tree, state); - } - if (checker instanceof BugChecker.ArrayTypeTreeMatcher && tree instanceof ArrayTypeTree) { - return ((BugChecker.ArrayTypeTreeMatcher) checker).matchArrayType((ArrayTypeTree) tree, state); - } - if (checker instanceof BugChecker.AssertTreeMatcher && tree instanceof AssertTree) { - return ((BugChecker.AssertTreeMatcher) checker).matchAssert((AssertTree) tree, state); - } - if (checker instanceof BugChecker.AssignmentTreeMatcher && tree instanceof AssignmentTree) { - return ((BugChecker.AssignmentTreeMatcher) checker).matchAssignment((AssignmentTree) tree, state); - } - if (checker instanceof BugChecker.BinaryTreeMatcher && tree instanceof BinaryTree) { - return ((BugChecker.BinaryTreeMatcher) checker).matchBinary((BinaryTree) tree, state); - } - if (checker instanceof BugChecker.BlockTreeMatcher && tree instanceof BlockTree) { - return ((BugChecker.BlockTreeMatcher) checker).matchBlock((BlockTree) tree, state); - } - if (checker instanceof BugChecker.BreakTreeMatcher && tree instanceof BreakTree) { - return ((BugChecker.BreakTreeMatcher) checker).matchBreak((BreakTree) tree, state); - } - if (checker instanceof BugChecker.CaseTreeMatcher && tree instanceof CaseTree) { - return ((BugChecker.CaseTreeMatcher) checker).matchCase((CaseTree) tree, state); - } - if (checker instanceof BugChecker.CatchTreeMatcher && tree instanceof CatchTree) { - return ((BugChecker.CatchTreeMatcher) checker).matchCatch((CatchTree) tree, state); - } - if (checker instanceof BugChecker.ClassTreeMatcher && tree instanceof ClassTree) { - return ((BugChecker.ClassTreeMatcher) checker).matchClass((ClassTree) tree, state); - } - if (checker instanceof BugChecker.CompilationUnitTreeMatcher && tree instanceof CompilationUnitTree) { - return ((BugChecker.CompilationUnitTreeMatcher) checker) - .matchCompilationUnit((CompilationUnitTree) tree, state); - } - if (checker instanceof BugChecker.CompoundAssignmentTreeMatcher && tree instanceof CompoundAssignmentTree) { - return ((BugChecker.CompoundAssignmentTreeMatcher) checker) - .matchCompoundAssignment((CompoundAssignmentTree) tree, state); - } - if (checker instanceof BugChecker.ConditionalExpressionTreeMatcher - && tree instanceof ConditionalExpressionTree) { - return ((BugChecker.ConditionalExpressionTreeMatcher) checker) - .matchConditionalExpression((ConditionalExpressionTree) tree, state); - } - if (checker instanceof BugChecker.ContinueTreeMatcher && tree instanceof ContinueTree) { - return ((BugChecker.ContinueTreeMatcher) checker).matchContinue((ContinueTree) tree, state); - } - if (checker instanceof BugChecker.DoWhileLoopTreeMatcher && tree instanceof DoWhileLoopTree) { - return ((BugChecker.DoWhileLoopTreeMatcher) checker).matchDoWhileLoop((DoWhileLoopTree) tree, state); - } - if (checker instanceof BugChecker.EmptyStatementTreeMatcher && tree instanceof EmptyStatementTree) { - return ((BugChecker.EmptyStatementTreeMatcher) checker) - .matchEmptyStatement((EmptyStatementTree) tree, state); - } - if (checker instanceof BugChecker.EnhancedForLoopTreeMatcher && tree instanceof EnhancedForLoopTree) { - return ((BugChecker.EnhancedForLoopTreeMatcher) checker) - .matchEnhancedForLoop((EnhancedForLoopTree) tree, state); - } - if (checker instanceof BugChecker.ExpressionStatementTreeMatcher - && tree instanceof ExpressionStatementTree) { - return ((BugChecker.ExpressionStatementTreeMatcher) checker) - .matchExpressionStatement((ExpressionStatementTree) tree, state); - } - if (checker instanceof BugChecker.ForLoopTreeMatcher && tree instanceof ForLoopTree) { - return ((BugChecker.ForLoopTreeMatcher) checker).matchForLoop((ForLoopTree) tree, state); - } - if (checker instanceof BugChecker.IdentifierTreeMatcher && tree instanceof IdentifierTree) { - return ((BugChecker.IdentifierTreeMatcher) checker).matchIdentifier((IdentifierTree) tree, state); - } - if (checker instanceof BugChecker.IfTreeMatcher && tree instanceof IfTree) { - return ((BugChecker.IfTreeMatcher) checker).matchIf((IfTree) tree, state); - } - if (checker instanceof BugChecker.ImportTreeMatcher && tree instanceof ImportTree) { - return ((BugChecker.ImportTreeMatcher) checker).matchImport((ImportTree) tree, state); - } - if (checker instanceof BugChecker.InstanceOfTreeMatcher && tree instanceof InstanceOfTree) { - return ((BugChecker.InstanceOfTreeMatcher) checker).matchInstanceOf((InstanceOfTree) tree, state); - } - if (checker instanceof BugChecker.LabeledStatementTreeMatcher && tree instanceof LabeledStatementTree) { - return ((BugChecker.LabeledStatementTreeMatcher) checker) - .matchLabeledStatement((LabeledStatementTree) tree, state); - } - if (checker instanceof BugChecker.LambdaExpressionTreeMatcher && tree instanceof LambdaExpressionTree) { - return ((BugChecker.LambdaExpressionTreeMatcher) checker) - .matchLambdaExpression((LambdaExpressionTree) tree, state); - } - if (checker instanceof BugChecker.LiteralTreeMatcher && tree instanceof LiteralTree) { - return ((BugChecker.LiteralTreeMatcher) checker).matchLiteral((LiteralTree) tree, state); - } - if (checker instanceof BugChecker.MemberReferenceTreeMatcher && tree instanceof MemberReferenceTree) { - return ((BugChecker.MemberReferenceTreeMatcher) checker) - .matchMemberReference((MemberReferenceTree) tree, state); - } - if (checker instanceof BugChecker.MemberSelectTreeMatcher && tree instanceof MemberSelectTree) { - return ((BugChecker.MemberSelectTreeMatcher) checker).matchMemberSelect((MemberSelectTree) tree, state); - } - if (checker instanceof BugChecker.MethodTreeMatcher && tree instanceof MethodTree) { - return ((BugChecker.MethodTreeMatcher) checker).matchMethod((MethodTree) tree, state); - } - if (checker instanceof BugChecker.MethodInvocationTreeMatcher && tree instanceof MethodInvocationTree) { - return ((BugChecker.MethodInvocationTreeMatcher) checker) - .matchMethodInvocation((MethodInvocationTree) tree, state); - } - if (checker instanceof BugChecker.ModifiersTreeMatcher && tree instanceof ModifiersTree) { - return ((BugChecker.ModifiersTreeMatcher) checker).matchModifiers((ModifiersTree) tree, state); - } - if (checker instanceof BugChecker.NewArrayTreeMatcher && tree instanceof NewArrayTree) { - return ((BugChecker.NewArrayTreeMatcher) checker).matchNewArray((NewArrayTree) tree, state); - } - if (checker instanceof BugChecker.NewClassTreeMatcher && tree instanceof NewClassTree) { - return ((BugChecker.NewClassTreeMatcher) checker).matchNewClass((NewClassTree) tree, state); - } - if (checker instanceof BugChecker.ParenthesizedTreeMatcher && tree instanceof ParenthesizedTree) { - return ((BugChecker.ParenthesizedTreeMatcher) checker) - .matchParenthesized((ParenthesizedTree) tree, state); - } - if (checker instanceof BugChecker.ReturnTreeMatcher && tree instanceof ReturnTree) { - return ((BugChecker.ReturnTreeMatcher) checker).matchReturn((ReturnTree) tree, state); - } - if (checker instanceof BugChecker.SwitchTreeMatcher && tree instanceof SwitchTree) { - return ((BugChecker.SwitchTreeMatcher) checker).matchSwitch((SwitchTree) tree, state); - } - if (checker instanceof BugChecker.SynchronizedTreeMatcher && tree instanceof SynchronizedTree) { - return ((BugChecker.SynchronizedTreeMatcher) checker).matchSynchronized((SynchronizedTree) tree, state); - } - if (checker instanceof BugChecker.ThrowTreeMatcher && tree instanceof ThrowTree) { - return ((BugChecker.ThrowTreeMatcher) checker).matchThrow((ThrowTree) tree, state); - } - if (checker instanceof BugChecker.TryTreeMatcher && tree instanceof TryTree) { - return ((BugChecker.TryTreeMatcher) checker).matchTry((TryTree) tree, state); - } - if (checker instanceof BugChecker.TypeCastTreeMatcher && tree instanceof TypeCastTree) { - return ((BugChecker.TypeCastTreeMatcher) checker).matchTypeCast((TypeCastTree) tree, state); - } - if (checker instanceof BugChecker.UnaryTreeMatcher && tree instanceof UnaryTree) { - return ((BugChecker.UnaryTreeMatcher) checker).matchUnary((UnaryTree) tree, state); - } - if (checker instanceof BugChecker.VariableTreeMatcher && tree instanceof VariableTree) { - return ((BugChecker.VariableTreeMatcher) checker).matchVariable((VariableTree) tree, state); - } - if (checker instanceof BugChecker.WhileLoopTreeMatcher && tree instanceof WhileLoopTree) { - return ((BugChecker.WhileLoopTreeMatcher) checker).matchWhileLoop((WhileLoopTree) tree, state); - } - - return Description.NO_MATCH; - } - } - private static Fix createSuppressionFix( AnnotationTree suppressWarnings, Set unusedSuppressions, VisitorState state) { List currentSuppressions = diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleTreePathScannerModifications.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleTreePathScannerModifications.java index 2bbe2b94..9ab4acd1 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleTreePathScannerModifications.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleTreePathScannerModifications.java @@ -23,4 +23,6 @@ public class SuppressibleTreePathScannerModifications { public static boolean shouldBypassSuppressions(VisitorState state) { return state.errorProneOptions().isIgnoreSuppressionAnnotations(); } + + private SuppressibleTreePathScannerModifications() {} } diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/UnusedSuppressionsTree.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/UnusedSuppressionsTree.java index 8bcd2438..89b77449 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/UnusedSuppressionsTree.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/UnusedSuppressionsTree.java @@ -52,29 +52,29 @@ public static UnusedSuppressionsTree initializeWithSuppressions(CompilationUnitT new TreePathScanner() { @Override - public Void scan(Tree tree, Void p) { + public Void scan(Tree tree, Void unused) { if (tree != null) { treeToPath.put(tree, new TreePath(getCurrentPath(), tree)); } - return super.scan(tree, p); + return super.scan(tree, unused); } @Override - public Void visitMethod(MethodTree node, Void p) { + public Void visitMethod(MethodTree node, Void unused) { collectSuppressions(node, node.getModifiers()); - return super.visitMethod(node, p); + return super.visitMethod(node, unused); } @Override - public Void visitClass(ClassTree node, Void p) { + public Void visitClass(ClassTree node, Void unused) { collectSuppressions(node, node.getModifiers()); - return super.visitClass(node, p); + return super.visitClass(node, unused); } @Override - public Void visitVariable(VariableTree node, Void p) { + public Void visitVariable(VariableTree node, Void unused) { collectSuppressions(node, node.getModifiers()); - return super.visitVariable(node, p); + return super.visitVariable(node, unused); } private void collectSuppressions(Tree tree, ModifiersTree modifiers) { @@ -111,30 +111,19 @@ public Set allSuppressionNames() { * construction. */ public void flagFirstParentSuppressionAsUsed(Tree tree, String suppressionName) { - System.err.println("Flagging suppressions as used: " + suppressionName); - System.err.println("tree: " + tree); TreePath treePath = treeToPath.get(tree); if (treePath == null) { - System.err.println("No TreePath found for tree - tree not in our suppression map"); - System.err.println("========================================\n"); return; // Tree not found in our map } - System.err.println("leaf of path: " + treePath.getLeaf()); for (TreePath path = treePath; path != null; path = path.getParentPath()) { Tree curr = path.getLeaf(); Set suppressions = treeToSuppressions.get(curr); if (suppressions != null && suppressions.contains(suppressionName)) { usedSuppressions.get(curr).add(suppressionName); - System.err.println("Flagged suppression '" + suppressionName + "' as used: " + curr); - System.err.println("========================================\n"); - return; } } - - System.err.println("No parent suppression found for '" + suppressionName + "' - walked entire parent chain"); - System.err.println("========================================\n"); } public void markAllSuppressionsAsUsed(String suppressionName) { From b3768ccc09134f28d74d62bb8223440b35bab274 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 24 Sep 2025 21:15:19 +0100 Subject: [PATCH 08/12] raitlawks --- versions.lock | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/versions.lock b/versions.lock index 85bda524..fd78de54 100644 --- a/versions.lock +++ b/versions.lock @@ -4,35 +4,41 @@ com.github.ben-manes.caffeine:caffeine:3.0.5 (1 constraints: e312a21b) com.github.kevinstern:software-and-algorithms:1.0 (1 constraints: 7e12fcf5) -com.google.auto:auto-common:1.2.1 (2 constraints: b321cc9d) +com.google.auto:auto-common:1.2.2 (3 constraints: a432fc93) com.google.auto.service:auto-service:1.1.1 (1 constraints: 0505f435) -com.google.auto.service:auto-service-annotations:1.1.1 (1 constraints: 9c0f6a86) +com.google.auto.service:auto-service-annotations:1.1.1 (2 constraints: 8a20341c) -com.google.auto.value:auto-value-annotations:1.10.4 (2 constraints: 141da40a) +com.google.auto.value:auto-value-annotations:1.10.4 (3 constraints: ac2df2f5) -com.google.errorprone:error_prone_annotation:2.41.0 (3 constraints: db2cc946) +com.google.errorprone:error_prone_annotation:2.41.0 (4 constraints: fe3de95e) -com.google.errorprone:error_prone_annotations:2.41.0 (5 constraints: 9e4c911f) +com.google.errorprone:error_prone_annotations:2.41.0 (6 constraints: c15ddb8c) -com.google.errorprone:error_prone_check_api:2.41.0 (2 constraints: ca195cd6) +com.google.errorprone:error_prone_check_api:2.41.0 (3 constraints: ed2a1f5b) + +com.google.errorprone:error_prone_core:2.41.0 (1 constraints: 3e054c3b) + +com.google.googlejavaformat:google-java-format:1.27.0 (2 constraints: b6258eff) com.google.guava:failureaccess:1.0.3 (1 constraints: 160ae3b4) -com.google.guava:guava:33.5.0-jre (9 constraints: de94a155) +com.google.guava:guava:33.5.0-jre (10 constraints: 73a7e8ff) com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava (1 constraints: bd17c918) com.google.j2objc:j2objc-annotations:3.1 (1 constraints: b809f1a0) +com.google.protobuf:protobuf-java:3.25.5 (1 constraints: 2c1160c9) + com.palantir.gradle.utils:environment-variables:0.19.0 (1 constraints: 3c053e3b) -io.github.eisop:dataflow-errorprone:3.41.0-eisop1 (2 constraints: 9c2c4f1c) +io.github.eisop:dataflow-errorprone:3.41.0-eisop1 (3 constraints: 3e40ddde) io.github.java-diff-utils:java-diff-utils:4.12 (1 constraints: b412c908) -javax.inject:javax.inject:1 (1 constraints: 201230d1) +javax.inject:javax.inject:1 (2 constraints: 51221d52) net.ltgt.gradle:gradle-errorprone-plugin:4.3.0 (1 constraints: 09050836) @@ -40,10 +46,12 @@ one.util:streamex:0.8.4 (1 constraints: 0e050736) org.checkerframework:checker-qual:3.42.0 (3 constraints: 102d771b) -org.jspecify:jspecify:1.0.0 (3 constraints: 4431fdfd) +org.jspecify:jspecify:1.0.0 (4 constraints: 31427edc) org.ow2.asm:asm:9.8 (2 constraints: bd0e5959) +org.pcollections:pcollections:4.0.1 (1 constraints: f21028b8) + [Test dependencies] @@ -54,8 +62,6 @@ com.google.auto.value:auto-value:1.10 (1 constraints: e711f8e8) com.google.errorprone:error_prone_test_helpers:2.41.0 (1 constraints: 3e054c3b) -com.google.googlejavaformat:google-java-format:1.27.0 (1 constraints: 9014ad75) - com.google.jimfs:jimfs:1.3.0 (1 constraints: 5a141061) com.google.testing.compile:compile-testing:0.21.0 (1 constraints: 89149575) From 4e3994aead0eb42e3fc9fcad4f7aff5e463f541c Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 24 Sep 2025 21:16:36 +0100 Subject: [PATCH 09/12] Remove irrelevant comment --- .../suppressibleerrorprone/modes/modes/RemoveUnusedMode.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/RemoveUnusedMode.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/RemoveUnusedMode.java index 0d661e42..9ba786a1 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/RemoveUnusedMode.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/RemoveUnusedMode.java @@ -40,10 +40,7 @@ public PatchChecksOption patchChecks() { @Override public Map extraErrorProneCheckOptions() { - // For the suppressions to remove, if no specific check is enabled, we need to just remove everything - // We can't explicitly list all possible checks, because some might not exist anymore - // The logic itself needs to consider an empty list as "remove all" - + // Simplify the logic by only permitting blanket removal return Map.of("SuppressibleErrorProne:RemoveUnusedSuppressions", ALL_CHECKS); } From b78e06279206928bea293e752e849a3eb6ca0418 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 24 Sep 2025 21:46:54 +0100 Subject: [PATCH 10/12] Clean stuff up --- ...ppressibleTreePathScannerClassVisitor.java | 13 +++++++++- .../BugCheckerRegistry.java | 4 +--- .../RemoveUnusedSuppressions.java | 19 +++++---------- ...pressibleTreePathScannerModifications.java | 2 +- .../UnusedSuppressionsTree.java | 24 +++++++++---------- 5 files changed, 32 insertions(+), 30 deletions(-) diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/transform/SuppressibleTreePathScannerClassVisitor.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/transform/SuppressibleTreePathScannerClassVisitor.java index 12643aae..dce49e60 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/transform/SuppressibleTreePathScannerClassVisitor.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/transform/SuppressibleTreePathScannerClassVisitor.java @@ -21,6 +21,17 @@ import org.objectweb.asm.MethodVisitor; import org.objectweb.asm.Opcodes; +/** + * Forces {@code SuppressibleTreePathScanner} to respect the ignore suppressions option. + * + *

{@code RemoveUnusedSuppressions} must see violations that are normally hidden by suppressions to + * determine which suppressions are actually needed. While ErrorProneOptions has an + * {@code ignoreSuppressionAnnotations} flag for this purpose, many BugCheckers use + * {@code SuppressibleTreePathScanner}, which doesn't respect this flag. + * + *

This class uses bytecode manipulation to patch {@code SuppressibleTreePathScanner::isSuppressed} + * to respect the ErrorProneOptions setting. + */ final class SuppressibleTreePathScannerClassVisitor extends ClassVisitor { SuppressibleTreePathScannerClassVisitor(ClassVisitor classVisitor) { super(Opcodes.ASM9, classVisitor); @@ -58,7 +69,7 @@ public void visitCode() { mv.visitMethodInsn( Opcodes.INVOKESTATIC, "com/palantir/suppressibleerrorprone/SuppressibleTreePathScannerModifications", - "shouldBypassSuppressions", + "shouldIgnoreSuppressions", "(Lcom/google/errorprone/VisitorState;)Z", false); diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/BugCheckerRegistry.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/BugCheckerRegistry.java index 5f6543a5..d0b2ae36 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/BugCheckerRegistry.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/BugCheckerRegistry.java @@ -56,9 +56,7 @@ public static BugCheckerRegistry constructFromEnabledCheckers(VisitorState state Map enabledBugCheckers = defaultAndPluginBugCheckers.getAllChecks().values().stream() .filter(info -> info.severity(severityMap) != SeverityLevel.SUGGESTION) .collect(Collectors.toMap( - BugCheckerInfo::canonicalName, - info -> injector.getInstance(info.checkerClass()) - )); + BugCheckerInfo::canonicalName, info -> injector.getInstance(info.checkerClass()))); return new BugCheckerRegistry(enabledBugCheckers); } diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveUnusedSuppressions.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveUnusedSuppressions.java index e47193ae..b4403c14 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveUnusedSuppressions.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveUnusedSuppressions.java @@ -95,16 +95,12 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s for (TreeWithUnusedSuppressions treeWithUnusedSuppressions : unusedSuppressionsTree.unused()) { Set unusedSuppressions = treeWithUnusedSuppressions.unusedSuppressions(); - - // Get modifiers tree based on tree type List suppressions = getModifiersTree(treeWithUnusedSuppressions).getAnnotations().stream() .filter(AnnotationUtils::isSuppressWarningsAnnotation) .toList(); - - // Find @SuppressWarnings annotation for (AnnotationTree suppression : suppressions) { - Fix fix = createSuppressionFix(suppression, unusedSuppressions, state); + Fix fix = createFix(suppression, unusedSuppressions, state); state.reportMatch(buildDescription(tree) .setMessage("Remove unused @SuppressWarnings: " + unusedSuppressions) .addFix(fix) @@ -116,23 +112,21 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s } private static ModifiersTree getModifiersTree(TreeWithUnusedSuppressions treeWithUnusedSuppressions) { - ModifiersTree modifiers; Tree declarationTree = treeWithUnusedSuppressions.tree(); if (declarationTree instanceof MethodTree methodTree) { - modifiers = methodTree.getModifiers(); + return methodTree.getModifiers(); } else if (declarationTree instanceof ClassTree classTree) { - modifiers = classTree.getModifiers(); + return classTree.getModifiers(); } else if (declarationTree instanceof VariableTree variableTree) { - modifiers = variableTree.getModifiers(); + return variableTree.getModifiers(); } else { throw new IllegalStateException("Unexpected tree type: " + declarationTree.getClass()); } - return modifiers; } // Annoyingly, we have to construct a fresh ErrorProneOptions and copy the rest of the flags manually, // before turning on XepIgnoreSuppressionAnnotations. This is so fragile :| - @SuppressWarnings("CyclomaticComplexity") // mostly just copying options + @SuppressWarnings("CyclomaticComplexity") private static ErrorProneOptions ignoreSuppressions(ErrorProneOptions originalOptions) { List args = new ArrayList<>(); args.add("-XepIgnoreSuppressionAnnotations"); @@ -179,8 +173,7 @@ private static ErrorProneOptions ignoreSuppressions(ErrorProneOptions originalOp return ErrorProneOptions.processArgs(args); } - private static Fix createSuppressionFix( - AnnotationTree suppressWarnings, Set unusedSuppressions, VisitorState state) { + private static Fix createFix(AnnotationTree suppressWarnings, Set unusedSuppressions, VisitorState state) { List currentSuppressions = AnnotationUtils.annotationStringValues(suppressWarnings).toList(); List remainingSuppressions = currentSuppressions.stream() diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleTreePathScannerModifications.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleTreePathScannerModifications.java index 9ab4acd1..30714d0c 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleTreePathScannerModifications.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleTreePathScannerModifications.java @@ -20,7 +20,7 @@ public class SuppressibleTreePathScannerModifications { - public static boolean shouldBypassSuppressions(VisitorState state) { + public static boolean shouldIgnoreSuppressions(VisitorState state) { return state.errorProneOptions().isIgnoreSuppressionAnnotations(); } diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/UnusedSuppressionsTree.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/UnusedSuppressionsTree.java index 89b77449..5d5543fd 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/UnusedSuppressionsTree.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/UnusedSuppressionsTree.java @@ -33,17 +33,17 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.stream.Collectors; -@SuppressWarnings("BadAssert") public class UnusedSuppressionsTree { + private final Map pathCache; // exists for performance reasons + private final Map> treeToSuppressions; - private final Map> usedSuppressions; - private final Map treeToPath; + private final Map> treeToUsedSuppressions; - private UnusedSuppressionsTree(Map> treeToSuppressions, Map treeToPath) { + private UnusedSuppressionsTree(Map> treeToSuppressions, Map pathCache) { + this.pathCache = Map.copyOf(pathCache); this.treeToSuppressions = Map.copyOf(treeToSuppressions); - this.treeToPath = Map.copyOf(treeToPath); - this.usedSuppressions = new HashMap<>(); - treeToSuppressions.keySet().forEach(tree -> usedSuppressions.put(tree, ConcurrentHashMap.newKeySet())); + this.treeToUsedSuppressions = new HashMap<>(); + treeToSuppressions.keySet().forEach(tree -> treeToUsedSuppressions.put(tree, ConcurrentHashMap.newKeySet())); } public static UnusedSuppressionsTree initializeWithSuppressions(CompilationUnitTree tree) { @@ -106,12 +106,12 @@ public Set allSuppressionNames() { * Starting from {@code tree}, look for the first tree along the path which has a suppression on * {@code suppressionName}, and mark that suppression as used. * - * This method is forced to take in a {@code Tree} rather than a {@code TreePath}, because it is called from + *

This method is forced to take in a {@code Tree} rather than a {@code TreePath}, because it is called from * {@code description.position.getTree()}. To avoid doing a tree walk, we cache the tree->path mapping during * construction. */ public void flagFirstParentSuppressionAsUsed(Tree tree, String suppressionName) { - TreePath treePath = treeToPath.get(tree); + TreePath treePath = pathCache.get(tree); if (treePath == null) { return; // Tree not found in our map } @@ -120,7 +120,7 @@ public void flagFirstParentSuppressionAsUsed(Tree tree, String suppressionName) Tree curr = path.getLeaf(); Set suppressions = treeToSuppressions.get(curr); if (suppressions != null && suppressions.contains(suppressionName)) { - usedSuppressions.get(curr).add(suppressionName); + treeToUsedSuppressions.get(curr).add(suppressionName); return; } } @@ -129,7 +129,7 @@ public void flagFirstParentSuppressionAsUsed(Tree tree, String suppressionName) public void markAllSuppressionsAsUsed(String suppressionName) { treeToSuppressions.entrySet().stream() .filter(entry -> entry.getValue().contains(suppressionName)) - .forEach(entry -> usedSuppressions.get(entry.getKey()).add(suppressionName)); + .forEach(entry -> treeToUsedSuppressions.get(entry.getKey()).add(suppressionName)); } public Set unused() { @@ -137,7 +137,7 @@ public Set unused() { .map(entry -> { Tree tree = entry.getKey(); Set allSuppressions = entry.getValue(); - Set used = usedSuppressions.get(tree); + Set used = treeToUsedSuppressions.get(tree); Set unused = allSuppressions.stream() .filter(s -> !used.contains(s)) .collect(Collectors.toSet()); From a3e939764e64beda04bd141b859ccf5ed16c130b Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 24 Sep 2025 21:52:50 +0100 Subject: [PATCH 11/12] Make test good --- ...ppressibleErrorPronePluginIntegrationTest.groovy | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 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 2996e0ee..0dbd5228 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 @@ -1340,7 +1340,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec '''.stripIndent(true) } - def 'errorProneRemoveUnused only removes the first suppression in the path of a violation'() { + def 'errorProneRemoveUnused only keeps the first suppression in the path of a violation'() { // language=Java writeJavaSourceFileToSourceSets ''' package app; @@ -1353,7 +1353,10 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec class Inner { @SuppressWarnings("InlineTrivialConstant") class InnerInner { - private static final String EMPTY = ""; + @SuppressWarnings("InlineTrivialConstant") + class InnerInnerInner { + private static final String EMPTY = ""; + } } } } @@ -1372,9 +1375,11 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec private static final String EMPTY_STRING = ""; class Inner { - @SuppressWarnings("InlineTrivialConstant") class InnerInner { - private static final String EMPTY = ""; + @SuppressWarnings("InlineTrivialConstant") + class InnerInnerInner { + private static final String EMPTY = ""; + } } } } From 60b8dd5a5be2d01ddd6426c7750a728fb14aaf79 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Thu, 25 Sep 2025 10:25:06 +0100 Subject: [PATCH 12/12] Add a good test --- ...ibleErrorPronePluginIntegrationTest.groovy | 59 ++++++++++++++++++- 1 file changed, 58 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 0dbd5228..ca0599da 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 @@ -1340,7 +1340,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec '''.stripIndent(true) } - def 'errorProneRemoveUnused only keeps the first suppression in the path of a violation'() { + def 'errorProneRemoveUnused only keeps the closest suppression to a violation'() { // language=Java writeJavaSourceFileToSourceSets ''' package app; @@ -1386,6 +1386,63 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec '''.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 = ""; + + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + boolean truism = new int[3].equals(new int[3]); + + @SuppressWarnings("InlineTrivialConstant") + class InnerInner { + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused') + + then: + + // language=Java + appJavaTextEquals ''' + package app; + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + @SuppressWarnings("ArrayEquals") + class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + boolean truism = new int[3].equals(new int[3]); + + 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 '''