From ccd3ca657b2bf41224eccb1633d94046d09ede82 Mon Sep 17 00:00:00 2001 From: Error Prone Team Date: Thu, 1 Aug 2024 16:57:40 -0700 Subject: [PATCH] Add handling of toBuilder() toBuilder() was identified as a getter leading to not correctly identifying the case where all the getters are prefixed. PiperOrigin-RevId: 658585727 --- .../bugpatterns/AutoValueBoxedValues.java | 68 +++++++++++-------- .../bugpatterns/AutoValueBoxedValuesTest.java | 4 +- 2 files changed, 44 insertions(+), 28 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AutoValueBoxedValues.java b/core/src/main/java/com/google/errorprone/bugpatterns/AutoValueBoxedValues.java index 6b57f5f6989..7a8c240d0f4 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/AutoValueBoxedValues.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/AutoValueBoxedValues.java @@ -21,6 +21,9 @@ import static com.google.errorprone.matchers.Description.NO_MATCH; import static com.google.errorprone.matchers.Matchers.hasModifier; import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static com.google.errorprone.util.ASTHelpers.getType; +import static com.google.errorprone.util.ASTHelpers.hasAnnotation; +import static com.google.errorprone.util.ASTHelpers.isSameType; import static java.beans.Introspector.decapitalize; import com.google.auto.value.AutoValue; @@ -34,7 +37,6 @@ import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; -import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ClassTree; import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.MethodTree; @@ -60,31 +62,24 @@ public class AutoValueBoxedValues extends BugChecker implements ClassTreeMatcher @Override public Description matchClass(ClassTree tree, VisitorState state) { - if (!ASTHelpers.hasAnnotation(tree, AutoValue.class.getName(), state)) { + if (!hasAnnotation(tree, AutoValue.class.getName(), state)) { return NO_MATCH; } + Optional builderClass = findBuilderClass(tree, state); + // Identify and potentially fix the getters. - ImmutableList getters = handleGetterMethods(tree, state); + ImmutableList getters = handleGetterMethods(tree, state, builderClass); // If we haven't modified any getter, it's ok to stop. if (getters.stream().allMatch(getter -> getter.fix().isEmpty())) { return NO_MATCH; } - // Handle the Builder class, if there is one. - boolean builderFound = false; - for (Tree memberTree : tree.getMembers()) { - if (memberTree instanceof ClassTree - && ASTHelpers.hasAnnotation(memberTree, AutoValue.Builder.class.getName(), state)) { - handleSetterMethods((ClassTree) memberTree, state, getters); - builderFound = true; - break; - } - } - - // If a builder was not found, handle the factory methods. - if (!builderFound) { + // Handle the Builder class, if there is one. Otherwise handle the factory methods. + if (builderClass.isPresent()) { + handleSetterMethods(builderClass.get(), state, getters); + } else { handleFactoryMethods(tree, state, getters); } @@ -103,13 +98,16 @@ public Description matchClass(ClassTree tree, VisitorState state) { * @param state The visitor state. * @return The list of {@link Getter} in the class. */ - private ImmutableList handleGetterMethods(ClassTree classTree, VisitorState state) { + private ImmutableList handleGetterMethods( + ClassTree classTree, VisitorState state, Optional builderClass) { return classTree.getMembers().stream() .filter(MethodTree.class::isInstance) .map(memberTree -> (MethodTree) memberTree) .filter( methodTree -> - ABSTRACT_MATCHER.matches(methodTree, state) && methodTree.getParameters().isEmpty()) + ABSTRACT_MATCHER.matches(methodTree, state) + && methodTree.getParameters().isEmpty() + && !isToBuilderMethod(methodTree, state, builderClass)) .map(methodTree -> maybeFixGetter(methodTree, state)) .collect(toImmutableList()); } @@ -120,7 +118,7 @@ private ImmutableList handleGetterMethods(ClassTree classTree, VisitorSt */ private Getter maybeFixGetter(MethodTree method, VisitorState state) { Getter getter = Getter.of(method); - Type type = ASTHelpers.getType(method.getReturnType()); + Type type = getType(method.getReturnType()); if (!isSuppressed(method, state) && !hasNullableAnnotation(method) && isBoxedPrimitive(state, type)) { @@ -143,7 +141,8 @@ private void handleSetterMethods(ClassTree classTree, VisitorState state, List ABSTRACT_MATCHER.matches(methodTree, state) - && methodTree.getParameters().size() == 1) + && methodTree.getParameters().size() == 1 + && isSameType(getType(methodTree.getReturnType()), getType(classTree), state)) .forEach(methodTree -> maybeFixSetter(methodTree, state, getters)); } @@ -162,7 +161,7 @@ && matchGetterAndSetter(getter.method(), methodTree, allGettersPrefixed)) .findAny(); if (fixedGetter.isPresent()) { var parameter = methodTree.getParameters().get(0); - Type type = ASTHelpers.getType(parameter); + Type type = getType(parameter); if (isBoxedPrimitive(state, type) && !hasNullableAnnotation(parameter)) { suggestRemoveUnnecessaryBoxing(parameter.getType(), state, type, fixedGetter.get().fix()); } @@ -188,10 +187,8 @@ private void handleFactoryMethods(ClassTree classTree, VisitorState state, List< .filter( methodTree -> STATIC_MATCHER.matches(methodTree, state) - && ASTHelpers.isSameType( - ASTHelpers.getType(methodTree.getReturnType()), - ASTHelpers.getType(classTree), - state) + && isSameType( + getType(methodTree.getReturnType()), getType(classTree), state) && isTrivialFactoryMethod(methodTree, getters.size())) .findAny(); if (trivialFactoryMethod.isEmpty()) { @@ -201,7 +198,7 @@ && isTrivialFactoryMethod(methodTree, getters.size())) Getter getter = getters.get(idx); if (!getter.fix().isEmpty()) { var parameter = trivialFactoryMethod.get().getParameters().get(idx); - Type type = ASTHelpers.getType(parameter); + Type type = getType(parameter); if (isBoxedPrimitive(state, type) && !hasNullableAnnotation(parameter)) { suggestRemoveUnnecessaryBoxing(parameter.getType(), state, type, getter.fix()); } @@ -228,6 +225,23 @@ private static boolean isBoxedPrimitive(VisitorState state, Type type) { return unboxed != null && unboxed.getTag() != TypeTag.NONE && unboxed.getTag() != TypeTag.VOID; } + private static Optional findBuilderClass(ClassTree tree, VisitorState state) { + return tree.getMembers().stream() + .filter( + memberTree -> + memberTree instanceof ClassTree + && hasAnnotation(memberTree, AutoValue.Builder.class.getName(), state)) + .map(memberTree -> (ClassTree) memberTree) + .findAny(); + } + + private static boolean isToBuilderMethod( + MethodTree methodTree, VisitorState state, Optional builderClass) { + return builderClass.isPresent() + && !STATIC_MATCHER.matches(methodTree, state) + && isSameType(getType(methodTree.getReturnType()), getType(builderClass.get()), state); + } + private static boolean allGettersPrefixed(List getters) { return getters.stream().allMatch(getter -> !getterPrefix(getter.method()).isEmpty()); } @@ -238,7 +252,7 @@ private static String getterPrefix(MethodTree getterMethod) { return "get"; } else if (name.startsWith("is") && !name.equals("is") - && ASTHelpers.getType(getterMethod.getReturnType()).getKind() == TypeKind.BOOLEAN) { + && getType(getterMethod.getReturnType()).getKind() == TypeKind.BOOLEAN) { return "is"; } return ""; diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/AutoValueBoxedValuesTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/AutoValueBoxedValuesTest.java index 392692d391f..93a0b1fa96b 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/AutoValueBoxedValuesTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/AutoValueBoxedValuesTest.java @@ -536,7 +536,7 @@ public void settersWithoutSetPrefix() { } @Test - public void allGettersWithPrefix() { + public void allGettersWithPrefix_ignoreToBuilder() { if (!withBuilder) { return; } @@ -548,6 +548,7 @@ public void allGettersWithPrefix() { "abstract class Test {", " public abstract Long getLongId();", " public abstract boolean isBooleanId();", + " public abstract Builder toBuilder();", " @AutoValue.Builder", " abstract static class Builder {", " abstract Builder setLongId(Long value);", @@ -562,6 +563,7 @@ public void allGettersWithPrefix() { "abstract class Test {", " public abstract long getLongId();", " public abstract boolean isBooleanId();", + " public abstract Builder toBuilder();", " @AutoValue.Builder", " abstract static class Builder {", " abstract Builder setLongId(long value);",