From c3f0868e851e4810b8285a0bd0b48d9721adab59 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Fri, 17 Oct 2025 12:21:21 +0100 Subject: [PATCH 01/15] Chonky boi --- .../suppressibleerrorprone/modes/Modes.java | 12 +- .../modes/common/CommonOptions.java | 4 +- .../modes/common/ModeName.java | 1 + ...oveRolloutAndSuppressingInterference.java} | 2 +- .../RemoveUnusedAndApplyingInterference.java | 49 +++ .../RemoveUnusedAndSuppressInterference.java | 37 ++ .../modes/modes/ApplyMode.java | 6 + .../modes/modes/RemoveRolloutMode.java | 4 +- .../modes/modes/RemoveUnusedMode.java | 54 +++ .../modes/modes/SuppressMode.java | 6 + ...ibleErrorPronePluginIntegrationTest.groovy | 352 +++++++++++++++++- suppressible-error-prone/build.gradle | 2 + .../AllErrorprones.java | 43 +++ .../AnnotationUtils.java | 20 + .../ReportedFixCache.java | 95 +++++ .../SuppressWarningsUtils.java | 12 + .../SuppressibleErrorProne.java | 32 ++ .../VisitorStateModifications.java | 158 +++++--- 18 files changed, 820 insertions(+), 69 deletions(-) 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 create mode 100644 gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/RemoveUnusedMode.java create mode 100644 suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AllErrorprones.java create mode 100644 suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java create mode 100644 suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleErrorProne.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 32135d9c..32bb5bc8 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,11 +28,14 @@ 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.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; 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,13 +63,16 @@ 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()); private final Set interferences = Set.of( new DisableModeInterference(), - new RemovingAndSuppressingInterference(), - new SuppressingAndApplyingInterference()); + new RemoveRolloutAndSuppressingInterference(), + new SuppressingAndApplyingInterference(), + new RemoveUnusedAndApplyingInterference(), + new RemoveUnusedAndSuppressInterference()); public final CombinedValue modifyCheckApi() { return ModifyCheckApiOption.combine( 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 796db385..730c7c6d 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; /** @@ -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 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/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/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 new file mode 100644 index 00000000..60ef9981 --- /dev/null +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/RemoveUnusedMode.java @@ -0,0 +1,54 @@ +/* + * (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.ModifyCheckApiOption.ModifiedFile; +import com.palantir.gradle.suppressibleerrorprone.modes.common.PatchChecksOption; +import java.util.Map; + +public final class RemoveUnusedMode implements Mode { + private static final String ALL_CHECKS = ""; + + @Override + public ModifyCheckApiOption modifyCheckApi() { + return ModifyCheckApiOption.mustModify( + ModifiedFile.BUG_CHECKER_INFO, ModifiedFile.SUPPRESSIBLE_TREE_PATH_SCANNER, ModifiedFile.VISITOR_STATE); + } + + @Override + public CommonOptions configureAndReturnCommonOptions(ModeOptionContext context) { + return new CommonOptions() { + @Override + public PatchChecksOption patchChecks() { + return PatchChecksOption.allChecks(); + } + + @Override + public Map extraErrorProneCheckOptions() { + return Map.of("SuppressibleErrorProne:Mode", "RemoveUnused"); + } + + @Override + 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 f4b177f5..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 @@ -21,6 +21,7 @@ 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 @@ -35,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/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy b/gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy index 42ffc553..9e9be04c 100644 --- a/gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy +++ b/gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy @@ -92,7 +92,6 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec errorprone 'com.palantir.suppressible-error-prone:test-error-prone-checks:' + project.findProperty("suppressibleErrorProneVersion") } - suppressibleErrorProne { configureEachErrorProneOptions { // These interfere with some tests, so disable them @@ -1611,6 +1610,355 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec !output.contains('[RemoveRolloutSuppressions]') } + def 'errorProneRemoveUnused removes only unused error-prone suppressions, and leaves unknown suppressions untouched'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + + @SuppressWarnings({"ArrayToString", "UnnecessaryFinal", "InlineTrivialConstant", "NotAnErrorProne", "checkstyle:Bla",}) + public final class App { + private static final String EMPTY_STRING = ""; + + public static void main(String[] args) { + new int[3].toString(); + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused=ArrayToString') + + then: + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + @SuppressWarnings({"ArrayToString", "InlineTrivialConstant", "NotAnErrorProne", "checkstyle:Bla"}) + public final class App { + private static final String EMPTY_STRING = ""; + + public static void main(String[] args) { + new int[3].toString(); + } + } + '''.stripIndent(true) + } + + def 'errorProneRemoveUnused only keeps the closest suppression to a violation'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + + @SuppressWarnings("InlineTrivialConstant") + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + @SuppressWarnings("InlineTrivialConstant") + class Inner { + @SuppressWarnings("InlineTrivialConstant") + class InnerInner { + @SuppressWarnings("InlineTrivialConstant") + class InnerInnerInner { + private static final String EMPTY = ""; + } + } + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused') + + then: + + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + class Inner { + class InnerInner { + @SuppressWarnings("InlineTrivialConstant") + class InnerInnerInner { + private static final String EMPTY = ""; + } + } + } + } + '''.stripIndent(true) + } + + def 'errorProneRemoveUnused handles multiple suppressions on different tree types gracefully'() { + // Here we test the three types of trees you can suppress — ClassTree, MethodTree, VariableTree + + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + // Doesn't move an already existing suppression, even if it could be closer to the violation + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + boolean truism = new int[3].equals(new int[3]); + + @SuppressWarnings("InlineTrivialConstant") + static class InnerInner { + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused') + + then: + + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + // Doesn't move an already existing suppression, even if it could be closer to the violation + @SuppressWarnings("ArrayEquals") + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + + boolean truism = new int[3].equals(new int[3]); + + static class InnerInner { + @SuppressWarnings("ArrayEquals") + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + } + + def 'errorProneRemoveUnused removes entire SuppressWarnings annotation when all suppressions are unused'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + + @SuppressWarnings({"UnusedVariable", "ArrayToString"}) + public final class App { + public static void main(String[] args) { + System.out.println("No violations here"); + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused') + + then: + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + public final class App { + public static void main(String[] args) { + System.out.println("No violations here"); + } + } + '''.stripIndent(true) + } + + def 'errorProneRemoveUnused and errorProneSuppress uses existing suppressions if possible'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + + @SuppressWarnings("InlineTrivialConstant") + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + @SuppressWarnings("InlineTrivialConstant") + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + boolean truism = new int[3].equals(new int[3]); + + @SuppressWarnings("InlineTrivialConstant") + static class InnerInner { + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused', '-PerrorProneSuppress') + + then: + + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + + @SuppressWarnings("for-rollout:ArrayEquals") + boolean truism = new int[3].equals(new int[3]); + + static class InnerInner { + @SuppressWarnings("ArrayEquals") + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + } + + def 'errorProneRemoveUnused and errorProneApply applies fixes on previously suppressed elements'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + boolean truism = new int[3].equals(new int[3]); + + @SuppressWarnings("InlineTrivialConstant") + static class InnerInner { + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused', '-PerrorProneApply=ArrayEquals') + + then: + + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + import java.util.Arrays; + + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + + boolean truism = Arrays.equals(new int[3], new int[3]); + + static class InnerInner { + void method() { + Arrays.equals(new int[3], new int[3]); + } + } + } + } + '''.stripIndent(true) + } + + def 'errorProneRemoveUnused + errorProneApply + errorProneSuppress applies fixes and suppressions on previously suppressed elements'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + + @SuppressWarnings("ArrayEquals") + public final class App { + private static final String EMPTY_STRING = ""; + + // Although InlineTrivialConstant can be placed lower in the AST hierarchy, + // we preserve existing suppressions whenever possible rather than move suppressions around. + // Also, note that we don't add for-rollout here. + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + static class Inner { + private static final String EMPTY = ""; + boolean truism = new int[3].equals(new int[3]); + + @SuppressWarnings("InlineTrivialConstant") + static class InnerInner { + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused', '-PerrorProneSuppress', '-PerrorProneApply=ArrayEquals') + + then: + + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + import java.util.Arrays; + + public final class App { + @SuppressWarnings("for-rollout:InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + // Although InlineTrivialConstant can be placed lower in the AST hierarchy, + // we preserve existing suppressions whenever possible rather than move suppressions around. + // Also, note that we don't add for-rollout here. + @SuppressWarnings("InlineTrivialConstant") + static class Inner { + private static final String EMPTY = ""; + boolean truism = Arrays.equals(new int[3], new int[3]); + + static class InnerInner { + void method() { + Arrays.equals(new int[3], new int[3]); + } + } + } + } + '''.stripIndent(true) + } + def 'error-prone dependencies have versions bound together by a virtual platform'() { setup: 'when an error-prone dependency is forced to certain version' // language=Gradle @@ -1714,4 +2062,4 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec def expectedOther = normalizeSource(source) assert outputOther != expectedOther } -} +} \ No newline at end of file diff --git a/suppressible-error-prone/build.gradle b/suppressible-error-prone/build.gradle index ece60e95..98b1067e 100644 --- a/suppressible-error-prone/build.gradle +++ b/suppressible-error-prone/build.gradle @@ -4,6 +4,8 @@ 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' + implementation 'one.util:streamex' 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/AllErrorprones.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AllErrorprones.java new file mode 100644 index 00000000..4f08a94f --- /dev/null +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AllErrorprones.java @@ -0,0 +1,43 @@ +/* + * (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.BugCheckerInfo; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.scanner.BuiltInCheckerSuppliers; +import java.util.ServiceLoader; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import one.util.streamex.StreamEx; + +public final class AllErrorprones { + public static Set allBugcheckerNames() { + Stream pluginChecks = + ServiceLoader.load(BugChecker.class).stream().map(ServiceLoader.Provider::get); + Stream pluginCheckNames = + StreamEx.of(pluginChecks).flatMap(bugchecker -> bugchecker.allNames().stream()); + + Stream builtInChecks = BuiltInCheckerSuppliers.allChecks().getAllChecks().values().stream(); + Stream builtInCheckNames = + StreamEx.of(builtInChecks).flatMap(bugchecker -> bugchecker.allNames().stream()); + + return Stream.concat(pluginCheckNames, builtInCheckNames).collect(Collectors.toSet()); + } + + private AllErrorprones() {} +} 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..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; @@ -67,5 +71,21 @@ static Name annotationName(Tree annotationType) { "Unsupported annotation type: " + annotationType.getClass().getCanonicalName()); } + 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/ReportedFixCache.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java new file mode 100644 index 00000000..a51cf597 --- /dev/null +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java @@ -0,0 +1,95 @@ +/* + * (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.base.Predicates; +import com.google.errorprone.VisitorState; +import com.google.errorprone.matchers.Description; +import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.ModifiersTree; +import com.sun.source.tree.Tree; +import com.sun.source.util.TreePath; +import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.WeakHashMap; +import java.util.function.Predicate; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import javax.lang.model.element.Name; + +final class ReportedFixCache { + private final Map cache = new WeakHashMap<>(); + private final Predicate initializer; + + private ReportedFixCache(Predicate initializer) { + this.initializer = initializer; + } + + public static ReportedFixCache startWithUnmodified() { + return new ReportedFixCache(suppression -> true); + } + + public static ReportedFixCache startWithRemovingAll() { + return new ReportedFixCache(suppression -> false); + } + + public static ReportedFixCache startWithRemoving(Set suppressionsToRemove) { + System.err.println(suppressionsToRemove); + return new ReportedFixCache(Predicates.not(suppressionsToRemove::contains)); + } + + /** + * Gets an existing fix on this declaration if it exists. Otherwise, initialize the fix with + * declaration is a TreePath because we need information about it's parent to check if it is suppressible + */ + public LazySuppressionFix getOrReportNew(TreePath declaration, VisitorState visitorState) { + return cache.computeIfAbsent(declaration.getLeaf(), _ignored -> createAndReportFix(declaration, visitorState)); + } + + @SuppressWarnings("RestrictedApi") + private LazySuppressionFix createAndReportFix(TreePath declaration, VisitorState state) { + if (!SuppressWarningsUtils.suppressibleTreePath(declaration)) { + throw new IllegalArgumentException("Not suppressible: " + declaration); + } + ModifiersTree modifiersTree = AnnotationUtils.getModifiers(declaration.getLeaf()); + Optional suppressWarnings = modifiersTree.getAnnotations().stream() + .filter(annotation -> { + Name annotationName = AnnotationUtils.annotationName(annotation.getAnnotationType()); + return annotationName.contentEquals(CommonConstants.SUPPRESS_WARNINGS_ANNOTATION); + }) + .findFirst(); + + Stream allExistingSuppressions = + suppressWarnings.map(AnnotationUtils::annotationStringValues).orElseGet(Stream::of); + Set filteredExistingSuppressions = + allExistingSuppressions.filter(initializer).collect(Collectors.toSet()); + + LazySuppressionFix fix = new LazySuppressionFix( + Optional.ofNullable(state.getSourceCode()), + suppressWarnings, + declaration.getLeaf(), + filteredExistingSuppressions); + Description description = Description.builder( + (DiagnosticPosition) declaration.getLeaf(), "SuppressibleErrorProne", "", "") + .addFix(fix) + .build(); + state.reportMatch(description); + return fix; + } +} 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 ed390993..c5773298 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,6 +16,7 @@ package com.palantir.suppressibleerrorprone; +import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.ClassTree; import com.sun.source.tree.LambdaExpressionTree; import com.sun.source.tree.MethodTree; @@ -26,6 +27,7 @@ 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; @@ -130,5 +132,15 @@ private static boolean isLambdaParameter(TreePath tree) { return lambdaExpressionTree.getParameters().contains(variableTree); } + public static Optional getSuppressWarnings(TreePath suppressible) { + if (!suppressibleTreePath(suppressible)) { + throw new IllegalArgumentException("Suppress annotations not allowed in " + suppressible); + } + + return AnnotationUtils.getModifiers(suppressible.getLeaf()).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/VisitorStateModifications.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java index d6f17b21..699d728b 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 @@ -22,12 +22,11 @@ import com.google.errorprone.VisitorState; import com.google.errorprone.matchers.Description; import com.sun.source.tree.AnnotationTree; -import com.sun.source.tree.ClassTree; -import com.sun.source.tree.MethodTree; -import com.sun.source.tree.ModifiersTree; +import com.sun.source.tree.CompilationUnitTree; 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.HashSet; import java.util.Map; import java.util.Optional; import java.util.Set; @@ -35,10 +34,9 @@ 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()); @@ -46,14 +44,28 @@ public final class VisitorStateModifications { // 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 = + ReportedFixCache.startWithRemoving(AllErrorprones.allBugcheckerNames()); - @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 @@ -82,6 +94,26 @@ public static Description interceptDescription(VisitorState visitorState, Descri TreePath pathToActualError = TreePath.getPath(visitorState.getPath().getCompilationUnit(), description.position.getTree()); + Optional firstSuppressibleWhichSuppressesDescription = Stream.iterate( + pathToActualError, treePath -> treePath.getParentPath() != null, TreePath::getParentPath) + .dropWhile(path -> !suppresses(path, description)) + .findFirst(); + + if (firstSuppressibleWhichSuppressesDescription.isPresent() && modes.contains("RemoveUnused")) { + suppressCheck(visitorState, description.checkName, firstSuppressibleWhichSuppressesDescription.get()); + 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 -> !SuppressWarningsUtils.suppressibleTreePath(path)) @@ -98,71 +130,75 @@ public static Description interceptDescription(VisitorState visitorState, Descri return Description.NO_MATCH; } - 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); - - LazySuppressionFix suppressingFix = FIXES.computeIfAbsent(firstSuppressibleParent, _ignored -> { - // Initialize every fix with all remove-rollout: suppressions removed - Stream existingSuppressions = suppressWarnings - .map(AnnotationUtils::annotationStringValues) - .orElseGet(Stream::of); - Set existingNonRolloutSuppressions = existingSuppressions - .filter(sup -> !sup.startsWith(CommonConstants.AUTOMATICALLY_ADDED_PREFIX)) - .collect(Collectors.toSet()); - return new LazySuppressionFix( - Optional.ofNullable(visitorState.getSourceCode()), - suppressWarnings, - firstSuppressibleParent, - existingNonRolloutSuppressions); - }); - - suppressingFix.addSuppression(CommonConstants.AUTOMATICALLY_ADDED_PREFIX + description.checkName); - - // 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; - } + suppressCheck( + visitorState, + CommonConstants.AUTOMATICALLY_ADDED_PREFIX + description.checkName, + firstSuppressible.get()); - return Description.builder( - description.position, - description.checkName, - description.getLink(), - description.getMessageWithoutCheckName()) - .addFix(suppressingFix) - .build(); + return Description.NO_MATCH; } - private static Optional modifiersTree(Tree tree) { - // This covers all type definitions eg class, interface, enum, record, annotation, future kinds - // of class-like type definitions. - if (tree instanceof ClassTree classTree) { - return Optional.of(classTree.getModifiers()); - } + 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())); + } + + return super.visitAnnotation(node, unused); + } + }.scan(compilationUnit, null); + } + + private static void suppressCheck(VisitorState visitorState, String suppression, TreePath suppressibleParent) { + LazySuppressionFix lazyFix = FIXES.getOrReportNew(suppressibleParent, visitorState); + lazyFix.addSuppression(suppression); + } - if (tree instanceof MethodTree methodTree) { - return Optional.of(methodTree.getModifiers()); + private static boolean suppresses(TreePath declaration, Description description) { + if (!SuppressWarningsUtils.suppressibleTreePath(declaration)) { + return false; } - if (tree instanceof VariableTree variableTree) { - return Optional.of(variableTree.getModifiers()); + Optional suppressWarningsMaybe = + SuppressWarningsUtils.getSuppressWarnings(declaration); + if (suppressWarningsMaybe.isEmpty()) { + return false; } - return Optional.empty(); + 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)) { + TreePath declaration = getCurrentPath().getParentPath().getParentPath(); + 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.getOrReportNew(declaration, visitorState); + } + + return super.visitAnnotation(node, unused); + } + }.scan(compilationUnit, null); } private VisitorStateModifications() {} From f05e3f4ebae5663c62bc0f1ba3e1e36ef535601a Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Mon, 20 Oct 2025 17:24:13 +0100 Subject: [PATCH 02/15] Push # Conflicts: # versions.lock --- .../ReportedFixCache.java | 51 ++++++++++-------- .../VisitorStateModifications.java | 54 +++++-------------- versions.lock | 30 ++++++----- 3 files changed, 59 insertions(+), 76 deletions(-) diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java index a51cf597..5017353f 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java @@ -16,7 +16,6 @@ package com.palantir.suppressibleerrorprone; -import com.google.common.base.Predicates; import com.google.errorprone.VisitorState; import com.google.errorprone.matchers.Description; import com.sun.source.tree.AnnotationTree; @@ -24,7 +23,6 @@ import com.sun.source.tree.Tree; import com.sun.source.util.TreePath; import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition; -import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.WeakHashMap; @@ -34,36 +32,43 @@ import javax.lang.model.element.Name; final class ReportedFixCache { - private final Map cache = new WeakHashMap<>(); - private final Predicate initializer; + // 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 final WeakHashMap cache = new WeakHashMap<>(); - private ReportedFixCache(Predicate initializer) { - this.initializer = initializer; - } - - public static ReportedFixCache startWithUnmodified() { - return new ReportedFixCache(suppression -> true); - } + public static final Predicate REMOVE_EVERYTHING = bc -> false; + public static final Predicate KEEP_EVERYTHING = bc -> true; - public static ReportedFixCache startWithRemovingAll() { - return new ReportedFixCache(suppression -> false); - } + ReportedFixCache() {} - public static ReportedFixCache startWithRemoving(Set suppressionsToRemove) { - System.err.println(suppressionsToRemove); - return new ReportedFixCache(Predicates.not(suppressionsToRemove::contains)); + /** + * When first called on a {@code declaration}, initialize a {@code LazySuppressionFix} on it, choosing which + * suppressions to keep with {@code filterExisting}. Subsequent calls on this {@code declaration} will return the + * initialized {@code LazySuppressionFix}. + */ + public LazySuppressionFix getOrReportNew( + TreePath declaration, VisitorState visitorState, Predicate filterExisting) { + return cache.computeIfAbsent( + declaration.getLeaf(), _ignored -> createAndReportFix(declaration, visitorState, filterExisting)); } /** - * Gets an existing fix on this declaration if it exists. Otherwise, initialize the fix with - * declaration is a TreePath because we need information about it's parent to check if it is suppressible + * Initialize a {@code LazySuppressionFix} on {@code declaration}, asserting that none exists yet. */ - public LazySuppressionFix getOrReportNew(TreePath declaration, VisitorState visitorState) { - return cache.computeIfAbsent(declaration.getLeaf(), _ignored -> createAndReportFix(declaration, visitorState)); + @SuppressWarnings("PreferSafeLoggableExceptions") // It doesn't matter in our internal codebases + public LazySuppressionFix reportNew( + TreePath declaration, VisitorState visitorState, Predicate filterExisting) { + if (cache.containsKey(declaration.getLeaf())) { + throw new IllegalArgumentException("A fix on this declaration already exists"); + } + return cache.put(declaration.getLeaf(), createAndReportFix(declaration, visitorState, filterExisting)); } @SuppressWarnings("RestrictedApi") - private LazySuppressionFix createAndReportFix(TreePath declaration, VisitorState state) { + private LazySuppressionFix createAndReportFix( + TreePath declaration, VisitorState state, Predicate filterExisting) { if (!SuppressWarningsUtils.suppressibleTreePath(declaration)) { throw new IllegalArgumentException("Not suppressible: " + declaration); } @@ -78,7 +83,7 @@ private LazySuppressionFix createAndReportFix(TreePath declaration, VisitorState Stream allExistingSuppressions = suppressWarnings.map(AnnotationUtils::annotationStringValues).orElseGet(Stream::of); Set filteredExistingSuppressions = - allExistingSuppressions.filter(initializer).collect(Collectors.toSet()); + allExistingSuppressions.filter(filterExisting).collect(Collectors.toSet()); LazySuppressionFix fix = new LazySuppressionFix( Optional.ofNullable(state.getSourceCode()), 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 699d728b..a9e75e68 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,16 +23,13 @@ import com.google.errorprone.matchers.Description; import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.CompilationUnitTree; -import com.sun.source.tree.Tree; import com.sun.source.util.TreePath; import com.sun.source.util.TreePathScanner; import java.util.HashSet; -import java.util.Map; import java.util.Optional; import java.util.Set; -import java.util.WeakHashMap; +import java.util.function.Predicate; import java.util.logging.Logger; -import java.util.stream.Collectors; import java.util.stream.Stream; // CHECKSTYLE:ON @@ -40,29 +37,24 @@ public final class VisitorStateModifications { private static final Logger log = Logger.getLogger(VisitorStateModifications.class.getName()); - // 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 Set SEEN = new HashSet<>(); - private static final Map> OLD_SUPPRESSIONS = new WeakHashMap<>(); + private static final ReportedFixCache FIXES = new ReportedFixCache(); + private static final Set ALL_BUGCHECKER_NAMES = AllErrorprones.allBugcheckerNames(); - private static final ReportedFixCache FIXES = - ReportedFixCache.startWithRemoving(AllErrorprones.allBugcheckerNames()); + // Used during RemoveUnused mode to attach fixes when we initially see this compilation unit + private static final Set SEEN = new HashSet<>(); @SuppressWarnings("CyclomaticComplexity") // Unfortunately this logic is just complex public static Description interceptDescription(VisitorState visitorState, Description description) { + // Prevent infinite recursion on reported fixes 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); + removeAllErrorproneSuppressionsOnCompilationUnit(compilationUnit, visitorState); SEEN.add(compilationUnit); } @@ -105,7 +97,6 @@ public static Description interceptDescription(VisitorState visitorState, Descri } 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() + "." @@ -143,25 +134,9 @@ public static Description interceptDescription(VisitorState visitorState, Descri return Description.NO_MATCH; } - 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())); - } - - return super.visitAnnotation(node, unused); - } - }.scan(compilationUnit, null); - } - private static void suppressCheck(VisitorState visitorState, String suppression, TreePath suppressibleParent) { - LazySuppressionFix lazyFix = FIXES.getOrReportNew(suppressibleParent, visitorState); + LazySuppressionFix lazyFix = + FIXES.getOrReportNew(suppressibleParent, visitorState, ReportedFixCache.KEEP_EVERYTHING); lazyFix.addSuppression(suppression); } @@ -181,22 +156,19 @@ private static boolean suppresses(TreePath declaration, Description description) .anyMatch(checkName::equals); } - private static void attachEmptySuppressionFixesToAllSuppressibles( + private static void removeAllErrorproneSuppressionsOnCompilationUnit( CompilationUnitTree compilationUnit, VisitorState visitorState) { + Predicate keepNonErrorproneSuppressions = Predicate.not(ALL_BUGCHECKER_NAMES::contains); new TreePathScanner() { @Override public Void visitAnnotation(AnnotationTree node, Void unused) { if (AnnotationUtils.isSuppressWarningsAnnotation(node)) { TreePath declaration = getCurrentPath().getParentPath().getParentPath(); - 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.getOrReportNew(declaration, visitorState); + FIXES.reportNew(declaration, visitorState, keepNonErrorproneSuppressions); } - return super.visitAnnotation(node, unused); + return super.visitAnnotation(node, null); } }.scan(compilationUnit, null); } diff --git a/versions.lock b/versions.lock index 8c06c735..c68c6397 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 (13 constraints: 4bf2c3cf) +com.google.guava:guava:33.5.0-jre (14 constraints: ef047572) 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 (9 constraints: 8497a3d2) +org.jspecify:jspecify:1.0.0 (10 constraints: 71a8e1bc) org.ow2.asm:asm:9.9 (2 constraints: be0e7759) +org.pcollections:pcollections:4.0.1 (1 constraints: f21028b8) + [Test dependencies] @@ -68,8 +76,6 @@ 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.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 0817d59a5e3206e1c07471de483f06ee3fb49eb7 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Fri, 17 Oct 2025 13:02:11 +0100 Subject: [PATCH 03/15] Improve docs --- .../com/palantir/suppressibleerrorprone/ReportedFixCache.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java index 5017353f..8bd3a775 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java @@ -55,7 +55,8 @@ public LazySuppressionFix getOrReportNew( } /** - * Initialize a {@code LazySuppressionFix} on {@code declaration}, asserting that none exists yet. + * Initialize a {@code LazySuppressionFix} on {@code declaration}, choosing which suppressions to keep with + * {@code filterExisting}. */ @SuppressWarnings("PreferSafeLoggableExceptions") // It doesn't matter in our internal codebases public LazySuppressionFix reportNew( From 3548966c7ac3fc5f9a0f4a8cfaa7bcdaa002c113 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Fri, 17 Oct 2025 13:05:16 +0100 Subject: [PATCH 04/15] Improve docs --- .../suppressibleerrorprone/SuppressibleErrorProne.java | 3 +++ 1 file changed, 3 insertions(+) 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 index e22b9d32..b7e83908 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleErrorProne.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleErrorProne.java @@ -21,6 +21,9 @@ import com.google.errorprone.BugPattern.SeverityLevel; import com.google.errorprone.bugpatterns.BugChecker; +/** + * Used by {@code ReportedFixCache} so we don't recursively call {@code VisitorStateModifications} + */ @AutoService(BugChecker.class) @BugPattern( link = "https://github.com/palantir/suppressible-error-prone", From 48091ca500c803bed8d36f4417b964f91d5c1629 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Fri, 17 Oct 2025 13:33:04 +0100 Subject: [PATCH 05/15] Remove changes to VisitorStateModifications --- ...ibleErrorPronePluginIntegrationTest.groovy | 352 +----------------- .../AllErrorprones.java | 43 --- .../ReportedFixCache.java | 101 ----- .../SuppressibleErrorProne.java | 35 -- .../VisitorStateModifications.java | 145 ++++---- 5 files changed, 73 insertions(+), 603 deletions(-) delete mode 100644 suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AllErrorprones.java delete mode 100644 suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java delete mode 100644 suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleErrorProne.java diff --git a/gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy b/gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy index 9e9be04c..42ffc553 100644 --- a/gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy +++ b/gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy @@ -92,6 +92,7 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec errorprone 'com.palantir.suppressible-error-prone:test-error-prone-checks:' + project.findProperty("suppressibleErrorProneVersion") } + suppressibleErrorProne { configureEachErrorProneOptions { // These interfere with some tests, so disable them @@ -1610,355 +1611,6 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec !output.contains('[RemoveRolloutSuppressions]') } - def 'errorProneRemoveUnused removes only unused error-prone suppressions, and leaves unknown suppressions untouched'() { - // language=Java - writeJavaSourceFileToSourceSets ''' - package app; - - @SuppressWarnings({"ArrayToString", "UnnecessaryFinal", "InlineTrivialConstant", "NotAnErrorProne", "checkstyle:Bla",}) - public final class App { - private static final String EMPTY_STRING = ""; - - public static void main(String[] args) { - new int[3].toString(); - } - } - '''.stripIndent(true) - - when: - runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused=ArrayToString') - - then: - // language=Java - javaSourceIsSyntacticallyEqualTo ''' - package app; - - @SuppressWarnings({"ArrayToString", "InlineTrivialConstant", "NotAnErrorProne", "checkstyle:Bla"}) - public final class App { - private static final String EMPTY_STRING = ""; - - public static void main(String[] args) { - new int[3].toString(); - } - } - '''.stripIndent(true) - } - - def 'errorProneRemoveUnused only keeps the closest suppression to a violation'() { - // language=Java - writeJavaSourceFileToSourceSets ''' - package app; - - @SuppressWarnings("InlineTrivialConstant") - public final class App { - @SuppressWarnings("InlineTrivialConstant") - private static final String EMPTY_STRING = ""; - - @SuppressWarnings("InlineTrivialConstant") - class Inner { - @SuppressWarnings("InlineTrivialConstant") - class InnerInner { - @SuppressWarnings("InlineTrivialConstant") - class InnerInnerInner { - private static final String EMPTY = ""; - } - } - } - } - '''.stripIndent(true) - - when: - runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused') - - then: - - // language=Java - javaSourceIsSyntacticallyEqualTo ''' - package app; - - public final class App { - @SuppressWarnings("InlineTrivialConstant") - private static final String EMPTY_STRING = ""; - - class Inner { - class InnerInner { - @SuppressWarnings("InlineTrivialConstant") - class InnerInnerInner { - private static final String EMPTY = ""; - } - } - } - } - '''.stripIndent(true) - } - - def 'errorProneRemoveUnused handles multiple suppressions on different tree types gracefully'() { - // Here we test the three types of trees you can suppress — ClassTree, MethodTree, VariableTree - - // language=Java - writeJavaSourceFileToSourceSets ''' - package app; - - @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) - public final class App { - @SuppressWarnings("InlineTrivialConstant") - private static final String EMPTY_STRING = ""; - - // Doesn't move an already existing suppression, even if it could be closer to the violation - @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) - static class Inner { - @SuppressWarnings("InlineTrivialConstant") - private static final String EMPTY = ""; - boolean truism = new int[3].equals(new int[3]); - - @SuppressWarnings("InlineTrivialConstant") - static class InnerInner { - @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) - void method() { - new int[3].equals(new int[3]); - } - } - } - } - '''.stripIndent(true) - - when: - runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused') - - then: - - // language=Java - javaSourceIsSyntacticallyEqualTo ''' - package app; - - public final class App { - @SuppressWarnings("InlineTrivialConstant") - private static final String EMPTY_STRING = ""; - - // Doesn't move an already existing suppression, even if it could be closer to the violation - @SuppressWarnings("ArrayEquals") - static class Inner { - @SuppressWarnings("InlineTrivialConstant") - private static final String EMPTY = ""; - - boolean truism = new int[3].equals(new int[3]); - - static class InnerInner { - @SuppressWarnings("ArrayEquals") - void method() { - new int[3].equals(new int[3]); - } - } - } - } - '''.stripIndent(true) - } - - def 'errorProneRemoveUnused removes entire SuppressWarnings annotation when all suppressions are unused'() { - // language=Java - writeJavaSourceFileToSourceSets ''' - package app; - - @SuppressWarnings({"UnusedVariable", "ArrayToString"}) - public final class App { - public static void main(String[] args) { - System.out.println("No violations here"); - } - } - '''.stripIndent(true) - - when: - runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused') - - then: - // language=Java - javaSourceIsSyntacticallyEqualTo ''' - package app; - - public final class App { - public static void main(String[] args) { - System.out.println("No violations here"); - } - } - '''.stripIndent(true) - } - - def 'errorProneRemoveUnused and errorProneSuppress uses existing suppressions if possible'() { - // language=Java - writeJavaSourceFileToSourceSets ''' - package app; - - @SuppressWarnings("InlineTrivialConstant") - public final class App { - @SuppressWarnings("InlineTrivialConstant") - private static final String EMPTY_STRING = ""; - - @SuppressWarnings("InlineTrivialConstant") - static class Inner { - @SuppressWarnings("InlineTrivialConstant") - private static final String EMPTY = ""; - boolean truism = new int[3].equals(new int[3]); - - @SuppressWarnings("InlineTrivialConstant") - static class InnerInner { - @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) - void method() { - new int[3].equals(new int[3]); - } - } - } - } - '''.stripIndent(true) - - when: - runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused', '-PerrorProneSuppress') - - then: - - // language=Java - javaSourceIsSyntacticallyEqualTo ''' - package app; - - public final class App { - @SuppressWarnings("InlineTrivialConstant") - private static final String EMPTY_STRING = ""; - - static class Inner { - @SuppressWarnings("InlineTrivialConstant") - private static final String EMPTY = ""; - - @SuppressWarnings("for-rollout:ArrayEquals") - boolean truism = new int[3].equals(new int[3]); - - static class InnerInner { - @SuppressWarnings("ArrayEquals") - void method() { - new int[3].equals(new int[3]); - } - } - } - } - '''.stripIndent(true) - } - - def 'errorProneRemoveUnused and errorProneApply applies fixes on previously suppressed elements'() { - // language=Java - writeJavaSourceFileToSourceSets ''' - package app; - @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) - public final class App { - @SuppressWarnings("InlineTrivialConstant") - private static final String EMPTY_STRING = ""; - - @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) - static class Inner { - @SuppressWarnings("InlineTrivialConstant") - private static final String EMPTY = ""; - boolean truism = new int[3].equals(new int[3]); - - @SuppressWarnings("InlineTrivialConstant") - static class InnerInner { - @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) - void method() { - new int[3].equals(new int[3]); - } - } - } - } - '''.stripIndent(true) - - when: - runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused', '-PerrorProneApply=ArrayEquals') - - then: - - // language=Java - javaSourceIsSyntacticallyEqualTo ''' - package app; - - import java.util.Arrays; - - public final class App { - @SuppressWarnings("InlineTrivialConstant") - private static final String EMPTY_STRING = ""; - - static class Inner { - @SuppressWarnings("InlineTrivialConstant") - private static final String EMPTY = ""; - - boolean truism = Arrays.equals(new int[3], new int[3]); - - static class InnerInner { - void method() { - Arrays.equals(new int[3], new int[3]); - } - } - } - } - '''.stripIndent(true) - } - - def 'errorProneRemoveUnused + errorProneApply + errorProneSuppress applies fixes and suppressions on previously suppressed elements'() { - // language=Java - writeJavaSourceFileToSourceSets ''' - package app; - - @SuppressWarnings("ArrayEquals") - public final class App { - private static final String EMPTY_STRING = ""; - - // Although InlineTrivialConstant can be placed lower in the AST hierarchy, - // we preserve existing suppressions whenever possible rather than move suppressions around. - // Also, note that we don't add for-rollout here. - @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) - static class Inner { - private static final String EMPTY = ""; - boolean truism = new int[3].equals(new int[3]); - - @SuppressWarnings("InlineTrivialConstant") - static class InnerInner { - @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) - void method() { - new int[3].equals(new int[3]); - } - } - } - } - '''.stripIndent(true) - - when: - runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused', '-PerrorProneSuppress', '-PerrorProneApply=ArrayEquals') - - then: - - // language=Java - javaSourceIsSyntacticallyEqualTo ''' - package app; - - import java.util.Arrays; - - public final class App { - @SuppressWarnings("for-rollout:InlineTrivialConstant") - private static final String EMPTY_STRING = ""; - - // Although InlineTrivialConstant can be placed lower in the AST hierarchy, - // we preserve existing suppressions whenever possible rather than move suppressions around. - // Also, note that we don't add for-rollout here. - @SuppressWarnings("InlineTrivialConstant") - static class Inner { - private static final String EMPTY = ""; - boolean truism = Arrays.equals(new int[3], new int[3]); - - static class InnerInner { - void method() { - Arrays.equals(new int[3], new int[3]); - } - } - } - } - '''.stripIndent(true) - } - def 'error-prone dependencies have versions bound together by a virtual platform'() { setup: 'when an error-prone dependency is forced to certain version' // language=Gradle @@ -2062,4 +1714,4 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec def expectedOther = normalizeSource(source) assert outputOther != expectedOther } -} \ No newline at end of file +} diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AllErrorprones.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AllErrorprones.java deleted file mode 100644 index 4f08a94f..00000000 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AllErrorprones.java +++ /dev/null @@ -1,43 +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.errorprone.BugCheckerInfo; -import com.google.errorprone.bugpatterns.BugChecker; -import com.google.errorprone.scanner.BuiltInCheckerSuppliers; -import java.util.ServiceLoader; -import java.util.Set; -import java.util.stream.Collectors; -import java.util.stream.Stream; -import one.util.streamex.StreamEx; - -public final class AllErrorprones { - public static Set allBugcheckerNames() { - Stream pluginChecks = - ServiceLoader.load(BugChecker.class).stream().map(ServiceLoader.Provider::get); - Stream pluginCheckNames = - StreamEx.of(pluginChecks).flatMap(bugchecker -> bugchecker.allNames().stream()); - - Stream builtInChecks = BuiltInCheckerSuppliers.allChecks().getAllChecks().values().stream(); - Stream builtInCheckNames = - StreamEx.of(builtInChecks).flatMap(bugchecker -> bugchecker.allNames().stream()); - - return Stream.concat(pluginCheckNames, builtInCheckNames).collect(Collectors.toSet()); - } - - private AllErrorprones() {} -} diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java deleted file mode 100644 index 8bd3a775..00000000 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java +++ /dev/null @@ -1,101 +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.errorprone.VisitorState; -import com.google.errorprone.matchers.Description; -import com.sun.source.tree.AnnotationTree; -import com.sun.source.tree.ModifiersTree; -import com.sun.source.tree.Tree; -import com.sun.source.util.TreePath; -import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition; -import java.util.Optional; -import java.util.Set; -import java.util.WeakHashMap; -import java.util.function.Predicate; -import java.util.stream.Collectors; -import java.util.stream.Stream; -import javax.lang.model.element.Name; - -final class ReportedFixCache { - // Weak map so that we don't leak memory by keeping hold of references to the source element tree keys and our - // mutable fixes values around forever, once error-prone has finished with the source element tree used as a key - // here (once the file has been visited by all the error-prone checks), our SuppressingFixes can be safely - // garbage collected. - private final WeakHashMap cache = new WeakHashMap<>(); - - public static final Predicate REMOVE_EVERYTHING = bc -> false; - public static final Predicate KEEP_EVERYTHING = bc -> true; - - ReportedFixCache() {} - - /** - * When first called on a {@code declaration}, initialize a {@code LazySuppressionFix} on it, choosing which - * suppressions to keep with {@code filterExisting}. Subsequent calls on this {@code declaration} will return the - * initialized {@code LazySuppressionFix}. - */ - public LazySuppressionFix getOrReportNew( - TreePath declaration, VisitorState visitorState, Predicate filterExisting) { - return cache.computeIfAbsent( - declaration.getLeaf(), _ignored -> createAndReportFix(declaration, visitorState, filterExisting)); - } - - /** - * Initialize a {@code LazySuppressionFix} on {@code declaration}, choosing which suppressions to keep with - * {@code filterExisting}. - */ - @SuppressWarnings("PreferSafeLoggableExceptions") // It doesn't matter in our internal codebases - public LazySuppressionFix reportNew( - TreePath declaration, VisitorState visitorState, Predicate filterExisting) { - if (cache.containsKey(declaration.getLeaf())) { - throw new IllegalArgumentException("A fix on this declaration already exists"); - } - return cache.put(declaration.getLeaf(), createAndReportFix(declaration, visitorState, filterExisting)); - } - - @SuppressWarnings("RestrictedApi") - private LazySuppressionFix createAndReportFix( - TreePath declaration, VisitorState state, Predicate filterExisting) { - if (!SuppressWarningsUtils.suppressibleTreePath(declaration)) { - throw new IllegalArgumentException("Not suppressible: " + declaration); - } - ModifiersTree modifiersTree = AnnotationUtils.getModifiers(declaration.getLeaf()); - Optional suppressWarnings = modifiersTree.getAnnotations().stream() - .filter(annotation -> { - Name annotationName = AnnotationUtils.annotationName(annotation.getAnnotationType()); - return annotationName.contentEquals(CommonConstants.SUPPRESS_WARNINGS_ANNOTATION); - }) - .findFirst(); - - Stream allExistingSuppressions = - suppressWarnings.map(AnnotationUtils::annotationStringValues).orElseGet(Stream::of); - Set filteredExistingSuppressions = - allExistingSuppressions.filter(filterExisting).collect(Collectors.toSet()); - - LazySuppressionFix fix = new LazySuppressionFix( - Optional.ofNullable(state.getSourceCode()), - suppressWarnings, - declaration.getLeaf(), - filteredExistingSuppressions); - Description description = Description.builder( - (DiagnosticPosition) declaration.getLeaf(), "SuppressibleErrorProne", "", "") - .addFix(fix) - .build(); - state.reportMatch(description); - return fix; - } -} 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 deleted file mode 100644 index b7e83908..00000000 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleErrorProne.java +++ /dev/null @@ -1,35 +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.BugPattern.SeverityLevel; -import com.google.errorprone.bugpatterns.BugChecker; - -/** - * Used by {@code ReportedFixCache} so we don't recursively call {@code VisitorStateModifications} - */ -@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/VisitorStateModifications.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java index a9e75e68..699d15b2 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 @@ -22,40 +22,41 @@ import com.google.errorprone.VisitorState; import com.google.errorprone.matchers.Description; import com.sun.source.tree.AnnotationTree; -import com.sun.source.tree.CompilationUnitTree; +import com.sun.source.tree.ClassTree; +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.HashSet; +import java.util.Map; import java.util.Optional; import java.util.Set; -import java.util.function.Predicate; +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 ReportedFixCache FIXES = new ReportedFixCache(); - private static final Set ALL_BUGCHECKER_NAMES = AllErrorprones.allBugcheckerNames(); - - // Used during RemoveUnused mode to attach fixes when we initially see this compilation unit - private static final Set SEEN = new HashSet<>(); + // 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<>(); - @SuppressWarnings("CyclomaticComplexity") // Unfortunately this logic is just complex + @SuppressWarnings("RestrictedApi") public static Description interceptDescription(VisitorState visitorState, Description description) { - // Prevent infinite recursion on reported fixes - if (description == Description.NO_MATCH || description.checkName.equals("SuppressibleErrorProne")) { + if (description == Description.NO_MATCH) { return description; } Set modes = visitorState.errorProneOptions().getFlags().getSetOrEmpty("SuppressibleErrorProne:Mode"); - CompilationUnitTree compilationUnit = visitorState.getPath().getCompilationUnit(); - - if (modes.contains("RemoveUnused") && !SEEN.contains(compilationUnit)) { - removeAllErrorproneSuppressionsOnCompilationUnit(compilationUnit, visitorState); - SEEN.add(compilationUnit); + if (modes.contains("RemoveUnused")) { + throw new UnsupportedOperationException("RemoveUnused mode is not yet supported"); } // If both -PerrorProneSuppress and -PerrorProneApply are used at the same time, for the checks configured as @@ -86,25 +87,6 @@ public static Description interceptDescription(VisitorState visitorState, Descri TreePath pathToActualError = TreePath.getPath(visitorState.getPath().getCompilationUnit(), description.position.getTree()); - Optional firstSuppressibleWhichSuppressesDescription = Stream.iterate( - pathToActualError, treePath -> treePath.getParentPath() != null, TreePath::getParentPath) - .dropWhile(path -> !suppresses(path, description)) - .findFirst(); - - if (firstSuppressibleWhichSuppressesDescription.isPresent() && modes.contains("RemoveUnused")) { - suppressCheck(visitorState, description.checkName, firstSuppressibleWhichSuppressesDescription.get()); - return Description.NO_MATCH; - } - - if (!modes.contains("Suppress")) { - log.warning("No autofix was found for " + description.checkName + " at position " - + description.position.getStartPosition() + " in " - + visitorState.getPath().getCompilationUnit().getSourceFile() + "." - + " -PerrorProneSuppress was not passed either. " - + " SuppressibleErrorProne will not be able to add a suppression for this error."); - return Description.NO_MATCH; - } - Optional firstSuppressible = Stream.iterate( pathToActualError, treePath -> treePath.getParentPath() != null, TreePath::getParentPath) .dropWhile(path -> !SuppressWarningsUtils.suppressibleTreePath(path)) @@ -121,56 +103,71 @@ public static Description interceptDescription(VisitorState visitorState, Descri return Description.NO_MATCH; } + 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. - suppressCheck( - visitorState, - CommonConstants.AUTOMATICALLY_ADDED_PREFIX + description.checkName, - firstSuppressible.get()); - - return Description.NO_MATCH; - } + boolean alreadyReportedFix = FIXES.containsKey(firstSuppressibleParent); + + LazySuppressionFix suppressingFix = FIXES.computeIfAbsent(firstSuppressibleParent, _ignored -> { + // Initialize every fix with all remove-rollout: suppressions removed + Stream existingSuppressions = suppressWarnings + .map(AnnotationUtils::annotationStringValues) + .orElseGet(Stream::of); + Set existingNonRolloutSuppressions = existingSuppressions + .filter(sup -> !sup.startsWith(CommonConstants.AUTOMATICALLY_ADDED_PREFIX)) + .collect(Collectors.toSet()); + return new LazySuppressionFix( + Optional.ofNullable(visitorState.getSourceCode()), + suppressWarnings, + firstSuppressibleParent, + existingNonRolloutSuppressions); + }); + + suppressingFix.addSuppression(CommonConstants.AUTOMATICALLY_ADDED_PREFIX + description.checkName); + + // 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; + } - private static void suppressCheck(VisitorState visitorState, String suppression, TreePath suppressibleParent) { - LazySuppressionFix lazyFix = - FIXES.getOrReportNew(suppressibleParent, visitorState, ReportedFixCache.KEEP_EVERYTHING); - lazyFix.addSuppression(suppression); + return Description.builder( + description.position, + description.checkName, + description.getLink(), + description.getMessageWithoutCheckName()) + .addFix(suppressingFix) + .build(); } - private static boolean suppresses(TreePath declaration, Description description) { - if (!SuppressWarningsUtils.suppressibleTreePath(declaration)) { - return false; + private static Optional modifiersTree(Tree tree) { + // This covers all type definitions eg class, interface, enum, record, annotation, future kinds + // of class-like type definitions. + if (tree instanceof ClassTree classTree) { + return Optional.of(classTree.getModifiers()); } - Optional suppressWarningsMaybe = - SuppressWarningsUtils.getSuppressWarnings(declaration); - if (suppressWarningsMaybe.isEmpty()) { - return false; + if (tree instanceof MethodTree methodTree) { + return Optional.of(methodTree.getModifiers()); } - String checkName = description.checkName; - return AnnotationUtils.annotationStringValues(suppressWarningsMaybe.get()) - .anyMatch(checkName::equals); - } + if (tree instanceof VariableTree variableTree) { + return Optional.of(variableTree.getModifiers()); + } - private static void removeAllErrorproneSuppressionsOnCompilationUnit( - CompilationUnitTree compilationUnit, VisitorState visitorState) { - Predicate keepNonErrorproneSuppressions = Predicate.not(ALL_BUGCHECKER_NAMES::contains); - - new TreePathScanner() { - @Override - public Void visitAnnotation(AnnotationTree node, Void unused) { - if (AnnotationUtils.isSuppressWarningsAnnotation(node)) { - TreePath declaration = getCurrentPath().getParentPath().getParentPath(); - FIXES.reportNew(declaration, visitorState, keepNonErrorproneSuppressions); - } - - return super.visitAnnotation(node, null); - } - }.scan(compilationUnit, null); + return Optional.empty(); } private VisitorStateModifications() {} From 429d12a7819f10e6d8e28a79e625fda9aefb3100 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Fri, 17 Oct 2025 13:37:10 +0100 Subject: [PATCH 06/15] Remove changes to VisitorStateModifications --- .../AllErrorprones.java | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AllErrorprones.java diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AllErrorprones.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AllErrorprones.java new file mode 100644 index 00000000..4f08a94f --- /dev/null +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AllErrorprones.java @@ -0,0 +1,43 @@ +/* + * (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.BugCheckerInfo; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.scanner.BuiltInCheckerSuppliers; +import java.util.ServiceLoader; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import one.util.streamex.StreamEx; + +public final class AllErrorprones { + public static Set allBugcheckerNames() { + Stream pluginChecks = + ServiceLoader.load(BugChecker.class).stream().map(ServiceLoader.Provider::get); + Stream pluginCheckNames = + StreamEx.of(pluginChecks).flatMap(bugchecker -> bugchecker.allNames().stream()); + + Stream builtInChecks = BuiltInCheckerSuppliers.allChecks().getAllChecks().values().stream(); + Stream builtInCheckNames = + StreamEx.of(builtInChecks).flatMap(bugchecker -> bugchecker.allNames().stream()); + + return Stream.concat(pluginCheckNames, builtInCheckNames).collect(Collectors.toSet()); + } + + private AllErrorprones() {} +} From 6e4fc4bffdc809c8f5d942f0ba4ea08278288d96 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Fri, 17 Oct 2025 13:46:43 +0100 Subject: [PATCH 07/15] Use new ReportedFixCache --- .../ReportedFixCache.java | 101 ++++++++++++++++++ .../SuppressibleErrorProne.java | 32 ++++++ .../VisitorStateModifications.java | 86 ++------------- 3 files changed, 144 insertions(+), 75 deletions(-) create mode 100644 suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java create mode 100644 suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleErrorProne.java diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java new file mode 100644 index 00000000..8bd3a775 --- /dev/null +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java @@ -0,0 +1,101 @@ +/* + * (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.google.errorprone.matchers.Description; +import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.ModifiersTree; +import com.sun.source.tree.Tree; +import com.sun.source.util.TreePath; +import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition; +import java.util.Optional; +import java.util.Set; +import java.util.WeakHashMap; +import java.util.function.Predicate; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import javax.lang.model.element.Name; + +final class ReportedFixCache { + // Weak map so that we don't leak memory by keeping hold of references to the source element tree keys and our + // mutable fixes values around forever, once error-prone has finished with the source element tree used as a key + // here (once the file has been visited by all the error-prone checks), our SuppressingFixes can be safely + // garbage collected. + private final WeakHashMap cache = new WeakHashMap<>(); + + public static final Predicate REMOVE_EVERYTHING = bc -> false; + public static final Predicate KEEP_EVERYTHING = bc -> true; + + ReportedFixCache() {} + + /** + * When first called on a {@code declaration}, initialize a {@code LazySuppressionFix} on it, choosing which + * suppressions to keep with {@code filterExisting}. Subsequent calls on this {@code declaration} will return the + * initialized {@code LazySuppressionFix}. + */ + public LazySuppressionFix getOrReportNew( + TreePath declaration, VisitorState visitorState, Predicate filterExisting) { + return cache.computeIfAbsent( + declaration.getLeaf(), _ignored -> createAndReportFix(declaration, visitorState, filterExisting)); + } + + /** + * Initialize a {@code LazySuppressionFix} on {@code declaration}, choosing which suppressions to keep with + * {@code filterExisting}. + */ + @SuppressWarnings("PreferSafeLoggableExceptions") // It doesn't matter in our internal codebases + public LazySuppressionFix reportNew( + TreePath declaration, VisitorState visitorState, Predicate filterExisting) { + if (cache.containsKey(declaration.getLeaf())) { + throw new IllegalArgumentException("A fix on this declaration already exists"); + } + return cache.put(declaration.getLeaf(), createAndReportFix(declaration, visitorState, filterExisting)); + } + + @SuppressWarnings("RestrictedApi") + private LazySuppressionFix createAndReportFix( + TreePath declaration, VisitorState state, Predicate filterExisting) { + if (!SuppressWarningsUtils.suppressibleTreePath(declaration)) { + throw new IllegalArgumentException("Not suppressible: " + declaration); + } + ModifiersTree modifiersTree = AnnotationUtils.getModifiers(declaration.getLeaf()); + Optional suppressWarnings = modifiersTree.getAnnotations().stream() + .filter(annotation -> { + Name annotationName = AnnotationUtils.annotationName(annotation.getAnnotationType()); + return annotationName.contentEquals(CommonConstants.SUPPRESS_WARNINGS_ANNOTATION); + }) + .findFirst(); + + Stream allExistingSuppressions = + suppressWarnings.map(AnnotationUtils::annotationStringValues).orElseGet(Stream::of); + Set filteredExistingSuppressions = + allExistingSuppressions.filter(filterExisting).collect(Collectors.toSet()); + + LazySuppressionFix fix = new LazySuppressionFix( + Optional.ofNullable(state.getSourceCode()), + suppressWarnings, + declaration.getLeaf(), + filteredExistingSuppressions); + Description description = Description.builder( + (DiagnosticPosition) declaration.getLeaf(), "SuppressibleErrorProne", "", "") + .addFix(fix) + .build(); + state.reportMatch(description); + return fix; + } +} 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/VisitorStateModifications.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java index 699d15b2..6d101f66 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java @@ -21,41 +21,27 @@ import com.google.errorprone.BugPattern.SeverityLevel; import com.google.errorprone.VisitorState; import com.google.errorprone.matchers.Description; -import com.sun.source.tree.AnnotationTree; -import com.sun.source.tree.ClassTree; -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 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 public final class VisitorStateModifications { private static final Logger log = Logger.getLogger(VisitorStateModifications.class.getName()); - // 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 ReportedFixCache FIXES = new ReportedFixCache(); @SuppressWarnings("RestrictedApi") public static Description interceptDescription(VisitorState visitorState, Description description) { - if (description == Description.NO_MATCH) { + // Prevent infinite recursion on reported fixes + if (description == Description.NO_MATCH || description.checkName.equals("SuppressibleErrorProne")) { return description; } - Set modes = visitorState.errorProneOptions().getFlags().getSetOrEmpty("SuppressibleErrorProne:Mode"); - if (modes.contains("RemoveUnused")) { + if (getModes(visitorState).contains("RemoveUnused")) { throw new UnsupportedOperationException("RemoveUnused mode is not yet supported"); } @@ -103,71 +89,21 @@ public static Description interceptDescription(VisitorState visitorState, Descri return Description.NO_MATCH; } - 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); - - LazySuppressionFix suppressingFix = FIXES.computeIfAbsent(firstSuppressibleParent, _ignored -> { - // Initialize every fix with all remove-rollout: suppressions removed - Stream existingSuppressions = suppressWarnings - .map(AnnotationUtils::annotationStringValues) - .orElseGet(Stream::of); - Set existingNonRolloutSuppressions = existingSuppressions - .filter(sup -> !sup.startsWith(CommonConstants.AUTOMATICALLY_ADDED_PREFIX)) - .collect(Collectors.toSet()); - return new LazySuppressionFix( - Optional.ofNullable(visitorState.getSourceCode()), - suppressWarnings, - firstSuppressibleParent, - existingNonRolloutSuppressions); - }); - + LazySuppressionFix suppressingFix = FIXES.getOrReportNew( + firstSuppressible.get(), + visitorState, + bugchecker -> !bugchecker.startsWith(CommonConstants.AUTOMATICALLY_ADDED_PREFIX)); suppressingFix.addSuppression(CommonConstants.AUTOMATICALLY_ADDED_PREFIX + description.checkName); - - // 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 Description.builder( - description.position, - description.checkName, - description.getLink(), - description.getMessageWithoutCheckName()) - .addFix(suppressingFix) - .build(); + return Description.NO_MATCH; } - private static Optional modifiersTree(Tree tree) { - // This covers all type definitions eg class, interface, enum, record, annotation, future kinds - // of class-like type definitions. - if (tree instanceof ClassTree classTree) { - return Optional.of(classTree.getModifiers()); - } - - if (tree instanceof MethodTree methodTree) { - return Optional.of(methodTree.getModifiers()); - } - - if (tree instanceof VariableTree variableTree) { - return Optional.of(variableTree.getModifiers()); - } - - return Optional.empty(); + private static Set getModes(VisitorState state) { + return state.errorProneOptions().getFlags().getSetOrEmpty("SuppressibleErrorProne:Mode"); } private VisitorStateModifications() {} From fbc545c94ce4899ac2f0feb5f5fc4b20bd66f01b Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Fri, 17 Oct 2025 14:01:21 +0100 Subject: [PATCH 08/15] Docs --- .../palantir/suppressibleerrorprone/ReportedFixCache.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java index 8bd3a775..117bc578 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java @@ -92,7 +92,10 @@ private LazySuppressionFix createAndReportFix( declaration.getLeaf(), filteredExistingSuppressions); Description description = Description.builder( - (DiagnosticPosition) declaration.getLeaf(), "SuppressibleErrorProne", "", "") + (DiagnosticPosition) declaration.getLeaf(), + "SuppressibleErrorProne", + "https://github.com/palantir/suppressible-error-prone", + "A fix on a suppressible by SuppressibleErrorProne") .addFix(fix) .build(); state.reportMatch(description); From 30effe79c9d334579eb67685651f132abcc4134a Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Fri, 17 Oct 2025 14:02:32 +0100 Subject: [PATCH 09/15] Docs --- .../com/palantir/suppressibleerrorprone/ReportedFixCache.java | 2 +- .../palantir/suppressibleerrorprone/SuppressibleErrorProne.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java index 117bc578..289c797b 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java @@ -95,7 +95,7 @@ private LazySuppressionFix createAndReportFix( (DiagnosticPosition) declaration.getLeaf(), "SuppressibleErrorProne", "https://github.com/palantir/suppressible-error-prone", - "A fix on a suppressible by SuppressibleErrorProne") + "A fix on a suppressible by suppressible-error-prone") .addFix(fix) .build(); state.reportMatch(description); 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 index e22b9d32..0dab8fe3 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleErrorProne.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleErrorProne.java @@ -26,7 +26,7 @@ link = "https://github.com/palantir/suppressible-error-prone", linkType = BugPattern.LinkType.CUSTOM, severity = SeverityLevel.WARNING, - summary = "A change made by suppressible-error-prone", + summary = "A fix on a suppressible by suppressible-error-prone", // Make it unsuppressible so that it can actually remove itself suppressionAnnotations = {}) public class SuppressibleErrorProne extends BugChecker {} From 9de5225ad11e9188b6dacbde1f0d13550217eb89 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Mon, 20 Oct 2025 12:52:42 +0100 Subject: [PATCH 10/15] Add changes --- .../suppressibleerrorprone/modes/Modes.java | 4 +- .../RemoveUnusedAndSuppressInterference.java | 37 ------------------- .../AllErrorprones.java | 23 ++++++++---- .../ReportedFixCache.java | 2 +- .../SuppressibleErrorProne.java | 5 +++ 5 files changed, 22 insertions(+), 49 deletions(-) delete mode 100644 gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedAndSuppressInterference.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 32bb5bc8..2987482e 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 @@ -30,7 +30,6 @@ import com.palantir.gradle.suppressibleerrorprone.modes.interferences.DisableModeInterference; 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; @@ -71,8 +70,7 @@ ModeName.SUPPRESS, new SuppressMode(), new DisableModeInterference(), new RemoveRolloutAndSuppressingInterference(), new SuppressingAndApplyingInterference(), - new RemoveUnusedAndApplyingInterference(), - new RemoveUnusedAndSuppressInterference()); + new RemoveUnusedAndApplyingInterference()); public final CombinedValue modifyCheckApi() { return ModifyCheckApiOption.combine( 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 deleted file mode 100644 index 3d165e4e..00000000 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedAndSuppressInterference.java +++ /dev/null @@ -1,37 +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.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/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AllErrorprones.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AllErrorprones.java index 4f08a94f..7a0cd2e9 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AllErrorprones.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AllErrorprones.java @@ -26,17 +26,24 @@ import one.util.streamex.StreamEx; public final class AllErrorprones { + private static Set cachedAllBugcheckerNames; + public static Set allBugcheckerNames() { - Stream pluginChecks = - ServiceLoader.load(BugChecker.class).stream().map(ServiceLoader.Provider::get); - Stream pluginCheckNames = - StreamEx.of(pluginChecks).flatMap(bugchecker -> bugchecker.allNames().stream()); + if (cachedAllBugcheckerNames == null) { + Stream pluginChecks = + ServiceLoader.load(BugChecker.class).stream().map(ServiceLoader.Provider::get); + Stream pluginCheckNames = + StreamEx.of(pluginChecks).flatMap(bugchecker -> bugchecker.allNames().stream()); + + Stream builtInChecks = BuiltInCheckerSuppliers.allChecks().getAllChecks().values().stream(); + Stream builtInCheckNames = + StreamEx.of(builtInChecks).flatMap(bugchecker -> bugchecker.allNames().stream()); - Stream builtInChecks = BuiltInCheckerSuppliers.allChecks().getAllChecks().values().stream(); - Stream builtInCheckNames = - StreamEx.of(builtInChecks).flatMap(bugchecker -> bugchecker.allNames().stream()); + cachedAllBugcheckerNames = + Stream.concat(pluginCheckNames, builtInCheckNames).collect(Collectors.toSet()); + } - return Stream.concat(pluginCheckNames, builtInCheckNames).collect(Collectors.toSet()); + return cachedAllBugcheckerNames; } private AllErrorprones() {} diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java index 289c797b..c7c6a9ae 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java @@ -67,7 +67,6 @@ public LazySuppressionFix reportNew( return cache.put(declaration.getLeaf(), createAndReportFix(declaration, visitorState, filterExisting)); } - @SuppressWarnings("RestrictedApi") private LazySuppressionFix createAndReportFix( TreePath declaration, VisitorState state, Predicate filterExisting) { if (!SuppressWarningsUtils.suppressibleTreePath(declaration)) { @@ -91,6 +90,7 @@ private LazySuppressionFix createAndReportFix( suppressWarnings, declaration.getLeaf(), filteredExistingSuppressions); + @SuppressWarnings("RestrictedApi") Description description = Description.builder( (DiagnosticPosition) declaration.getLeaf(), "SuppressibleErrorProne", 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 index 0dab8fe3..347fc098 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleErrorProne.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleErrorProne.java @@ -21,6 +21,11 @@ import com.google.errorprone.BugPattern.SeverityLevel; import com.google.errorprone.bugpatterns.BugChecker; +/** + * Used by {@link ReportedFixCache} to register {@link LazySuppressionFix}es. + * We need this "default" bugchecker type, as using the name of any individual suppression being removed/added is + * inappropriate — a {@link LazySuppressionFix} may remove or add multiple suppressions. + */ @AutoService(BugChecker.class) @BugPattern( link = "https://github.com/palantir/suppressible-error-prone", From 1f1ff19f83865dc2a9f026fa52c86ca74b6f57ac Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Mon, 20 Oct 2025 18:21:28 +0100 Subject: [PATCH 11/15] Consolidate interferences --- .../suppressibleerrorprone/modes/Modes.java | 6 +- ...SuppressingOrRemoveUnusedInterference.java | 74 +++++++++++++++++++ .../RemoveUnusedAndApplyingInterference.java | 49 ------------ .../SuppressingAndApplyingInterference.java | 48 ------------ 4 files changed, 76 insertions(+), 101 deletions(-) create mode 100644 gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/ApplyingAndSuppressingOrRemoveUnusedInterference.java delete mode 100644 gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedAndApplyingInterference.java delete mode 100644 gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/SuppressingAndApplyingInterference.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 2987482e..10bbe5a0 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 @@ -27,10 +27,9 @@ import com.palantir.gradle.suppressibleerrorprone.modes.common.ModeName; import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption; import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.CombinedValue; +import com.palantir.gradle.suppressibleerrorprone.modes.interferences.ApplyingAndSuppressingOrRemoveUnusedInterference; import com.palantir.gradle.suppressibleerrorprone.modes.interferences.DisableModeInterference; import com.palantir.gradle.suppressibleerrorprone.modes.interferences.RemoveRolloutAndSuppressingInterference; -import com.palantir.gradle.suppressibleerrorprone.modes.interferences.RemoveUnusedAndApplyingInterference; -import com.palantir.gradle.suppressibleerrorprone.modes.interferences.SuppressingAndApplyingInterference; import com.palantir.gradle.suppressibleerrorprone.modes.modes.ApplyMode; import com.palantir.gradle.suppressibleerrorprone.modes.modes.DisableMode; import com.palantir.gradle.suppressibleerrorprone.modes.modes.RemoveRolloutMode; @@ -69,8 +68,7 @@ ModeName.SUPPRESS, new SuppressMode(), private final Set interferences = Set.of( new DisableModeInterference(), new RemoveRolloutAndSuppressingInterference(), - new SuppressingAndApplyingInterference(), - new RemoveUnusedAndApplyingInterference()); + new ApplyingAndSuppressingOrRemoveUnusedInterference()); public final CombinedValue modifyCheckApi() { return ModifyCheckApiOption.combine( diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/ApplyingAndSuppressingOrRemoveUnusedInterference.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/ApplyingAndSuppressingOrRemoveUnusedInterference.java new file mode 100644 index 00000000..3e4c3e88 --- /dev/null +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/ApplyingAndSuppressingOrRemoveUnusedInterference.java @@ -0,0 +1,74 @@ +/* + * (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.google.common.collect.Sets; +import com.palantir.gradle.suppressibleerrorprone.modes.common.CommonOptions; +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.Map; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Stream; +import one.util.streamex.EntryStream; + +/** + * Interference between Apply && (Suppress || RemoveUnused). + */ +public final class ApplyingAndSuppressingOrRemoveUnusedInterference implements ModeInterference { + private static final Set APPLY_AND_REMOVE_UNUSED_AND_SUPPRESS = + Set.of(ModeName.APPLY, ModeName.REMOVE_UNUSED, ModeName.SUPPRESS); + private static final Set APPLY_AND_REMOVE_UNUSED = Set.of(ModeName.APPLY, ModeName.REMOVE_UNUSED); + private static final Set APPLY_AND_SUPPRESS = Set.of(ModeName.APPLY, ModeName.SUPPRESS); + + @Override + public ModeInterferenceResult interferesWith(Set modeNames) { + Optional> maximalInterference = getMaximalInterference(modeNames); + return maximalInterference + .map(ModeInterferenceResult::interferenceBetween) + .orElseGet(ModeInterferenceResult::noInterference); + } + + @Override + public CommonOptions interfere(Map modeCommonOptions) { + Set maximalInterference = + getMaximalInterference(modeCommonOptions.keySet()).get(); + CommonOptions apply = modeCommonOptions.get(ModeName.APPLY); + CommonOptions naivelyCombined = EntryStream.of(modeCommonOptions) + .filterKeys(maximalInterference::contains) + .values() + .reduce(CommonOptions.empty(), CommonOptions::naivelyCombinedWith); + + // 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 naivelyCombined.withExtraErrorProneCheckFlag( + "SuppressibleErrorProne:PreferPatchChecks", + () -> apply.patchChecks().asCommaSeparated().orElse("")); + } + + private static Optional> getMaximalInterference(Set modeNames) { + return Stream.of(APPLY_AND_REMOVE_UNUSED_AND_SUPPRESS, APPLY_AND_REMOVE_UNUSED, APPLY_AND_SUPPRESS) + .filter(set -> Sets.difference(set, modeNames).isEmpty()) + .findFirst(); + } +} 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 deleted file mode 100644 index c87f3cbf..00000000 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/RemoveUnusedAndApplyingInterference.java +++ /dev/null @@ -1,49 +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.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/SuppressingAndApplyingInterference.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/SuppressingAndApplyingInterference.java deleted file mode 100644 index 397bf38d..00000000 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/SuppressingAndApplyingInterference.java +++ /dev/null @@ -1,48 +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.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 SuppressingAndApplyingInterference extends SpecificModeInterference { - @Override - protected Set interferingModes() { - return Set.of(ModeName.SUPPRESS, ModeName.APPLY); - } - - @Override - public CommonOptions interfere(Map modeCommonOptions) { - CommonOptions suppress = modeCommonOptions.get(ModeName.SUPPRESS); - 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 suppress.naivelyCombinedWith(apply) - .withExtraErrorProneCheckFlag( - "SuppressibleErrorProne:PreferPatchChecks", - () -> apply.patchChecks().asCommaSeparated().orElse("")); - } -} From a4bb47c55d32d45e834a1f91fe3c2a40c3840019 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Tue, 21 Oct 2025 14:08:57 +0100 Subject: [PATCH 12/15] Simplify set logic --- ...SuppressingOrRemoveUnusedInterference.java | 31 ++++++++----------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/ApplyingAndSuppressingOrRemoveUnusedInterference.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/ApplyingAndSuppressingOrRemoveUnusedInterference.java index 3e4c3e88..afbbc642 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/ApplyingAndSuppressingOrRemoveUnusedInterference.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/ApplyingAndSuppressingOrRemoveUnusedInterference.java @@ -22,32 +22,33 @@ import com.palantir.gradle.suppressibleerrorprone.modes.common.ModeInterferenceResult; import com.palantir.gradle.suppressibleerrorprone.modes.common.ModeName; import java.util.Map; -import java.util.Optional; import java.util.Set; -import java.util.stream.Stream; import one.util.streamex.EntryStream; /** * Interference between Apply && (Suppress || RemoveUnused). */ public final class ApplyingAndSuppressingOrRemoveUnusedInterference implements ModeInterference { - private static final Set APPLY_AND_REMOVE_UNUSED_AND_SUPPRESS = - Set.of(ModeName.APPLY, ModeName.REMOVE_UNUSED, ModeName.SUPPRESS); - private static final Set APPLY_AND_REMOVE_UNUSED = Set.of(ModeName.APPLY, ModeName.REMOVE_UNUSED); - private static final Set APPLY_AND_SUPPRESS = Set.of(ModeName.APPLY, ModeName.SUPPRESS); @Override public ModeInterferenceResult interferesWith(Set modeNames) { - Optional> maximalInterference = getMaximalInterference(modeNames); - return maximalInterference - .map(ModeInterferenceResult::interferenceBetween) - .orElseGet(ModeInterferenceResult::noInterference); + if (!modeNames.contains(ModeName.APPLY)) { + return ModeInterferenceResult.noInterference(); + } + + Set interferingWithApply = + Sets.intersection(modeNames, Set.of(ModeName.SUPPRESS, ModeName.REMOVE_UNUSED)); + + if (interferingWithApply.isEmpty()) { + return ModeInterferenceResult.noInterference(); + } + + return ModeInterferenceResult.interferenceBetween(Sets.union(Set.of(ModeName.APPLY), interferingWithApply)); } @Override public CommonOptions interfere(Map modeCommonOptions) { - Set maximalInterference = - getMaximalInterference(modeCommonOptions.keySet()).get(); + Set maximalInterference = modeCommonOptions.keySet(); CommonOptions apply = modeCommonOptions.get(ModeName.APPLY); CommonOptions naivelyCombined = EntryStream.of(modeCommonOptions) .filterKeys(maximalInterference::contains) @@ -65,10 +66,4 @@ public CommonOptions interfere(Map modeCommonOptions) { "SuppressibleErrorProne:PreferPatchChecks", () -> apply.patchChecks().asCommaSeparated().orElse("")); } - - private static Optional> getMaximalInterference(Set modeNames) { - return Stream.of(APPLY_AND_REMOVE_UNUSED_AND_SUPPRESS, APPLY_AND_REMOVE_UNUSED, APPLY_AND_SUPPRESS) - .filter(set -> Sets.difference(set, modeNames).isEmpty()) - .findFirst(); - } } From e835876c3495e588f575f95159e5fe342f4224ae Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Tue, 21 Oct 2025 14:12:53 +0100 Subject: [PATCH 13/15] Simplify set logic even more --- .../ApplyingAndSuppressingOrRemoveUnusedInterference.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/ApplyingAndSuppressingOrRemoveUnusedInterference.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/ApplyingAndSuppressingOrRemoveUnusedInterference.java index afbbc642..43847f7d 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/ApplyingAndSuppressingOrRemoveUnusedInterference.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/interferences/ApplyingAndSuppressingOrRemoveUnusedInterference.java @@ -48,10 +48,8 @@ public ModeInterferenceResult interferesWith(Set modeNames) { @Override public CommonOptions interfere(Map modeCommonOptions) { - Set maximalInterference = modeCommonOptions.keySet(); CommonOptions apply = modeCommonOptions.get(ModeName.APPLY); CommonOptions naivelyCombined = EntryStream.of(modeCommonOptions) - .filterKeys(maximalInterference::contains) .values() .reduce(CommonOptions.empty(), CommonOptions::naivelyCombinedWith); From 5e003a30e180425710c614f59b5490143a7ca58a Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Tue, 21 Oct 2025 17:33:38 +0100 Subject: [PATCH 14/15] Add common constants --- .../modes/common/Mode.java | 2 ++ .../modes/modes/ApplyMode.java | 2 +- .../modes/modes/RemoveRolloutMode.java | 2 +- .../modes/modes/RemoveUnusedMode.java | 2 +- .../modes/modes/SuppressMode.java | 2 +- suppressible-error-prone/build.gradle | 2 +- .../AllErrorprones.java | 29 +++++++++---------- .../ReportedFixCache.java | 2 +- .../VisitorStateModifications.java | 5 ++-- 9 files changed, 25 insertions(+), 23 deletions(-) diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/Mode.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/Mode.java index 137452f2..c7cbd1d6 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/Mode.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/Mode.java @@ -31,6 +31,8 @@ * (see {@link CommonOptions}). Modes can interfere with each other, see {@link ModeInterference}. */ public interface Mode { + String SUPPRESSIBLE_ERROR_PRONE_MODE = "SuppressibleErrorProne:Mode"; + default ModifyCheckApiOption modifyCheckApi() { return ModifyCheckApiOption.noEffect(); } 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 09605765..c35df4b5 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 @@ -35,7 +35,7 @@ public PatchChecksOption patchChecks() { @Override public Map extraErrorProneCheckOptions() { - return Map.of("SuppressibleErrorProne:Mode", "Apply"); + return Map.of(SUPPRESSIBLE_ERROR_PRONE_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 99a9bf11..531cb1f6 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 @@ -50,7 +50,7 @@ public Map extraErrorProneCheckOptions() { return Map.of( "SuppressibleErrorProne:RemoveRolloutSuppressions", context.flagValue().orElse(ALL_CHECKS), - "SuppressibleErrorProne:Mode", + SUPPRESSIBLE_ERROR_PRONE_MODE, "RemoveRollout"); } 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 60ef9981..11994699 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 @@ -42,7 +42,7 @@ public PatchChecksOption patchChecks() { @Override public Map extraErrorProneCheckOptions() { - return Map.of("SuppressibleErrorProne:Mode", "RemoveUnused"); + return Map.of(SUPPRESSIBLE_ERROR_PRONE_MODE, "RemoveUnused"); } @Override 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 7e359881..6ab6b064 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 @@ -39,7 +39,7 @@ public PatchChecksOption patchChecks() { @Override public Map extraErrorProneCheckOptions() { - return Map.of("SuppressibleErrorProne:Mode", "Suppress"); + return Map.of(SUPPRESSIBLE_ERROR_PRONE_MODE, "Suppress"); } }; } diff --git a/suppressible-error-prone/build.gradle b/suppressible-error-prone/build.gradle index 98b1067e..3327a9f2 100644 --- a/suppressible-error-prone/build.gradle +++ b/suppressible-error-prone/build.gradle @@ -47,6 +47,6 @@ publishing { tasks.withType(JavaCompile) { options.errorprone { - disable('StrictUnusedVariable') + disable('StrictUnusedVariable', 'PreferSafeLoggableExceptions') } } diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AllErrorprones.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AllErrorprones.java index 7a0cd2e9..c7ed15ce 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AllErrorprones.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AllErrorprones.java @@ -16,34 +16,33 @@ package com.palantir.suppressibleerrorprone; +import com.google.common.base.Suppliers; import com.google.errorprone.BugCheckerInfo; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.scanner.BuiltInCheckerSuppliers; import java.util.ServiceLoader; import java.util.Set; +import java.util.function.Supplier; import java.util.stream.Collectors; import java.util.stream.Stream; import one.util.streamex.StreamEx; public final class AllErrorprones { - private static Set cachedAllBugcheckerNames; + private static Supplier> cachedAllBugcheckerNames = Suppliers.memoize(() -> { + Stream pluginChecks = + ServiceLoader.load(BugChecker.class).stream().map(ServiceLoader.Provider::get); + Stream pluginCheckNames = + StreamEx.of(pluginChecks).flatMap(bugchecker -> bugchecker.allNames().stream()); - public static Set allBugcheckerNames() { - if (cachedAllBugcheckerNames == null) { - Stream pluginChecks = - ServiceLoader.load(BugChecker.class).stream().map(ServiceLoader.Provider::get); - Stream pluginCheckNames = - StreamEx.of(pluginChecks).flatMap(bugchecker -> bugchecker.allNames().stream()); - - Stream builtInChecks = BuiltInCheckerSuppliers.allChecks().getAllChecks().values().stream(); - Stream builtInCheckNames = - StreamEx.of(builtInChecks).flatMap(bugchecker -> bugchecker.allNames().stream()); + Stream builtInChecks = BuiltInCheckerSuppliers.allChecks().getAllChecks().values().stream(); + Stream builtInCheckNames = + StreamEx.of(builtInChecks).flatMap(bugchecker -> bugchecker.allNames().stream()); - cachedAllBugcheckerNames = - Stream.concat(pluginCheckNames, builtInCheckNames).collect(Collectors.toSet()); - } + return Stream.concat(pluginCheckNames, builtInCheckNames).collect(Collectors.toSet()); + }); - return cachedAllBugcheckerNames; + public static Set allBugcheckerNames() { + return cachedAllBugcheckerNames.get(); } private AllErrorprones() {} diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java index c7c6a9ae..754672bb 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/ReportedFixCache.java @@ -93,7 +93,7 @@ private LazySuppressionFix createAndReportFix( @SuppressWarnings("RestrictedApi") Description description = Description.builder( (DiagnosticPosition) declaration.getLeaf(), - "SuppressibleErrorProne", + SuppressibleErrorProne.class.getSimpleName(), "https://github.com/palantir/suppressible-error-prone", "A fix on a suppressible by suppressible-error-prone") .addFix(fix) 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 6d101f66..93bea9ba 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 @@ -37,7 +37,8 @@ public final class VisitorStateModifications { @SuppressWarnings("RestrictedApi") public static Description interceptDescription(VisitorState visitorState, Description description) { // Prevent infinite recursion on reported fixes - if (description == Description.NO_MATCH || description.checkName.equals("SuppressibleErrorProne")) { + if (description == Description.NO_MATCH + || description.checkName.equals(SuppressibleErrorProne.class.getSimpleName())) { return description; } @@ -103,7 +104,7 @@ public static Description interceptDescription(VisitorState visitorState, Descri } private static Set getModes(VisitorState state) { - return state.errorProneOptions().getFlags().getSetOrEmpty("SuppressibleErrorProne:Mode"); + return state.errorProneOptions().getFlags().getSetOrEmpty("SuppressibleErrorProne:Modes"); } private VisitorStateModifications() {} From 78001a95290fe9f41b5df81b35a1e1211aa0907c Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Tue, 21 Oct 2025 17:50:50 +0100 Subject: [PATCH 15/15] Add common constants --- .../suppressibleerrorprone/VisitorStateModifications.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 93bea9ba..82a47850 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 @@ -104,7 +104,7 @@ public static Description interceptDescription(VisitorState visitorState, Descri } private static Set getModes(VisitorState state) { - return state.errorProneOptions().getFlags().getSetOrEmpty("SuppressibleErrorProne:Modes"); + return state.errorProneOptions().getFlags().getSetOrEmpty("SuppressibleErrorProne:Mode"); } private VisitorStateModifications() {}