Skip to content

Commit ce784a9

Browse files
cushonError Prone Team
cushon
authored and
Error Prone Team
committed
Detect non-private, non-override methods in anonymous classes
#5156 PiperOrigin-RevId: 783488629
1 parent 43759cd commit ce784a9

File tree

7 files changed

+467
-159
lines changed

7 files changed

+467
-159
lines changed

‎core/src/main/java/com/google/errorprone/bugpatterns/EffectivelyPrivate.java

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
/*
2+
* Copyright 2025 The Error Prone Authors.
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.google.errorprone.bugpatterns;
18+
19+
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
20+
import static com.google.errorprone.fixes.SuggestedFixes.removeModifiers;
21+
import static com.google.errorprone.matchers.Description.NO_MATCH;
22+
import static com.google.errorprone.util.ASTHelpers.getSymbol;
23+
import static com.google.errorprone.util.ASTHelpers.hasAnnotation;
24+
import static com.google.errorprone.util.ASTHelpers.isEffectivelyPrivate;
25+
import static com.google.errorprone.util.ASTHelpers.streamSuperMethods;
26+
27+
import com.google.common.collect.ImmutableSet;
28+
import com.google.errorprone.BugPattern;
29+
import com.google.errorprone.VisitorState;
30+
import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;
31+
import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
32+
import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher;
33+
import com.google.errorprone.fixes.SuggestedFix;
34+
import com.google.errorprone.matchers.Description;
35+
import com.sun.source.tree.ClassTree;
36+
import com.sun.source.tree.MethodTree;
37+
import com.sun.source.tree.ModifiersTree;
38+
import com.sun.source.tree.Tree;
39+
import com.sun.source.tree.VariableTree;
40+
import com.sun.tools.javac.code.Symbol;
41+
import com.sun.tools.javac.code.Symbol.MethodSymbol;
42+
import com.sun.tools.javac.code.Symbol.VarSymbol;
43+
import java.util.Collections;
44+
import java.util.Optional;
45+
import javax.inject.Inject;
46+
import javax.lang.model.element.Modifier;
47+
48+
/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
49+
@BugPattern(
50+
summary = "This declaration has public or protected modifiers, but is effectively private.",
51+
severity = WARNING)
52+
public final class EffectivelyPrivate extends BugChecker
53+
implements MethodTreeMatcher, VariableTreeMatcher, ClassTreeMatcher {
54+
55+
private final WellKnownKeep wellKnownKeep;
56+
57+
@Inject
58+
EffectivelyPrivate(WellKnownKeep wellKnownKeep) {
59+
this.wellKnownKeep = wellKnownKeep;
60+
}
61+
62+
@Override
63+
public Description matchVariable(VariableTree tree, VisitorState state) {
64+
VarSymbol sym = getSymbol(tree);
65+
if (!sym.getKind().isField()) {
66+
return NO_MATCH;
67+
}
68+
return match(tree, tree.getModifiers(), state);
69+
}
70+
71+
@Override
72+
public Description matchClass(ClassTree tree, VisitorState state) {
73+
return match(tree, tree.getModifiers(), state);
74+
}
75+
76+
@Override
77+
public Description matchMethod(MethodTree tree, VisitorState state) {
78+
return match(tree, tree.getModifiers(), state);
79+
}
80+
81+
private Description match(Tree tree, ModifiersTree modifiers, VisitorState state) {
82+
Symbol sym = getSymbol(tree);
83+
if (!isEffectivelyPrivate(sym)) {
84+
return NO_MATCH;
85+
}
86+
if (wellKnownKeep.shouldKeep(tree)) {
87+
return NO_MATCH;
88+
}
89+
if (sym instanceof MethodSymbol methodSymbol) {
90+
if (hasAnnotation(methodSymbol, "java.lang.Override", state)) {
91+
return NO_MATCH;
92+
}
93+
// TODO: cushon - technically this should only match final classes, otherwise it could break
94+
// a subclass that relies on inheriting a method of a particular visibility to fulfil and
95+
// interface contract. Skip that for now, since many classes don't rely on that and also
96+
// aren't explicitly final.
97+
if (streamSuperMethods(methodSymbol, state.getTypes()).findAny().isPresent()) {
98+
return NO_MATCH;
99+
}
100+
}
101+
if (Collections.disjoint(modifiers.getFlags(), MODIFIER_TO_REMOVE)) {
102+
return NO_MATCH;
103+
}
104+
Optional<SuggestedFix> fix = removeModifiers(modifiers, state, MODIFIER_TO_REMOVE);
105+
if (fix.isEmpty()) {
106+
// The fix may be empty for implicit modifiers, e.g. on enum constant fields
107+
return NO_MATCH;
108+
}
109+
return describeMatch(tree, fix.get());
110+
}
111+
112+
private static final ImmutableSet<Modifier> MODIFIER_TO_REMOVE =
113+
ImmutableSet.of(Modifier.PUBLIC, Modifier.PROTECTED);
114+
}

‎core/src/main/java/com/google/errorprone/bugpatterns/UnusedMethod.java

Lines changed: 4 additions & 106 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
import static com.google.common.collect.Iterables.getLast;
2323
import static com.google.common.collect.Iterables.size;
2424
import static com.google.common.collect.Multimaps.asMap;
25-
import static com.google.common.collect.Sets.union;
2625
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
2726
import static com.google.errorprone.fixes.SuggestedFix.emptyFix;
2827
import static com.google.errorprone.fixes.SuggestedFixes.replaceIncludingComments;
@@ -34,7 +33,6 @@
3433
import static com.google.errorprone.util.ASTHelpers.hasAnnotation;
3534
import static com.google.errorprone.util.ASTHelpers.isGeneratedConstructor;
3635
import static com.google.errorprone.util.ASTHelpers.isSubtype;
37-
import static com.google.errorprone.util.ASTHelpers.shouldKeep;
3836
import static com.google.errorprone.util.MoreAnnotations.asStrings;
3937
import static com.google.errorprone.util.MoreAnnotations.getAnnotationValue;
4038
import static java.lang.String.format;
@@ -46,7 +44,6 @@
4644
import com.google.common.collect.ImmutableListMultimap;
4745
import com.google.common.collect.ImmutableSet;
4846
import com.google.errorprone.BugPattern;
49-
import com.google.errorprone.ErrorProneFlags;
5047
import com.google.errorprone.VisitorState;
5148
import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher;
5249
import com.google.errorprone.fixes.SuggestedFix;
@@ -69,7 +66,6 @@
6966
import com.sun.tools.javac.code.Symbol;
7067
import com.sun.tools.javac.code.Symbol.ClassSymbol;
7168
import com.sun.tools.javac.code.Symbol.MethodSymbol;
72-
import com.sun.tools.javac.code.Symbol.TypeSymbol;
7369
import com.sun.tools.javac.code.Type;
7470
import com.sun.tools.javac.tree.JCTree.JCAnnotation;
7571
import com.sun.tools.javac.tree.JCTree.JCAssign;
@@ -96,77 +92,6 @@ public final class UnusedMethod extends BugChecker implements CompilationUnitTre
9692
private static final String JUNIT_PARAMS_VALUE = "value";
9793
private static final String JUNIT_PARAMS_ANNOTATION_TYPE = "junitparams.Parameters";
9894

99-
/**
100-
* Annotations that exempt methods from being considered unused.
101-
*
102-
* <p>Try to avoid adding more annotations here. Annotating these annotations with {@code @Keep}
103-
* has the same effect; this list is chiefly for third-party annotations which cannot be
104-
* annotated.
105-
*/
106-
private static final ImmutableSet<String> EXEMPTING_METHOD_ANNOTATIONS =
107-
ImmutableSet.of(
108-
"android.webkit.JavascriptInterface",
109-
"com.fasterxml.jackson.annotation.JsonCreator",
110-
"com.fasterxml.jackson.annotation.JsonProperty",
111-
"com.fasterxml.jackson.annotation.JsonSetter",
112-
"com.fasterxml.jackson.annotation.JsonValue",
113-
"com.google.acai.AfterTest",
114-
"com.google.acai.BeforeSuite",
115-
"com.google.acai.BeforeTest",
116-
"com.google.caliper.Benchmark",
117-
"com.google.common.eventbus.Subscribe",
118-
"com.google.inject.Provides",
119-
"com.google.inject.Inject",
120-
"com.google.inject.multibindings.ProvidesIntoMap",
121-
"com.google.inject.multibindings.ProvidesIntoSet",
122-
"com.google.inject.throwingproviders.CheckedProvides",
123-
"com.tngtech.java.junit.dataprovider.DataProvider",
124-
"jakarta.annotation.PreDestroy",
125-
"jakarta.annotation.PostConstruct",
126-
"jakarta.inject.Inject",
127-
"jakarta.persistence.PostLoad",
128-
"jakarta.persistence.PostPersist",
129-
"jakarta.persistence.PostRemove",
130-
"jakarta.persistence.PostUpdate",
131-
"jakarta.persistence.PrePersist",
132-
"jakarta.persistence.PreRemove",
133-
"jakarta.persistence.PreUpdate",
134-
"jakarta.validation.constraints.AssertFalse",
135-
"jakarta.validation.constraints.AssertTrue",
136-
"javax.annotation.PreDestroy",
137-
"javax.annotation.PostConstruct",
138-
"javax.inject.Inject",
139-
"javax.persistence.PostLoad",
140-
"javax.persistence.PostPersist",
141-
"javax.persistence.PostRemove",
142-
"javax.persistence.PostUpdate",
143-
"javax.persistence.PrePersist",
144-
"javax.persistence.PreRemove",
145-
"javax.persistence.PreUpdate",
146-
"javax.validation.constraints.AssertFalse",
147-
"javax.validation.constraints.AssertTrue",
148-
"net.bytebuddy.asm.Advice.OnMethodEnter",
149-
"net.bytebuddy.asm.Advice.OnMethodExit",
150-
"org.apache.beam.sdk.transforms.DoFn.FinishBundle",
151-
"org.apache.beam.sdk.transforms.DoFn.ProcessElement",
152-
"org.apache.beam.sdk.transforms.DoFn.StartBundle",
153-
"org.aspectj.lang.annotation.Pointcut",
154-
"org.aspectj.lang.annotation.After",
155-
"org.aspectj.lang.annotation.Before",
156-
"org.springframework.context.annotation.Bean",
157-
"org.testng.annotations.AfterClass",
158-
"org.testng.annotations.AfterMethod",
159-
"org.testng.annotations.BeforeClass",
160-
"org.testng.annotations.BeforeMethod",
161-
"org.testng.annotations.DataProvider",
162-
"org.junit.jupiter.api.BeforeAll",
163-
"org.junit.jupiter.api.AfterAll",
164-
"org.junit.jupiter.api.AfterEach",
165-
"org.junit.jupiter.api.BeforeEach",
166-
"org.junit.jupiter.api.RepeatedTest",
167-
"org.junit.jupiter.api.Test",
168-
"org.junit.jupiter.params.ParameterizedTest");
169-
17095
/**
17196
* Class annotations which exempt methods within the annotated class from findings.
17297
*
@@ -185,15 +110,11 @@ public final class UnusedMethod extends BugChecker implements CompilationUnitTre
185110
*/
186111
private static final ImmutableSet<String> EXEMPTING_SUPER_TYPES = ImmutableSet.of();
187112

