From 66852e764962f3b0b3ed9c827806fc17958998de Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Mon, 22 Sep 2025 11:13:19 +0100 Subject: [PATCH 01/15] 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 9a6ce54c..913d7882 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 @@ -1313,6 +1313,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 a6a0146bc71b8289f1af37790e8b8f56e7c3cde7 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 24 Sep 2025 19:33:13 +0100 Subject: [PATCH 02/15] 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 913d7882..a680d699 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 @@ -1313,41 +1313,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"); @@ -1356,7 +1399,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 87251ba91b0397bb249f680e8af5ad88d301c07f Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 24 Sep 2025 19:35:42 +0100 Subject: [PATCH 03/15] 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 a680d699..15da7425 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 @@ -1313,7 +1313,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 c942e9c12b0454b9da8093b2019f8b3ce7fb05bd Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 24 Sep 2025 20:43:28 +0100 Subject: [PATCH 04/15] 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 0af0180edb26c778c4a89f17f61c1242423c9511 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 24 Sep 2025 20:44:58 +0100 Subject: [PATCH 05/15] 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 15da7425..37e759f2 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 @@ -1345,7 +1345,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 7a1d8bb5c9fa39e76a39a70918e7e9f7893d4f23 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 24 Sep 2025 20:58:43 +0100 Subject: [PATCH 06/15] 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 2b1277b723b6987225b1aa1249614f24dcd5eaa5 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 24 Sep 2025 21:13:19 +0100 Subject: [PATCH 07/15] 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 109859cbb160bfcc7b06c8134700b65440326d98 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 24 Sep 2025 21:15:19 +0100 Subject: [PATCH 08/15] 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 add8ced9233f72ca80e1b55543f1f21d5d99732c Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 24 Sep 2025 21:16:36 +0100 Subject: [PATCH 09/15] 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 86ff1be78ded5d61da613a0b22e9efdbf7eb7faf Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 24 Sep 2025 21:46:54 +0100 Subject: [PATCH 10/15] 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 c2a950397c9af9534147c0d92d35674c289d9257 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Wed, 24 Sep 2025 21:52:50 +0100 Subject: [PATCH 11/15] 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 37e759f2..0abe7295 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 @@ -1345,7 +1345,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; @@ -1358,7 +1358,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 = ""; + } } } } @@ -1377,9 +1380,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 e660318d889487f45167edb3a402d4989e41b557 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Thu, 25 Sep 2025 10:25:06 +0100 Subject: [PATCH 12/15] 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 0abe7295..674bc2ee 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 @@ -1345,7 +1345,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; @@ -1391,6 +1391,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 ''' From 442df3ed7b7683f932089d0bb15a4acd6e4491bb Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Mon, 29 Sep 2025 14:42:11 +0100 Subject: [PATCH 13/15] LETS GOOOOOO --- gradle-suppressible-error-prone/build.gradle | 1 + .../SuppressibleErrorPronePlugin.java | 18 +- .../suppressibleerrorprone/modes/Modes.java | 15 +- .../modes/common/CommonOptions.java | 33 ++- .../modes/common/ModifyCheckApiOption.java | 42 ++- .../modes/common/RemoveUnusedCheck.java | 38 --- ...oveRolloutAndSuppressingInterference.java} | 2 +- .../RemoveUnusedAndApplyingInterference.java | 49 ++++ .../RemoveUnusedAndSuppressInterference.java | 37 +++ .../RemoveUnusedModeInterference.java | 46 --- .../modes/modes/ApplyMode.java | 6 + .../modes/modes/RemoveRolloutMode.java | 4 +- .../modes/modes/RemoveUnusedMode.java | 15 +- .../modes/modes/SuppressMode.java | 9 +- .../transform/ModifyErrorProneCheckApi.java | 12 +- ...ibleErrorPronePluginIntegrationTest.groovy | 272 +++++++++++++++--- .../common/ModifyCheckApiOptionTest.java | 44 +-- .../AnnotationUtils.java | 16 ++ .../BugCheckerRegistry.java | 73 ----- ...essingFix.java => LazySuppressionFix.java} | 48 ++-- ...t.java => LazySuppressionReplacement.java} | 56 ++-- .../LineRemovingReplacementFix.java | 93 ------ .../RemoveRolloutSuppressions.java | 13 +- .../RemoveUnusedSuppressions.java | 186 ------------ .../SuppressWarningsUtils.java | 34 +++ .../SuppressibleErrorProne.java | 32 +++ .../UnusedSuppressionsTree.java | 158 ---------- .../VisitorStateModifications.java | 169 +++++++++-- versions.lock | 22 +- versions.props | 1 + 30 files changed, 766 insertions(+), 778 deletions(-) delete mode 100644 gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/RemoveUnusedCheck.java rename gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/{RemovingAndSuppressingInterference.java => RemoveRolloutAndSuppressingInterference.java} (95%) create mode 100644 gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedAndApplyingInterference.java create mode 100644 gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedAndSuppressInterference.java delete mode 100644 gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedModeInterference.java delete mode 100644 suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/BugCheckerRegistry.java rename suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/{SuppressingFix.java => LazySuppressionFix.java} (54%) rename suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/{SuppressingReplacement.java => LazySuppressionReplacement.java} (62%) delete mode 100644 suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/LineRemovingReplacementFix.java delete 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/SuppressibleErrorProne.java delete mode 100644 suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/UnusedSuppressionsTree.java diff --git a/gradle-suppressible-error-prone/build.gradle b/gradle-suppressible-error-prone/build.gradle index b489ad5d..20ec00c6 100644 --- a/gradle-suppressible-error-prone/build.gradle +++ b/gradle-suppressible-error-prone/build.gradle @@ -18,6 +18,7 @@ dependencies { testImplementation 'commons-io:commons-io' testImplementation 'one.util:streamex' testImplementation 'com.palantir.gradle.plugintesting:configuration-cache-spec' + testImplementation 'com.palantir.javaformat:palantir-java-format' } gradlePlugin { 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 4d14f776..b4fe0286 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 @@ -19,9 +19,11 @@ import com.palantir.gradle.suppressibleerrorprone.modes.Modes; import com.palantir.gradle.suppressibleerrorprone.modes.common.CommonOptions; import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption; +import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.ModifiedFile; import com.palantir.gradle.suppressibleerrorprone.transform.ModifyErrorProneCheckApi; import java.util.List; import java.util.Optional; +import java.util.Set; import java.util.stream.Collectors; import javax.inject.Inject; import net.ltgt.gradle.errorprone.ErrorProneOptions; @@ -75,7 +77,7 @@ private void applyToJavaProject(Project project) { if (getModes().modifyCheckApi() instanceof ModifyCheckApiOption.MustModify mustModify) { // When auto-suppressing, the logic will run a bytecode patched version of errorprone // (via an artifact transform) that intercepts every error from every check and adds a custom fix - setupErrorProneArtifactTransform(project, mustModify.modifyVisitorState()); + setupErrorProneArtifactTransform(project, mustModify.modifiedFiles()); } project.getConfigurations().named(ErrorPronePlugin.CONFIGURATION_NAME).configure(errorProneConfiguration -> { @@ -108,7 +110,7 @@ private void applyToJavaProject(Project project) { }); } - private static void setupErrorProneArtifactTransform(Project project, boolean modifyVisitorState) { + private static void setupErrorProneArtifactTransform(Project project, Set modifiedFiles) { Attribute suppressible = Attribute.of("com.palantir.suppressible-error-prone.suppressible", Boolean.class); project.getDependencies().getAttributesSchema().attribute(suppressible); @@ -120,7 +122,7 @@ private static void setupErrorProneArtifactTransform(Project project, boolean mo .attribute(suppressible, false); project.getDependencies().registerTransform(ModifyErrorProneCheckApi.class, spec -> { - spec.getParameters().getModifyVisitorState().set(modifyVisitorState); + spec.getParameters().getFilesToModify().set(modifiedFiles); Attribute artifactType = Attribute.of("artifactType", String.class); spec.getFrom().attribute(suppressible, false).attribute(artifactType, "jar"); @@ -206,14 +208,12 @@ private void setupErrorProneOptions(CommonOptions commonOptions, ErrorProneOptio errorProneOptions.getErrorproneArgumentProviders().add(patchChecksCommandLineArgumentProvider); errorProneOptions - .getCheckOptions() - .putAll(getProviderFactory().provider(commonOptions::extraErrorProneCheckOptions)); + .getIgnoreSuppressionAnnotations() + .set(getProviderFactory().provider(commonOptions::ignoreSuppressionAnnotations)); errorProneOptions - .getChecks() - .put("RemoveUnusedSuppressions", getProviderFactory().provider(() -> commonOptions - .removeUnusedCheck() - .toCheckSeverity())); + .getCheckOptions() + .putAll(getProviderFactory().provider(commonOptions::extraErrorProneCheckOptions)); // 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, 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 6ab78eca..1bc92472 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,8 +28,9 @@ 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.RemoveRolloutAndSuppressingInterference; +import com.palantir.gradle.suppressibleerrorprone.modes.interferences.RemoveUnusedAndApplyingInterference; +import com.palantir.gradle.suppressibleerrorprone.modes.interferences.RemoveUnusedAndSuppressInterference; import com.palantir.gradle.suppressibleerrorprone.modes.interferences.SuppressingAndApplyingInterference; import com.palantir.gradle.suppressibleerrorprone.modes.modes.ApplyMode; import com.palantir.gradle.suppressibleerrorprone.modes.modes.DisableMode; @@ -68,9 +69,10 @@ ModeName.SUPPRESS, new SuppressMode(), private final Set interferences = Set.of( new DisableModeInterference(), - new RemoveUnusedModeInterference(), - new RemovingAndSuppressingInterference(), - new SuppressingAndApplyingInterference()); + new RemoveRolloutAndSuppressingInterference(), + new SuppressingAndApplyingInterference(), + new RemoveUnusedAndApplyingInterference(), + new RemoveUnusedAndSuppressInterference()); public final CombinedValue modifyCheckApi() { return ModifyCheckApiOption.combine( @@ -131,7 +133,8 @@ public final CommonOptions commonOptionsFor(JavaCompile javaCompile) { .append(nonInterferingCommonOptions) .toSet(); - return CommonOptions.naivelyCombine(allCommonOptions); + CommonOptions commonOptions = CommonOptions.naivelyCombine(allCommonOptions); + return commonOptions; } private Map> modesEnabledAndFlagValues() { 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 d7445784..cba777d8 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 @@ -21,6 +21,7 @@ import java.util.HashMap; import java.util.Map; import java.util.function.Supplier; +import java.util.stream.Collectors; import one.util.streamex.EntryStream; /** @@ -41,8 +42,8 @@ default RemoveRolloutCheck removeRolloutCheck() { return RemoveRolloutCheck.DISABLE; } - default RemoveUnusedCheck removeUnusedCheck() { - return RemoveUnusedCheck.DISABLE; + default boolean ignoreSuppressionAnnotations() { + return false; } default CommonOptions naivelyCombinedWith(CommonOptions other) { @@ -56,7 +57,8 @@ public PatchChecksOption patchChecks() { public Map extraErrorProneCheckOptions() { return EntryStream.of(CommonOptions.this.extraErrorProneCheckOptions()) .append(other.extraErrorProneCheckOptions()) - .toMap(); + .collect(Collectors.toMap( + Map.Entry::getKey, Map.Entry::getValue, (val1, val2) -> val1 + "," + val2)); } @Override @@ -65,8 +67,8 @@ public RemoveRolloutCheck removeRolloutCheck() { } @Override - public RemoveUnusedCheck removeUnusedCheck() { - return CommonOptions.this.removeUnusedCheck().or(other.removeUnusedCheck()); + public boolean ignoreSuppressionAnnotations() { + return CommonOptions.this.ignoreSuppressionAnnotations() || other.ignoreSuppressionAnnotations(); } }; } @@ -75,18 +77,31 @@ static CommonOptions naivelyCombine(Collection commonOptions) { return commonOptions.stream().reduce(CommonOptions.empty(), CommonOptions::naivelyCombinedWith); } + // NOTE: Maybe CommonOptions itself should be an interface without default methods, while a + // DefaultCommonOptions provides sensible defaults. + // Or maybe CommonOptions should just be a record with methods default CommonOptions withExtraErrorProneCheckFlag(String key, Supplier value) { return new CommonOptions() { + @Override + public Map extraErrorProneCheckOptions() { + Map map = new HashMap<>(CommonOptions.this.extraErrorProneCheckOptions()); + map.put(key, value.get()); + return Collections.unmodifiableMap(map); + } + @Override public PatchChecksOption patchChecks() { return CommonOptions.this.patchChecks(); } @Override - public Map extraErrorProneCheckOptions() { - Map map = new HashMap<>(CommonOptions.this.extraErrorProneCheckOptions()); - map.put(key, value.get()); - return Collections.unmodifiableMap(map); + public RemoveRolloutCheck removeRolloutCheck() { + return CommonOptions.this.removeRolloutCheck(); + } + + @Override + public boolean ignoreSuppressionAnnotations() { + return CommonOptions.this.ignoreSuppressionAnnotations(); } }; } diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOption.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOption.java index 41969618..5b6bce13 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOption.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOption.java @@ -19,9 +19,12 @@ import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.DoNotModify; import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.MustModify; import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.NoEffect; +import java.util.Arrays; import java.util.Collection; import java.util.Set; import java.util.function.Predicate; +import java.util.stream.Collectors; +import java.util.stream.Stream; import one.util.streamex.StreamEx; /** @@ -47,19 +50,16 @@ static ModifyCheckApiOption noEffect() { } /** - * The {@code error_prone_check_api} jar must be modified to allow {@code `for-rollout:`} suppressions to work. + * Modify these files in the error-prone API */ - static ModifyCheckApiOption mustModify() { - return new MustModify(false); + static MustModify mustModify(ModifiedFile... modifiedFile) { + return new MustModify(Arrays.stream(modifiedFile).collect(Collectors.toSet())); } - /** - * The {@code error_prone_check_api} jar must be modified to allow {@code `for-rollout:`} suppressions to work, - * and {@code VisitorState} must be modified to intercept reportMatch. - */ - static ModifyCheckApiOption mustModifyIncludingVisitorState() { - return new MustModify(true); + default Set getModifiedFiles() { + return Set.of(); } + ; enum DoNotModify implements ModifyCheckApiOption, CombinedValue { INSTANCE @@ -69,9 +69,11 @@ enum NoEffect implements ModifyCheckApiOption { INSTANCE } - record MustModify(boolean modifyVisitorState) implements ModifyCheckApiOption, CombinedValue { + record MustModify(Set modifiedFiles) implements ModifyCheckApiOption, CombinedValue { public MustModify combine(MustModify other) { - return new MustModify(modifyVisitorState || other.modifyVisitorState); + Set union = Stream.concat(modifiedFiles.stream(), other.modifiedFiles.stream()) + .collect(Collectors.toSet()); + return new MustModify(union); } } @@ -84,7 +86,7 @@ static CombinedValue combine(Collection options) { if (withoutNoEffects.isEmpty()) { // By default, we need to modify the check API to support for-rollout suppressions - return new MustModify(false); + return mustModify(ModifiedFile.BUG_CHECKER_INFO); } boolean doNotModify = withoutNoEffects.contains(DoNotModify.INSTANCE); @@ -103,4 +105,20 @@ static CombinedValue combine(Collection options) { .reduce(MustModify::combine) .get(); } + + enum ModifiedFile { + BUG_CHECKER_INFO("BugCheckerInfo"), + VISITOR_STATE("VisitorState"), + SUPPRESSIBLE_TREE_PATH_SCANNER("SuppressibleTreePathScanner"); + + private final String className; + + ModifiedFile(String className) { + this.className = className; + } + + public String getClassName() { + return className; + } + } } 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 deleted file mode 100644 index d3ba4bf4..00000000 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/RemoveUnusedCheck.java +++ /dev/null @@ -1,38 +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.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/interferences/RemovingAndSuppressingInterference.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveRolloutAndSuppressingInterference.java similarity index 95% rename from gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemovingAndSuppressingInterference.java rename to gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveRolloutAndSuppressingInterference.java index db25a1b6..29508b5b 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemovingAndSuppressingInterference.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveRolloutAndSuppressingInterference.java @@ -25,7 +25,7 @@ * Removing and suppressing cannot run at the same time, as removing just runs an errorprone to remove the for-rollout: * suppressions - if suppressing happened at the same time, this errorprone would just get suppressed. */ -public final class RemovingAndSuppressingInterference implements ModeInterference { +public final class RemoveRolloutAndSuppressingInterference implements ModeInterference { @Override public ModeInterferenceResult interferesWith(Set modeNames) { if (modeNames.containsAll(Set.of(ModeName.REMOVE_ROLLOUT, ModeName.SUPPRESS))) { diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedAndApplyingInterference.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedAndApplyingInterference.java new file mode 100644 index 00000000..c87f3cbf --- /dev/null +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedAndApplyingInterference.java @@ -0,0 +1,49 @@ +/* + * (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.CommonOptions; +import com.palantir.gradle.suppressibleerrorprone.modes.common.ModeName; +import com.palantir.gradle.suppressibleerrorprone.modes.common.SpecificModeInterference; +import java.util.Map; +import java.util.Set; + +public final class RemoveUnusedAndApplyingInterference extends SpecificModeInterference { + @Override + protected Set interferingModes() { + return Set.of(ModeName.REMOVE_UNUSED, ModeName.APPLY); + } + + @Override + public CommonOptions interfere(Map modeCommonOptions) { + CommonOptions removeUnused = modeCommonOptions.get(ModeName.REMOVE_UNUSED); + CommonOptions apply = modeCommonOptions.get(ModeName.APPLY); + + // If we're applying suggested patches at the same time as suppressing, we still need to tell + // errorprone to patch all checks, so we can make suggested fixes for suppressions in any check. + // However, inside our changes to errorprone, we need to get the list of checks that we're going + // to use the default suggested fixes for, so we can work out which ones to use the suggested + // fixes for and which to suppress. So we add the PreferPatchChecks argument here, which we can + // use inside error-prone/the compiler. + + return removeUnused + .naivelyCombinedWith(apply) + .withExtraErrorProneCheckFlag( + "SuppressibleErrorProne:PreferPatchChecks", + () -> apply.patchChecks().asCommaSeparated().orElse("")); + } +} diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedAndSuppressInterference.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedAndSuppressInterference.java new file mode 100644 index 00000000..3d165e4e --- /dev/null +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedAndSuppressInterference.java @@ -0,0 +1,37 @@ +/* + * (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.CommonOptions; +import com.palantir.gradle.suppressibleerrorprone.modes.common.ModeName; +import com.palantir.gradle.suppressibleerrorprone.modes.common.SpecificModeInterference; +import java.util.Map; +import java.util.Set; + +public final class RemoveUnusedAndSuppressInterference extends SpecificModeInterference { + @Override + protected Set interferingModes() { + return Set.of(ModeName.REMOVE_UNUSED, ModeName.SUPPRESS); + } + + @Override + public CommonOptions interfere(Map modeCommonOptions) { + CommonOptions removeUnused = modeCommonOptions.get(ModeName.REMOVE_UNUSED); + CommonOptions suppress = modeCommonOptions.get(ModeName.SUPPRESS); + return removeUnused.naivelyCombinedWith(suppress); + } +} 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 deleted file mode 100644 index 24206a39..00000000 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedModeInterference.java +++ /dev/null @@ -1,46 +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.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 final 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(); - } -} diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/ApplyMode.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/ApplyMode.java index 513b5cb3..09605765 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/ApplyMode.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/ApplyMode.java @@ -20,6 +20,7 @@ import com.palantir.gradle.suppressibleerrorprone.modes.common.CommonOptions; import com.palantir.gradle.suppressibleerrorprone.modes.common.Mode; import com.palantir.gradle.suppressibleerrorprone.modes.common.PatchChecksOption; +import java.util.Map; import java.util.Set; import java.util.stream.Collectors; @@ -31,6 +32,11 @@ public CommonOptions configureAndReturnCommonOptions(ModeOptionContext context) public PatchChecksOption patchChecks() { return PatchChecksOption.someChecks(() -> checksToApplySuggestedPatchesFor(context)); } + + @Override + public Map extraErrorProneCheckOptions() { + return Map.of("SuppressibleErrorProne:Mode", "Apply"); + } }; } diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/RemoveRolloutMode.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/RemoveRolloutMode.java index 9d74af3c..99a9bf11 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/RemoveRolloutMode.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/RemoveRolloutMode.java @@ -49,7 +49,9 @@ public Map extraErrorProneCheckOptions() { return Map.of( "SuppressibleErrorProne:RemoveRolloutSuppressions", - context.flagValue().orElse(ALL_CHECKS)); + context.flagValue().orElse(ALL_CHECKS), + "SuppressibleErrorProne:Mode", + "RemoveRollout"); } @Override 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 9ba786a1..60ef9981 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 @@ -19,15 +19,17 @@ 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.ModifyCheckApiOption.ModifiedFile; import com.palantir.gradle.suppressibleerrorprone.modes.common.PatchChecksOption; -import com.palantir.gradle.suppressibleerrorprone.modes.common.RemoveUnusedCheck; import java.util.Map; public final class RemoveUnusedMode implements Mode { private static final String ALL_CHECKS = ""; + @Override public ModifyCheckApiOption modifyCheckApi() { - return ModifyCheckApiOption.mustModify(); + return ModifyCheckApiOption.mustModify( + ModifiedFile.BUG_CHECKER_INFO, ModifiedFile.SUPPRESSIBLE_TREE_PATH_SCANNER, ModifiedFile.VISITOR_STATE); } @Override @@ -35,18 +37,17 @@ public CommonOptions configureAndReturnCommonOptions(ModeOptionContext context) return new CommonOptions() { @Override public PatchChecksOption patchChecks() { - return PatchChecksOption.someChecks("RemoveUnusedSuppressions"); + return PatchChecksOption.allChecks(); } @Override public Map extraErrorProneCheckOptions() { - // Simplify the logic by only permitting blanket removal - return Map.of("SuppressibleErrorProne:RemoveUnusedSuppressions", ALL_CHECKS); + return Map.of("SuppressibleErrorProne:Mode", "RemoveUnused"); } @Override - public RemoveUnusedCheck removeUnusedCheck() { - return RemoveUnusedCheck.ENABLED; + public boolean ignoreSuppressionAnnotations() { + return true; } }; } diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/SuppressMode.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/SuppressMode.java index 68ef5a42..7e359881 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/SuppressMode.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/SuppressMode.java @@ -19,12 +19,14 @@ 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.ModifyCheckApiOption.ModifiedFile; import com.palantir.gradle.suppressibleerrorprone.modes.common.PatchChecksOption; +import java.util.Map; public final class SuppressMode implements Mode { @Override public ModifyCheckApiOption modifyCheckApi() { - return ModifyCheckApiOption.mustModifyIncludingVisitorState(); + return ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO, ModifiedFile.VISITOR_STATE); } @Override @@ -34,6 +36,11 @@ public CommonOptions configureAndReturnCommonOptions(ModeOptionContext context) public PatchChecksOption patchChecks() { return PatchChecksOption.allChecks(); } + + @Override + public Map extraErrorProneCheckOptions() { + return Map.of("SuppressibleErrorProne:Mode", "Suppress"); + } }; } } 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 95013a71..e26e3777 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 @@ -16,6 +16,7 @@ package com.palantir.gradle.suppressibleerrorprone.transform; +import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.ModifiedFile; import com.palantir.gradle.suppressibleerrorprone.transform.ModifyErrorProneCheckApi.Params; import com.palantir.gradle.utils.environmentvariables.EnvironmentVariables; import java.io.BufferedOutputStream; @@ -36,6 +37,7 @@ import org.gradle.api.file.FileSystemLocation; import org.gradle.api.provider.Property; import org.gradle.api.provider.Provider; +import org.gradle.api.provider.SetProperty; import org.gradle.api.tasks.Input; import org.gradle.api.tasks.Nested; import org.objectweb.asm.ClassReader; @@ -79,7 +81,8 @@ private Optional> classVisitorFor(String classJarPat // We always want to modify the BugCheckerInfo class, as this is where we help errorprone understand // what the `for-rollout:` prefix for suppressions mean (which means this class is always modified, // even during normal compilation). - if (classJarPath.equals("com/google/errorprone/BugCheckerInfo.class")) { + if (classJarPath.equals("com/google/errorprone/BugCheckerInfo.class") + && getParameters().getFilesToModify().get().contains(ModifiedFile.BUG_CHECKER_INFO)) { return Optional.of(BugCheckerInfoVisitor::new); } @@ -87,11 +90,12 @@ private Optional> classVisitorFor(String classJarPat // it intercepts all errors and changes them. So when we're just running normal compilation, we want // to avoid running our modifications at all, and let the errors continue on their way unchanged. if (classJarPath.equals("com/google/errorprone/VisitorState.class") - && getParameters().getModifyVisitorState().get()) { + && getParameters().getFilesToModify().get().contains(ModifiedFile.VISITOR_STATE)) { return Optional.of(VisitorStateClassVisitor::new); } - if (classJarPath.equals("com/google/errorprone/bugpatterns/BugChecker$SuppressibleTreePathScanner.class")) { + if (classJarPath.equals("com/google/errorprone/bugpatterns/BugChecker$SuppressibleTreePathScanner.class") + && getParameters().getFilesToModify().get().contains(ModifiedFile.SUPPRESSIBLE_TREE_PATH_SCANNER)) { return Optional.of(SuppressibleTreePathScannerClassVisitor::new); } @@ -136,7 +140,7 @@ private void visitJar(File output, ClassTransformer classTransformer) { public abstract static class Params implements TransformParameters { @Input - public abstract Property getModifyVisitorState(); + public abstract SetProperty getFilesToModify(); @Input public abstract Property getCacheBust(); 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 674bc2ee..63756bd2 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 @@ -18,6 +18,7 @@ package com.palantir.gradle.suppressibleerrorprone import com.palantir.gradle.plugintesting.ConfigurationCacheSpec +import com.palantir.javaformat.java.Formatter; import org.apache.commons.io.FileUtils import org.gradle.testkit.runner.BuildResult import spock.lang.Unroll @@ -84,7 +85,6 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec errorprone 'com.google.errorprone:error_prone_core:2.31.0' } - suppressibleErrorProne { configureEachErrorProneOptions { // These interfere with some tests, so disable them @@ -370,7 +370,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; public final class App { @SuppressWarnings("for-rollout:ArrayToString") @@ -433,7 +433,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; public final class App { public void variables() { @@ -464,7 +464,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; public final class App { @SuppressWarnings("for-rollout:NamedLikeContextualKeyword") @@ -504,7 +504,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; public final class App { @SuppressWarnings("UnusedVariable") @@ -546,7 +546,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; import java.util.Arrays; @@ -595,7 +595,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; import java.util.Arrays; @@ -830,7 +830,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: 'it is suppressed' // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; public final class App { @SuppressWarnings("for-rollout:ArrayToString") @@ -877,7 +877,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec stderr2.contains('[NonCanonicalStaticImport]') // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; import static app.B.Inner; public final class App {} @@ -939,7 +939,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec runTasksSuccessfully('compileAllErrorProne', *mode) then: 'changes are actually made, it was not up-to-date' - appJavaTextNotEquals originalSource + resultIsSyntacticallyNotEqualTo originalSource where: mode << [ @@ -977,7 +977,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; public final class App {} '''.stripIndent(true) @@ -997,7 +997,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; public final class App {} '''.stripIndent(true) @@ -1017,7 +1017,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; @SuppressWarnings("for-rollout:Test") public final class App {} @@ -1037,7 +1037,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; @SuppressWarnings("for-rollout:Test") public final class App {} @@ -1057,7 +1057,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; public final class App {} '''.stripIndent(true) @@ -1080,7 +1080,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; import java.util.Arrays; @@ -1109,7 +1109,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; public final class App { @SuppressWarnings("ArrayToString") @@ -1136,7 +1136,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; import java.util.Arrays; @@ -1164,7 +1164,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; public final class App { public static void main(String[] args) { @@ -1191,7 +1191,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; public final class App { public static void main(String[] args) { @@ -1219,7 +1219,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; public final class App { public static void main(String[] args) { @@ -1246,7 +1246,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; import java.util.Arrays; @@ -1282,7 +1282,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; import java.util.Arrays; @@ -1328,11 +1328,11 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec '''.stripIndent(true) when: - runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused') + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused=ArrayToString') then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; @SuppressWarnings({"ArrayToString", "InlineTrivialConstant"}) public final class App { @@ -1373,7 +1373,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; public final class App { @SuppressWarnings("InlineTrivialConstant") @@ -1402,14 +1402,14 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec @SuppressWarnings("InlineTrivialConstant") private static final String EMPTY_STRING = ""; - @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) - class Inner { + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) // Doesn't move existing ArrayEquals closer to the violation + static class Inner { @SuppressWarnings("InlineTrivialConstant") private static final String EMPTY = ""; boolean truism = new int[3].equals(new int[3]); @SuppressWarnings("InlineTrivialConstant") - class InnerInner { + static class InnerInner { @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) void method() { new int[3].equals(new int[3]); @@ -1425,19 +1425,19 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; public final class App { @SuppressWarnings("InlineTrivialConstant") private static final String EMPTY_STRING = ""; - @SuppressWarnings("ArrayEquals") - class Inner { + @SuppressWarnings("ArrayEquals") // Doesn't move existing ArrayEquals closer to the violation + static class Inner { @SuppressWarnings("InlineTrivialConstant") private static final String EMPTY = ""; boolean truism = new int[3].equals(new int[3]); - class InnerInner { + static class InnerInner { @SuppressWarnings("ArrayEquals") void method() { new int[3].equals(new int[3]); @@ -1465,7 +1465,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec then: // language=Java - appJavaTextEquals ''' + resultIsSyntacticallyEqualTo ''' package app; public final class App { public static void main(String[] args) { @@ -1475,6 +1475,177 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec '''.stripIndent(true) } + def 'errorProneRemoveUnused and errorProneSuppress uses existing suppressions if possible'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + @SuppressWarnings("InlineTrivialConstant") + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + @SuppressWarnings("InlineTrivialConstant") + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + boolean truism = new int[3].equals(new int[3]); + + @SuppressWarnings("InlineTrivialConstant") + static class InnerInner { + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused', '-PerrorProneSuppress') + + then: + + // language=Java + resultIsSyntacticallyEqualTo ''' + package app; + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + @SuppressWarnings("for-rollout:ArrayEquals") + boolean truism = new int[3].equals(new int[3]); + + static class InnerInner { + @SuppressWarnings("ArrayEquals") + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + } + + def 'errorProneRemoveUnused and errorProneApply applies fixes on previously suppressed elements'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + boolean truism = new int[3].equals(new int[3]); + + @SuppressWarnings("InlineTrivialConstant") + static class InnerInner { + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused', '-PerrorProneApply=ArrayEquals') + + then: + + // language=Java + resultIsSyntacticallyEqualTo ''' + package app; + import java.util.Arrays; + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + boolean truism = Arrays.equals(new int[3], new int[3]); + + static class InnerInner { + void method() { + Arrays.equals(new int[3], new int[3]); + } + } + } + } + '''.stripIndent(true) + } + + def 'errorProneRemoveUnused + errorProneApply + errorProneSuppress applies fixes and suppressions on previously suppressed elements'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + @SuppressWarnings("ArrayEquals") + public final class App { + private static final String EMPTY_STRING = ""; + + // Although InlineTrivialConstant can be placed lower in the AST hierarchy, + // we preserve existing suppressions whenever possible rather than move suppressions around. + // Also, note that we don't add for-rollout here. + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + static class Inner { + private static final String EMPTY = ""; + boolean truism = new int[3].equals(new int[3]); + + @SuppressWarnings("InlineTrivialConstant") + static class InnerInner { + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused', '-PerrorProneSuppress', '-PerrorProneApply=ArrayEquals') + + then: + + // language=Java + resultIsSyntacticallyEqualTo ''' + package app; + import java.util.Arrays; + + public final class App { + @SuppressWarnings("for-rollout:InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + // Although InlineTrivialConstant can be placed lower in the AST hierarchy, + // we preserve existing suppressions whenever possible rather than move suppressions around. + // Also, note that we don't add for-rollout here. + @SuppressWarnings("InlineTrivialConstant") + static class Inner { + private static final String EMPTY = ""; + boolean truism = Arrays.equals(new int[3], new int[3]); + + static class InnerInner { + void method() { + Arrays.equals(new int[3], new int[3]); + } + } + } + } + '''.stripIndent(true) + } + + def 'errorProneRemoveUnused + errorProneApply + errorProneSuppress works with specified checkers'() { + + } def 'error-prone dependencies have versions bound together by a virtual platform'() { setup: 'when an error-prone dependency is forced to certain version' @@ -1533,9 +1704,18 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec super.writeJavaSourceFile(source, 'src/other/java', sourceSetRoot) } + // Normalize expected and actual source fixtures before comparing, because the formatting of error-prone's output + // shouldn't matter. It is a syntactic analyzer, and it should do one thing well. + private static String normalizeSource(String content) { + String stripped = content.readLines() + .collect { it.trim() } // Remove leading/trailing whitespace + .findAll { !it.isEmpty() } // Remove empty lines + .join('\n') + return Formatter.create().formatSource(stripped) + } + void appJavaTextContains(String substring) { assert file('app/App.java', mainSourceSet).text.contains(substring) - assert file('app/App.java', otherSourceSet).text.contains(substring) } void appJavaTextNotContains(String substring) { @@ -1543,13 +1723,23 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec assert !file('app/App.java', otherSourceSet).text.contains(substring) } - void appJavaTextEquals(String source) { - assert file('app/App.java', mainSourceSet).text == source - assert file('app/App.java', otherSourceSet).text == source + void resultIsSyntacticallyEqualTo(String source) { + var output = normalizeSource(file('app/App.java', mainSourceSet).text) + var expected = normalizeSource(source) + assert output == expected + + var outputOther = normalizeSource(file('app/App.java', otherSourceSet).text) + var expectedOther = normalizeSource(source) + assert outputOther == expectedOther } - void appJavaTextNotEquals(String source) { - assert file('app/App.java', mainSourceSet).text != source - assert file('app/App.java', otherSourceSet).text != source + void resultIsSyntacticallyNotEqualTo(String source) { + var output = normalizeSource(file('app/App.java', mainSourceSet).text) + var expected = normalizeSource(source) + assert output != expected + + var outputOther = normalizeSource(file('app/App.java', otherSourceSet).text) + var expectedOther = normalizeSource(source) + assert outputOther != expectedOther } } diff --git a/gradle-suppressible-error-prone/src/test/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOptionTest.java b/gradle-suppressible-error-prone/src/test/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOptionTest.java index 546d98ee..c741161e 100644 --- a/gradle-suppressible-error-prone/src/test/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOptionTest.java +++ b/gradle-suppressible-error-prone/src/test/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOptionTest.java @@ -20,6 +20,7 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.DoNotModify; +import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.ModifiedFile; import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.MustModify; import java.util.List; import org.junit.jupiter.api.Test; @@ -27,36 +28,42 @@ class ModifyCheckApiOptionTest { @Test void combining_must_modify_instances_with_different_visitor_states_results_in_true() { - MustModify withoutVisitorState = new MustModify(false); - MustModify withVisitorState = new MustModify(true); + MustModify withoutVisitorState = ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO); + MustModify withVisitorState = + ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO, ModifiedFile.VISITOR_STATE); - assertThat(withoutVisitorState.combine(withVisitorState).modifyVisitorState()) + assertThat(withoutVisitorState.combine(withVisitorState).modifiedFiles().contains(ModifiedFile.VISITOR_STATE)) .isTrue(); - assertThat(withVisitorState.combine(withoutVisitorState).modifyVisitorState()) + assertThat(withVisitorState.combine(withoutVisitorState).modifiedFiles().contains(ModifiedFile.VISITOR_STATE)) .isTrue(); } @Test void combining_must_modify_instances_with_same_visitor_states_preserves_state() { - MustModify bothFalse1 = new MustModify(false); - MustModify bothFalse2 = new MustModify(false); - assertThat(bothFalse1.combine(bothFalse2).modifyVisitorState()).isFalse(); + MustModify bothFalse1 = ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO); + MustModify bothFalse2 = ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO); + assertThat(bothFalse1.combine(bothFalse2).modifiedFiles().contains(ModifiedFile.VISITOR_STATE)) + .isFalse(); - MustModify bothTrue1 = new MustModify(true); - MustModify bothTrue2 = new MustModify(true); - assertThat(bothTrue1.combine(bothTrue2).modifyVisitorState()).isTrue(); + MustModify bothTrue1 = + ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO, ModifiedFile.VISITOR_STATE); + MustModify bothTrue2 = + ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO, ModifiedFile.VISITOR_STATE); + assertThat(bothTrue1.combine(bothTrue2).modifiedFiles().contains(ModifiedFile.VISITOR_STATE)) + .isTrue(); } @Test void combining_empty_collection_returns_must_modify() { - assertThat(ModifyCheckApiOption.combine(List.of())).isEqualTo(new MustModify(false)); + assertThat(ModifyCheckApiOption.combine(List.of())) + .isEqualTo(ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO)); } @Test void combining_only_no_effect_returns_must_modify() { assertThat(ModifyCheckApiOption.combine( List.of(ModifyCheckApiOption.noEffect(), ModifyCheckApiOption.noEffect()))) - .isEqualTo(new MustModify(false)); + .isEqualTo(ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO)); } @Test @@ -71,23 +78,24 @@ void combining_do_not_modify_with_no_effect_returns_do_not_modify() { void combining_must_modify_with_no_effect_causes_no_change() { assertThat(ModifyCheckApiOption.combine( List.of(ModifyCheckApiOption.mustModify(), ModifyCheckApiOption.noEffect()))) - .isEqualTo(new MustModify(false)); + .isEqualTo(ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO)); } @Test void combining_must_modify_including_visitor_state_with_no_effect_causes_no_change() { assertThat(ModifyCheckApiOption.combine(List.of( - ModifyCheckApiOption.mustModifyIncludingVisitorState(), ModifyCheckApiOption.noEffect()))) - .isEqualTo(new MustModify(true)); + ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO, ModifiedFile.VISITOR_STATE), + ModifyCheckApiOption.noEffect()))) + .isEqualTo(ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO, ModifiedFile.VISITOR_STATE)); } @Test void combining_multiple_must_modify_options_combines_visitor_states() { assertThat(ModifyCheckApiOption.combine(List.of( - ModifyCheckApiOption.mustModify(), - ModifyCheckApiOption.mustModifyIncludingVisitorState(), + ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO), + ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO, ModifiedFile.VISITOR_STATE), ModifyCheckApiOption.noEffect()))) - .isEqualTo(new MustModify(true)); + .isEqualTo(ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO, ModifiedFile.VISITOR_STATE)); } @Test 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 5026aade..a64bbf7b 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 @@ -18,12 +18,16 @@ import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.AssignmentTree; +import com.sun.source.tree.ClassTree; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.LiteralTree; import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.ModifiersTree; import com.sun.source.tree.NewArrayTree; import com.sun.source.tree.Tree; +import com.sun.source.tree.VariableTree; import java.util.stream.Stream; import javax.lang.model.element.Name; @@ -71,5 +75,17 @@ static boolean isSuppressWarningsAnnotation(AnnotationTree annotation) { return AnnotationUtils.annotationName(annotation.getAnnotationType()).contentEquals("SuppressWarnings"); } + static ModifiersTree getModifiers(Tree suppressible) { + if (suppressible instanceof ClassTree classTree) { + return classTree.getModifiers(); + } else if (suppressible instanceof MethodTree methodTree) { + return methodTree.getModifiers(); + } else if (suppressible instanceof VariableTree variableTree) { + return variableTree.getModifiers(); + } else { + throw new IllegalArgumentException("Unsupported tree type: " + suppressible.getClass()); + } + } + 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 deleted file mode 100644 index d0b2ae36..00000000 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/BugCheckerRegistry.java +++ /dev/null @@ -1,73 +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 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/SuppressingFix.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/LazySuppressionFix.java similarity index 54% rename from suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressingFix.java rename to suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/LazySuppressionFix.java index 650497c2..0ff6c62c 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressingFix.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/LazySuppressionFix.java @@ -29,26 +29,40 @@ import java.util.Set; import java.util.function.Function; -final class SuppressingFix implements Fix { - private final Set newSuppressions = new HashSet<>(); +/** + * A Fix that can lazily add/modify/remove @SuppressWarnings annotations with proper line handling. + * When removing suppressions entirely, it will also remove preceding whitespace up to and including + * the newline to avoid leaving empty lines. + * Sorts suppressions by human-authored first, then alphabetically. + */ +final class LazySuppressionFix implements Fix { + private final Set desiredSuppressions = new HashSet<>(); private final Function> replacement; - SuppressingFix(Optional sourceCode, Optional suppressWarnings, Tree tree) { - // See note in SuppressingReplacement about when we have to calculate stuff - // We *cannot* simply make this a memoized supplier. The first thing error-prone does with the Fix is to - // evaluate it to produce a nice error message, and we don't want to fix the number of suppression we make - // until we're ready to produce the Replacement after *all* the error-prone checks have been run. - // In order for SuppressingReplacement to calculate source code positions elements when it's constructed, it - // needs an EndPosTable. However, we don't get the EndPosTable until getReplacements is called. So we have - // to use this FirstTimeMemoizingFunction thing, that will allow use to defer creating the Replacement until - // we have access to the EndPosTable, then keep hold of the created SuppressingReplacement. We only need a - // single instance of EndPosTable to evaluate the source positions exactly once, so this works out. - this.replacement = new FirstTimeMemoizingFunction<>((EndPosTable endPositions) -> ImmutableSet.of( - new SuppressingReplacement(endPositions, newSuppressions, sourceCode, suppressWarnings, tree))); + private LazySuppressionFix( + Optional sourceCode, Optional suppression, Tree declaration) { + // Defer replacement creation until we have EndPosTable and all suppressions have been added + this.replacement = new FirstTimeMemoizingFunction<>( + (EndPosTable endPositions) -> ImmutableSet.of(new LazySuppressionReplacement( + endPositions, desiredSuppressions, sourceCode, suppression, declaration))); + } + + LazySuppressionFix( + Optional sourceCode, + Optional suppressWarnings, + Tree tree, + Set desiredSuppressions) { + this(sourceCode, suppressWarnings, tree); + this.desiredSuppressions.addAll(desiredSuppressions); + } + + public static LazySuppressionFix empty( + Optional sourceCode, Optional suppression, Tree tree) { + return new LazySuppressionFix(sourceCode, suppression, tree); } public void addSuppression(String suppression) { - newSuppressions.add(suppression); + desiredSuppressions.add(suppression); } @Override @@ -58,12 +72,12 @@ public ImmutableSet getReplacements(EndPosTable endPositions) { @Override public String toString(JCCompilationUnit compilationUnit) { - return "SuppressingFix"; + return "LazySuppressionFix"; } @Override public String getShortDescription() { - return "Adding automatic suppressions"; + return "Adding/modifying/removing @SuppressWarnings with proper line handling"; } @Override diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressingReplacement.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/LazySuppressionReplacement.java similarity index 62% rename from suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressingReplacement.java rename to suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/LazySuppressionReplacement.java index be92a289..a88e0d55 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressingReplacement.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/LazySuppressionReplacement.java @@ -27,42 +27,37 @@ import java.util.Set; import java.util.stream.Collectors; -final class SuppressingReplacement extends Replacement { - // We really do need to be this lazy for generating the Replacements, as error-prone immediately converts - // the Fix to a Replacement when a Description is given to it, and we need to defer the computation of the - // Replacement until a number of Descriptions have been produced, to handle multiple errors being suppressed - // at the same level. - +/** + * A Replacement that handles @SuppressWarnings modifications with line-removing capabilities. + * When the final replacement text is empty (no suppressions), it will remove preceding + * whitespace up to and including the newline. + */ +final class LazySuppressionReplacement extends Replacement { private final Range range; private final List existingSuppressions; private final String suffix; - private final Set newSuppressions; + private final Set desiredSuppressions; + private final Optional sourceCode; + private final Optional suppressWarnings; - SuppressingReplacement( + LazySuppressionReplacement( EndPosTable endPositions, - Set newSuppressions, + Set desiredSuppressions, Optional sourceCode, Optional suppressWarnings, - Tree tree) { - // Note this is a *mutable* set from SuppressingFix, we need to able to add a new suppression before this - // instance is instantiated - this.newSuppressions = newSuppressions; + Tree declaration) { + this.desiredSuppressions = desiredSuppressions; + this.sourceCode = sourceCode; + this.suppressWarnings = suppressWarnings; - // There is an additional issue that by the time error-prone comes around to apply the replacements, the - // compiler seems to change the representation of the tree for another phase - `App.Builder` becomes - // `App$Builder` etc and the start position for the expression changes to be after `App` rather than at the - // start of `App`. If we calculate the replacement range too late, we insert our @SuppressWarnings at the - // wrong location, and the indentation is miscalculated. But we can't calculate the replacement string - // straight away, as we might not have all the new suppressions added yet. So we have to immediately - // calculate the replacement range and indentation/suffix, but hold off building the final replacement - // string until we have all the new suppressions. - this.range = calculateRange(endPositions, suppressWarnings, tree); + // Calculate range immediately to avoid tree representation changes + this.range = calculateRange(endPositions, suppressWarnings, declaration); this.suffix = suppressWarnings // If we're replacing an existing @SuppressWarnings, there's no need to add an indent .map(_ignored -> "") // If we're adding a new @SuppressWarnings, we need to indent the next line correctly - .orElseGet(() -> "\n" + SourceCodeUtils.indentForTree(sourceCode, tree)); + .orElseGet(() -> "\n" + SourceCodeUtils.indentForTree(sourceCode, declaration)); this.existingSuppressions = suppressWarnings.stream() .flatMap(AnnotationUtils::annotationStringValues) @@ -76,8 +71,19 @@ public Range range() { @Override public String replaceWith() { + String result = calculateReplacementText(); + return result; + } + + private String calculateReplacementText() { + if (desiredSuppressions.isEmpty()) { + // No suppressions left, return empty string (line removal will be handled by range()) + return ""; + } + + // Generate the @SuppressWarnings annotation with remaining suppressions return SuppressWarningsUtils.suppressWarningsString( - SuppressWarningsUtils.modifySuppressions(existingSuppressions, newSuppressions)) + SuppressWarningsUtils.sortHumanFirstThenAlphabetical(desiredSuppressions)) + suffix; } @@ -85,7 +91,7 @@ private static Range calculateRange( EndPosTable endPositions, Optional suppressWarnings, Tree tree) { return suppressWarnings .map(annotationTree -> { - // @SuppressWarnings already exists, we need to replace the whole expression with our own + // @SuppressWarnings already exists, we need to replace the whole expression DiagnosticPosition position = (DiagnosticPosition) annotationTree; return Range.closedOpen(position.getStartPosition(), position.getEndPosition(endPositions)); }) 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 deleted file mode 100644 index 9d5eb445..00000000 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/LineRemovingReplacementFix.java +++ /dev/null @@ -1,93 +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.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 2ffc8ce0..9aae843e 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 @@ -22,8 +22,10 @@ import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.matchers.Description; import com.sun.source.tree.AnnotationTree; -import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition; +import com.sun.source.tree.Tree; +import java.util.HashSet; import java.util.List; +import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; import javax.lang.model.element.Name; @@ -81,10 +83,13 @@ public Description matchAnnotation(AnnotationTree tree, VisitorState state) { return Description.NO_MATCH; } - String updatedText = SuppressWarningsUtils.suppressWarningsString(updatedSuppressions); - + Tree declaration = state.getPath().getParentPath().getParentPath().getLeaf(); return buildDescription(tree) - .addFix(new LineRemovingReplacementFix(state.getSourceCode(), (DiagnosticPosition) tree, updatedText)) + .addFix(new LazySuppressionFix( + Optional.ofNullable(state.getSourceCode()), + Optional.of(tree), + declaration, + new HashSet<>(updatedSuppressions))) .build(); } } 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 deleted file mode 100644 index b4403c14..00000000 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveUnusedSuppressions.java +++ /dev/null @@ -1,186 +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.auto.service.AutoService; -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.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.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.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 Supplier enabledBugCheckers = - VisitorState.memoize(BugCheckerRegistry::constructFromEnabledCheckers); - - @Override - public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) { - UnusedSuppressionsTree unusedSuppressionsTree = UnusedSuppressionsTree.initializeWithSuppressions(tree); - - if (unusedSuppressionsTree.allSuppressionNames().isEmpty()) { - return Description.NO_MATCH; - } - - for (String suppression : unusedSuppressionsTree.allSuppressionNames()) { - Optional bugCheckerMaybe = enabledBugCheckers.get(state).get(suppression); - if (bugCheckerMaybe.isEmpty()) { - unusedSuppressionsTree.markAllSuppressionsAsUsed(suppression); - continue; - } - - // 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) { - unusedSuppressionsTree.flagFirstParentSuppressionAsUsed( - description.position.getTree(), description.checkName); - } - }, - state.severityMap(), - ignoreSuppressions(state.errorProneOptions())) - .withPath(state.getPath()); - new ErrorProneScanner(bugCheckerMaybe.get()).scan(tree, customState); - } - - for (TreeWithUnusedSuppressions treeWithUnusedSuppressions : unusedSuppressionsTree.unused()) { - Set unusedSuppressions = treeWithUnusedSuppressions.unusedSuppressions(); - List suppressions = - getModifiersTree(treeWithUnusedSuppressions).getAnnotations().stream() - .filter(AnnotationUtils::isSuppressWarningsAnnotation) - .toList(); - for (AnnotationTree suppression : suppressions) { - Fix fix = createFix(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) { - Tree declarationTree = treeWithUnusedSuppressions.tree(); - if (declarationTree instanceof MethodTree methodTree) { - return methodTree.getModifiers(); - } else if (declarationTree instanceof ClassTree classTree) { - return classTree.getModifiers(); - } else if (declarationTree instanceof VariableTree variableTree) { - return variableTree.getModifiers(); - } else { - throw new IllegalStateException("Unexpected tree type: " + declarationTree.getClass()); - } - } - - // 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") - 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); - } - - private static Fix createFix(AnnotationTree suppressWarnings, Set unusedSuppressions, VisitorState state) { - List currentSuppressions = - AnnotationUtils.annotationStringValues(suppressWarnings).toList(); - List remainingSuppressions = currentSuppressions.stream() - .filter(s -> !unusedSuppressions.contains(s)) - .collect(Collectors.toList()); - 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/SuppressWarningsUtils.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressWarningsUtils.java index b89c7f18..0b12136a 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressWarningsUtils.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressWarningsUtils.java @@ -16,8 +16,16 @@ package com.palantir.suppressibleerrorprone; +import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.VariableTree; +import java.util.Collection; +import java.util.Comparator; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.function.Predicate; import java.util.stream.Collectors; @@ -33,6 +41,14 @@ public static SuppressionsType fromName(String name) { } } + public static List sortHumanFirstThenAlphabetical(Collection suppressions) { + return suppressions.stream() + .sorted(Comparator.comparing((String suppression) -> + SuppressionsType.fromName(suppression) == SuppressionsType.AUTOMATICALLY_ADDED) + .thenComparing(String::compareTo)) + .collect(Collectors.toList()); + } + public static List modifySuppressions( List existingSuppressions, Set newAutomatedSuppressions) { Map> automaticallyAddedOrNotSuppressions = existingSuppressions.stream() @@ -74,5 +90,23 @@ public static String suppressWarningsString(List warningsToSuppress) { return "@" + CommonConstants.SUPPRESS_WARNINGS_ANNOTATION + "(" + suppressWarningsString + ")"; } + /** + * Only these trees are suppressible, as per + * error-prone. + */ + public static boolean isSuppressible(Tree tree) { + return tree instanceof ClassTree || tree instanceof MethodTree || tree instanceof VariableTree; + } + + public static Optional getSuppressWarnings(Tree suppressible) { + if (!isSuppressible(suppressible)) { + throw new IllegalArgumentException("Suppress annotations not allowed in " + suppressible); + } + + return AnnotationUtils.getModifiers(suppressible).getAnnotations().stream() + .filter(AnnotationUtils::isSuppressWarningsAnnotation) + .findFirst(); + } + private SuppressWarningsUtils() {} } diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleErrorProne.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleErrorProne.java new file mode 100644 index 00000000..e22b9d32 --- /dev/null +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleErrorProne.java @@ -0,0 +1,32 @@ +/* + * (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.errorprone.BugPattern; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.bugpatterns.BugChecker; + +@AutoService(BugChecker.class) +@BugPattern( + link = "https://github.com/palantir/suppressible-error-prone", + linkType = BugPattern.LinkType.CUSTOM, + severity = SeverityLevel.WARNING, + summary = "A change made by suppressible-error-prone", + // Make it unsuppressible so that it can actually remove itself + suppressionAnnotations = {}) +public class SuppressibleErrorProne extends BugChecker {} 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 deleted file mode 100644 index 5d5543fd..00000000 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/UnusedSuppressionsTree.java +++ /dev/null @@ -1,158 +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.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; - -public class UnusedSuppressionsTree { - private final Map pathCache; // exists for performance reasons - - private final Map> treeToSuppressions; - private final Map> treeToUsedSuppressions; - - private UnusedSuppressionsTree(Map> treeToSuppressions, Map pathCache) { - this.pathCache = Map.copyOf(pathCache); - this.treeToSuppressions = Map.copyOf(treeToSuppressions); - this.treeToUsedSuppressions = new HashMap<>(); - treeToSuppressions.keySet().forEach(tree -> treeToUsedSuppressions.put(tree, ConcurrentHashMap.newKeySet())); - } - - public static UnusedSuppressionsTree initializeWithSuppressions(CompilationUnitTree tree) { - Map> treeToSuppressions = new HashMap<>(); - Map treeToPath = new HashMap<>(); - - new TreePathScanner() { - @Override - public Void scan(Tree tree, Void unused) { - if (tree != null) { - treeToPath.put(tree, new TreePath(getCurrentPath(), tree)); - } - return super.scan(tree, unused); - } - - @Override - public Void visitMethod(MethodTree node, Void unused) { - collectSuppressions(node, node.getModifiers()); - return super.visitMethod(node, unused); - } - - @Override - public Void visitClass(ClassTree node, Void unused) { - collectSuppressions(node, node.getModifiers()); - return super.visitClass(node, unused); - } - - @Override - public Void visitVariable(VariableTree node, Void unused) { - collectSuppressions(node, node.getModifiers()); - return super.visitVariable(node, unused); - } - - 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 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) { - TreePath treePath = pathCache.get(tree); - if (treePath == null) { - return; // Tree not found in our map - } - - for (TreePath path = treePath; path != null; path = path.getParentPath()) { - Tree curr = path.getLeaf(); - Set suppressions = treeToSuppressions.get(curr); - if (suppressions != null && suppressions.contains(suppressionName)) { - treeToUsedSuppressions.get(curr).add(suppressionName); - return; - } - } - } - - public void markAllSuppressionsAsUsed(String suppressionName) { - treeToSuppressions.entrySet().stream() - .filter(entry -> entry.getValue().contains(suppressionName)) - .forEach(entry -> treeToUsedSuppressions.get(entry.getKey()).add(suppressionName)); - } - - public Set unused() { - return treeToSuppressions.entrySet().stream() - .map(entry -> { - Tree tree = entry.getKey(); - Set allSuppressions = entry.getValue(); - Set used = treeToUsedSuppressions.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); - } - } -} diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java index 44eed2d1..0de630c1 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java @@ -23,35 +23,91 @@ import com.google.errorprone.matchers.Description; 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 com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition; +import java.util.HashSet; import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.WeakHashMap; import java.util.logging.Logger; +import java.util.stream.Collectors; import java.util.stream.Stream; import javax.lang.model.element.Name; // CHECKSTYLE:ON +@SuppressWarnings("RestrictedApi") public final class VisitorStateModifications { private static final Logger log = Logger.getLogger(VisitorStateModifications.class.getName()); + private static final class ReportedFixCache { + private final Map cache = new WeakHashMap<>(); + + LazySuppressionFix getOrCreate(Tree declaration, VisitorState visitorState) { + return cache.computeIfAbsent(declaration, _ignored -> createFix(declaration, visitorState)); + } + + private LazySuppressionFix createFix(Tree declaration, VisitorState visitorState) { + if (!SuppressWarningsUtils.isSuppressible(declaration)) { + throw new IllegalArgumentException("Not suppressible: " + declaration); + } + ModifiersTree modifiersTree = modifiersTree(declaration).get(); + Optional suppressWarnings = modifiersTree.getAnnotations().stream() + .filter(annotation -> { + Name annotationName = AnnotationUtils.annotationName(annotation.getAnnotationType()); + return annotationName.contentEquals(CommonConstants.SUPPRESS_WARNINGS_ANNOTATION); + }) + .findFirst(); + + LazySuppressionFix fix = LazySuppressionFix.empty( + Optional.ofNullable(visitorState.getSourceCode()), suppressWarnings, declaration); + Description description = Description.builder( + (DiagnosticPosition) declaration, "SuppressibleErrorProne", "", "") + .addFix(fix) + .build(); + visitorState.reportMatch(description); + return fix; + } + + boolean containsKey(Tree key) { + return cache.containsKey(key); + } + + int size() { + return cache.size(); + } + } + // Weak map so that we don't leak memory by keeping hold of references to the source element tree keys and our // mutable fixes values around forever, once error-prone has finished with the source element tree used as a key // here (once the file has been visited by all the error-prone checks), our SuppressingFixes can be safely // garbage collected. - private static final Map FIXES = new WeakHashMap<>(); + private static final Set SEEN = new HashSet<>(); + private static final Map> OLD_SUPPRESSIONS = new WeakHashMap<>(); + private static final ReportedFixCache FIXES = new ReportedFixCache(); - @SuppressWarnings("RestrictedApi") + @SuppressWarnings("CyclomaticComplexity") // Unfortunately this logic is just complex public static Description interceptDescription(VisitorState visitorState, Description description) { - if (description == Description.NO_MATCH) { + if (description == Description.NO_MATCH || description.checkName.equals("SuppressibleErrorProne")) { return description; } + Set modes = visitorState.errorProneOptions().getFlags().getSetOrEmpty("SuppressibleErrorProne:Mode"); + + CompilationUnitTree compilationUnit = visitorState.getPath().getCompilationUnit(); + + if (modes.contains("RemoveUnused") && !SEEN.contains(compilationUnit)) { + attachEmptySuppressionFixesToAllSuppressibles(compilationUnit, visitorState); + cacheOldSuppressions(compilationUnit, visitorState); + SEEN.add(compilationUnit); + } + // If both -PerrorProneSuppress and -PerrorProneApply are used at the same time, for the checks configured as // "patchChecks" in the extension we need to use their suggested fixes instead of suppressing, so we can do // both in one pass as if you'd run -PerrorProneApply and -PerrorProneSuppress separately. We pass the checks @@ -80,6 +136,29 @@ public static Description interceptDescription(VisitorState visitorState, Descri TreePath pathToActualError = TreePath.getPath(visitorState.getPath().getCompilationUnit(), description.position.getTree()); + Optional firstSuppressibleWhichSuppressesDescription = Stream.iterate( + pathToActualError, treePath -> treePath.getParentPath() != null, TreePath::getParentPath) + .dropWhile(path -> !suppresses(path.getLeaf(), description)) + .findFirst(); + + if (firstSuppressibleWhichSuppressesDescription.isPresent() && modes.contains("RemoveUnused")) { + suppressCheck( + visitorState, + description.checkName, + firstSuppressibleWhichSuppressesDescription.get().getLeaf()); + return Description.NO_MATCH; + } + + if (!modes.contains("Suppress")) { + // Maybe follow the previous behavior of ignoring this diagnostic rather than relaying to javac? + log.warning("No autofix was found for " + description.checkName + " at position " + + description.position.getStartPosition() + " in " + + visitorState.getPath().getCompilationUnit().getSourceFile() + "." + + " -PerrorProneSuppress was not passed either. " + + " SuppressibleErrorProne will not be able to add a suppression for this error."); + return Description.NO_MATCH; + } + Optional firstSuppressible = Stream.iterate( pathToActualError, treePath -> treePath.getParentPath() != null, TreePath::getParentPath) .dropWhile(path -> !suppressibleTree(path.getLeaf())) @@ -98,41 +177,75 @@ public static Description interceptDescription(VisitorState visitorState, Descri Tree firstSuppressibleParent = firstSuppressible.get().getLeaf(); - ModifiersTree modifiersTree = modifiersTree(firstSuppressibleParent).get(); - - Optional suppressWarnings = modifiersTree.getAnnotations().stream() - .filter(annotation -> { - Name annotationName = AnnotationUtils.annotationName(annotation.getAnnotationType()); - return annotationName.contentEquals(CommonConstants.SUPPRESS_WARNINGS_ANNOTATION); - }) - .findFirst(); - // In order to be able to suppress multiple errors in one pass on the same element, we need to do a single // Fix/Replacement in error-prone. It's not possible to do this bit by bit with multiple Replacements. To do // this, we make sure we only make one fix per source element we put the suppression on by using a Map. This // way we have our own mutable Fix that we can add errors to, and only once the file has been visited by all // the error-prone checks it will then produce a replacement with all the checks suppressed. - boolean alreadyReportedFix = FIXES.containsKey(firstSuppressibleParent); + suppressCheck( + visitorState, + CommonConstants.AUTOMATICALLY_ADDED_PREFIX + description.checkName, + firstSuppressibleParent); - SuppressingFix suppressingFix = FIXES.computeIfAbsent( - firstSuppressibleParent, - _ignored -> new SuppressingFix( - Optional.ofNullable(visitorState.getSourceCode()), suppressWarnings, firstSuppressibleParent)); + return Description.NO_MATCH; + } - suppressingFix.addSuppression(description.checkName); + private static void cacheOldSuppressions(CompilationUnitTree compilationUnit, VisitorState visitorState) { + new TreePathScanner() { + @Override + public Void visitAnnotation(AnnotationTree node, Void unused) { + if (AnnotationUtils.isSuppressWarningsAnnotation(node)) { + Tree declaration = + getCurrentPath().getParentPath().getParentPath().getLeaf(); + OLD_SUPPRESSIONS.put( + declaration, + AnnotationUtils.annotationStringValues(node).collect(Collectors.toSet())); + } - // If we already submitted our mutable fix, we don't need to do so again, just need to add the error to the fix. - if (alreadyReportedFix) { - return Description.NO_MATCH; + return super.visitAnnotation(node, unused); + } + }.scan(compilationUnit, null); + } + + private static void suppressCheck(VisitorState visitorState, String suppression, Tree suppressibleParent) { + LazySuppressionFix lazyFix = FIXES.getOrCreate(suppressibleParent, visitorState); + lazyFix.addSuppression(suppression); + } + + private static boolean suppresses(Tree leaf, Description description) { + if (!SuppressWarningsUtils.isSuppressible(leaf)) { + return false; + } + + Optional suppressWarningsMaybe = SuppressWarningsUtils.getSuppressWarnings(leaf); + if (suppressWarningsMaybe.isEmpty()) { + return false; } - return Description.builder( - description.position, - description.checkName, - description.getLink(), - description.getMessageWithoutCheckName()) - .addFix(suppressingFix) - .build(); + String checkName = description.checkName; + return AnnotationUtils.annotationStringValues(suppressWarningsMaybe.get()) + .anyMatch(checkName::equals); + } + + private static void attachEmptySuppressionFixesToAllSuppressibles( + CompilationUnitTree compilationUnit, VisitorState visitorState) { + + new TreePathScanner() { + @Override + public Void visitAnnotation(AnnotationTree node, Void unused) { + if (AnnotationUtils.isSuppressWarningsAnnotation(node)) { + Tree declaration = + getCurrentPath().getParentPath().getParentPath().getLeaf(); + Optional source = Optional.ofNullable(visitorState.getSourceCode()); + + // Start by making all suppressions empty + // Then, as we encounter Descriptions without fixes, we add back the closest suppression + LazySuppressionFix fix = FIXES.getOrCreate(declaration, visitorState); + } + + return super.visitAnnotation(node, unused); + } + }.scan(compilationUnit, null); } private static boolean suppressibleTree(Tree tree) { diff --git a/versions.lock b/versions.lock index fd78de54..1752d24b 100644 --- a/versions.lock +++ b/versions.lock @@ -24,7 +24,7 @@ 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 (10 constraints: 73a7e8ff) +com.google.guava:guava:33.5.0-jre (13 constraints: a0e99652) com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava (1 constraints: bd17c918) @@ -58,8 +58,22 @@ org.pcollections:pcollections:4.0.1 (1 constraints: f21028b8) cglib:cglib-nodep:3.2.2 (1 constraints: 490ded24) +com.fasterxml.jackson.core:jackson-annotations:2.19.2 (3 constraints: bd3ddca3) + +com.fasterxml.jackson.core:jackson-core:2.19.2 (5 constraints: 2f6b3a28) + +com.fasterxml.jackson.core:jackson-databind:2.19.2 (4 constraints: a2584b0d) + +com.fasterxml.jackson.datatype:jackson-datatype-guava:2.19.2 (1 constraints: 8a142d90) + +com.fasterxml.jackson.datatype:jackson-datatype-jdk8:2.19.2 (1 constraints: 11132d39) + +com.fasterxml.jackson.module:jackson-module-parameter-names:2.19.2 (1 constraints: 11132d39) + com.google.auto.value:auto-value:1.10 (1 constraints: e711f8e8) +com.google.code.findbugs:jsr305:3.0.2 (1 constraints: d8120c26) + com.google.errorprone:error_prone_test_helpers:2.41.0 (1 constraints: 3e054c3b) com.google.jimfs:jimfs:1.3.0 (1 constraints: 5a141061) @@ -74,6 +88,10 @@ com.palantir.gradle.plugintesting:configuration-cache-spec:0.11.0 (1 constraints com.palantir.gradle.plugintesting:plugin-testing-core:0.11.0 (1 constraints: 3405263b) +com.palantir.javaformat:palantir-java-format:2.74.0 (1 constraints: 3f05533b) + +com.palantir.javaformat:palantir-java-format-spi:2.74.0 (1 constraints: 10133439) + commons-io:commons-io:2.20.0 (1 constraints: 3605333b) junit:junit:4.13.2 (4 constraints: 873fce86) @@ -86,6 +104,8 @@ org.assertj:assertj-core:3.27.6 (1 constraints: 4405543b) org.codehaus.groovy:groovy:3.0.25 (3 constraints: 14347add) +org.functionaljava:functionaljava:4.8 (1 constraints: 81129900) + org.hamcrest:hamcrest:2.2 (2 constraints: 43187376) org.hamcrest:hamcrest-core:2.2 (3 constraints: 84262b80) diff --git a/versions.props b/versions.props index b87dd2f4..54a1fae3 100644 --- a/versions.props +++ b/versions.props @@ -3,6 +3,7 @@ com.google.guava:guava = 33.5.0-jre com.netflix.nebula:nebula-test = 10.6.2 com.palantir.gradle.utils:environment-variables = 0.19.0 com.palantir.gradle.plugintesting:* = 0.11.0 +com.palantir.javaformat:* = 2.74.0 commons-io:commons-io = 2.20.0 net.ltgt.gradle:gradle-errorprone-plugin = 4.3.0 org.assertj:assertj-core = 3.27.6 From d83a78a2b92ef4cd08073c748d7dd32e5d58a9f3 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Mon, 29 Sep 2025 14:54:59 +0100 Subject: [PATCH 14/15] limao --- .../modes/common/ModifyCheckApiOption.java | 2 +- .../SuppressibleErrorPronePluginIntegrationTest.groovy | 4 ++-- .../modes/common/ModifyCheckApiOptionTest.java | 5 +++-- .../RemoveRolloutSuppressionsTest.java | 6 +++--- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOption.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOption.java index 5b6bce13..68958a64 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOption.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOption.java @@ -50,7 +50,7 @@ static ModifyCheckApiOption noEffect() { } /** - * Modify these files in the error-prone API + * Modify these files in the error-prone API. */ static MustModify mustModify(ModifiedFile... modifiedFile) { return new MustModify(Arrays.stream(modifiedFile).collect(Collectors.toSet())); 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 63756bd2..35b6e08e 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 @@ -1402,7 +1402,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec @SuppressWarnings("InlineTrivialConstant") private static final String EMPTY_STRING = ""; - @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) // Doesn't move existing ArrayEquals closer to the violation + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) // Doesn't move an already existing suppression, even if it could be closer to the violation static class Inner { @SuppressWarnings("InlineTrivialConstant") private static final String EMPTY = ""; @@ -1431,7 +1431,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec @SuppressWarnings("InlineTrivialConstant") private static final String EMPTY_STRING = ""; - @SuppressWarnings("ArrayEquals") // Doesn't move existing ArrayEquals closer to the violation + @SuppressWarnings("ArrayEquals") // Doesn't move an already existing suppression, even if it could be closer to the violation static class Inner { @SuppressWarnings("InlineTrivialConstant") private static final String EMPTY = ""; diff --git a/gradle-suppressible-error-prone/src/test/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOptionTest.java b/gradle-suppressible-error-prone/src/test/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOptionTest.java index c741161e..019e4ef8 100644 --- a/gradle-suppressible-error-prone/src/test/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOptionTest.java +++ b/gradle-suppressible-error-prone/src/test/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOptionTest.java @@ -76,8 +76,9 @@ void combining_do_not_modify_with_no_effect_returns_do_not_modify() { @Test void combining_must_modify_with_no_effect_causes_no_change() { - assertThat(ModifyCheckApiOption.combine( - List.of(ModifyCheckApiOption.mustModify(), ModifyCheckApiOption.noEffect()))) + assertThat(ModifyCheckApiOption.combine(List.of( + ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO), + ModifyCheckApiOption.noEffect()))) .isEqualTo(ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO)); } diff --git a/suppressible-error-prone/src/test/java/com/palantir/suppressibleerrorprone/RemoveRolloutSuppressionsTest.java b/suppressible-error-prone/src/test/java/com/palantir/suppressibleerrorprone/RemoveRolloutSuppressionsTest.java index 5d8c82a1..89266a24 100644 --- a/suppressible-error-prone/src/test/java/com/palantir/suppressibleerrorprone/RemoveRolloutSuppressionsTest.java +++ b/suppressible-error-prone/src/test/java/com/palantir/suppressibleerrorprone/RemoveRolloutSuppressionsTest.java @@ -178,19 +178,19 @@ public final class App {} } @Test - void do_not_reorder_existing_annotations() { + void sorts_by_human_authored_first_then_by_alnum() { fix().addInputLines( "App.java", // language=Java """ - @SuppressWarnings({"for-rollout:3", "for-rollout:2", "1"}) + @SuppressWarnings({"for-rollout:3", "for-rollout:2", "for-rollout:1", "b", "a"}) public final class App {} """) .addOutputLines( "App.java", // language=Java """ - @SuppressWarnings({"for-rollout:3", "1"}) + @SuppressWarnings({"a", "b", "for-rollout:1", "for-rollout:3"}) public final class App {} """) .setArgs("-XepOpt:" + RemoveRolloutSuppressions.ARGUMENT + "=2") From c8add2f9d7c12742c141d42098af7cfdd5e867ff Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Mon, 29 Sep 2025 14:59:24 +0100 Subject: [PATCH 15/15] limao --- .../SuppressibleErrorPronePluginIntegrationTest.groovy | 4 ---- 1 file changed, 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 35b6e08e..e4397a04 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 @@ -1643,10 +1643,6 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec '''.stripIndent(true) } - def 'errorProneRemoveUnused + errorProneApply + errorProneSuppress works with specified checkers'() { - - } - 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