Skip to content

Commit fe07236

Browse files
java-team-github-botError Prone Team
java-team-github-bot
authored and
Error Prone Team
committed
Add Error Prone check for unnecessary boxed types in AutoValue classes.
This check will warn users when they are using boxed types in their AutoValue classes that are not Nullable. This is because boxed types are not necessary in AutoValue classes, and they can actually be harmful because they can cause unnecessary boxing and unboxing. Clean up reference: unknown commit This check is currently enabled as a warning, but it can be made into an error in the future. PiperOrigin-RevId: 658493053
1 parent 4ec7faa commit fe07236

File tree

4 files changed

+928
-0
lines changed

4 files changed

+928
-0
lines changed

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

+325
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,325 @@
1+
/*
2+
* Copyright 2024 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.common.collect.ImmutableList.toImmutableList;
20+
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
21+
import static com.google.errorprone.matchers.Description.NO_MATCH;
22+
import static com.google.errorprone.matchers.Matchers.hasModifier;
23+
import static com.google.errorprone.util.ASTHelpers.getSymbol;
24+
import static java.beans.Introspector.decapitalize;
25+
26+
import com.google.auto.value.AutoValue;
27+
import com.google.common.base.Ascii;
28+
import com.google.common.collect.ImmutableList;
29+
import com.google.errorprone.BugPattern;
30+
import com.google.errorprone.VisitorState;
31+
import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;
32+
import com.google.errorprone.dataflow.nullnesspropagation.Nullness;
33+
import com.google.errorprone.dataflow.nullnesspropagation.NullnessAnnotations;
34+
import com.google.errorprone.fixes.SuggestedFix;
35+
import com.google.errorprone.matchers.Description;
36+
import com.google.errorprone.matchers.Matcher;
37+
import com.google.errorprone.util.ASTHelpers;
38+
import com.sun.source.tree.ClassTree;
39+
import com.sun.source.tree.IdentifierTree;
40+
import com.sun.source.tree.MethodTree;
41+
import com.sun.source.tree.NewClassTree;
42+
import com.sun.source.tree.ReturnTree;
43+
import com.sun.source.tree.Tree;
44+
import com.sun.tools.javac.code.Type;
45+
import com.sun.tools.javac.code.TypeTag;
46+
import java.util.List;
47+
import java.util.Optional;
48+
import javax.lang.model.element.Modifier;
49+
import javax.lang.model.type.TypeKind;
50+
51+
/** See summary for details. */
52+
@BugPattern(
53+
summary =
54+
"AutoValue instances should not usually contain boxed types that are not Nullable. We"
55+
+ " recommend removing the unnecessary boxing.",
56+
severity = WARNING)
57+
public class AutoValueBoxedValues extends BugChecker implements ClassTreeMatcher {
58+
private static final Matcher<MethodTree> ABSTRACT_MATCHER = hasModifier(Modifier.ABSTRACT);
59+
private static final Matcher<MethodTree> STATIC_MATCHER = hasModifier(Modifier.STATIC);
60+
61+
@Override
62+
public Description matchClass(ClassTree tree, VisitorState state) {
63+
if (!ASTHelpers.hasAnnotation(tree, AutoValue.class.getName(), state)) {
64+
return NO_MATCH;
65+
}
66+
67+
// Identify and potentially fix the getters.
68+
ImmutableList<Getter> getters = handleGetterMethods(tree, state);
69+
70+
// If we haven't modified any getter, it's ok to stop.
71+
if (getters.stream().allMatch(getter -> getter.fix().isEmpty())) {
72+
return NO_MATCH;
73+
}
74+
75+
// Handle the Builder class, if there is one.
76+
boolean builderFound = false;
77+
for (Tree memberTree : tree.getMembers()) {
78+
if (memberTree instanceof ClassTree
79+
&& ASTHelpers.hasAnnotation(memberTree, AutoValue.Builder.class.getName(), state)) {
80+
handleSetterMethods((ClassTree) memberTree, state, getters);
81+
builderFound = true;
82+
break;
83+
}
84+
}
85+
86+
// If a builder was not found, handle the factory methods.
87+
if (!builderFound) {
88+
handleFactoryMethods(tree, state, getters);
89+
}
90+
91+
getters.stream()
92+
.filter(getter -> !getter.fix().isEmpty())
93+
.forEach(getter -> state.reportMatch(describeMatch(getter.method(), getter.fix().build())));
94+
95+
return NO_MATCH;
96+
}
97+
98+
/**
99+
* Returns the {@link List} of {@link Getter} in the {@link AutoValue} class along with the fixes
100+
* to be applied.
101+
*
102+
* @param classTree The {@link AutoValue} class tree.
103+
* @param state The visitor state.
104+
* @return The list of {@link Getter} in the class.
105+
*/
106+
private ImmutableList<Getter> handleGetterMethods(ClassTree classTree, VisitorState state) {
107+
return classTree.getMembers().stream()
108+
.filter(MethodTree.class::isInstance)
109+
.map(memberTree -> (MethodTree) memberTree)
110+
.filter(
111+
methodTree ->
112+
ABSTRACT_MATCHER.matches(methodTree, state) && methodTree.getParameters().isEmpty())
113+
.map(methodTree -> maybeFixGetter(methodTree, state))
114+
.collect(toImmutableList());
115+
}
116+
117+
/**
118+
* Converts the {@link MethodTree} of a getter to a {@link Getter}. If the getter needs to be
119+
* fixed, it returns a {@link Getter} with a non-empty {@link SuggestedFix}.
120+
*/
121+
private Getter maybeFixGetter(MethodTree method, VisitorState state) {
122+
Getter getter = Getter.of(method);
123+
Type type = ASTHelpers.getType(method.getReturnType());
124+
if (!isSuppressed(method, state)
125+
&& !hasNullableAnnotation(method)
126+
&& isBoxedPrimitive(state, type)) {
127+
suggestRemoveUnnecessaryBoxing(method.getReturnType(), state, type, getter.fix());
128+
}
129+
return getter;
130+
}
131+
132+
/**
133+
* Identifies and fixes the setters in the {@link AutoValue.Builder} class.
134+
*
135+
* @param classTree The {@link AutoValue.Builder} class tree.
136+
* @param state The visitor state.
137+
* @param getters The {@link List} of {@link Getter} in the {@link AutoValue} class.
138+
*/
139+
private void handleSetterMethods(ClassTree classTree, VisitorState state, List<Getter> getters) {
140+
classTree.getMembers().stream()
141+
.filter(MethodTree.class::isInstance)
142+
.map(memberTree -> (MethodTree) memberTree)
143+
.filter(
144+
methodTree ->
145+
ABSTRACT_MATCHER.matches(methodTree, state)
146+
&& methodTree.getParameters().size() == 1)
147+
.forEach(methodTree -> maybeFixSetter(methodTree, state, getters));
148+
}
149+
150+
/** Given a setter, it tries to apply a fix if the corresponding getter was also fixed. */
151+
private void maybeFixSetter(MethodTree methodTree, VisitorState state, List<Getter> getters) {
152+
if (isSuppressed(methodTree, state)) {
153+
return;
154+
}
155+
boolean allGettersPrefixed = allGettersPrefixed(getters);
156+
Optional<Getter> fixedGetter =
157+
getters.stream()
158+
.filter(
159+
getter ->
160+
!getter.fix().isEmpty()
161+
&& matchGetterAndSetter(getter.method(), methodTree, allGettersPrefixed))
162+
.findAny();
163+
if (fixedGetter.isPresent()) {
164+
var parameter = methodTree.getParameters().get(0);
165+
Type type = ASTHelpers.getType(parameter);
166+
if (isBoxedPrimitive(state, type) && !hasNullableAnnotation(parameter)) {
167+
suggestRemoveUnnecessaryBoxing(parameter.getType(), state, type, fixedGetter.get().fix());
168+
}
169+
}
170+
}
171+
172+
/**
173+
* Identifies and fixes the factory method in the {@link AutoValue} class.
174+
*
175+
* <p>This method only handles the case of "trivial" factory methods, i.e. methods that have one
176+
* argument for each getter in the class and contains a single return statement passing all the
177+
* arguments to the constructor in the same order.
178+
*
179+
* @param classTree The {@link AutoValue} class tree.
180+
* @param state The visitor state.
181+
* @param getters The {@link List} of {@link Getter} in the {@link AutoValue} class.
182+
*/
183+
private void handleFactoryMethods(ClassTree classTree, VisitorState state, List<Getter> getters) {
184+
Optional<MethodTree> trivialFactoryMethod =
185+
classTree.getMembers().stream()
186+
.filter(MethodTree.class::isInstance)
187+
.map(memberTree -> (MethodTree) memberTree)
188+
.filter(
189+
methodTree ->
190+
STATIC_MATCHER.matches(methodTree, state)
191+
&& ASTHelpers.isSameType(
192+
ASTHelpers.getType(methodTree.getReturnType()),
193+
ASTHelpers.getType(classTree),
194+
state)
195+
&& isTrivialFactoryMethod(methodTree, getters.size()))
196+
.findAny();
197+
if (trivialFactoryMethod.isEmpty()) {
198+
return;
199+
}
200+
for (int idx = 0; idx < getters.size(); idx++) {
201+
Getter getter = getters.get(idx);
202+
if (!getter.fix().isEmpty()) {
203+
var parameter = trivialFactoryMethod.get().getParameters().get(idx);
204+
Type type = ASTHelpers.getType(parameter);
205+
if (isBoxedPrimitive(state, type) && !hasNullableAnnotation(parameter)) {
206+
suggestRemoveUnnecessaryBoxing(parameter.getType(), state, type, getter.fix());
207+
}
208+
}
209+
}
210+
}
211+
212+
/** Returns true if the given tree has a {@code Nullable} annotation. */
213+
private static boolean hasNullableAnnotation(Tree tree) {
214+
return NullnessAnnotations.fromAnnotationsOn(getSymbol(tree)).orElse(null) == Nullness.NULLABLE;
215+
}
216+
217+
/** Returns the primitive type corresponding to a boxed type. */
218+
private static Type unbox(VisitorState state, Type type) {
219+
return state.getTypes().unboxedType(type);
220+
}
221+
222+
/** Returns true if the value of {@link Type} is a boxed primitive. */
223+
private static boolean isBoxedPrimitive(VisitorState state, Type type) {
224+
if (type.isPrimitive()) {
225+
return false;
226+
}
227+
Type unboxed = unbox(state, type);
228+
return unboxed != null && unboxed.getTag() != TypeTag.NONE && unboxed.getTag() != TypeTag.VOID;
229+
}
230+
231+
private static boolean allGettersPrefixed(List<Getter> getters) {
232+
return getters.stream().allMatch(getter -> !getterPrefix(getter.method()).isEmpty());
233+
}
234+
235+
private static String getterPrefix(MethodTree getterMethod) {
236+
String name = getterMethod.getName().toString();
237+
if (name.startsWith("get") && !name.equals("get")) {
238+
return "get";
239+
} else if (name.startsWith("is")
240+
&& !name.equals("is")
241+
&& ASTHelpers.getType(getterMethod.getReturnType()).getKind() == TypeKind.BOOLEAN) {
242+
return "is";
243+
}
244+
return "";
245+
}
246+
247+
/** Returns true if the getter and the setter are for the same field. */
248+
private static boolean matchGetterAndSetter(
249+
MethodTree getter, MethodTree setter, boolean allGettersPrefixed) {
250+
String getterName = getter.getName().toString();
251+
if (allGettersPrefixed) {
252+
String prefix = getterPrefix(getter);
253+
getterName = decapitalize(getterName.substring(prefix.length()));
254+
}
255+
String setterName = setter.getName().toString();
256+
return getterName.equals(setterName)
257+
|| setterName.equals(
258+
"set" + Ascii.toUpperCase(getterName.charAt(0)) + getterName.substring(1));
259+
}
260+
261+
/**
262+
* Returns true if the method is a trivial factory method.
263+
*
264+
* <p>A trivial factory method is a static method that has one argument for each getter in the
265+
* class and contains a single return statement passing all the arguments to the constructor in
266+
* the same order.
267+
*
268+
* @param methodTree The method tree to be checked.
269+
* @param gettersCount The total number of getters in the class.
270+
* @return True if the method is a trivial factory method, false otherwise.
271+
*/
272+
private static boolean isTrivialFactoryMethod(MethodTree methodTree, int gettersCount) {
273+
var params = methodTree.getParameters();
274+
var statements = methodTree.getBody().getStatements();
275+
276+
// Trivial factory method must have one argument for each getter and a single return statement.
277+
if (params.size() != gettersCount
278+
|| statements.size() != 1
279+
|| !(statements.get(0) instanceof ReturnTree)) {
280+
return false;
281+
}
282+
// Trivial factory method must return a new instance.
283+
ReturnTree returnTree = (ReturnTree) statements.get(0);
284+
if (!(returnTree.getExpression() instanceof NewClassTree)) {
285+
return false;
286+
}
287+
// Trivial factory method must pass all the arguments to the constructor.
288+
NewClassTree newClassTree = (NewClassTree) returnTree.getExpression();
289+
if (newClassTree.getArguments().stream().anyMatch(r -> !(r instanceof IdentifierTree))) {
290+
return false;
291+
}
292+
// Compare the arguments passed to the method to those passed to the constructor.
293+
var paramsNames = params.stream().map(p -> p.getName().toString()).collect(toImmutableList());
294+
var constructorArgs =
295+
newClassTree.getArguments().stream()
296+
.map(r -> ((IdentifierTree) r).getName().toString())
297+
.collect(toImmutableList());
298+
return paramsNames.equals(constructorArgs);
299+
}
300+
301+
/**
302+
* Suggests a fix to replace the boxed type with the unboxed type.
303+
*
304+
* <p>For example, if the type is `Integer`, the fix would be to replace `Integer` with `int`.
305+
*
306+
* @param tree The tree to be replaced, which can be either a return value or a parameter.
307+
* @param state The visitor state.
308+
* @param type The boxed type to be replaced.
309+
*/
310+
private static void suggestRemoveUnnecessaryBoxing(
311+
Tree tree, VisitorState state, Type type, SuggestedFix.Builder fix) {
312+
fix.replace(tree, unbox(state, type).tsym.getSimpleName().toString());
313+
}
314+
315+
@AutoValue
316+
abstract static class Getter {
317+
abstract MethodTree method();
318+
319+
abstract SuggestedFix.Builder fix();
320+
321+
static Getter of(MethodTree method) {
322+
return new AutoValue_AutoValueBoxedValues_Getter(method, SuggestedFix.builder());
323+
}
324+
}
325+
}

‎core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java

+2
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
import com.google.errorprone.bugpatterns.AsyncCallableReturnsNull;
4343
import com.google.errorprone.bugpatterns.AsyncFunctionReturnsNull;
4444
import com.google.errorprone.bugpatterns.AttemptedNegativeZero;
45+
import com.google.errorprone.bugpatterns.AutoValueBoxedValues;
4546
import com.google.errorprone.bugpatterns.AutoValueBuilderDefaultsInConstructor;
4647
import com.google.errorprone.bugpatterns.AutoValueFinalMethods;
4748
import com.google.errorprone.bugpatterns.AutoValueImmutableFields;
@@ -1128,6 +1129,7 @@ public static ScannerSupplier warningChecks() {
11281129
AssertFalse.class,
11291130
AssistedInjectAndInjectOnConstructors.class,
11301131
AutoFactoryAtInject.class,
1132+
AutoValueBoxedValues.class,
11311133
AvoidObjectArrays.class,
11321134
BanClassLoader.class,
11331135
BanSerializableRead.class,

0 commit comments

Comments
 (0)