Skip to content

Commit b997df0

Browse files
kelvinou01kelvinou01
authored
Add the ability to ignore suppressions in SuppressibleTreePathScanner (#200)
Add the ability to ignore suppressions in `SuppressibleTreePathScanner`
1 parent 2e5360f commit b997df0

File tree

8 files changed

+215
-44
lines changed

8 files changed

+215
-44
lines changed

‎gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePlugin.java‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,11 @@
1919
import com.palantir.gradle.suppressibleerrorprone.modes.Modes;
2020
import com.palantir.gradle.suppressibleerrorprone.modes.common.CommonOptions;
2121
import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption;
22+
import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.ModifiedFile;
2223
import com.palantir.gradle.suppressibleerrorprone.transform.ModifyErrorProneCheckApi;
2324
import java.util.List;
2425
import java.util.Optional;
26+
import java.util.Set;
2527
import java.util.stream.Collectors;
2628
import javax.inject.Inject;
2729
import net.ltgt.gradle.errorprone.ErrorProneOptions;
@@ -75,7 +77,7 @@ private void applyToJavaProject(Project project) {
7577
if (getModes().modifyCheckApi() instanceof ModifyCheckApiOption.MustModify mustModify) {
7678
// When auto-suppressing, the logic will run a bytecode patched version of errorprone
7779
// (via an artifact transform) that intercepts every error from every check and adds a custom fix
78-
setupErrorProneArtifactTransform(project, mustModify.modifyVisitorState());
80+
setupErrorProneArtifactTransform(project, mustModify.modifiedFiles());
7981
}
8082

8183
project.getConfigurations().named(ErrorPronePlugin.CONFIGURATION_NAME).configure(errorProneConfiguration -> {
@@ -108,7 +110,7 @@ private void applyToJavaProject(Project project) {
108110
});
109111
}
110112

111-
private static void setupErrorProneArtifactTransform(Project project, boolean modifyVisitorState) {
113+
private static void setupErrorProneArtifactTransform(Project project, Set<ModifiedFile> modifiedFiles) {
112114
Attribute<Boolean> suppressible =
113115
Attribute.of("com.palantir.suppressible-error-prone.suppressible", Boolean.class);
114116
project.getDependencies().getAttributesSchema().attribute(suppressible);
@@ -120,7 +122,7 @@ private static void setupErrorProneArtifactTransform(Project project, boolean mo
120122
.attribute(suppressible, false);
121123

122124
project.getDependencies().registerTransform(ModifyErrorProneCheckApi.class, spec -> {
123-
spec.getParameters().getModifyVisitorState().set(modifyVisitorState);
125+
spec.getParameters().getFilesToModify().set(modifiedFiles);
124126

125127
Attribute<String> artifactType = Attribute.of("artifactType", String.class);
126128
spec.getFrom().attribute(suppressible, false).attribute(artifactType, "jar");
@@ -205,6 +207,10 @@ private void setupErrorProneOptions(CommonOptions commonOptions, ErrorProneOptio
205207

206208
errorProneOptions.getErrorproneArgumentProviders().add(patchChecksCommandLineArgumentProvider);
207209

210+
errorProneOptions
211+
.getIgnoreSuppressionAnnotations()
212+
.set(getProviderFactory().provider(commonOptions::ignoreSuppressionAnnotations));
213+
208214
errorProneOptions
209215
.getCheckOptions()
210216
.putAll(getProviderFactory().provider(commonOptions::extraErrorProneCheckOptions));

‎gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/CommonOptions.java‎

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,10 @@ default RemoveRolloutCheck removeRolloutCheck() {
4141
return RemoveRolloutCheck.DISABLE;
4242
}
4343

44+
default boolean ignoreSuppressionAnnotations() {
45+
return false;
46+
}
47+
4448
default CommonOptions naivelyCombinedWith(CommonOptions other) {
4549
return new CommonOptions() {
4650
@Override
@@ -59,6 +63,11 @@ public Map<String, String> extraErrorProneCheckOptions() {
5963
public RemoveRolloutCheck removeRolloutCheck() {
6064
return CommonOptions.this.removeRolloutCheck().or(other.removeRolloutCheck());
6165
}
66+
67+
@Override
68+
public boolean ignoreSuppressionAnnotations() {
69+
return CommonOptions.this.ignoreSuppressionAnnotations() || other.ignoreSuppressionAnnotations();
70+
}
6271
};
6372
}
6473

@@ -68,16 +77,26 @@ static CommonOptions naivelyCombine(Collection<CommonOptions> commonOptions) {
6877

6978
default CommonOptions withExtraErrorProneCheckFlag(String key, Supplier<String> value) {
7079
return new CommonOptions() {
80+
@Override
81+
public Map<String, String> extraErrorProneCheckOptions() {
82+
Map<String, String> map = new HashMap<>(CommonOptions.this.extraErrorProneCheckOptions());
83+
map.put(key, value.get());
84+
return Collections.unmodifiableMap(map);
85+
}
86+
7187
@Override
7288
public PatchChecksOption patchChecks() {
7389
return CommonOptions.this.patchChecks();
7490
}
7591

7692
@Override
77-
public Map<String, String> extraErrorProneCheckOptions() {
78-
Map<String, String> map = new HashMap<>(CommonOptions.this.extraErrorProneCheckOptions());
79-
map.put(key, value.get());
80-
return Collections.unmodifiableMap(map);
93+
public RemoveRolloutCheck removeRolloutCheck() {
94+
return CommonOptions.this.removeRolloutCheck();
95+
}
96+
97+
@Override
98+
public boolean ignoreSuppressionAnnotations() {
99+
return CommonOptions.this.ignoreSuppressionAnnotations();
81100
}
82101
};
83102
}

‎gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/common/ModifyCheckApiOption.java‎

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -47,19 +47,16 @@ static ModifyCheckApiOption noEffect() {
4747
}
4848

4949
/**
50-
* The {@code error_prone_check_api} jar must be modified to allow {@code `for-rollout:`} suppressions to work.
50+
* Modify these files in the error-prone API.
5151
*/
52-
static ModifyCheckApiOption mustModify() {
53-
return new MustModify(false);
52+
static MustModify mustModify(ModifiedFile... modifiedFile) {
53+
return new MustModify(Set.of(modifiedFile));
5454
}
5555

56-
/**
57-
* The {@code error_prone_check_api} jar must be modified to allow {@code `for-rollout:`} suppressions to work,
58-
* and {@code VisitorState} must be modified to intercept reportMatch.
59-
*/
60-
static ModifyCheckApiOption mustModifyIncludingVisitorState() {
61-
return new MustModify(true);
56+
default Set<ModifiedFile> getModifiedFiles() {
57+
return Set.of();
6258
}
59+
;
6360

6461
enum DoNotModify implements ModifyCheckApiOption, CombinedValue {
6562
INSTANCE
@@ -69,9 +66,11 @@ enum NoEffect implements ModifyCheckApiOption {
6966
INSTANCE
7067
}
7168

72-
record MustModify(boolean modifyVisitorState) implements ModifyCheckApiOption, CombinedValue {
69+
record MustModify(Set<ModifiedFile> modifiedFiles) implements ModifyCheckApiOption, CombinedValue {
7370
public MustModify combine(MustModify other) {
74-
return new MustModify(modifyVisitorState || other.modifyVisitorState);
71+
Set<ModifiedFile> union =
72+
StreamEx.of(modifiedFiles).append(other.modifiedFiles).toSet();
73+
return new MustModify(union);
7574
}
7675
}
7776

@@ -84,7 +83,7 @@ static CombinedValue combine(Collection<ModifyCheckApiOption> options) {
8483

8584
if (withoutNoEffects.isEmpty()) {
8685
// By default, we need to modify the check API to support for-rollout suppressions
87-
return new MustModify(false);
86+
return mustModify(ModifiedFile.BUG_CHECKER_INFO);
8887
}
8988

9089
boolean doNotModify = withoutNoEffects.contains(DoNotModify.INSTANCE);
@@ -103,4 +102,16 @@ static CombinedValue combine(Collection<ModifyCheckApiOption> options) {
103102
.reduce(MustModify::combine)
104103
.get();
105104
}
105+
106+
enum ModifiedFile {
107+
BUG_CHECKER_INFO("BugCheckerInfo"),
108+
VISITOR_STATE("VisitorState"),
109+
SUPPRESSIBLE_TREE_PATH_SCANNER("SuppressibleTreePathScanner");
110+
111+
private final String className;
112+
113+
ModifiedFile(String className) {
114+
this.className = className;
115+
}
116+
}
106117
}

‎gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/modes/modes/SuppressMode.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,13 @@
1919
import com.palantir.gradle.suppressibleerrorprone.modes.common.CommonOptions;
2020
import com.palantir.gradle.suppressibleerrorprone.modes.common.Mode;
2121
import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption;
22+
import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.ModifiedFile;
2223
import com.palantir.gradle.suppressibleerrorprone.modes.common.PatchChecksOption;
2324

2425
public final class SuppressMode implements Mode {
2526
@Override
2627
public ModifyCheckApiOption modifyCheckApi() {
27-
return ModifyCheckApiOption.mustModifyIncludingVisitorState();
28+
return ModifyCheckApiOption.mustModify(ModifiedFile.BUG_CHECKER_INFO, ModifiedFile.VISITOR_STATE);
2829
}
2930

3031
@Override

‎gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/transform/ModifyErrorProneCheckApi.java‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
package com.palantir.gradle.suppressibleerrorprone.transform;
1818

19+
import com.palantir.gradle.suppressibleerrorprone.modes.common.ModifyCheckApiOption.ModifiedFile;
1920
import com.palantir.gradle.suppressibleerrorprone.transform.ModifyErrorProneCheckApi.Params;
2021
import com.palantir.gradle.utils.environmentvariables.EnvironmentVariables;
2122
import java.io.BufferedOutputStream;
@@ -25,6 +26,7 @@
2526
import java.io.InputStream;
2627
import java.io.UncheckedIOException;
2728
import java.util.Optional;
29+
import java.util.Set;
2830
import java.util.UUID;
2931
import java.util.function.UnaryOperator;
3032
import java.util.jar.JarFile;
@@ -36,6 +38,7 @@
3638
import org.gradle.api.file.FileSystemLocation;
3739
import org.gradle.api.provider.Property;
3840
import org.gradle.api.provider.Provider;
41+
import org.gradle.api.provider.SetProperty;
3942
import org.gradle.api.tasks.Input;
4043
import org.gradle.api.tasks.Nested;
4144
import org.objectweb.asm.ClassReader;
@@ -67,7 +70,7 @@ private void suppressCheckApi(File output) {
6770
visitJar(output, (classJarPath, inputStream) -> classVisitorFor(classJarPath)
6871
.map(classVisitorFactory -> {
6972
ClassReader classReader = newClassReader(inputStream);
70-
ClassWriter classWriter = new ClassWriter(classReader, 0);
73+
ClassWriter classWriter = new ClassWriter(classReader, ClassWriter.COMPUTE_FRAMES);
7174
ClassVisitor classVisitor = classVisitorFactory.apply(classWriter);
7275

7376
classReader.accept(classVisitor, 0);
@@ -79,18 +82,26 @@ private Optional<UnaryOperator<ClassVisitor>> classVisitorFor(String classJarPat
7982
// We always want to modify the BugCheckerInfo class, as this is where we help errorprone understand
8083
// what the `for-rollout:` prefix for suppressions mean (which means this class is always modified,
8184
// even during normal compilation).
82-
if (classJarPath.equals("com/google/errorprone/BugCheckerInfo.class")) {
85+
86+
Set<ModifiedFile> filesToModify = getParameters().getFilesToModify().get();
87+
if (classJarPath.equals("com/google/errorprone/BugCheckerInfo.class")
88+
&& filesToModify.contains(ModifiedFile.BUG_CHECKER_INFO)) {
8389
return Optional.of(BugCheckerInfoVisitor::new);
8490
}
8591

8692
// We only want to modify the VisitorState class if we're in stage 1 of the suppression process, as
8793
// it intercepts all errors and changes them. So when we're just running normal compilation, we want
8894
// to avoid running our modifications at all, and let the errors continue on their way unchanged.
8995
if (classJarPath.equals("com/google/errorprone/VisitorState.class")
90-
&& getParameters().getModifyVisitorState().get()) {
96+
&& filesToModify.contains(ModifiedFile.VISITOR_STATE)) {
9197
return Optional.of(VisitorStateClassVisitor::new);
9298
}
9399

100+
if (classJarPath.equals("com/google/errorprone/bugpatterns/BugChecker$SuppressibleTreePathScanner.class")
101+
&& filesToModify.contains(ModifiedFile.SUPPRESSIBLE_TREE_PATH_SCANNER)) {
102+
return Optional.of(SuppressibleTreePathScannerClassVisitor::new);
103+
}
104+
94105
return Optional.empty();
95106
}
96107

@@ -132,7 +143,7 @@ private void visitJar(File output, ClassTransformer classTransformer) {
132143

133144
public abstract static class Params implements TransformParameters {
134145
@Input
135-
public abstract Property<Boolean> getModifyVisitorState();
146+
public abstract SetProperty<ModifiedFile> getFilesToModify();
136147

137148
@Input
138149
public abstract Property<String> getCacheBust();

‎gradle-suppressible-error-prone/src/main/java/com/palantir/gradle/suppressibleerrorprone/transform/SuppressibleTreePathScannerClassVisitor.java‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
/*
2+
* (c) Copyright 2025 Palantir Technologies Inc. All rights reserved.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.palantir.gradle.suppressibleerrorprone.transform;
18+
19+
import org.objectweb.asm.ClassVisitor;
20+
import org.objectweb.asm.Label;
21+
import org.objectweb.asm.MethodVisitor;
22+
import org.objectweb.asm.Opcodes;
23+
24+
/**
25+
* Forces {@code SuppressibleTreePathScanner} to respect the ignore suppressions option.
26+
*
27+
* <p>{@code RemoveUnusedSuppressions} must see violations that are normally hidden by suppressions to
28+
* determine which suppressions are actually needed. While ErrorProneOptions has an
29+
* {@code ignoreSuppressionAnnotations} flag for this purpose, many BugCheckers use
30+
* {@code SuppressibleTreePathScanner}, which doesn't respect this flag.
31+
*
32+
* <p>This class uses bytecode manipulation to patch {@code SuppressibleTreePathScanner::isSuppressed}
33+
* to respect the ErrorProneOptions setting.
34+
*/
35+
final class SuppressibleTreePathScannerClassVisitor extends ClassVisitor {
36+
SuppressibleTreePathScannerClassVisitor(ClassVisitor classVisitor) {
37+
super(Opcodes.ASM9, classVisitor);
38+
}
39+
40+
@Override
41+
public MethodVisitor visitMethod(
42+
int access, String name, String descriptor, String signature, String[] exceptions) {
43+
MethodVisitor methodVisitor = super.visitMethod(access, name, descriptor, signature, exceptions);
44+
45+
if (name.equals("suppressed") && descriptor.equals("(Lcom/sun/source/tree/Tree;)Z")) {
46+
return new SuppressedMethodVisitor(methodVisitor);
47+
}
48+
49+
return methodVisitor;
50+
}
51+
52+
private static final class SuppressedMethodVisitor extends MethodVisitor {
53+
SuppressedMethodVisitor(MethodVisitor methodVisitor) {
54+
super(Opcodes.ASM9, methodVisitor);
55+
}
56+
57+
@Override
58+
public void visitCode() {
59+
super.visitCode();
60+
// Load this (SuppressibleTreePathScanner instance)
61+
mv.visitVarInsn(Opcodes.ALOAD, 0);
62+
// Get the state field
63+
mv.visitFieldInsn(
64+
Opcodes.GETFIELD,
65+
"com/google/errorprone/bugpatterns/BugChecker$SuppressibleTreePathScanner",
66+
"state",
67+
"Lcom/google/errorprone/VisitorState;");
68+
// Check condition and potentially return false early
69+
mv.visitMethodInsn(
70+
Opcodes.INVOKESTATIC,
71+
"com/palantir/suppressibleerrorprone/SuppressibleTreePathScannerModifications",
72+
"shouldIgnoreSuppressions",
73+
"(Lcom/google/errorprone/VisitorState;)Z",
74+
false);
75+
76+
// If condition is true, return false immediately
77+
Label continueLabel = new Label();
78+
mv.visitJumpInsn(Opcodes.IFEQ, continueLabel);
79+
mv.visitInsn(Opcodes.ICONST_0); // push false
80+
mv.visitInsn(Opcodes.IRETURN);
81+
mv.visitLabel(continueLabel);
82+
// Add a frame here to fix verification
83+
mv.visitFrame(Opcodes.F_SAME, 0, null, 0, null);
84+
}
85+
}
86+
}

0 commit comments

Comments
 (0)