diff --git a/value/src/it/functional/src/test/java/com/google/auto/value/AutoValueJava8Test.java b/value/src/it/functional/src/test/java/com/google/auto/value/AutoValueJava8Test.java index 25cd68b12b..0b438302cd 100644 --- a/value/src/it/functional/src/test/java/com/google/auto/value/AutoValueJava8Test.java +++ b/value/src/it/functional/src/test/java/com/google/auto/value/AutoValueJava8Test.java @@ -16,6 +16,7 @@ package com.google.auto.value; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth8.assertThat; import static com.google.testing.compile.CompilationSubject.assertThat; import static org.junit.Assert.fail; import static org.junit.Assume.assumeTrue; @@ -37,6 +38,7 @@ import java.lang.reflect.TypeVariable; import java.util.Arrays; import java.util.List; +import java.util.Optional; import java.util.Set; import javax.annotation.processing.AbstractProcessor; import javax.annotation.processing.RoundEnvironment; @@ -368,6 +370,54 @@ public void testOmitNullableWithBuilder() { } } + @AutoValue + public abstract static class OptionalPropertyWithNullableBuilder { + public abstract String notOptional(); + + public abstract Optional optional(); + + public static Builder builder() { + return new AutoValue_AutoValueJava8Test_OptionalPropertyWithNullableBuilder.Builder(); + } + + @AutoValue.Builder + public interface Builder { + Builder notOptional(String s); + + Builder optional(@Nullable String s); + + OptionalPropertyWithNullableBuilder build(); + } + } + + @Test + public void testOmitOptionalWithNullableBuilder() { + OptionalPropertyWithNullableBuilder instance1 = + OptionalPropertyWithNullableBuilder.builder().notOptional("hello").build(); + assertThat(instance1.notOptional()).isEqualTo("hello"); + assertThat(instance1.optional()).isEmpty(); + + OptionalPropertyWithNullableBuilder instance2 = + OptionalPropertyWithNullableBuilder.builder().notOptional("hello").optional(null).build(); + assertThat(instance2.notOptional()).isEqualTo("hello"); + assertThat(instance2.optional()).isEmpty(); + assertThat(instance1).isEqualTo(instance2); + + OptionalPropertyWithNullableBuilder instance3 = + OptionalPropertyWithNullableBuilder.builder() + .notOptional("hello") + .optional("world") + .build(); + assertThat(instance3.notOptional()).isEqualTo("hello"); + assertThat(instance3.optional()).hasValue("world"); + + try { + OptionalPropertyWithNullableBuilder.builder().build(); + fail("Expected IllegalStateException for unset non-Optional property"); + } catch (IllegalStateException expected) { + } + } + @AutoValue public abstract static class BuilderWithUnprefixedGetters> { public abstract ImmutableList list(); @@ -497,6 +547,8 @@ abstract static class FunkyNullable { abstract @Nullable String foo(); + abstract Optional bar(); + static Builder builder() { return new AutoValue_AutoValueJava8Test_FunkyNullable.Builder(); } @@ -504,13 +556,14 @@ static Builder builder() { @AutoValue.Builder interface Builder { Builder setFoo(@Nullable String foo); + Builder setBar(@Nullable String bar); FunkyNullable build(); } } @Test public void testFunkyNullable() { - FunkyNullable explicitNull = FunkyNullable.builder().setFoo(null).build(); + FunkyNullable explicitNull = FunkyNullable.builder().setFoo(null).setBar(null).build(); FunkyNullable implicitNull = FunkyNullable.builder().build(); assertThat(explicitNull).isEqualTo(implicitNull); } diff --git a/value/src/it/functional/src/test/java/com/google/auto/value/AutoValueTest.java b/value/src/it/functional/src/test/java/com/google/auto/value/AutoValueTest.java index ae05891fa8..5b1dc310fe 100644 --- a/value/src/it/functional/src/test/java/com/google/auto/value/AutoValueTest.java +++ b/value/src/it/functional/src/test/java/com/google/auto/value/AutoValueTest.java @@ -1511,6 +1511,54 @@ public void testOmitOptionalWithBuilder() { assertThat(suppliedDirectly.optionalInteger()).hasValue(23); } + @AutoValue + public abstract static class OptionalPropertyWithNullableBuilder { + public abstract String notOptional(); + + public abstract com.google.common.base.Optional optional(); + + public static Builder builder() { + return new AutoValue_AutoValueTest_OptionalPropertyWithNullableBuilder.Builder(); + } + + @AutoValue.Builder + public interface Builder { + Builder notOptional(String s); + + Builder optional(@Nullable String s); + + OptionalPropertyWithNullableBuilder build(); + } + } + + @Test + public void testOmitOptionalWithNullableBuilder() { + OptionalPropertyWithNullableBuilder instance1 = + OptionalPropertyWithNullableBuilder.builder().notOptional("hello").build(); + assertThat(instance1.notOptional()).isEqualTo("hello"); + assertThat(instance1.optional()).isAbsent(); + + OptionalPropertyWithNullableBuilder instance2 = + OptionalPropertyWithNullableBuilder.builder().notOptional("hello").optional(null).build(); + assertThat(instance2.notOptional()).isEqualTo("hello"); + assertThat(instance2.optional()).isAbsent(); + assertThat(instance1).isEqualTo(instance2); + + OptionalPropertyWithNullableBuilder instance3 = + OptionalPropertyWithNullableBuilder.builder() + .notOptional("hello") + .optional("world") + .build(); + assertThat(instance3.notOptional()).isEqualTo("hello"); + assertThat(instance3.optional()).hasValue("world"); + + try { + OptionalPropertyWithNullableBuilder.builder().build(); + fail("Expected IllegalStateException for unset non-Optional property"); + } catch (IllegalStateException expected) { + } + } + @AutoValue public abstract static class NullableOptionalPropertiesWithBuilder { @Nullable diff --git a/value/src/main/java/com/google/auto/value/extension/memoized/Memoized.java b/value/src/main/java/com/google/auto/value/extension/memoized/Memoized.java index 44471908a1..0c94ef9fab 100644 --- a/value/src/main/java/com/google/auto/value/extension/memoized/Memoized.java +++ b/value/src/main/java/com/google/auto/value/extension/memoized/Memoized.java @@ -43,7 +43,7 @@ * returns {@code null}, the overriding method will throw a {@link NullPointerException}. * *

The overriding method uses - * double-checked locking to + * double-checked locking to * ensure that the annotated method is called at most once. * *

Example

diff --git a/value/src/main/java/com/google/auto/value/processor/AutoOneOfProcessor.java b/value/src/main/java/com/google/auto/value/processor/AutoOneOfProcessor.java index a9d84eb145..d8388fd4b8 100644 --- a/value/src/main/java/com/google/auto/value/processor/AutoOneOfProcessor.java +++ b/value/src/main/java/com/google/auto/value/processor/AutoOneOfProcessor.java @@ -25,7 +25,6 @@ import com.google.auto.common.MoreTypes; import com.google.auto.service.AutoService; import com.google.common.collect.ImmutableBiMap; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -246,8 +245,7 @@ private void defineVarsForType( } @Override - Optional nullableAnnotationForMethod( - ExecutableElement propertyMethod, ImmutableList methodAnnotations) { + Optional nullableAnnotationForMethod(ExecutableElement propertyMethod) { return Optional.empty(); } diff --git a/value/src/main/java/com/google/auto/value/processor/AutoValueOrOneOfProcessor.java b/value/src/main/java/com/google/auto/value/processor/AutoValueOrOneOfProcessor.java index d2e08f7a18..6e6752e75b 100644 --- a/value/src/main/java/com/google/auto/value/processor/AutoValueOrOneOfProcessor.java +++ b/value/src/main/java/com/google/auto/value/processor/AutoValueOrOneOfProcessor.java @@ -222,7 +222,7 @@ public Optionalish getOptional() { * but empty. */ public final String getNullableAnnotation() { - return nullableAnnotation.map(s -> s + " ").orElse(""); + return nullableAnnotation.orElse(""); } public boolean isNullable() { @@ -318,12 +318,8 @@ public final boolean process(Set annotations, RoundEnviro * {@code @Nullable}, this method returns {@code Optional.of("")}, to indicate that the property * is nullable but the method isn't. The {@code @Nullable} annotation will instead appear * when the return type of the method is spelled out in the implementation. - * - * @param methodAnnotations the annotations to include on {@code propertyMethod}. This is - * governed by the {@code AutoValue.CopyAnnotations} logic. */ - abstract Optional nullableAnnotationForMethod( - ExecutableElement propertyMethod, ImmutableList methodAnnotations); + abstract Optional nullableAnnotationForMethod(ExecutableElement propertyMethod); /** * Returns the ordered set of {@link Property} definitions for the given {@code @AutoValue} or @@ -357,8 +353,7 @@ final ImmutableSet propertySet( ImmutableList annotationMirrors = annotatedPropertyMethods.get(propertyMethod); ImmutableList annotations = annotationStrings(annotationMirrors); - Optional nullableAnnotation = - nullableAnnotationForMethod(propertyMethod, annotationMirrors); + Optional nullableAnnotation = nullableAnnotationForMethod(propertyMethod); Property p = new Property( propertyName, identifier, propertyMethod, propertyType, annotations, nullableAnnotation); props.add(p); @@ -394,8 +389,7 @@ final void defineSharedVarsForType( } /** Returns the spelling to be used in the generated code for the given list of annotations. */ - static ImmutableList annotationStrings( - ImmutableList annotations) { + static ImmutableList annotationStrings(List annotations) { // TODO(b/68008628): use ImmutableList.toImmutableList() when that works. return ImmutableList.copyOf(annotations .stream() diff --git a/value/src/main/java/com/google/auto/value/processor/AutoValueProcessor.java b/value/src/main/java/com/google/auto/value/processor/AutoValueProcessor.java index e063ef2182..23be0a824c 100644 --- a/value/src/main/java/com/google/auto/value/processor/AutoValueProcessor.java +++ b/value/src/main/java/com/google/auto/value/processor/AutoValueProcessor.java @@ -45,6 +45,7 @@ import javax.annotation.processing.SupportedAnnotationTypes; import javax.annotation.processing.SupportedOptions; import javax.lang.model.element.AnnotationMirror; +import javax.lang.model.element.Element; import javax.lang.model.element.ElementKind; import javax.lang.model.element.ExecutableElement; import javax.lang.model.element.TypeElement; @@ -369,19 +370,35 @@ private void defineVarsForType( } @Override - Optional nullableAnnotationForMethod( - ExecutableElement propertyMethod, ImmutableList methodAnnotations) { - OptionalInt nullableAnnotationIndex = nullableAnnotationIndex(methodAnnotations); + Optional nullableAnnotationForMethod(ExecutableElement propertyMethod) { + return nullableAnnotationFor(propertyMethod, propertyMethod.getReturnType()); + } + + /** + * Returns an appropriate annotation spelling to indicate the nullability of an element. If the + * return value is a non-empty Optional, that indicates that the element is nullable, and the + * string should be used to annotate it. If the return value is an empty Optional, the element + * is not nullable. The return value can be {@code Optional.of("")}, which indicates that the + * element is nullable but that the nullability comes from a type annotation. In this case, the + * annotation will appear when the type is written, and must not be specified again. If the + * Optional contains a present non-empty string then that string will end with a space. + * + * @param element the element that might be {@code @Nullable}, either a method or a parameter. + * @param elementType the relevant type of the element: the return type for a method, or the + * parameter type for a parameter. + */ + static Optional nullableAnnotationFor(Element element, TypeMirror elementType) { + List typeAnnotations = elementType.getAnnotationMirrors(); + if (nullableAnnotationIndex(typeAnnotations).isPresent()) { + return Optional.of(""); + } + List elementAnnotations = element.getAnnotationMirrors(); + OptionalInt nullableAnnotationIndex = nullableAnnotationIndex(elementAnnotations); if (nullableAnnotationIndex.isPresent()) { - ImmutableList annotations = annotationStrings(methodAnnotations); - return Optional.of(annotations.get(nullableAnnotationIndex.getAsInt())); + ImmutableList annotations = annotationStrings(elementAnnotations); + return Optional.of(annotations.get(nullableAnnotationIndex.getAsInt()) + " "); } else { - List typeAnnotations = - propertyMethod.getReturnType().getAnnotationMirrors(); - return - nullableAnnotationIndex(typeAnnotations).isPresent() - ? Optional.of("") - : Optional.empty(); + return Optional.empty(); } } diff --git a/value/src/main/java/com/google/auto/value/processor/BuilderSpec.java b/value/src/main/java/com/google/auto/value/processor/BuilderSpec.java index 6d5350a107..ad93794d48 100644 --- a/value/src/main/java/com/google/auto/value/processor/BuilderSpec.java +++ b/value/src/main/java/com/google/auto/value/processor/BuilderSpec.java @@ -40,6 +40,7 @@ import javax.lang.model.element.Modifier; import javax.lang.model.element.TypeElement; import javax.lang.model.element.TypeParameterElement; +import javax.lang.model.element.VariableElement; import javax.lang.model.type.DeclaredType; import javax.lang.model.type.TypeKind; import javax.lang.model.type.TypeMirror; @@ -207,7 +208,8 @@ void defineVars( String property = entry.getKey(); ExecutableElement setter = entry.getValue(); TypeMirror propertyType = getterToPropertyName.inverse().get(property).getReturnType(); - setterBuilder.put(property, new PropertySetter(setter, propertyType)); + setterBuilder.put( + property, new PropertySetter(setter, propertyType, processingEnv.getTypeUtils())); } vars.builderSetters = setterBuilder.build(); @@ -282,32 +284,29 @@ public Optionalish getOptional() { * method {@code setFoo(Collection foos)}. And, if {@code T} is {@code Optional}, * it can have a setter with a type that can be copied to {@code T} through {@code Optional.of}. */ - public class PropertySetter { + public static class PropertySetter { private final String access; private final String name; private final String parameterTypeString; private final boolean primitiveParameter; + private final String nullableAnnotation; private final String copyOf; - public PropertySetter(ExecutableElement setter, TypeMirror propertyType) { + PropertySetter(ExecutableElement setter, TypeMirror propertyType, Types typeUtils) { this.access = SimpleMethod.access(setter); this.name = setter.getSimpleName().toString(); - TypeMirror parameterType = Iterables.getOnlyElement(setter.getParameters()).asType(); + VariableElement parameterElement = Iterables.getOnlyElement(setter.getParameters()); + TypeMirror parameterType = parameterElement.asType(); primitiveParameter = parameterType.getKind().isPrimitive(); this.parameterTypeString = parameterTypeString(setter, parameterType); - Types typeUtils = processingEnv.getTypeUtils(); - TypeMirror erasedPropertyType = typeUtils.erasure(propertyType); - boolean sameType = typeUtils.isSameType(typeUtils.erasure(parameterType), erasedPropertyType); - if (sameType) { - this.copyOf = null; - } else { - String rawTarget = TypeEncoder.encodeRaw(erasedPropertyType); - String of = Optionalish.isOptional(propertyType) ? "of" : "copyOf"; - this.copyOf = rawTarget + "." + of + "(%s)"; - } + Optional maybeNullable = + AutoValueProcessor.nullableAnnotationFor(parameterElement, parameterType); + this.nullableAnnotation = maybeNullable.orElse(""); + boolean nullable = maybeNullable.isPresent(); + this.copyOf = copyOfString(propertyType, parameterType, typeUtils, nullable); } - private String parameterTypeString(ExecutableElement setter, TypeMirror parameterType) { + private static String parameterTypeString(ExecutableElement setter, TypeMirror parameterType) { if (setter.isVarArgs()) { TypeMirror componentType = MoreTypes.asArray(parameterType).getComponentType(); // This is a bit ugly. It's OK to annotate just the component type, because if it is @@ -321,6 +320,26 @@ private String parameterTypeString(ExecutableElement setter, TypeMirror paramete } } + private static String copyOfString( + TypeMirror propertyType, TypeMirror parameterType, Types typeUtils, boolean nullable) { + TypeMirror erasedPropertyType = typeUtils.erasure(propertyType); + boolean sameType = typeUtils.isSameType(typeUtils.erasure(parameterType), erasedPropertyType); + if (sameType) { + return null; + } + String rawTarget = TypeEncoder.encodeRaw(erasedPropertyType); + String of; + Optionalish optionalish = Optionalish.createIfOptional(propertyType); + if (optionalish == null) { + of = "copyOf"; + } else if (nullable) { + of = optionalish.ofNullable(); + } else { + of = "of"; + } + return rawTarget + "." + of + "(%s)"; + } + public String getAccess() { return access; } @@ -337,6 +356,10 @@ public boolean getPrimitiveParameter() { return primitiveParameter; } + public String getNullableAnnotation() { + return nullableAnnotation; + } + public String copy(AutoValueProcessor.Property property) { if (copyOf == null) { return property.toString(); diff --git a/value/src/main/java/com/google/auto/value/processor/Optionalish.java b/value/src/main/java/com/google/auto/value/processor/Optionalish.java index c4a68a8d1b..9b165bb657 100644 --- a/value/src/main/java/com/google/auto/value/processor/Optionalish.java +++ b/value/src/main/java/com/google/auto/value/processor/Optionalish.java @@ -42,9 +42,11 @@ public class Optionalish { "java.util.OptionalLong"); private final DeclaredType optionalType; + private final String className; private Optionalish(DeclaredType optionalType) { this.optionalType = optionalType; + this.className = MoreElements.asType(optionalType.asElement()).getQualifiedName().toString(); } /** @@ -89,8 +91,7 @@ public String getRawType() { * templates. */ public String getEmpty() { - TypeElement typeElement = MoreElements.asType(optionalType.asElement()); - String empty = typeElement.getQualifiedName().toString().startsWith("java.util.") + String empty = className.startsWith("java.util.") ? ".empty()" : ".absent()"; return TypeEncoder.encodeRaw(optionalType) + empty; @@ -108,6 +109,12 @@ TypeMirror getContainedType(Types typeUtils) { } } + String ofNullable() { + return className.equals("java.util.Optional") + ? "ofNullable" + : "fromNullable"; + } + private static final ImmutableMap PRIMITIVE_TYPE_KINDS = ImmutableMap.of( "OptionalDouble", TypeKind.DOUBLE, "OptionalInt", TypeKind.INT, diff --git a/value/src/main/java/com/google/auto/value/processor/autovalue.vm b/value/src/main/java/com/google/auto/value/processor/autovalue.vm index cf79193de9..17c766d70c 100644 --- a/value/src/main/java/com/google/auto/value/processor/autovalue.vm +++ b/value/src/main/java/com/google/auto/value/processor/autovalue.vm @@ -242,9 +242,11 @@ ${modifiers}class $subclass$formalTypes extends $origClass$actualTypes { @Override ${setter.access}${builderTypeName}${builderActualTypes} ## - ${setter.name}(${p.nullableAnnotation}$setter.parameterType $p) { + ${setter.name}(${setter.nullableAnnotation}$setter.parameterType $p) { - #if (!$setter.primitiveParameter && !$p.nullable) + ## Omit null check for primitive, or @Nullable, or if we are going to be calling a copy method + ## such as Optional.of, which will have its own null check if appropriate. + #if (!$setter.primitiveParameter && !$p.nullable && ${setter.copy($p)} == $p) #if ($identifiers) diff --git a/value/src/test/java/com/google/auto/value/processor/CompilationTest.java b/value/src/test/java/com/google/auto/value/processor/CompilationTest.java index c30d884521..1450ebffbe 100644 --- a/value/src/test/java/com/google/auto/value/processor/CompilationTest.java +++ b/value/src/test/java/com/google/auto/value/processor/CompilationTest.java @@ -1029,9 +1029,6 @@ public void correctBuilder() throws Exception { "", " @Override", " public Baz.Builder anImmutableList(List anImmutableList) {", - " if (anImmutableList == null) {", - " throw new NullPointerException(\"Null anImmutableList\");", - " }", " if (anImmutableListBuilder$ != null) {", " throw new IllegalStateException(" + "\"Cannot set anImmutableList after calling anImmutableListBuilder()\");", @@ -1076,9 +1073,6 @@ public void correctBuilder() throws Exception { "", " @Override", " public Baz.Builder anOptionalString(String anOptionalString) {", - " if (anOptionalString == null) {", - " throw new NullPointerException(\"Null anOptionalString\");", - " }", " this.anOptionalString = Optional.of(anOptionalString);", " return this;", " }", diff --git a/value/userguide/howto.md b/value/userguide/howto.md index 33319ab70d..d59dae6b11 100644 --- a/value/userguide/howto.md +++ b/value/userguide/howto.md @@ -512,7 +512,7 @@ public class Client { Switching on an enum like this can lead to more robust code than using `instanceof` checks, especially if a tool like [Error -Prone](http://errorprone.info/bugpattern/MissingCasesInEnumSwitch) can alert you +Prone](https://errorprone.info/bugpattern/MissingCasesInEnumSwitch) can alert you if you add a new variant without updating all your switches. (On the other hand, if nothing outside your class references `getKind()`, you should consider if a solution using inheritance might be better.)