Skip to content

Commit 5911eb6

Browse files
msridharmsridhar
committed
initial implementation of warnings for unneeded suppressions
1 parent 28f119a commit 5911eb6

File tree

9 files changed

+471
-29
lines changed

9 files changed

+471
-29
lines changed

‎check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java

+15
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ public class ErrorProneOptions {
5858
private static final String IGNORE_SUPPRESSION_ANNOTATIONS = "-XepIgnoreSuppressionAnnotations";
5959
private static final String DISABLE_ALL_CHECKS = "-XepDisableAllChecks";
6060
private static final String DISABLE_ALL_WARNINGS = "-XepDisableAllWarnings";
61+
private static final String WARN_ON_UNNEEDED_SUPPRESSIONS = "-XepWarnOnUnneededSuppressions";
6162
private static final String IGNORE_UNKNOWN_CHECKS_FLAG = "-XepIgnoreUnknownCheckNames";
6263
private static final String DISABLE_WARNINGS_IN_GENERATED_CODE_FLAG =
6364
"-XepDisableWarningsInGeneratedCode";
@@ -93,6 +94,10 @@ public boolean isDisableAllChecks() {
9394
return disableAllChecks;
9495
}
9596

97+
public boolean isWarnOnUnneededSuppressions() {
98+
return warnOnUnneededSuppressions;
99+
}
100+
96101
/**
97102
* Severity levels for an error-prone check that define how the check results should be presented.
98103
*/
@@ -153,6 +158,7 @@ abstract static class Builder {
153158
private final boolean suggestionsAsWarnings;
154159
private final boolean enableAllChecksAsWarnings;
155160
private final boolean disableAllChecks;
161+
private final boolean warnOnUnneededSuppressions;
156162
private final boolean isTestOnlyTarget;
157163
private final boolean isPubliclyVisibleTarget;
158164
private final ErrorProneFlags flags;
@@ -171,6 +177,7 @@ private ErrorProneOptions(
171177
boolean suggestionsAsWarnings,
172178
boolean enableAllChecksAsWarnings,
173179
boolean disableAllChecks,
180+
boolean warnOnUnneededSuppressions,
174181
boolean isTestOnlyTarget,
175182
boolean isPubliclyVisibleTarget,
176183
ErrorProneFlags flags,
@@ -187,6 +194,7 @@ private ErrorProneOptions(
187194
this.suggestionsAsWarnings = suggestionsAsWarnings;
188195
this.enableAllChecksAsWarnings = enableAllChecksAsWarnings;
189196
this.disableAllChecks = disableAllChecks;
197+
this.warnOnUnneededSuppressions = warnOnUnneededSuppressions;
190198
this.isTestOnlyTarget = isTestOnlyTarget;
191199
this.isPubliclyVisibleTarget = isPubliclyVisibleTarget;
192200
this.flags = flags;
@@ -260,6 +268,7 @@ private static class Builder {
260268
private boolean suggestionsAsWarnings = false;
261269
private boolean enableAllChecksAsWarnings = false;
262270
private boolean disableAllChecks = false;
271+
private boolean warnOnUnneededSuppressions = false;
263272
private boolean isTestOnlyTarget = false;
264273
private boolean isPubliclyVisibleTarget = false;
265274
private boolean ignoreSuppressionAnnotations = false;
@@ -344,6 +353,10 @@ public void setDisableAllChecks(boolean disableAllChecks) {
344353
this.disableAllChecks = disableAllChecks;
345354
}
346355

356+
public void setWarnOnUnneededSuppressions(boolean warnOnUnneededSuppressions) {
357+
this.warnOnUnneededSuppressions = warnOnUnneededSuppressions;
358+
}
359+
347360
public void setTestOnlyTarget(boolean isTestOnlyTarget) {
348361
this.isTestOnlyTarget = isTestOnlyTarget;
349362
}
@@ -367,6 +380,7 @@ public ErrorProneOptions build(ImmutableList<String> remainingArgs) {
367380
suggestionsAsWarnings,
368381
enableAllChecksAsWarnings,
369382
disableAllChecks,
383+
warnOnUnneededSuppressions,
370384
isTestOnlyTarget,
371385
isPubliclyVisibleTarget,
372386
flagsBuilder.build(),
@@ -419,6 +433,7 @@ public static ErrorProneOptions processArgs(Iterable<String> args) {
419433
case SUGGESTIONS_AS_WARNINGS_FLAG -> builder.setSuggestionsAsWarnings(true);
420434
case ENABLE_ALL_CHECKS -> builder.setEnableAllChecksAsWarnings(true);
421435
case DISABLE_ALL_CHECKS -> builder.setDisableAllChecks(true);
436+
case WARN_ON_UNNEEDED_SUPPRESSIONS -> builder.setWarnOnUnneededSuppressions(true);
422437
case COMPILING_TEST_ONLY_CODE -> builder.setTestOnlyTarget(true);
423438
case COMPILING_PUBLICLY_VISIBLE_CODE -> builder.setPubliclyVisibleTarget(true);
424439
case DISABLE_ALL_WARNINGS -> builder.setDisableAllWarnings(true);

‎check_api/src/main/java/com/google/errorprone/SuppressionInfo.java

+157-9
Original file line numberDiff line numberDiff line change
@@ -16,25 +16,33 @@
1616

1717
package com.google.errorprone;
1818

19+
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
20+
1921
import com.google.common.collect.ImmutableSet;
2022
import com.google.errorprone.annotations.CheckReturnValue;
2123
import com.google.errorprone.annotations.Immutable;
24+
import com.google.errorprone.bugpatterns.BugChecker;
25+
import com.google.errorprone.matchers.Description;
2226
import com.google.errorprone.matchers.Suppressible;
2327
import com.google.errorprone.suppliers.Supplier;
2428
import com.google.errorprone.util.ASTHelpers;
2529
import com.sun.source.tree.ClassTree;
2630
import com.sun.source.tree.CompilationUnitTree;
31+
import com.sun.source.tree.Tree;
2732
import com.sun.source.util.SimpleTreeVisitor;
33+
import com.sun.source.util.Trees;
2834
import com.sun.tools.javac.code.Attribute;
2935
import com.sun.tools.javac.code.Symbol;
3036
import com.sun.tools.javac.code.Symbol.ClassSymbol;
3137
import com.sun.tools.javac.code.Symbol.MethodSymbol;
38+
import com.sun.tools.javac.processing.JavacProcessingEnvironment;
3239
import com.sun.tools.javac.util.Name;
3340
import com.sun.tools.javac.util.Pair;
3441
import java.util.Collections;
3542
import java.util.HashSet;
3643
import java.util.Set;
3744
import java.util.concurrent.atomic.AtomicBoolean;
45+
import org.jspecify.annotations.Nullable;
3846

3947
/**
4048
* Immutable container of "suppression signals" - annotations or other information gathered from
@@ -61,11 +69,85 @@ public class SuppressionInfo {
6169

6270
private final boolean inGeneratedCode;
6371

72+
@BugPattern(summary = "Warns when a @SuppressWarnings is not needed", severity = WARNING)
73+
private static final class UnusedSuppressionChecker extends BugChecker {}
74+
75+
private static final UnusedSuppressionChecker UNUSED_SUPPRESSION_CHECKER =
76+
new UnusedSuppressionChecker();
77+
78+
// for tracking unneeded suppressions
79+
static class SuppressionInfoForSymbol {
80+
private final @Nullable Symbol symbol;
81+
private final ImmutableSet<String> suppressWarningStringsForSymbol;
82+
private final Set<String> usedSuppressWarningStrings = new HashSet<>();
83+
private final ImmutableSet<Name> customSuppressionsForSymbol;
84+
private final @Nullable SuppressionInfoForSymbol parent;
85+
86+
public SuppressionInfoForSymbol(
87+
@Nullable Symbol symbol,
88+
ImmutableSet<String> suppressWarningStringsForSymbol,
89+
ImmutableSet<Name> customSuppressionsForSymbol,
90+
@Nullable SuppressionInfoForSymbol parent) {
91+
this.symbol = symbol;
92+
this.suppressWarningStringsForSymbol = suppressWarningStringsForSymbol;
93+
this.customSuppressionsForSymbol = customSuppressionsForSymbol;
94+
this.parent = parent;
95+
}
96+
97+
public void markSuppressionStringAsUsed(String suppressionName) {
98+
if (suppressWarningStringsForSymbol.contains(suppressionName)) {
99+
usedSuppressWarningStrings.add(suppressionName);
100+
} else if (parent != null) {
101+
parent.markSuppressionStringAsUsed(suppressionName);
102+
} else {
103+
throw new IllegalArgumentException("Suppression string not found: " + suppressionName);
104+
}
105+
}
106+
}
107+
108+
private final @Nullable SuppressionInfoForSymbol infoForClosestSymbol;
109+
110+
public void updatedUsedSuppressions(Suppressed suppressed) {
111+
String suppressionName = suppressed.getSuppressionName();
112+
if (suppressionName != null && suppressed.isUsed()) {
113+
infoForClosestSymbol.markSuppressionStringAsUsed(suppressionName);
114+
}
115+
}
116+
117+
public void warnOnUnusedSuppressions(VisitorState state) {
118+
if (infoForClosestSymbol == null) {
119+
return;
120+
}
121+
for (String warn : infoForClosestSymbol.suppressWarningStringsForSymbol) {
122+
if (!infoForClosestSymbol.usedSuppressWarningStrings.contains(warn)) {
123+
Tree tree =
124+
Trees.instance(JavacProcessingEnvironment.instance(state.context))
125+
.getTree(infoForClosestSymbol.symbol);
126+
Description description =
127+
UNUSED_SUPPRESSION_CHECKER
128+
.buildDescription(tree)
129+
.setMessage("Unnecessary @SuppressWarnings(\"" + warn + "\")")
130+
.overrideSeverity(WARNING)
131+
.build();
132+
state.reportMatch(description);
133+
}
134+
}
135+
}
136+
64137
private SuppressionInfo(
65138
Set<String> suppressWarningsStrings, Set<Name> customSuppressions, boolean inGeneratedCode) {
139+
this(suppressWarningsStrings, customSuppressions, inGeneratedCode, null);
140+
}
141+
142+
private SuppressionInfo(
143+
Set<String> suppressWarningsStrings,
144+
Set<Name> customSuppressions,
145+
boolean inGeneratedCode,
146+
@Nullable SuppressionInfoForSymbol infoForClosestSymbol) {
66147
this.suppressWarningsStrings = ImmutableSet.copyOf(suppressWarningsStrings);
67148
this.customSuppressions = ImmutableSet.copyOf(customSuppressions);
68149
this.inGeneratedCode = inGeneratedCode;
150+
this.infoForClosestSymbol = infoForClosestSymbol;
69151
}
70152

71153
private static boolean isGenerated(Symbol sym, VisitorState state) {
@@ -82,18 +164,29 @@ private static boolean isGenerated(Symbol sym, VisitorState state) {
82164
public SuppressedState suppressedState(
83165
Suppressible suppressible, boolean suppressedInGeneratedCode, VisitorState state) {
84166
if (inGeneratedCode && suppressedInGeneratedCode) {
85-
return SuppressedState.SUPPRESSED;
167+
return new Suppressed(null);
86168
}
87169
if (suppressible.supportsSuppressWarnings()
88170
&& (suppressWarningsStrings.contains("all")
89171
|| !Collections.disjoint(suppressible.allNames(), suppressWarningsStrings))) {
90-
return SuppressedState.SUPPRESSED;
172+
String name;
173+
if (suppressWarningsStrings.contains("all")) {
174+
name = "all";
175+
} else {
176+
// find the first name that suppresses this check
177+
name =
178+
suppressible.allNames().stream()
179+
.filter(suppressWarningsStrings::contains)
180+
.findAny()
181+
.get();
182+
}
183+
return new Suppressed(name);
91184
}
92185
if (suppressible.suppressedByAnyOf(customSuppressions, state)) {
93-
return SuppressedState.SUPPRESSED;
186+
return new Suppressed(null);
94187
}
95188

96-
return SuppressedState.UNSUPPRESSED;
189+
return Unsuppressed.UNSUPPRESSED;
97190
}
98191

99192
/**
@@ -128,14 +221,20 @@ public Void visitClass(ClassTree node, Void unused) {
128221
* @param sym The {@code Symbol} for the AST node currently being scanned
129222
* @param state VisitorState for checking the current tree, as well as for getting the {@code
130223
* SuppressWarnings symbol type}.
224+
* @param warnOnUnneededSuppressWarningsStrings
131225
*/
132226
public SuppressionInfo withExtendedSuppressions(
133-
Symbol sym, VisitorState state, Set<? extends Name> customSuppressionAnnosToLookFor) {
227+
Symbol sym,
228+
VisitorState state,
229+
Set<? extends Name> customSuppressionAnnosToLookFor,
230+
Set<String> warnOnUnneededSuppressWarningsStrings) {
134231
boolean newInGeneratedCode = inGeneratedCode || isGenerated(sym, state);
135232
boolean anyModification = newInGeneratedCode != inGeneratedCode;
136233

137234
/* Handle custom suppression annotations. */
138235
Set<Name> lookingFor = new HashSet<>(customSuppressionAnnosToLookFor);
236+
// TODO what if we have nested suppressions with the same customSuppression annotation?
237+
// Is the inner one unused? Let's just say no for now. We'll report on the outermost one.
139238
lookingFor.removeAll(customSuppressions);
140239
Set<Name> newlyPresent = ASTHelpers.annotationsAmong(sym, lookingFor, state);
141240
Set<Name> newCustomSuppressions;
@@ -151,6 +250,7 @@ public SuppressionInfo withExtendedSuppressions(
151250
Name suppressLint = ANDROID_SUPPRESS_LINT.get(state);
152251
Name valueName = VALUE.get(state);
153252
Set<String> newSuppressions = null;
253+
Set<String> newWarnOnUnneededSuppressions = new HashSet<>();
154254
// Iterate over annotations on this symbol, looking for SuppressWarnings
155255
for (Attribute.Compound attr : sym.getAnnotationMirrors()) {
156256
if ((attr.type.tsym == state.getSymtab().suppressWarningsType.tsym)
@@ -167,6 +267,9 @@ public SuppressionInfo withExtendedSuppressions(
167267
newSuppressions = new HashSet<>(suppressWarningsStrings);
168268
}
169269
newSuppressions.add(suppressedWarning);
270+
if (warnOnUnneededSuppressWarningsStrings.contains(suppressedWarning)) {
271+
newWarnOnUnneededSuppressions.add(suppressedWarning);
272+
}
170273
}
171274
}
172275
} else {
@@ -187,11 +290,56 @@ public SuppressionInfo withExtendedSuppressions(
187290
if (newSuppressions == null) {
188291
newSuppressions = suppressWarningsStrings;
189292
}
190-
return new SuppressionInfo(newSuppressions, newCustomSuppressions, newInGeneratedCode);
293+
294+
SuppressionInfoForSymbol newInfoForClosestSymbol =
295+
new SuppressionInfoForSymbol(
296+
sym,
297+
ImmutableSet.copyOf(newWarnOnUnneededSuppressions),
298+
ImmutableSet.copyOf(newlyPresent),
299+
infoForClosestSymbol);
300+
return new SuppressionInfo(
301+
newSuppressions, newCustomSuppressions, newInGeneratedCode, newInfoForClosestSymbol);
191302
}
192303

193-
public enum SuppressedState {
194-
UNSUPPRESSED,
195-
SUPPRESSED
304+
public sealed interface SuppressedState permits Suppressed, Unsuppressed {
305+
boolean isSuppressed();
306+
}
307+
308+
public static final class Unsuppressed implements SuppressedState {
309+
public static final Unsuppressed UNSUPPRESSED = new Unsuppressed();
310+
311+
private Unsuppressed() {}
312+
313+
@Override
314+
public boolean isSuppressed() {
315+
return false;
316+
}
317+
}
318+
319+
public static final class Suppressed implements SuppressedState {
320+
private final @Nullable String suppressionName;
321+
322+
private boolean used = false;
323+
324+
public Suppressed(@Nullable String suppressionName) {
325+
this.suppressionName = suppressionName;
326+
}
327+
328+
public @Nullable String getSuppressionName() {
329+
return suppressionName;
330+
}
331+
332+
public boolean isUsed() {
333+
return used;
334+
}
335+
336+
public void setAsUsed() {
337+
this.used = true;
338+
}
339+
340+
@Override
341+
public boolean isSuppressed() {
342+
return true;
343+
}
196344
}
197345
}

0 commit comments

Comments
 (0)