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.