188-
private final ImmutableSet<String> exemptingMethodAnnotations;
113+
private final WellKnownKeep wellKnownKeep;
189114

190115
@Inject
191-
UnusedMethod(ErrorProneFlags errorProneFlags) {
192-
this.exemptingMethodAnnotations =
193-
union(
194-
errorProneFlags.getSetOrEmpty("UnusedMethod:ExemptingMethodAnnotations"),
195-
EXEMPTING_METHOD_ANNOTATIONS)
196-
.immutableCopy();
116+
UnusedMethod(WellKnownKeep wellKnownKeep) {
117+
this.wellKnownKeep = wellKnownKeep;
197118
}
198119

199120
@Override
@@ -281,11 +202,7 @@ private boolean isMethodSymbolEligibleForChecking(
281202
if (exemptedByName(tree.getName())) {
282203
return false;
283204
}
284-
// Assume the method is called if annotated with a called-reflectively annotation.
285-
if (exemptedByAnnotation(tree.getModifiers().getAnnotations())) {
286-
return false;
287-
}
288-
if (shouldKeep(tree)) {
205+
if (wellKnownKeep.shouldKeep(tree)) {
289206
return false;
290207
}
291208
MethodSymbol methodSymbol = getSymbol(tree);
@@ -506,25 +423,6 @@ public Void visitMethod(MethodTree tree, Void unused) {
506423
return hasAnyNativeMethods.get();
507424
}
508425

509-
/**
510-
* Looks at the list of {@code annotations} and see if there is any annotation which exists {@code
511-
* exemptingAnnotations}.
512-
*/
513-
private boolean exemptedByAnnotation(List<? extends AnnotationTree> annotations) {
514-
for (AnnotationTree annotation : annotations) {
515-
Type annotationType = getType(annotation);
516-
if (annotationType == null) {
517-
continue;
518-
}
519-
TypeSymbol tsym = annotationType.tsym;
520-
String annotationName = tsym.getQualifiedName().toString();
521-
if (exemptingMethodAnnotations.contains(annotationName)) {
522-
return true;
523-
}
524-
}
525-
return false;
526-
}
527-
528426
private static boolean exemptedByName(Name name) {
529427
return Ascii.toLowerCase(name.toString()).startsWith(EXEMPT_PREFIX);
530428
}

‎core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java

Lines changed: 6 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@
3434
import static com.google.errorprone.util.ASTHelpers.isAbstract;
3535
import static com.google.errorprone.util.ASTHelpers.isStatic;
3636
import static com.google.errorprone.util.ASTHelpers.isSubtype;
37-
import static com.google.errorprone.util.ASTHelpers.shouldKeep;
3837
import static com.google.errorprone.util.SideEffectAnalysis.hasSideEffect;
3938
import static com.sun.source.tree.Tree.Kind.POSTFIX_DECREMENT;
4039
import static com.sun.source.tree.Tree.Kind.POSTFIX_INCREMENT;
@@ -65,7 +64,6 @@
6564
import com.google.errorprone.suppliers.Supplier;
6665
import com.google.errorprone.suppliers.Suppliers;
6766
import com.google.errorprone.util.ASTHelpers;
68-
import com.sun.source.tree.AnnotationTree;
6967
import com.sun.source.tree.ArrayAccessTree;
7068
import com.sun.source.tree.AssignmentTree;
7169
import com.sun.source.tree.BindingPatternTree;
@@ -100,7 +98,6 @@
10098
import com.sun.tools.javac.code.Symbol;
10199
import com.sun.tools.javac.code.Symbol.ClassSymbol;
102100
import com.sun.tools.javac.code.Symbol.MethodSymbol;
103-
import com.sun.tools.javac.code.Symbol.TypeSymbol;
104101
import com.sun.tools.javac.code.Symbol.VarSymbol;
105102
import com.sun.tools.javac.code.Type;
106103
import com.sun.tools.javac.tree.JCTree;
@@ -135,35 +132,6 @@ public final class UnusedVariable extends BugChecker implements CompilationUnitT
135132

136133
private final ImmutableSet<String> exemptNames;
137134

138-
/**
139-
* The set of annotation full names which exempt annotated element from being reported as unused.
140-
*
141-
* <p>Try to avoid adding more annotations here. Annotating these annotations with {@code @Keep}
142-
* has the same effect; this list is chiefly for third-party annotations which cannot be
143-
* annotated.
144-
*/
145-
private static final ImmutableSet<String> EXEMPTING_VARIABLE_ANNOTATIONS =
146-
ImmutableSet.of(
147-
"jakarta.persistence.Basic",
148-
"jakarta.persistence.Column",
149-
"jakarta.persistence.Id",
150-
"jakarta.persistence.Version",
151-
"jakarta.xml.bind.annotation.XmlElement",
152-
"javax.persistence.Basic",
153-
"javax.persistence.Column",
154-
"javax.persistence.Id",
155-
"javax.persistence.Version",
156-
"javax.xml.bind.annotation.XmlElement",
157-
"net.starlark.java.annot.StarlarkBuiltin",
158-
"org.junit.Rule",
159-
"org.junit.jupiter.api.extension.RegisterExtension",
160-
"org.openqa.selenium.support.FindAll",
161-
"org.openqa.selenium.support.FindBy",
162-
"org.openqa.selenium.support.FindBys",
163-
"org.apache.beam.sdk.transforms.DoFn.TimerId",
164-
"org.apache.beam.sdk.transforms.DoFn.StateId",
165-
"org.springframework.boot.test.mock.mockito.MockBean");
166-
167135
// TODO(ghm): Find a sensible place to dedupe this with UnnecessarilyVisible.
168136
private static final ImmutableSet<String> ANNOTATIONS_INDICATING_PARAMETERS_SHOULD_BE_CHECKED =
169137
ImmutableSet.of(
@@ -200,8 +168,10 @@ public final class UnusedVariable extends BugChecker implements CompilationUnitT
200168

201169
private final boolean reportInjectedFields;
202170

171+
private final WellKnownKeep wellKnownKeep;
172+
203173
@Inject
204-
UnusedVariable(ErrorProneFlags flags) {
174+
UnusedVariable(ErrorProneFlags flags, WellKnownKeep wellKnownKeep) {
205175
this.methodAnnotationsExemptingParameters =
206176
ImmutableSet.<String>builder()
207177
.add("org.robolectric.annotation.Implementation")
@@ -221,6 +191,8 @@ public final class UnusedVariable extends BugChecker implements CompilationUnitT
221191
.add("unused")
222192
.addAll(flags.getSetOrEmpty("Unused:exemptPrefixes"))
223193
.build();
194+
195+
this.wellKnownKeep = wellKnownKeep;
224196
}
225197

226198
@Override
@@ -606,24 +578,6 @@ private static boolean isEnhancedForLoopVar(TreePath variablePath) {
606578
&& enhancedForLoopTree.getVariable() == tree;
607579
}
608580

609-
/**
610-
* Looks at the list of {@code annotations} and see if there is any annotation which exists {@code
611-
* exemptingAnnotations}.
612-
*/
613-
private static boolean exemptedByAnnotation(List<? extends AnnotationTree> annotations) {
614-
for (AnnotationTree annotation : annotations) {
615-
Type annotationType = ASTHelpers.getType(annotation);
616-
if (annotationType == null) {
617-
continue;
618-
}
619-
TypeSymbol tsym = annotationType.tsym;
620-
if (EXEMPTING_VARIABLE_ANNOTATIONS.contains(tsym.getQualifiedName().toString())) {
621-
return true;
622-
}
623-
}
624-
return false;
625-
}
626-
627581
private boolean exemptedByName(Name name) {
628582
String nameString = name.toString();
629583
String nameStringLower = Ascii.toLowerCase(nameString);
@@ -680,8 +634,7 @@ && exemptedFieldBySuperType(getType(variableTree), state)) {
680634
return;
681635
}
682636
// Return if the element is exempted by an annotation.
683-
if (exemptedByAnnotation(variableTree.getModifiers().getAnnotations())
684-
|| shouldKeep(variableTree)) {
637+
if (wellKnownKeep.shouldKeep(variableTree)) {
685638
return;
686639
}
687640
switch (symbol.getKind()) {

0 commit comments

Comments
 (0)