From fe072361840715ae907b28dee66f2c732c255551 Mon Sep 17 00:00:00 2001 From: Error Prone Team Date: Thu, 1 Aug 2024 12:14:01 -0700 Subject: [PATCH] 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 --- .../bugpatterns/AutoValueBoxedValues.java | 325 ++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../bugpatterns/AutoValueBoxedValuesTest.java | 590 ++++++++++++++++++ docs/bugpattern/AutoValueBoxedValues.md | 11 + 4 files changed, 928 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/AutoValueBoxedValues.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/AutoValueBoxedValuesTest.java create mode 100644 docs/bugpattern/AutoValueBoxedValues.md diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AutoValueBoxedValues.java b/core/src/main/java/com/google/errorprone/bugpatterns/AutoValueBoxedValues.java new file mode 100644 index 00000000000..6b57f5f6989 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/AutoValueBoxedValues.java @@ -0,0 +1,325 @@ +/* + * Copyright 2024 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns; + +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +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 java.beans.Introspector.decapitalize; + +import com.google.auto.value.AutoValue; +import com.google.common.base.Ascii; +import com.google.common.collect.ImmutableList; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; +import com.google.errorprone.dataflow.nullnesspropagation.Nullness; +import com.google.errorprone.dataflow.nullnesspropagation.NullnessAnnotations; +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; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.ReturnTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.code.TypeTag; +import java.util.List; +import java.util.Optional; +import javax.lang.model.element.Modifier; +import javax.lang.model.type.TypeKind; + +/** See summary for details. */ +@BugPattern( + summary = + "AutoValue instances should not usually contain boxed types that are not Nullable. We" + + " recommend removing the unnecessary boxing.", + severity = WARNING) +public class AutoValueBoxedValues extends BugChecker implements ClassTreeMatcher { + private static final Matcher ABSTRACT_MATCHER = hasModifier(Modifier.ABSTRACT); + private static final Matcher STATIC_MATCHER = hasModifier(Modifier.STATIC); + + @Override + public Description matchClass(ClassTree tree, VisitorState state) { + if (!ASTHelpers.hasAnnotation(tree, AutoValue.class.getName(), state)) { + return NO_MATCH; + } + + // Identify and potentially fix the getters. + ImmutableList getters = handleGetterMethods(tree, state); + + // 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) { + handleFactoryMethods(tree, state, getters); + } + + getters.stream() + .filter(getter -> !getter.fix().isEmpty()) + .forEach(getter -> state.reportMatch(describeMatch(getter.method(), getter.fix().build()))); + + return NO_MATCH; + } + + /** + * Returns the {@link List} of {@link Getter} in the {@link AutoValue} class along with the fixes + * to be applied. + * + * @param classTree The {@link AutoValue} class tree. + * @param state The visitor state. + * @return The list of {@link Getter} in the class. + */ + private ImmutableList handleGetterMethods(ClassTree classTree, VisitorState state) { + return classTree.getMembers().stream() + .filter(MethodTree.class::isInstance) + .map(memberTree -> (MethodTree) memberTree) + .filter( + methodTree -> + ABSTRACT_MATCHER.matches(methodTree, state) && methodTree.getParameters().isEmpty()) + .map(methodTree -> maybeFixGetter(methodTree, state)) + .collect(toImmutableList()); + } + + /** + * Converts the {@link MethodTree} of a getter to a {@link Getter}. If the getter needs to be + * fixed, it returns a {@link Getter} with a non-empty {@link SuggestedFix}. + */ + private Getter maybeFixGetter(MethodTree method, VisitorState state) { + Getter getter = Getter.of(method); + Type type = ASTHelpers.getType(method.getReturnType()); + if (!isSuppressed(method, state) + && !hasNullableAnnotation(method) + && isBoxedPrimitive(state, type)) { + suggestRemoveUnnecessaryBoxing(method.getReturnType(), state, type, getter.fix()); + } + return getter; + } + + /** + * Identifies and fixes the setters in the {@link AutoValue.Builder} class. + * + * @param classTree The {@link AutoValue.Builder} class tree. + * @param state The visitor state. + * @param getters The {@link List} of {@link Getter} in the {@link AutoValue} class. + */ + private void handleSetterMethods(ClassTree classTree, VisitorState state, List getters) { + classTree.getMembers().stream() + .filter(MethodTree.class::isInstance) + .map(memberTree -> (MethodTree) memberTree) + .filter( + methodTree -> + ABSTRACT_MATCHER.matches(methodTree, state) + && methodTree.getParameters().size() == 1) + .forEach(methodTree -> maybeFixSetter(methodTree, state, getters)); + } + + /** Given a setter, it tries to apply a fix if the corresponding getter was also fixed. */ + private void maybeFixSetter(MethodTree methodTree, VisitorState state, List getters) { + if (isSuppressed(methodTree, state)) { + return; + } + boolean allGettersPrefixed = allGettersPrefixed(getters); + Optional fixedGetter = + getters.stream() + .filter( + getter -> + !getter.fix().isEmpty() + && matchGetterAndSetter(getter.method(), methodTree, allGettersPrefixed)) + .findAny(); + if (fixedGetter.isPresent()) { + var parameter = methodTree.getParameters().get(0); + Type type = ASTHelpers.getType(parameter); + if (isBoxedPrimitive(state, type) && !hasNullableAnnotation(parameter)) { + suggestRemoveUnnecessaryBoxing(parameter.getType(), state, type, fixedGetter.get().fix()); + } + } + } + + /** + * Identifies and fixes the factory method in the {@link AutoValue} class. + * + *

This method only handles the case of "trivial" factory methods, i.e. methods that have one + * argument for each getter in the class and contains a single return statement passing all the + * arguments to the constructor in the same order. + * + * @param classTree The {@link AutoValue} class tree. + * @param state The visitor state. + * @param getters The {@link List} of {@link Getter} in the {@link AutoValue} class. + */ + private void handleFactoryMethods(ClassTree classTree, VisitorState state, List getters) { + Optional trivialFactoryMethod = + classTree.getMembers().stream() + .filter(MethodTree.class::isInstance) + .map(memberTree -> (MethodTree) memberTree) + .filter( + methodTree -> + STATIC_MATCHER.matches(methodTree, state) + && ASTHelpers.isSameType( + ASTHelpers.getType(methodTree.getReturnType()), + ASTHelpers.getType(classTree), + state) + && isTrivialFactoryMethod(methodTree, getters.size())) + .findAny(); + if (trivialFactoryMethod.isEmpty()) { + return; + } + for (int idx = 0; idx < getters.size(); idx++) { + Getter getter = getters.get(idx); + if (!getter.fix().isEmpty()) { + var parameter = trivialFactoryMethod.get().getParameters().get(idx); + Type type = ASTHelpers.getType(parameter); + if (isBoxedPrimitive(state, type) && !hasNullableAnnotation(parameter)) { + suggestRemoveUnnecessaryBoxing(parameter.getType(), state, type, getter.fix()); + } + } + } + } + + /** Returns true if the given tree has a {@code Nullable} annotation. */ + private static boolean hasNullableAnnotation(Tree tree) { + return NullnessAnnotations.fromAnnotationsOn(getSymbol(tree)).orElse(null) == Nullness.NULLABLE; + } + + /** Returns the primitive type corresponding to a boxed type. */ + private static Type unbox(VisitorState state, Type type) { + return state.getTypes().unboxedType(type); + } + + /** Returns true if the value of {@link Type} is a boxed primitive. */ + private static boolean isBoxedPrimitive(VisitorState state, Type type) { + if (type.isPrimitive()) { + return false; + } + Type unboxed = unbox(state, type); + return unboxed != null && unboxed.getTag() != TypeTag.NONE && unboxed.getTag() != TypeTag.VOID; + } + + private static boolean allGettersPrefixed(List getters) { + return getters.stream().allMatch(getter -> !getterPrefix(getter.method()).isEmpty()); + } + + private static String getterPrefix(MethodTree getterMethod) { + String name = getterMethod.getName().toString(); + if (name.startsWith("get") && !name.equals("get")) { + return "get"; + } else if (name.startsWith("is") + && !name.equals("is") + && ASTHelpers.getType(getterMethod.getReturnType()).getKind() == TypeKind.BOOLEAN) { + return "is"; + } + return ""; + } + + /** Returns true if the getter and the setter are for the same field. */ + private static boolean matchGetterAndSetter( + MethodTree getter, MethodTree setter, boolean allGettersPrefixed) { + String getterName = getter.getName().toString(); + if (allGettersPrefixed) { + String prefix = getterPrefix(getter); + getterName = decapitalize(getterName.substring(prefix.length())); + } + String setterName = setter.getName().toString(); + return getterName.equals(setterName) + || setterName.equals( + "set" + Ascii.toUpperCase(getterName.charAt(0)) + getterName.substring(1)); + } + + /** + * Returns true if the method is a trivial factory method. + * + *

A trivial factory method is a static method that has one argument for each getter in the + * class and contains a single return statement passing all the arguments to the constructor in + * the same order. + * + * @param methodTree The method tree to be checked. + * @param gettersCount The total number of getters in the class. + * @return True if the method is a trivial factory method, false otherwise. + */ + private static boolean isTrivialFactoryMethod(MethodTree methodTree, int gettersCount) { + var params = methodTree.getParameters(); + var statements = methodTree.getBody().getStatements(); + + // Trivial factory method must have one argument for each getter and a single return statement. + if (params.size() != gettersCount + || statements.size() != 1 + || !(statements.get(0) instanceof ReturnTree)) { + return false; + } + // Trivial factory method must return a new instance. + ReturnTree returnTree = (ReturnTree) statements.get(0); + if (!(returnTree.getExpression() instanceof NewClassTree)) { + return false; + } + // Trivial factory method must pass all the arguments to the constructor. + NewClassTree newClassTree = (NewClassTree) returnTree.getExpression(); + if (newClassTree.getArguments().stream().anyMatch(r -> !(r instanceof IdentifierTree))) { + return false; + } + // Compare the arguments passed to the method to those passed to the constructor. + var paramsNames = params.stream().map(p -> p.getName().toString()).collect(toImmutableList()); + var constructorArgs = + newClassTree.getArguments().stream() + .map(r -> ((IdentifierTree) r).getName().toString()) + .collect(toImmutableList()); + return paramsNames.equals(constructorArgs); + } + + /** + * Suggests a fix to replace the boxed type with the unboxed type. + * + *

For example, if the type is `Integer`, the fix would be to replace `Integer` with `int`. + * + * @param tree The tree to be replaced, which can be either a return value or a parameter. + * @param state The visitor state. + * @param type The boxed type to be replaced. + */ + private static void suggestRemoveUnnecessaryBoxing( + Tree tree, VisitorState state, Type type, SuggestedFix.Builder fix) { + fix.replace(tree, unbox(state, type).tsym.getSimpleName().toString()); + } + + @AutoValue + abstract static class Getter { + abstract MethodTree method(); + + abstract SuggestedFix.Builder fix(); + + static Getter of(MethodTree method) { + return new AutoValue_AutoValueBoxedValues_Getter(method, SuggestedFix.builder()); + } + } +} diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index e02825e352a..e6b874b486b 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -42,6 +42,7 @@ import com.google.errorprone.bugpatterns.AsyncCallableReturnsNull; import com.google.errorprone.bugpatterns.AsyncFunctionReturnsNull; import com.google.errorprone.bugpatterns.AttemptedNegativeZero; +import com.google.errorprone.bugpatterns.AutoValueBoxedValues; import com.google.errorprone.bugpatterns.AutoValueBuilderDefaultsInConstructor; import com.google.errorprone.bugpatterns.AutoValueFinalMethods; import com.google.errorprone.bugpatterns.AutoValueImmutableFields; @@ -1128,6 +1129,7 @@ public static ScannerSupplier warningChecks() { AssertFalse.class, AssistedInjectAndInjectOnConstructors.class, AutoFactoryAtInject.class, + AutoValueBoxedValues.class, AvoidObjectArrays.class, BanClassLoader.class, BanSerializableRead.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/AutoValueBoxedValuesTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/AutoValueBoxedValuesTest.java new file mode 100644 index 00000000000..392692d391f --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/AutoValueBoxedValuesTest.java @@ -0,0 +1,590 @@ +/* + * Copyright 2024 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns; + +import static java.util.Arrays.stream; + +import com.google.auto.value.processor.AutoValueProcessor; +import com.google.common.collect.ImmutableList; +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import com.google.testing.junit.testparameterinjector.TestParameter; +import com.google.testing.junit.testparameterinjector.TestParameterInjector; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import org.junit.Test; +import org.junit.runner.RunWith; + +/** Unit tests for {@link AutoValueBoxedValues}. */ +@RunWith(TestParameterInjector.class) +public class AutoValueBoxedValuesTest { + private final CompilationTestHelper compilationHelper = + CompilationTestHelper.newInstance(AutoValueBoxedValues.class, getClass()) + .setArgs(ImmutableList.of("-processor", AutoValueProcessor.class.getName())); + ; + private final BugCheckerRefactoringTestHelper refactoringHelper = + BugCheckerRefactoringTestHelper.newInstance(AutoValueBoxedValues.class, getClass()) + .setArgs(ImmutableList.of("-processor", AutoValueProcessor.class.getName())); + + @TestParameter private boolean withBuilder; + + @Test + public void unnecessaryBoxedTypes_refactoring() { + refactoringHelper + .addInputLines( + "in/Test.java", + mergeLines( + lines( + "import com.google.auto.value.AutoValue;", + "@AutoValue", + "abstract class Test {", + " public abstract Long longId();", + " public abstract Integer intId();", + " public abstract Byte byteId();", + " public abstract Short shortId();", + " public abstract Float floatId();", + " public abstract Double doubleId();", + " public abstract Boolean booleanId();", + " public abstract Character charId();"), + linesWithoutBuilder( + " static Test create(", + " Long longId, Integer intId, Byte byteId, Short shortId,", + " Float floatId, Double doubleId, Boolean booleanId, Character charId) {", + " return new AutoValue_Test(longId, intId, byteId, shortId, floatId,", + " doubleId, booleanId, charId);", + " }"), + linesWithBuilder( + " @AutoValue.Builder", + " abstract static class Builder {", + " abstract Builder setLongId(Long value);", + " abstract Builder setIntId(Integer value);", + " abstract Builder setByteId(Byte value);", + " abstract Builder setShortId(Short value);", + " abstract Builder setFloatId(Float value);", + " abstract Builder setDoubleId(Double value);", + " abstract Builder setBooleanId(Boolean value);", + " abstract Builder setCharId(Character value);", + " abstract Test build();", + " }"), + lines("}"))) + .addOutputLines( + "out/Test.java", + mergeLines( + lines( + "import com.google.auto.value.AutoValue;", + "@AutoValue", + "abstract class Test {", + " public abstract long longId();", + " public abstract int intId();", + " public abstract byte byteId();", + " public abstract short shortId();", + " public abstract float floatId();", + " public abstract double doubleId();", + " public abstract boolean booleanId();", + " public abstract char charId();"), + linesWithoutBuilder( + " static Test create(", + " long longId, int intId, byte byteId, short shortId,", + " float floatId, double doubleId, boolean booleanId, char charId) {", + " return new AutoValue_Test(longId, intId, byteId, shortId, floatId,", + " doubleId, booleanId, charId);", + " }"), + linesWithBuilder( + " @AutoValue.Builder", + " abstract static class Builder {", + " abstract Builder setLongId(long value);", + " abstract Builder setIntId(int value);", + " abstract Builder setByteId(byte value);", + " abstract Builder setShortId(short value);", + " abstract Builder setFloatId(float value);", + " abstract Builder setDoubleId(double value);", + " abstract Builder setBooleanId(boolean value);", + " abstract Builder setCharId(char value);", + " abstract Test build();", + " }"), + lines("}"))) + .doTest(); + } + + @Test + public void nullableBoxedTypes() { + compilationHelper + .addSourceLines( + "in/Test.java", + mergeLines( + lines( + "import com.google.auto.value.AutoValue;", + "import javax.annotation.Nullable;", + "@AutoValue", + "abstract class Test {", + " public abstract @Nullable Long longId();", + " public abstract @Nullable Integer intId();", + " public abstract @Nullable Byte byteId();", + " public abstract @Nullable Short shortId();", + " public abstract @Nullable Float floatId();", + " public abstract @Nullable Double doubleId();", + " public abstract @Nullable Boolean booleanId();", + " public abstract @Nullable Character charId();"), + linesWithoutBuilder( + " static Test create(", + " @Nullable Long longId, @Nullable Integer intId, @Nullable Byte byteId,", + " @Nullable Short shortId, @Nullable Float floatId,", + " @Nullable Double doubleId, @Nullable Boolean booleanId,", + " @Nullable Character charId) {", + " return new AutoValue_Test(longId, intId, byteId, shortId, floatId,", + " doubleId, booleanId, charId);", + " }"), + linesWithBuilder( + " @AutoValue.Builder", + " abstract static class Builder {", + " abstract Builder setLongId(@Nullable Long value);", + " abstract Builder setIntId(@Nullable Integer value);", + " abstract Builder setByteId(@Nullable Byte value);", + " abstract Builder setShortId(@Nullable Short value);", + " abstract Builder setFloatId(@Nullable Float value);", + " abstract Builder setDoubleId(@Nullable Double value);", + " abstract Builder setBooleanId(@Nullable Boolean value);", + " abstract Builder setCharId(@Nullable Character value);", + " abstract Test build();", + " }"), + lines("}"))) + .doTest(); + } + + @Test + public void genericNullableBoxedTypes() { + compilationHelper + .addSourceLines( + "in/Test.java", + mergeLines( + lines( + "import com.google.auto.value.AutoValue;", + "import org.checkerframework.checker.nullness.qual.Nullable;", + "@AutoValue", + "abstract class Test {", + " public abstract @Nullable Long longId();", + " public abstract @Nullable Integer intId();", + " public abstract @Nullable Byte byteId();", + " public abstract @Nullable Short shortId();", + " public abstract @Nullable Float floatId();", + " public abstract @Nullable Double doubleId();", + " public abstract @Nullable Boolean booleanId();", + " public abstract @Nullable Character charId();"), + linesWithoutBuilder( + " static Test create(", + " @Nullable Long longId, @Nullable Integer intId, @Nullable Byte byteId,", + " @Nullable Short shortId, @Nullable Float floatId,", + " @Nullable Double doubleId, @Nullable Boolean booleanId,", + " @Nullable Character charId) {", + " return new AutoValue_Test(longId, intId, byteId, shortId, floatId,", + " doubleId, booleanId, charId);", + " }"), + linesWithBuilder( + " @AutoValue.Builder", + " abstract static class Builder {", + " abstract Builder setLongId(@Nullable Long value);", + " abstract Builder setIntId(@Nullable Integer value);", + " abstract Builder setByteId(@Nullable Byte value);", + " abstract Builder setShortId(@Nullable Short value);", + " abstract Builder setFloatId(@Nullable Float value);", + " abstract Builder setDoubleId(@Nullable Double value);", + " abstract Builder setBooleanId(@Nullable Boolean value);", + " abstract Builder setCharId(@Nullable Character value);", + " abstract Test build();", + " }"), + lines("}"))) + .doTest(); + } + + @Test + public void primitiveTypes() { + compilationHelper + .addSourceLines( + "in/Test.java", + mergeLines( + lines( + "import com.google.auto.value.AutoValue;", + "@AutoValue", + "abstract class Test {", + " public abstract long longId();", + " public abstract int intId();", + " public abstract byte byteId();", + " public abstract short shortId();", + " public abstract float floatId();", + " public abstract double doubleId();", + " public abstract boolean booleanId();", + " public abstract char charId();"), + linesWithoutBuilder( + " static Test create(", + " long longId, int intId, byte byteId, short shortId,", + " float floatId, double doubleId, boolean booleanId, char charId) {", + " return new AutoValue_Test(longId, intId, byteId, shortId, floatId,", + " doubleId, booleanId, charId);", + " }"), + linesWithBuilder( + " @AutoValue.Builder", + " abstract static class Builder {", + " abstract Builder setLongId(long value);", + " abstract Builder setIntId(int value);", + " abstract Builder setByteId(byte value);", + " abstract Builder setShortId(short value);", + " abstract Builder setFloatId(float value);", + " abstract Builder setDoubleId(double value);", + " abstract Builder setBooleanId(boolean value);", + " abstract Builder setCharId(char value);", + " abstract Test build();", + " }"), + lines("}"))) + .doTest(); + } + + @Test + public void nonBoxableTypes() { + compilationHelper + .addSourceLines( + "in/Test.java", + mergeLines( + lines( + "import com.google.auto.value.AutoValue;", + "import javax.annotation.Nullable;", + "@AutoValue", + "abstract class Test {", + " public abstract String stringId();", + " public @Nullable abstract String nullableStringId();"), + linesWithoutBuilder( + " static Test create(String stringId, @Nullable String nullableStringId) {", + " return new AutoValue_Test(stringId, nullableStringId);", + " }"), + linesWithBuilder( + " @AutoValue.Builder", + " abstract static class Builder {", + " abstract Builder setStringId(String value);", + " abstract Builder setNullableStringId(@Nullable String value);", + " abstract Test build();", + " }"), + lines("}"))) + .doTest(); + } + + @Test + public void mixedTypes_refactoring() { + refactoringHelper + .addInputLines( + "in/Test.java", + mergeLines( + lines( + "import com.google.auto.value.AutoValue;", + "import javax.annotation.Nullable;", + "@AutoValue", + "abstract class Test {", + " public abstract @Nullable Long nullableId();", + " public abstract Long unnecessaryBoxedId();", + " public abstract long primitiveId();", + " public abstract String nonBoxableId();"), + linesWithoutBuilder( + " static Test create(", + " @Nullable Long nullableId, Long unnecessaryBoxedId,", + " long primitiveId, String nonBoxableId) {", + " return new AutoValue_Test(", + " nullableId, unnecessaryBoxedId, primitiveId, nonBoxableId);", + " }"), + linesWithBuilder( + " @AutoValue.Builder", + " abstract static class Builder {", + " abstract Builder setNullableId(@Nullable Long value);", + " abstract Builder setUnnecessaryBoxedId(Long value);", + " abstract Builder setPrimitiveId(long value);", + " abstract Builder setNonBoxableId(String value);", + " abstract Test build();", + " }"), + lines("}"))) + .addOutputLines( + "out/Test.java", + mergeLines( + lines( + "import com.google.auto.value.AutoValue;", + "import javax.annotation.Nullable;", + "@AutoValue", + "abstract class Test {", + " public abstract @Nullable Long nullableId();", + " public abstract long unnecessaryBoxedId();", + " public abstract long primitiveId();", + " public abstract String nonBoxableId();"), + linesWithoutBuilder( + " static Test create(", + " @Nullable Long nullableId, long unnecessaryBoxedId,", + " long primitiveId, String nonBoxableId) {", + " return new AutoValue_Test(", + " nullableId, unnecessaryBoxedId, primitiveId, nonBoxableId);", + " }"), + linesWithBuilder( + " @AutoValue.Builder", + " abstract static class Builder {", + " abstract Builder setNullableId(@Nullable Long value);", + " abstract Builder setUnnecessaryBoxedId(long value);", + " abstract Builder setPrimitiveId(long value);", + " abstract Builder setNonBoxableId(String value);", + " abstract Test build();", + " }"), + lines("}"))) + .doTest(); + } + + @Test + public void unnecessaryBoxedTypes_suppressWarnings() { + refactoringHelper + .addInputLines( + "in/Test.java", + mergeLines( + lines( + "import com.google.auto.value.AutoValue;", + "@AutoValue", + "abstract class Test {", + " public abstract Long longId();", + " @SuppressWarnings(\"AutoValueBoxedValues\")", + " public abstract Long longIdSuppressWarnings();"), + linesWithoutBuilder( + " static Test create(Long longId, Long longIdSuppressWarnings) {", + " return new AutoValue_Test(longId, longIdSuppressWarnings);", + " }"), + linesWithBuilder( + " @AutoValue.Builder", + " abstract static class Builder {", + " abstract Builder setLongId(Long value);", + " @SuppressWarnings(\"AutoValueBoxedValues\")", + " abstract Builder setLongIdSuppressWarnings(Long value);", + " abstract Test build();", + " }"), + lines("}"))) + .addOutputLines( + "out/Test.java", + mergeLines( + lines( + "import com.google.auto.value.AutoValue;", + "@AutoValue", + "abstract class Test {", + " public abstract long longId();", + " @SuppressWarnings(\"AutoValueBoxedValues\")", + " public abstract Long longIdSuppressWarnings();"), + linesWithoutBuilder( + " static Test create(long longId, Long longIdSuppressWarnings) {", + " return new AutoValue_Test(longId, longIdSuppressWarnings);", + " }"), + linesWithBuilder( + " @AutoValue.Builder", + " abstract static class Builder {", + " abstract Builder setLongId(long value);", + " @SuppressWarnings(\"AutoValueBoxedValues\")", + " abstract Builder setLongIdSuppressWarnings(Long value);", + " abstract Test build();", + " }"), + lines("}"))) + .doTest(); + } + + @Test + public void nullableGettersWithNonNullableSetters_noChange() { + if (!withBuilder) { + return; + } + compilationHelper + .addSourceLines( + "in/Test.java", + "import com.google.auto.value.AutoValue;", + "import javax.annotation.Nullable;", + "@AutoValue", + "abstract class Test {", + " public abstract @Nullable Long longId();", + " public abstract @Nullable Integer intId();", + " @AutoValue.Builder", + " abstract static class Builder {", + " abstract Builder setLongId(Long value);", + " abstract Builder setIntId(Integer value);", + " abstract Test build();", + " }", + "}") + .doTest(); + } + + @Test + public void nonTrivialFactoryMethods_refectoring() { + if (withBuilder) { + return; + } + refactoringHelper + .addInputLines( + "in/Test.java", + "package test;", + "import com.google.auto.value.AutoValue;", + "@AutoValue", + "abstract class Test {", + " abstract int foo();", + " abstract Long bar();", + " static Test createTrivial(int foo, Long bar) {", + " return new AutoValue_Test(foo, bar);", + " }", + " static String notFactoryMethod(int foo, Long bar) {", + " return String.format(\"foo: %d, bar: %d\", foo, bar);", + " }", + " static Test createWrongOrder(Long bar, int foo) {", + " return new AutoValue_Test(foo, bar);", + " }", + " static Test createLessArguments(int foo) {", + " return new AutoValue_Test(foo, 0L);", + " }", + " static Test createMoreArguments(int foo, Long bar, Long baz) {", + " return new AutoValue_Test(foo, bar + baz);", + " }", + " static Test createWithValidation(int foo, Long bar) {", + " if (bar == null) { throw new AssertionError(); }", + " return new AutoValue_Test(foo, bar);", + " }", + " static Test createModifyArgs(int foo, Long bar) {", + " return new AutoValue_Test(foo + 1, bar);", + " }", + " static Test createModifyArgsIfNull(int foo, Long bar) {", + " return new AutoValue_Test(foo, bar == null ? 0L : bar);", + " }", + "}") + .addOutputLines( + "out/Test.java", + "package test;", + "import com.google.auto.value.AutoValue;", + "@AutoValue", + "abstract class Test {", + " abstract int foo();", + " abstract long bar();", + " static Test createTrivial(int foo, long bar) {", + " return new AutoValue_Test(foo, bar);", + " }", + " static String notFactoryMethod(int foo, Long bar) {", + " return String.format(\"foo: %d, bar: %d\", foo, bar);", + " }", + " static Test createWrongOrder(Long bar, int foo) {", + " return new AutoValue_Test(foo, bar);", + " }", + " static Test createLessArguments(int foo) {", + " return new AutoValue_Test(foo, 0L);", + " }", + " static Test createMoreArguments(int foo, Long bar, Long baz) {", + " return new AutoValue_Test(foo, bar + baz);", + " }", + " static Test createWithValidation(int foo, Long bar) {", + " if (bar == null) { throw new AssertionError(); }", + " return new AutoValue_Test(foo, bar);", + " }", + " static Test createModifyArgs(int foo, Long bar) {", + " return new AutoValue_Test(foo + 1, bar);", + " }", + " static Test createModifyArgsIfNull(int foo, Long bar) {", + " return new AutoValue_Test(foo, bar == null ? 0L : bar);", + " }", + "}") + .doTest(); + } + + @Test + public void settersWithoutSetPrefix() { + if (!withBuilder) { + return; + } + refactoringHelper + .addInputLines( + "in/Test.java", + "import com.google.auto.value.AutoValue;", + "@AutoValue", + "abstract class Test {", + " public abstract Long longId();", + " public abstract Boolean booleanId();", + " @AutoValue.Builder", + " abstract static class Builder {", + " abstract Builder longId(Long value);", + " abstract Builder booleanId(Boolean value);", + " abstract Test build();", + " }", + "}") + .addOutputLines( + "out/Test.java", + "import com.google.auto.value.AutoValue;", + "@AutoValue", + "abstract class Test {", + " public abstract long longId();", + " public abstract boolean booleanId();", + " @AutoValue.Builder", + " abstract static class Builder {", + " abstract Builder longId(long value);", + " abstract Builder booleanId(boolean value);", + " abstract Test build();", + " }", + "}") + .doTest(); + } + + @Test + public void allGettersWithPrefix() { + if (!withBuilder) { + return; + } + refactoringHelper + .addInputLines( + "in/Test.java", + "import com.google.auto.value.AutoValue;", + "@AutoValue", + "abstract class Test {", + " public abstract Long getLongId();", + " public abstract boolean isBooleanId();", + " @AutoValue.Builder", + " abstract static class Builder {", + " abstract Builder setLongId(Long value);", + " abstract Builder setBooleanId(boolean value);", + " abstract Test build();", + " }", + "}") + .addOutputLines( + "out/Test.java", + "import com.google.auto.value.AutoValue;", + "@AutoValue", + "abstract class Test {", + " public abstract long getLongId();", + " public abstract boolean isBooleanId();", + " @AutoValue.Builder", + " abstract static class Builder {", + " abstract Builder setLongId(long value);", + " abstract Builder setBooleanId(boolean value);", + " abstract Test build();", + " }", + "}") + .doTest(); + } + + private static List lines(String... lines) { + return Arrays.asList(lines); + } + + private List linesWithBuilder(String... lines) { + return withBuilder ? Arrays.asList(lines) : new ArrayList<>(); + } + + private List linesWithoutBuilder(String... lines) { + return !withBuilder ? Arrays.asList(lines) : new ArrayList<>(); + } + + private static String[] mergeLines(List... blocks) { + return stream(blocks).flatMap(List::stream).toArray(String[]::new); + } +} diff --git a/docs/bugpattern/AutoValueBoxedValues.md b/docs/bugpattern/AutoValueBoxedValues.md new file mode 100644 index 00000000000..3c90729ad23 --- /dev/null +++ b/docs/bugpattern/AutoValueBoxedValues.md @@ -0,0 +1,11 @@ +AutoValue classes reject `null` values, unless the property is annotated with +`@Nullable`. For this reason, the usage of boxed primitives (e.g. `Long`) is +discouraged, except when annotated as `@Nullable`. Otherwise they can be +replaced with the corresponding primitive. There could be some cases where the +usage of a boxed primitive might be intentional to avoid boxing the value again +after invoking the getter. + +## Suppression + +Suppress violations by using `@SuppressWarnings("AutoValueBoxedValues")` on the +relevant `abstract` getter and/or setter.