diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePlugin.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePlugin.java index 6a5bd658..b4fe0286 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePlugin.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePlugin.java @@ -19,9 +19,11 @@ import com.palantir.gradle.suppressibleerrorprone.modes.Modes; import com.palantir.gradle.suppressibleerrorprone.modes.common.CommonOptions; import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption; +import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.ModifiedFile; import com.palantir.gradle.suppressibleerrorprone.transform.ModifyErrorProneCheckApi; import java.util.List; import java.util.Optional; +import java.util.Set; import java.util.stream.Collectors; import javax.inject.Inject; import net.ltgt.gradle.errorprone.ErrorProneOptions; @@ -75,7 +77,7 @@ private void applyToJavaProject(Project project) { if (getModes().modifyCheckApi() instanceof ModifyCheckApiOption.MustModify mustModify) { // When auto-suppressing, the logic will run a bytecode patched version of errorprone // (via an artifact transform) that intercepts every error from every check and adds a custom fix - setupErrorProneArtifactTransform(project, mustModify.modifyVisitorState()); + setupErrorProneArtifactTransform(project, mustModify.modifiedFiles()); } project.getConfigurations().named(ErrorPronePlugin.CONFIGURATION_NAME).configure(errorProneConfiguration -> { @@ -108,7 +110,7 @@ private void applyToJavaProject(Project project) { }); } - private static void setupErrorProneArtifactTransform(Project project, boolean modifyVisitorState) { + private static void setupErrorProneArtifactTransform(Project project, Set modifiedFiles) { Attribute suppressible = Attribute.of("com.palantir.suppressible-error-prone.suppressible", Boolean.class); project.getDependencies().getAttributesSchema().attribute(suppressible); @@ -120,7 +122,7 @@ private static void setupErrorProneArtifactTransform(Project project, boolean mo .attribute(suppressible, false); project.getDependencies().registerTransform(ModifyErrorProneCheckApi.class, spec -> { - spec.getParameters().getModifyVisitorState().set(modifyVisitorState); + spec.getParameters().getFilesToModify().set(modifiedFiles); Attribute artifactType = Attribute.of("artifactType", String.class); spec.getFrom().attribute(suppressible, false).attribute(artifactType, "jar"); @@ -205,6 +207,10 @@ private void setupErrorProneOptions(CommonOptions commonOptions, ErrorProneOptio errorProneOptions.getErrorproneArgumentProviders().add(patchChecksCommandLineArgumentProvider); + errorProneOptions + .getIgnoreSuppressionAnnotations() + .set(getProviderFactory().provider(commonOptions::ignoreSuppressionAnnotations)); + errorProneOptions .getCheckOptions() .putAll(getProviderFactory().provider(commonOptions::extraErrorProneCheckOptions)); 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..1bc92472 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/Modes.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/Modes.java @@ -28,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( @@ -127,7 +133,8 @@ public final CommonOptions commonOptionsFor(JavaCompile javaCompile) { .append(nonInterferingCommonOptions) .toSet(); - return CommonOptions.naivelyCombine(allCommonOptions); + CommonOptions commonOptions = CommonOptions.naivelyCombine(allCommonOptions); + return commonOptions; } private Map> modesEnabledAndFlagValues() { diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/CommonOptions.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/CommonOptions.java index a6adcfeb..cba777d8 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/CommonOptions.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/CommonOptions.java @@ -21,6 +21,7 @@ import java.util.HashMap; import java.util.Map; import java.util.function.Supplier; +import java.util.stream.Collectors; import one.util.streamex.EntryStream; /** @@ -41,6 +42,10 @@ default RemoveRolloutCheck removeRolloutCheck() { return RemoveRolloutCheck.DISABLE; } + default boolean ignoreSuppressionAnnotations() { + return false; + } + default CommonOptions naivelyCombinedWith(CommonOptions other) { return new CommonOptions() { @Override @@ -52,13 +57,19 @@ 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 public RemoveRolloutCheck removeRolloutCheck() { return CommonOptions.this.removeRolloutCheck().or(other.removeRolloutCheck()); } + + @Override + public boolean ignoreSuppressionAnnotations() { + return CommonOptions.this.ignoreSuppressionAnnotations() || other.ignoreSuppressionAnnotations(); + } }; } @@ -66,18 +77,31 @@ static CommonOptions naivelyCombine(Collection commonOptions) { return commonOptions.stream().reduce(CommonOptions.empty(), CommonOptions::naivelyCombinedWith); } + // NOTE: Maybe CommonOptions itself should be an interface without default methods, while a + // DefaultCommonOptions provides sensible defaults. + // Or maybe CommonOptions should just be a record with methods default CommonOptions withExtraErrorProneCheckFlag(String key, Supplier value) { return new CommonOptions() { + @Override + public Map extraErrorProneCheckOptions() { + Map map = new HashMap<>(CommonOptions.this.extraErrorProneCheckOptions()); + map.put(key, value.get()); + return Collections.unmodifiableMap(map); + } + @Override public PatchChecksOption patchChecks() { return CommonOptions.this.patchChecks(); } @Override - public Map extraErrorProneCheckOptions() { - Map map = new HashMap<>(CommonOptions.this.extraErrorProneCheckOptions()); - map.put(key, value.get()); - return Collections.unmodifiableMap(map); + public RemoveRolloutCheck removeRolloutCheck() { + return CommonOptions.this.removeRolloutCheck(); + } + + @Override + public boolean ignoreSuppressionAnnotations() { + return CommonOptions.this.ignoreSuppressionAnnotations(); } }; } diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModeName.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModeName.java index d4863897..8e7f3c4c 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModeName.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModeName.java @@ -26,6 +26,7 @@ public enum ModeName { APPLY("errorProneApply"), SUPPRESS("errorProneSuppress"), REMOVE_ROLLOUT("errorProneRemoveRollout"), + REMOVE_UNUSED("errorProneRemoveUnused"), TIMINGS("errorProneTimings"), // Historically, the logic of this plugin lived in baseline, so we need to support the old disable flag. DISABLE("errorProneDisable", "com.palantir.baseline-error-prone.disable"), diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOption.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOption.java index 41969618..68958a64 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOption.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOption.java @@ -19,9 +19,12 @@ import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.DoNotModify; import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.MustModify; import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.NoEffect; +import java.util.Arrays; import java.util.Collection; import java.util.Set; import java.util.function.Predicate; +import java.util.stream.Collectors; +import java.util.stream.Stream; import one.util.streamex.StreamEx; /** @@ -47,19 +50,16 @@ static ModifyCheckApiOption noEffect() { } /** - * The {@code error_prone_check_api} jar must be modified to allow {@code `for-rollout:`} suppressions to work. + * Modify these files in the error-prone API. */ - static ModifyCheckApiOption mustModify() { - return new MustModify(false); + static MustModify mustModify(ModifiedFile... modifiedFile) { + return new MustModify(Arrays.stream(modifiedFile).collect(Collectors.toSet())); } - /** - * The {@code error_prone_check_api} jar must be modified to allow {@code `for-rollout:`} suppressions to work, - * and {@code VisitorState} must be modified to intercept reportMatch. - */ - static ModifyCheckApiOption mustModifyIncludingVisitorState() { - return new MustModify(true); + default Set getModifiedFiles() { + return Set.of(); } + ; enum DoNotModify implements ModifyCheckApiOption, CombinedValue { INSTANCE @@ -69,9 +69,11 @@ enum NoEffect implements ModifyCheckApiOption { INSTANCE } - record MustModify(boolean modifyVisitorState) implements ModifyCheckApiOption, CombinedValue { + record MustModify(Set modifiedFiles) implements ModifyCheckApiOption, CombinedValue { public MustModify combine(MustModify other) { - return new MustModify(modifyVisitorState || other.modifyVisitorState); + Set union = Stream.concat(modifiedFiles.stream(), other.modifiedFiles.stream()) + .collect(Collectors.toSet()); + return new MustModify(union); } } @@ -84,7 +86,7 @@ static CombinedValue combine(Collection options) { if (withoutNoEffects.isEmpty()) { // By default, we need to modify the check API to support for-rollout suppressions - return new MustModify(false); + return mustModify(ModifiedFile.BUG_CHECKER_INFO); } boolean doNotModify = withoutNoEffects.contains(DoNotModify.INSTANCE); @@ -103,4 +105,20 @@ static CombinedValue combine(Collection options) { .reduce(MustModify::combine) .get(); } + + enum ModifiedFile { + BUG_CHECKER_INFO("BugCheckerInfo"), + VISITOR_STATE("VisitorState"), + SUPPRESSIBLE_TREE_PATH_SCANNER("SuppressibleTreePathScanner"); + + private final String className; + + ModifiedFile(String className) { + this.className = className; + } + + public String getClassName() { + return className; + } + } } diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/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 68ef5a42..7e359881 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/SuppressMode.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/SuppressMode.java @@ -19,12 +19,14 @@ import com.palantir.gradle.suppressibleerrorprone.modes.common.CommonOptions; import com.palantir.gradle.suppressibleerrorprone.modes.common.Mode; import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption; +import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.ModifiedFile; import com.palantir.gradle.suppressibleerrorprone.modes.common.PatchChecksOption; +import java.util.Map; public final class SuppressMode implements Mode { @Override public ModifyCheckApiOption modifyCheckApi() { - return ModifyCheckApiOption.mustModifyIncludingVisitorState(); + return ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO, ModifiedFile.VISITOR_STATE); } @Override @@ -34,6 +36,11 @@ public CommonOptions configureAndReturnCommonOptions(ModeOptionContext context) public PatchChecksOption patchChecks() { return PatchChecksOption.allChecks(); } + + @Override + public Map extraErrorProneCheckOptions() { + return Map.of("SuppressibleErrorProne:Mode", "Suppress"); + } }; } } diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/transform/ModifyErrorProneCheckApi.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/transform/ModifyErrorProneCheckApi.java index 420b50bc..e26e3777 100644 --- a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/transform/ModifyErrorProneCheckApi.java +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/transform/ModifyErrorProneCheckApi.java @@ -16,6 +16,7 @@ package com.palantir.gradle.suppressibleerrorprone.transform; +import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.ModifiedFile; import com.palantir.gradle.suppressibleerrorprone.transform.ModifyErrorProneCheckApi.Params; import com.palantir.gradle.utils.environmentvariables.EnvironmentVariables; import java.io.BufferedOutputStream; @@ -36,6 +37,7 @@ import org.gradle.api.file.FileSystemLocation; import org.gradle.api.provider.Property; import org.gradle.api.provider.Provider; +import org.gradle.api.provider.SetProperty; import org.gradle.api.tasks.Input; import org.gradle.api.tasks.Nested; import org.objectweb.asm.ClassReader; @@ -67,7 +69,7 @@ private void suppressCheckApi(File output) { visitJar(output, (classJarPath, inputStream) -> classVisitorFor(classJarPath) .map(classVisitorFactory -> { ClassReader classReader = newClassReader(inputStream); - ClassWriter classWriter = new ClassWriter(classReader, 0); + ClassWriter classWriter = new ClassWriter(classReader, ClassWriter.COMPUTE_FRAMES); ClassVisitor classVisitor = classVisitorFactory.apply(classWriter); classReader.accept(classVisitor, 0); @@ -79,7 +81,8 @@ private Optional> classVisitorFor(String classJarPat // We always want to modify the BugCheckerInfo class, as this is where we help errorprone understand // what the `for-rollout:` prefix for suppressions mean (which means this class is always modified, // even during normal compilation). - if (classJarPath.equals("com/google/errorprone/BugCheckerInfo.class")) { + if (classJarPath.equals("com/google/errorprone/BugCheckerInfo.class") + && getParameters().getFilesToModify().get().contains(ModifiedFile.BUG_CHECKER_INFO)) { return Optional.of(BugCheckerInfoVisitor::new); } @@ -87,10 +90,15 @@ private Optional> classVisitorFor(String classJarPat // it intercepts all errors and changes them. So when we're just running normal compilation, we want // to avoid running our modifications at all, and let the errors continue on their way unchanged. if (classJarPath.equals("com/google/errorprone/VisitorState.class") - && getParameters().getModifyVisitorState().get()) { + && getParameters().getFilesToModify().get().contains(ModifiedFile.VISITOR_STATE)) { return Optional.of(VisitorStateClassVisitor::new); } + if (classJarPath.equals("com/google/errorprone/bugpatterns/BugChecker$SuppressibleTreePathScanner.class") + && getParameters().getFilesToModify().get().contains(ModifiedFile.SUPPRESSIBLE_TREE_PATH_SCANNER)) { + return Optional.of(SuppressibleTreePathScannerClassVisitor::new); + } + return Optional.empty(); } @@ -132,7 +140,7 @@ private void visitJar(File output, ClassTransformer classTransformer) { public abstract static class Params implements TransformParameters { @Input - public abstract Property getModifyVisitorState(); + public abstract SetProperty getFilesToModify(); @Input public abstract Property getCacheBust(); diff --git a/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/transform/SuppressibleTreePathScannerClassVisitor.java b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/transform/SuppressibleTreePathScannerClassVisitor.java new file mode 100644 index 00000000..dce49e60 --- /dev/null +++ b/gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/transform/SuppressibleTreePathScannerClassVisitor.java @@ -0,0 +1,86 @@ +/* + * (c) Copyright 2025 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.gradle.suppressibleerrorprone.transform; + +import org.objectweb.asm.ClassVisitor; +import org.objectweb.asm.Label; +import org.objectweb.asm.MethodVisitor; +import org.objectweb.asm.Opcodes; + +/** + * Forces {@code SuppressibleTreePathScanner} to respect the ignore suppressions option. + * + *

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

This class uses bytecode manipulation to patch {@code SuppressibleTreePathScanner::isSuppressed} + * to respect the ErrorProneOptions setting. + */ +final class SuppressibleTreePathScannerClassVisitor extends ClassVisitor { + SuppressibleTreePathScannerClassVisitor(ClassVisitor classVisitor) { + super(Opcodes.ASM9, classVisitor); + } + + @Override + public MethodVisitor visitMethod( + int access, String name, String descriptor, String signature, String[] exceptions) { + MethodVisitor methodVisitor = super.visitMethod(access, name, descriptor, signature, exceptions); + + if (name.equals("suppressed") && descriptor.equals("(Lcom/sun/source/tree/Tree;)Z")) { + return new SuppressedMethodVisitor(methodVisitor); + } + + return methodVisitor; + } + + private static final class SuppressedMethodVisitor extends MethodVisitor { + SuppressedMethodVisitor(MethodVisitor methodVisitor) { + super(Opcodes.ASM9, methodVisitor); + } + + @Override + public void visitCode() { + super.visitCode(); + // Load this (SuppressibleTreePathScanner instance) + mv.visitVarInsn(Opcodes.ALOAD, 0); + // Get the state field + mv.visitFieldInsn( + Opcodes.GETFIELD, + "com/google/errorprone/bugpatterns/BugChecker$SuppressibleTreePathScanner", + "state", + "Lcom/google/errorprone/VisitorState;"); + // Check condition and potentially return false early + mv.visitMethodInsn( + Opcodes.INVOKESTATIC, + "com/palantir/suppressibleerrorprone/SuppressibleTreePathScannerModifications", + "shouldIgnoreSuppressions", + "(Lcom/google/errorprone/VisitorState;)Z", + false); + + // If condition is true, return false immediately + Label continueLabel = new Label(); + mv.visitJumpInsn(Opcodes.IFEQ, continueLabel); + mv.visitInsn(Opcodes.ICONST_0); // push false + mv.visitInsn(Opcodes.IRETURN); + mv.visitLabel(continueLabel); + // Add a frame here to fix verification + mv.visitFrame(Opcodes.F_SAME, 0, null, 0, null); + } + } +} diff --git a/gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy b/gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy index 37d350cf..9fe1e7c6 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 @@ -1589,6 +1588,355 @@ class SuppressibleErrorPronePluginIntegrationTest extends ConfigurationCacheSpec !output.contains('[RemoveRolloutSuppressions]') } + def 'errorProneRemoveUnused removes only unused suppressions'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + + @SuppressWarnings({"ArrayToString", "UnnecessaryFinal", "InlineTrivialConstant"}) + public final class App { + private static final String EMPTY_STRING = ""; + + public static void main(String[] args) { + new int[3].toString(); + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused=ArrayToString') + + then: + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + @SuppressWarnings({"ArrayToString", "InlineTrivialConstant"}) + public final class App { + private static final String EMPTY_STRING = ""; + + public static void main(String[] args) { + new int[3].toString(); + } + } + '''.stripIndent(true) + } + + def 'errorProneRemoveUnused only keeps the closest suppression to a violation'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + + @SuppressWarnings("InlineTrivialConstant") + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + @SuppressWarnings("InlineTrivialConstant") + class Inner { + @SuppressWarnings("InlineTrivialConstant") + class InnerInner { + @SuppressWarnings("InlineTrivialConstant") + class InnerInnerInner { + private static final String EMPTY = ""; + } + } + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused') + + then: + + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + class Inner { + class InnerInner { + @SuppressWarnings("InlineTrivialConstant") + class InnerInnerInner { + private static final String EMPTY = ""; + } + } + } + } + '''.stripIndent(true) + } + + def 'errorProneRemoveUnused handles multiple suppressions on different tree types gracefully'() { + // Here we test the three types of trees you can suppress — ClassTree, MethodTree, VariableTree + + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + // Doesn't move an already existing suppression, even if it could be closer to the violation + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + boolean truism = new int[3].equals(new int[3]); + + @SuppressWarnings("InlineTrivialConstant") + static class InnerInner { + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused') + + then: + + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + // Doesn't move an already existing suppression, even if it could be closer to the violation + @SuppressWarnings("ArrayEquals") + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + + boolean truism = new int[3].equals(new int[3]); + + static class InnerInner { + @SuppressWarnings("ArrayEquals") + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + } + + def 'errorProneRemoveUnused removes entire SuppressWarnings annotation when all suppressions are unused'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + + @SuppressWarnings({"UnusedVariable", "ArrayToString"}) + public final class App { + public static void main(String[] args) { + System.out.println("No violations here"); + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused') + + then: + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + public final class App { + public static void main(String[] args) { + System.out.println("No violations here"); + } + } + '''.stripIndent(true) + } + + def 'errorProneRemoveUnused and errorProneSuppress uses existing suppressions if possible'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + + @SuppressWarnings("InlineTrivialConstant") + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + @SuppressWarnings("InlineTrivialConstant") + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + boolean truism = new int[3].equals(new int[3]); + + @SuppressWarnings("InlineTrivialConstant") + static class InnerInner { + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused', '-PerrorProneSuppress') + + then: + + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + + @SuppressWarnings("for-rollout:ArrayEquals") + boolean truism = new int[3].equals(new int[3]); + + static class InnerInner { + @SuppressWarnings("ArrayEquals") + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + } + + def 'errorProneRemoveUnused and errorProneApply applies fixes on previously suppressed elements'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + boolean truism = new int[3].equals(new int[3]); + + @SuppressWarnings("InlineTrivialConstant") + static class InnerInner { + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused', '-PerrorProneApply=ArrayEquals') + + then: + + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + import java.util.Arrays; + + public final class App { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + static class Inner { + @SuppressWarnings("InlineTrivialConstant") + private static final String EMPTY = ""; + + boolean truism = Arrays.equals(new int[3], new int[3]); + + static class InnerInner { + void method() { + Arrays.equals(new int[3], new int[3]); + } + } + } + } + '''.stripIndent(true) + } + + def 'errorProneRemoveUnused + errorProneApply + errorProneSuppress applies fixes and suppressions on previously suppressed elements'() { + // language=Java + writeJavaSourceFileToSourceSets ''' + package app; + + @SuppressWarnings("ArrayEquals") + public final class App { + private static final String EMPTY_STRING = ""; + + // Although InlineTrivialConstant can be placed lower in the AST hierarchy, + // we preserve existing suppressions whenever possible rather than move suppressions around. + // Also, note that we don't add for-rollout here. + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + static class Inner { + private static final String EMPTY = ""; + boolean truism = new int[3].equals(new int[3]); + + @SuppressWarnings("InlineTrivialConstant") + static class InnerInner { + @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) + void method() { + new int[3].equals(new int[3]); + } + } + } + } + '''.stripIndent(true) + + when: + runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused', '-PerrorProneSuppress', '-PerrorProneApply=ArrayEquals') + + then: + + // language=Java + javaSourceIsSyntacticallyEqualTo ''' + package app; + + import java.util.Arrays; + + public final class App { + @SuppressWarnings("for-rollout:InlineTrivialConstant") + private static final String EMPTY_STRING = ""; + + // Although InlineTrivialConstant can be placed lower in the AST hierarchy, + // we preserve existing suppressions whenever possible rather than move suppressions around. + // Also, note that we don't add for-rollout here. + @SuppressWarnings("InlineTrivialConstant") + static class Inner { + private static final String EMPTY = ""; + boolean truism = Arrays.equals(new int[3], new int[3]); + + static class InnerInner { + void method() { + Arrays.equals(new int[3], new int[3]); + } + } + } + } + '''.stripIndent(true) + } + def 'error-prone dependencies have versions bound together by a virtual platform'() { setup: 'when an error-prone dependency is forced to certain version' // language=Gradle diff --git a/gradle-suppressible-error-prone/src/test/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOptionTest.java b/gradle-suppressible-error-prone/src/test/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOptionTest.java index 546d98ee..019e4ef8 100644 --- a/gradle-suppressible-error-prone/src/test/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOptionTest.java +++ b/gradle-suppressible-error-prone/src/test/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOptionTest.java @@ -20,6 +20,7 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.DoNotModify; +import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.ModifiedFile; import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.MustModify; import java.util.List; import org.junit.jupiter.api.Test; @@ -27,36 +28,42 @@ class ModifyCheckApiOptionTest { @Test void combining_must_modify_instances_with_different_visitor_states_results_in_true() { - MustModify withoutVisitorState = new MustModify(false); - MustModify withVisitorState = new MustModify(true); + MustModify withoutVisitorState = ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO); + MustModify withVisitorState = + ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO, ModifiedFile.VISITOR_STATE); - assertThat(withoutVisitorState.combine(withVisitorState).modifyVisitorState()) + assertThat(withoutVisitorState.combine(withVisitorState).modifiedFiles().contains(ModifiedFile.VISITOR_STATE)) .isTrue(); - assertThat(withVisitorState.combine(withoutVisitorState).modifyVisitorState()) + assertThat(withVisitorState.combine(withoutVisitorState).modifiedFiles().contains(ModifiedFile.VISITOR_STATE)) .isTrue(); } @Test void combining_must_modify_instances_with_same_visitor_states_preserves_state() { - MustModify bothFalse1 = new MustModify(false); - MustModify bothFalse2 = new MustModify(false); - assertThat(bothFalse1.combine(bothFalse2).modifyVisitorState()).isFalse(); + MustModify bothFalse1 = ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO); + MustModify bothFalse2 = ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO); + assertThat(bothFalse1.combine(bothFalse2).modifiedFiles().contains(ModifiedFile.VISITOR_STATE)) + .isFalse(); - MustModify bothTrue1 = new MustModify(true); - MustModify bothTrue2 = new MustModify(true); - assertThat(bothTrue1.combine(bothTrue2).modifyVisitorState()).isTrue(); + MustModify bothTrue1 = + ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO, ModifiedFile.VISITOR_STATE); + MustModify bothTrue2 = + ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO, ModifiedFile.VISITOR_STATE); + assertThat(bothTrue1.combine(bothTrue2).modifiedFiles().contains(ModifiedFile.VISITOR_STATE)) + .isTrue(); } @Test void combining_empty_collection_returns_must_modify() { - assertThat(ModifyCheckApiOption.combine(List.of())).isEqualTo(new MustModify(false)); + assertThat(ModifyCheckApiOption.combine(List.of())) + .isEqualTo(ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO)); } @Test void combining_only_no_effect_returns_must_modify() { assertThat(ModifyCheckApiOption.combine( List.of(ModifyCheckApiOption.noEffect(), ModifyCheckApiOption.noEffect()))) - .isEqualTo(new MustModify(false)); + .isEqualTo(ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO)); } @Test @@ -69,25 +76,27 @@ void combining_do_not_modify_with_no_effect_returns_do_not_modify() { @Test void combining_must_modify_with_no_effect_causes_no_change() { - assertThat(ModifyCheckApiOption.combine( - List.of(ModifyCheckApiOption.mustModify(), ModifyCheckApiOption.noEffect()))) - .isEqualTo(new MustModify(false)); + assertThat(ModifyCheckApiOption.combine(List.of( + ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO), + ModifyCheckApiOption.noEffect()))) + .isEqualTo(ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO)); } @Test void combining_must_modify_including_visitor_state_with_no_effect_causes_no_change() { assertThat(ModifyCheckApiOption.combine(List.of( - ModifyCheckApiOption.mustModifyIncludingVisitorState(), ModifyCheckApiOption.noEffect()))) - .isEqualTo(new MustModify(true)); + ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO, ModifiedFile.VISITOR_STATE), + ModifyCheckApiOption.noEffect()))) + .isEqualTo(ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO, ModifiedFile.VISITOR_STATE)); } @Test void combining_multiple_must_modify_options_combines_visitor_states() { assertThat(ModifyCheckApiOption.combine(List.of( - ModifyCheckApiOption.mustModify(), - ModifyCheckApiOption.mustModifyIncludingVisitorState(), + ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO), + ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO, ModifiedFile.VISITOR_STATE), ModifyCheckApiOption.noEffect()))) - .isEqualTo(new MustModify(true)); + .isEqualTo(ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO, ModifiedFile.VISITOR_STATE)); } @Test diff --git a/suppressible-error-prone/build.gradle b/suppressible-error-prone/build.gradle index ece60e95..c6aa36a7 100644 --- a/suppressible-error-prone/build.gradle +++ b/suppressible-error-prone/build.gradle @@ -4,6 +4,7 @@ apply plugin: 'com.palantir.external-publish-jar' dependencies { implementation 'com.google.errorprone:error_prone_annotation' implementation 'com.google.errorprone:error_prone_check_api' + implementation 'com.google.errorprone:error_prone_core' annotationProcessor 'com.google.auto.service:auto-service' compileOnly 'com.google.auto.service:auto-service' diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AnnotationUtils.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/AnnotationUtils.java index 12f7ef9b..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/SuppressingFix.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/LazySuppressionFix.java similarity index 54% rename from suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressingFix.java rename to suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/LazySuppressionFix.java index 650497c2..0ff6c62c 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressingFix.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/LazySuppressionFix.java @@ -29,26 +29,40 @@ import java.util.Set; import java.util.function.Function; -final class SuppressingFix implements Fix { - private final Set newSuppressions = new HashSet<>(); +/** + * A Fix that can lazily add/modify/remove @SuppressWarnings annotations with proper line handling. + * When removing suppressions entirely, it will also remove preceding whitespace up to and including + * the newline to avoid leaving empty lines. + * Sorts suppressions by human-authored first, then alphabetically. + */ +final class LazySuppressionFix implements Fix { + private final Set desiredSuppressions = new HashSet<>(); private final Function> replacement; - SuppressingFix(Optional sourceCode, Optional suppressWarnings, Tree tree) { - // See note in SuppressingReplacement about when we have to calculate stuff - // We *cannot* simply make this a memoized supplier. The first thing error-prone does with the Fix is to - // evaluate it to produce a nice error message, and we don't want to fix the number of suppression we make - // until we're ready to produce the Replacement after *all* the error-prone checks have been run. - // In order for SuppressingReplacement to calculate source code positions elements when it's constructed, it - // needs an EndPosTable. However, we don't get the EndPosTable until getReplacements is called. So we have - // to use this FirstTimeMemoizingFunction thing, that will allow use to defer creating the Replacement until - // we have access to the EndPosTable, then keep hold of the created SuppressingReplacement. We only need a - // single instance of EndPosTable to evaluate the source positions exactly once, so this works out. - this.replacement = new FirstTimeMemoizingFunction<>((EndPosTable endPositions) -> ImmutableSet.of( - new SuppressingReplacement(endPositions, newSuppressions, sourceCode, suppressWarnings, tree))); + private LazySuppressionFix( + Optional sourceCode, Optional suppression, Tree declaration) { + // Defer replacement creation until we have EndPosTable and all suppressions have been added + this.replacement = new FirstTimeMemoizingFunction<>( + (EndPosTable endPositions) -> ImmutableSet.of(new LazySuppressionReplacement( + endPositions, desiredSuppressions, sourceCode, suppression, declaration))); + } + + LazySuppressionFix( + Optional sourceCode, + Optional suppressWarnings, + Tree tree, + Set desiredSuppressions) { + this(sourceCode, suppressWarnings, tree); + this.desiredSuppressions.addAll(desiredSuppressions); + } + + public static LazySuppressionFix empty( + Optional sourceCode, Optional suppression, Tree tree) { + return new LazySuppressionFix(sourceCode, suppression, tree); } public void addSuppression(String suppression) { - newSuppressions.add(suppression); + desiredSuppressions.add(suppression); } @Override @@ -58,12 +72,12 @@ public ImmutableSet getReplacements(EndPosTable endPositions) { @Override public String toString(JCCompilationUnit compilationUnit) { - return "SuppressingFix"; + return "LazySuppressionFix"; } @Override public String getShortDescription() { - return "Adding automatic suppressions"; + return "Adding/modifying/removing @SuppressWarnings with proper line handling"; } @Override diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressingReplacement.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/LazySuppressionReplacement.java similarity index 59% rename from suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressingReplacement.java rename to suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/LazySuppressionReplacement.java index be92a289..4e44d8bf 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressingReplacement.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/LazySuppressionReplacement.java @@ -27,42 +27,37 @@ import java.util.Set; import java.util.stream.Collectors; -final class SuppressingReplacement extends Replacement { - // We really do need to be this lazy for generating the Replacements, as error-prone immediately converts - // the Fix to a Replacement when a Description is given to it, and we need to defer the computation of the - // Replacement until a number of Descriptions have been produced, to handle multiple errors being suppressed - // at the same level. - +/** + * A Replacement that handles @SuppressWarnings modifications with line-removing capabilities. + * When the final replacement text is empty (no suppressions), it will remove preceding + * whitespace up to and including the newline. + */ +final class LazySuppressionReplacement extends Replacement { private final Range range; private final List existingSuppressions; private final String suffix; - private final Set newSuppressions; + private final Set desiredSuppressions; + private final Optional sourceCode; + private final Optional suppressWarnings; - SuppressingReplacement( + LazySuppressionReplacement( EndPosTable endPositions, - Set newSuppressions, + Set desiredSuppressions, Optional sourceCode, Optional suppressWarnings, - Tree tree) { - // Note this is a *mutable* set from SuppressingFix, we need to able to add a new suppression before this - // instance is instantiated - this.newSuppressions = newSuppressions; + Tree declaration) { + this.desiredSuppressions = desiredSuppressions; + this.sourceCode = sourceCode; + this.suppressWarnings = suppressWarnings; - // There is an additional issue that by the time error-prone comes around to apply the replacements, the - // compiler seems to change the representation of the tree for another phase - `App.Builder` becomes - // `App$Builder` etc and the start position for the expression changes to be after `App` rather than at the - // start of `App`. If we calculate the replacement range too late, we insert our @SuppressWarnings at the - // wrong location, and the indentation is miscalculated. But we can't calculate the replacement string - // straight away, as we might not have all the new suppressions added yet. So we have to immediately - // calculate the replacement range and indentation/suffix, but hold off building the final replacement - // string until we have all the new suppressions. - this.range = calculateRange(endPositions, suppressWarnings, tree); + // Calculate range immediately to avoid tree representation changes + this.range = calculateRange(endPositions, suppressWarnings, declaration); this.suffix = suppressWarnings // If we're replacing an existing @SuppressWarnings, there's no need to add an indent .map(_ignored -> "") // If we're adding a new @SuppressWarnings, we need to indent the next line correctly - .orElseGet(() -> "\n" + SourceCodeUtils.indentForTree(sourceCode, tree)); + .orElseGet(() -> "\n" + SourceCodeUtils.indentForTree(sourceCode, declaration)); this.existingSuppressions = suppressWarnings.stream() .flatMap(AnnotationUtils::annotationStringValues) @@ -71,13 +66,32 @@ final class SuppressingReplacement extends Replacement { @Override public Range range() { + if (desiredSuppressions.isEmpty() && sourceCode.isPresent()) { + // If the next element is in a newline, remove the newline as well. + // Ideally, we also remove whitespace before the next element, but that would overlap with any done on + // the next element. We leave those spaces to the formatter. + // Otherwise, the next element is a non-whitespace. Remove up until the first non-whitespace. + int end = SourceCodeUtils.firstNonWhitespaceOrNextLineStart(sourceCode.get(), range.upperEndpoint()); + return Range.closedOpen(range.lowerEndpoint(), end); + } return range; } @Override public String replaceWith() { + String result = calculateReplacementText(); + return result; + } + + private String calculateReplacementText() { + if (desiredSuppressions.isEmpty()) { + // No suppressions left, return empty string (line removal will be handled by range()) + return ""; + } + + // Generate the @SuppressWarnings annotation with remaining suppressions return SuppressWarningsUtils.suppressWarningsString( - SuppressWarningsUtils.modifySuppressions(existingSuppressions, newSuppressions)) + SuppressWarningsUtils.sortHumanFirstThenAlphabetical(desiredSuppressions)) + suffix; } @@ -85,7 +99,7 @@ private static Range calculateRange( EndPosTable endPositions, Optional suppressWarnings, Tree tree) { return suppressWarnings .map(annotationTree -> { - // @SuppressWarnings already exists, we need to replace the whole expression with our own + // @SuppressWarnings already exists, we need to replace the whole expression DiagnosticPosition position = (DiagnosticPosition) annotationTree; return Range.closedOpen(position.getStartPosition(), position.getEndPosition(endPositions)); }) diff --git a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveRolloutSuppressions.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveRolloutSuppressions.java index fff0f272..654322c1 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveRolloutSuppressions.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveRolloutSuppressions.java @@ -17,19 +17,15 @@ package com.palantir.suppressibleerrorprone; import com.google.auto.service.AutoService; -import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; -import com.google.errorprone.fixes.Fix; -import com.google.errorprone.fixes.Replacement; -import com.google.errorprone.fixes.Replacements.CoalescePolicy; import com.google.errorprone.matchers.Description; import com.sun.source.tree.AnnotationTree; -import com.sun.tools.javac.tree.EndPosTable; -import com.sun.tools.javac.tree.JCTree.JCCompilationUnit; -import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition; +import com.sun.source.tree.Tree; +import java.util.HashSet; import java.util.List; +import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; import javax.lang.model.element.Name; @@ -87,81 +83,12 @@ public Description matchAnnotation(AnnotationTree tree, VisitorState state) { return Description.NO_MATCH; } - String updatedText = SuppressWarningsUtils.suppressWarningsString(updatedSuppressions); - - return buildDescription(tree) - .addFix(new LineRemovingReplacementFix(state.getSourceCode(), (DiagnosticPosition) tree, updatedText)) - .build(); - } - - /** - * This class has been introduced because the normal {@link com.google.errorprone.fixes.SuggestedFix} does not - * allow us to introduce a Replacement which has a different start position than the tree's normal start position. - * Here we want to: - * - replace the element defined by the provided position - * - also replace the whitespace before the element, up to and including the newline - * This way, if e.g. @SuppressWarnings("foo") must be removed entirely, we can remove the entire line, rather than - * just the annotation, leaving us with an empty line. - * - * Note that this will only delete the whitespace before the element if the entire element is removed - * (i.e. if the replacement text is null or empty). - */ - private static final class LineRemovingReplacementFix implements Fix { - private final CharSequence sourceCode; - private final DiagnosticPosition position; - private final String replacementText; - - private LineRemovingReplacementFix( - CharSequence sourceCode, DiagnosticPosition position, String replacementText) { - this.sourceCode = sourceCode; - this.position = position; - this.replacementText = replacementText; - } - - @Override - public String toString(JCCompilationUnit compilationUnit) { - return "LineRemovingReplacementFix"; - } - - @Override - public String getShortDescription() { - return "Replace text at the position with the provided text, " - + "or remove the text and all preceding whitespace"; - } - - @Override - public CoalescePolicy getCoalescePolicy() { - return CoalescePolicy.REJECT; - } - - @Override - public ImmutableSet getReplacements(EndPosTable endPositions) { - if (replacementText.isEmpty() && sourceCode != null) { - // If the next element is in a newline, remove the newline as well. - // Ideally, we also remove whitespace before the next element, but that would overlap with any done on - // the next element. We leave those spaces to the formatter. - // Otherwise, the next element is a non-whitespace. Remove up until the first non-whitespace. - int end = SourceCodeUtils.firstNonWhitespaceOrNextLineStart( - sourceCode, position.getEndPosition(endPositions)); - return ImmutableSet.of(Replacement.create(position.getStartPosition(), end, "")); - } - return ImmutableSet.of(Replacement.create( - position.getStartPosition(), position.getEndPosition(endPositions), replacementText)); - } - - @Override - public ImmutableSet getImportsToAdd() { - return ImmutableSet.of(); - } - - @Override - public ImmutableSet getImportsToRemove() { - return ImmutableSet.of(); - } - - @Override - public boolean isEmpty() { - return false; - } + Tree declaration = state.getPath().getParentPath().getParentPath().getLeaf(); + LazySuppressionFix fix = new LazySuppressionFix( + Optional.ofNullable(state.getSourceCode()), + Optional.of(tree), + declaration, + new HashSet<>(updatedSuppressions)); + return buildDescription(tree).addFix(fix).build(); } } 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 e7c0c376..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,14 +16,18 @@ 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; import com.sun.source.tree.Tree; import com.sun.source.tree.VariableTree; import com.sun.source.util.TreePath; +import java.util.Collection; +import java.util.Comparator; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.function.Predicate; import java.util.stream.Collectors; @@ -39,6 +43,14 @@ public static SuppressionsType fromName(String name) { } } + public static List sortHumanFirstThenAlphabetical(Collection suppressions) { + return suppressions.stream() + .sorted(Comparator.comparing((String suppression) -> + SuppressionsType.fromName(suppression) == SuppressionsType.AUTOMATICALLY_ADDED) + .thenComparing(String::compareTo)) + .collect(Collectors.toList()); + } + public static List modifySuppressions( List existingSuppressions, Set newAutomatedSuppressions) { Map> automaticallyAddedOrNotSuppressions = existingSuppressions.stream() @@ -120,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/SuppressibleTreePathScannerModifications.java b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleTreePathScannerModifications.java new file mode 100644 index 00000000..30714d0c --- /dev/null +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressibleTreePathScannerModifications.java @@ -0,0 +1,28 @@ +/* + * (c) Copyright 2025 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.suppressibleerrorprone; + +import com.google.errorprone.VisitorState; + +public class SuppressibleTreePathScannerModifications { + + public static boolean shouldIgnoreSuppressions(VisitorState state) { + return state.errorProneOptions().isIgnoreSuppressionAnnotations(); + } + + private SuppressibleTreePathScannerModifications() {} +} 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 251a14ae..e04dd9ba 100644 --- a/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java +++ b/suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java @@ -23,35 +23,92 @@ import com.google.errorprone.matchers.Description; import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.ClassTree; +import com.sun.source.tree.CompilationUnitTree; import com.sun.source.tree.MethodTree; import com.sun.source.tree.ModifiersTree; import com.sun.source.tree.Tree; import com.sun.source.tree.VariableTree; import com.sun.source.util.TreePath; +import com.sun.source.util.TreePathScanner; +import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition; +import java.util.HashSet; import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.WeakHashMap; import java.util.logging.Logger; +import java.util.stream.Collectors; import java.util.stream.Stream; import javax.lang.model.element.Name; // CHECKSTYLE:ON +@SuppressWarnings("RestrictedApi") public final class VisitorStateModifications { private static final Logger log = Logger.getLogger(VisitorStateModifications.class.getName()); + private static final class ReportedFixCache { + private final Map cache = new WeakHashMap<>(); + + // declaration is a TreePath because we need information about it's parent to check if it is suppressible + LazySuppressionFix getOrCreate(TreePath declaration, VisitorState visitorState) { + return cache.computeIfAbsent(declaration.getLeaf(), _ignored -> createFix(declaration, visitorState)); + } + + private LazySuppressionFix createFix(TreePath declaration, VisitorState visitorState) { + if (!SuppressWarningsUtils.suppressibleTreePath(declaration)) { + throw new IllegalArgumentException("Not suppressible: " + declaration); + } + ModifiersTree modifiersTree = modifiersTree(declaration.getLeaf()).get(); + Optional suppressWarnings = modifiersTree.getAnnotations().stream() + .filter(annotation -> { + Name annotationName = AnnotationUtils.annotationName(annotation.getAnnotationType()); + return annotationName.contentEquals(CommonConstants.SUPPRESS_WARNINGS_ANNOTATION); + }) + .findFirst(); + + LazySuppressionFix fix = LazySuppressionFix.empty( + Optional.ofNullable(visitorState.getSourceCode()), suppressWarnings, declaration.getLeaf()); + Description description = Description.builder( + (DiagnosticPosition) declaration.getLeaf(), "SuppressibleErrorProne", "", "") + .addFix(fix) + .build(); + visitorState.reportMatch(description); + return fix; + } + + boolean containsKey(Tree key) { + return cache.containsKey(key); + } + + int size() { + return cache.size(); + } + } + // Weak map so that we don't leak memory by keeping hold of references to the source element tree keys and our // mutable fixes values around forever, once error-prone has finished with the source element tree used as a key // here (once the file has been visited by all the error-prone checks), our SuppressingFixes can be safely // garbage collected. - private static final Map FIXES = new WeakHashMap<>(); + private static final Set SEEN = new HashSet<>(); + private static final Map> OLD_SUPPRESSIONS = new WeakHashMap<>(); + private static final ReportedFixCache FIXES = new ReportedFixCache(); - @SuppressWarnings("RestrictedApi") + @SuppressWarnings("CyclomaticComplexity") // Unfortunately this logic is just complex public static Description interceptDescription(VisitorState visitorState, Description description) { - if (description == Description.NO_MATCH) { + if (description == Description.NO_MATCH || description.checkName.equals("SuppressibleErrorProne")) { return description; } + Set modes = visitorState.errorProneOptions().getFlags().getSetOrEmpty("SuppressibleErrorProne:Mode"); + + CompilationUnitTree compilationUnit = visitorState.getPath().getCompilationUnit(); + + if (modes.contains("RemoveUnused") && !SEEN.contains(compilationUnit)) { + attachEmptySuppressionFixesToAllSuppressibles(compilationUnit, visitorState); + cacheOldSuppressions(compilationUnit, visitorState); + SEEN.add(compilationUnit); + } + // If both -PerrorProneSuppress and -PerrorProneApply are used at the same time, for the checks configured as // "patchChecks" in the extension we need to use their suggested fixes instead of suppressing, so we can do // both in one pass as if you'd run -PerrorProneApply and -PerrorProneSuppress separately. We pass the checks @@ -80,6 +137,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)) @@ -96,43 +173,77 @@ 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(); + TreePath firstSuppressibleParent = firstSuppressible.get(); // In order to be able to suppress multiple errors in one pass on the same element, we need to do a single // Fix/Replacement in error-prone. It's not possible to do this bit by bit with multiple Replacements. To do // this, we make sure we only make one fix per source element we put the suppression on by using a Map. This // way we have our own mutable Fix that we can add errors to, and only once the file has been visited by all // the error-prone checks it will then produce a replacement with all the checks suppressed. - boolean alreadyReportedFix = FIXES.containsKey(firstSuppressibleParent); + suppressCheck( + visitorState, + CommonConstants.AUTOMATICALLY_ADDED_PREFIX + description.checkName, + firstSuppressibleParent); - SuppressingFix suppressingFix = FIXES.computeIfAbsent( - firstSuppressibleParent, - _ignored -> new SuppressingFix( - Optional.ofNullable(visitorState.getSourceCode()), suppressWarnings, firstSuppressibleParent)); + return Description.NO_MATCH; + } - suppressingFix.addSuppression(description.checkName); + private static void cacheOldSuppressions(CompilationUnitTree compilationUnit, VisitorState visitorState) { + new TreePathScanner() { + @Override + public Void visitAnnotation(AnnotationTree node, Void unused) { + if (AnnotationUtils.isSuppressWarningsAnnotation(node)) { + Tree declaration = + getCurrentPath().getParentPath().getParentPath().getLeaf(); + OLD_SUPPRESSIONS.put( + declaration, + AnnotationUtils.annotationStringValues(node).collect(Collectors.toSet())); + } - // If we already submitted our mutable fix, we don't need to do so again, just need to add the error to the fix. - if (alreadyReportedFix) { - return Description.NO_MATCH; + return super.visitAnnotation(node, unused); + } + }.scan(compilationUnit, null); + } + + private static void suppressCheck(VisitorState visitorState, String suppression, TreePath suppressibleParent) { + LazySuppressionFix lazyFix = FIXES.getOrCreate(suppressibleParent, visitorState); + lazyFix.addSuppression(suppression); + } + + private static boolean suppresses(TreePath declaration, Description description) { + if (!SuppressWarningsUtils.suppressibleTreePath(declaration)) { + return false; + } + + Optional suppressWarningsMaybe = + SuppressWarningsUtils.getSuppressWarnings(declaration); + if (suppressWarningsMaybe.isEmpty()) { + return false; } - return Description.builder( - description.position, - description.checkName, - description.getLink(), - description.getMessageWithoutCheckName()) - .addFix(suppressingFix) - .build(); + String checkName = description.checkName; + return AnnotationUtils.annotationStringValues(suppressWarningsMaybe.get()) + .anyMatch(checkName::equals); + } + + private static void attachEmptySuppressionFixesToAllSuppressibles( + CompilationUnitTree compilationUnit, VisitorState visitorState) { + + new TreePathScanner() { + @Override + public Void visitAnnotation(AnnotationTree node, Void unused) { + if (AnnotationUtils.isSuppressWarningsAnnotation(node)) { + 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.getOrCreate(declaration, visitorState); + } + + return super.visitAnnotation(node, unused); + } + }.scan(compilationUnit, null); } private static Optional modifiersTree(Tree tree) { diff --git a/suppressible-error-prone/src/test/java/com/palantir/suppressibleerrorprone/RemoveRolloutSuppressionsTest.java b/suppressible-error-prone/src/test/java/com/palantir/suppressibleerrorprone/RemoveRolloutSuppressionsTest.java index 5d8c82a1..89266a24 100644 --- a/suppressible-error-prone/src/test/java/com/palantir/suppressibleerrorprone/RemoveRolloutSuppressionsTest.java +++ b/suppressible-error-prone/src/test/java/com/palantir/suppressibleerrorprone/RemoveRolloutSuppressionsTest.java @@ -178,19 +178,19 @@ public final class App {} } @Test - void do_not_reorder_existing_annotations() { + void sorts_by_human_authored_first_then_by_alnum() { fix().addInputLines( "App.java", // language=Java """ - @SuppressWarnings({"for-rollout:3", "for-rollout:2", "1"}) + @SuppressWarnings({"for-rollout:3", "for-rollout:2", "for-rollout:1", "b", "a"}) public final class App {} """) .addOutputLines( "App.java", // language=Java """ - @SuppressWarnings({"for-rollout:3", "1"}) + @SuppressWarnings({"a", "b", "for-rollout:1", "for-rollout:3"}) public final class App {} """) .setArgs("-XepOpt:" + RemoveRolloutSuppressions.ARGUMENT + "=2") diff --git a/versions.lock b/versions.lock index 4e45b73b..cc9f3955 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 (12 constraints: 04d77c1e) +com.google.guava:guava:33.5.0-jre (13 constraints: 99e92d4f) 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)