From d5cd6cf5179bebac1e1227785308c785a86fa0b9 Mon Sep 17 00:00:00 2001 From: Kenzie Togami Date: Thu, 30 Jun 2016 18:35:29 -0700 Subject: [PATCH 1/5] Add support for Nullable on Optional property builders --- .../auto/value/processor/BuilderSpec.java | 33 +++++++++++++++++-- .../auto/value/processor/Optionalish.java | 18 ++++++++++ .../google/auto/value/processor/autovalue.vm | 10 ++++-- .../auto/value/processor/CompilationTest.java | 32 +++++++++++++++--- 4 files changed, 83 insertions(+), 10 deletions(-) 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..43030033fa 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 @@ -34,6 +34,7 @@ import java.util.Optional; import java.util.Set; import javax.annotation.processing.ProcessingEnvironment; +import javax.lang.model.element.AnnotationMirror; import javax.lang.model.element.Element; import javax.lang.model.element.ElementKind; import javax.lang.model.element.ExecutableElement; @@ -288,11 +289,13 @@ public class PropertySetter { private final String parameterTypeString; private final boolean primitiveParameter; private final String copyOf; + private final String nullableAnnotation; public PropertySetter(ExecutableElement setter, TypeMirror propertyType) { 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(); @@ -300,10 +303,30 @@ public PropertySetter(ExecutableElement setter, TypeMirror propertyType) { boolean sameType = typeUtils.isSameType(typeUtils.erasure(parameterType), erasedPropertyType); if (sameType) { this.copyOf = null; + this.nullableAnnotation = ""; } else { String rawTarget = TypeEncoder.encodeRaw(erasedPropertyType); - String of = Optionalish.isOptional(propertyType) ? "of" : "copyOf"; + Optionalish optional = Optionalish.createIfOptional(propertyType, rawTarget); + String nullableAnnotation = ""; + String of = null; + if (optional != null) { + for (AnnotationMirror annotationMirror : parameterElement.getAnnotationMirrors()) { + AnnotationOutput annotationOutput = new AnnotationOutput(typeSimplifier); + String annotationName = annotationOutput.sourceFormForAnnotation(annotationMirror); + if (annotationName.equals("@Nullable") || annotationName.endsWith(".Nullable")) { + of = optional.getNullable(); + nullableAnnotation = annotationName + " "; + break; + } + } + if (of == null) { + of = "of"; + } + } else { + of = "copyOf"; + } this.copyOf = rawTarget + "." + of + "(%s)"; + this.nullableAnnotation = nullableAnnotation; } } @@ -337,6 +360,10 @@ public boolean getPrimitiveParameter() { return primitiveParameter; } + public String getNullableAnnotation() { + return nullableAnnotation; + } + public String copy(AutoValueProcessor.Property property) { if (copyOf == null) { return property.toString(); @@ -345,7 +372,7 @@ public String copy(AutoValueProcessor.Property property) { String copy = String.format(copyOf, property); // Add a null guard only in cases where we are using copyOf and the property is @Nullable. - if (property.isNullable()) { + if (property.isNullable() || nullableAnnotation != null) { copy = String.format("(%s == null ? null : %s)", property, copy); } 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..81a3531922 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 @@ -96,6 +96,24 @@ public String getEmpty() { return TypeEncoder.encodeRaw(optionalType) + empty; } + /** + * Returns a string representing the method call to obtain the nullable version of this Optional. + * This will be something like {@code "fromNullable()"} or possibly {@code "ofNullable()"}. It does not have a final semicolon. + * + *

This method is public so that it can be referenced as {@code p.optional.nullable} from + * templates. + */ + public String getNullable() { + if (optionalType.getTypeArguments().isEmpty()) { + // No typeArguments means a primitive wrapper -- it has no nullable input + return "of"; + } + TypeElement typeElement = MoreElements.asType(optionalType.asElement()); + return typeElement.getQualifiedName().toString().startsWith("java.util.") + ? "ofNullable" + : "fromNullable"; + } + TypeMirror getContainedType(Types typeUtils) { List typeArguments = optionalType.getTypeArguments(); switch (typeArguments.size()) { 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..52a8ffa8bc 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 @@ -240,11 +240,17 @@ ${modifiers}class $subclass$formalTypes extends $origClass$actualTypes { #foreach ($setter in $builderSetters[$p.name]) + #if ($p.nullable) + #set ($nullableAnnotation = $p.nullableAnnotation) + #else + #set ($nullableAnnotation = $setter.nullableAnnotation) + #end + @Override ${setter.access}${builderTypeName}${builderActualTypes} ## - ${setter.name}(${p.nullableAnnotation}$setter.parameterType $p) { + ${setter.name}(${nullableAnnotation}$setter.parameterType $p) { - #if (!$setter.primitiveParameter && !$p.nullable) + #if (!$setter.primitiveParameter && !$nullableAnnotation) #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..800505c9cc 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 @@ -776,6 +776,7 @@ public void correctBuilder() throws Exception { " public abstract ImmutableList anImmutableList();", " public abstract Optional anOptionalString();", " public abstract NestedAutoValue aNestedAutoValue();", + " public abstract Optional anOptionalObject();", "", " public abstract Builder toBuilder();", "", @@ -790,6 +791,7 @@ public void correctBuilder() throws Exception { " public abstract Builder anOptionalString(Optional s);", " public abstract Builder anOptionalString(String s);", " public abstract NestedAutoValue.Builder aNestedAutoValueBuilder();", + " public abstract Builder anOptionalObject(@Nullable Object s);", "", " public Builder aList(ArrayList x) {", // ArrayList should not be imported in the generated class. @@ -851,6 +853,7 @@ public void correctBuilder() throws Exception { " private final ImmutableList anImmutableList;", " private final Optional anOptionalString;", " private final NestedAutoValue aNestedAutoValue;", + " private final Optional anOptionalObject;", "", " private AutoValue_Baz(", " int anInt,", @@ -859,7 +862,8 @@ public void correctBuilder() throws Exception { " List aList,", " ImmutableList anImmutableList,", " Optional anOptionalString,", - " NestedAutoValue aNestedAutoValue) {", + " NestedAutoValue aNestedAutoValue,", + " Optional anOptionalObject) {", " this.anInt = anInt;", " this.aByteArray = aByteArray;", " this.aNullableIntArray = aNullableIntArray;", @@ -867,6 +871,7 @@ public void correctBuilder() throws Exception { " this.anImmutableList = anImmutableList;", " this.anOptionalString = anOptionalString;", " this.aNestedAutoValue = aNestedAutoValue;", + " this.anOptionalObject = anOptionalObject;", " }", "", " @Override public int anInt() {", @@ -900,6 +905,10 @@ public void correctBuilder() throws Exception { " return aNestedAutoValue;", " }", "", + " @Override public Optional anOptionalObject() {", + " return anOptionalObject;", + " }", + "", " @Override public String toString() {", " return \"Baz{\"", " + \"anInt=\" + anInt + \", \"", @@ -908,7 +917,8 @@ public void correctBuilder() throws Exception { " + \"aList=\" + aList + \", \"", " + \"anImmutableList=\" + anImmutableList + \", \"", " + \"anOptionalString=\" + anOptionalString + \", \"", - " + \"aNestedAutoValue=\" + aNestedAutoValue", + " + \"aNestedAutoValue=\" + aNestedAutoValue + \", \"", + " + \"anOptionalObject=\" + anOptionalObject", " + \"}\";", " }", "", @@ -928,7 +938,8 @@ public void correctBuilder() throws Exception { " && (this.aList.equals(that.aList()))", " && (this.anImmutableList.equals(that.anImmutableList()))", " && (this.anOptionalString.equals(that.anOptionalString()))", - " && (this.aNestedAutoValue.equals(that.aNestedAutoValue()));", + " && (this.aNestedAutoValue.equals(that.aNestedAutoValue()))", + " && (this.anOptionalObject.equals(that.anOptionalObject()));", " }", " return false;", " }", @@ -949,7 +960,9 @@ public void correctBuilder() throws Exception { " h$ ^= anOptionalString.hashCode();", " h$ *= 1000003;", " h$ ^= aNestedAutoValue.hashCode();", - " return h$;", + " h$ *= 1000003;", + " h$ ^= this.anOptionalObject.hashCode();", + " return h;", " }", "", " @Override public Baz.Builder toBuilder() {", @@ -966,6 +979,7 @@ public void correctBuilder() throws Exception { " private Optional anOptionalString = Optional.absent();", " private NestedAutoValue.Builder aNestedAutoValueBuilder$;", " private NestedAutoValue aNestedAutoValue;", + " private Optional anOptionalObject = Optional.absent();", "", " Builder() {", " }", @@ -978,6 +992,7 @@ public void correctBuilder() throws Exception { " this.anImmutableList = source.anImmutableList();", " this.anOptionalString = source.anOptionalString();", " this.aNestedAutoValue = source.aNestedAutoValue();", + " this.anOptionalObject = source.anOptionalObject();", " }", "", " @Override", @@ -1097,6 +1112,12 @@ public void correctBuilder() throws Exception { " }", "", " @Override", + " public Baz.Builder anOptionalObject(@Nullable Object anOptionalObject) {", + " this.anOptionalObject = Optional.fromNullable(anOptionalObject);", + " return this;", + " }", + "", + " @Override", " public Baz build() {", " if (anImmutableListBuilder$ != null) {", " this.anImmutableList = anImmutableListBuilder$.build();", @@ -1130,7 +1151,8 @@ public void correctBuilder() throws Exception { " this.aList,", " this.anImmutableList,", " this.anOptionalString,", - " this.aNestedAutoValue);", + " this.aNestedAutoValue,", + " this.anOptionalObject);", " }", " }", "}"); From 692ad5081a58d1d4a79ff33280c0cdc7f8d80f24 Mon Sep 17 00:00:00 2001 From: Kenzie Togami Date: Wed, 8 Mar 2017 22:37:07 -0800 Subject: [PATCH 2/5] Add Nullable Setters to AutoValueTest, fix some bugs --- .../com/google/auto/value/AutoValueTest.java | 48 +++++++++++++++++++ .../auto/value/processor/BuilderSpec.java | 3 +- .../google/auto/value/processor/autovalue.vm | 2 +- 3 files changed, 51 insertions(+), 2 deletions(-) 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..04fd9c154b 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 @@ -1510,6 +1510,54 @@ public void testOmitOptionalWithBuilder() { assertThat(suppliedDirectly.optionalString()).hasValue("foo"); 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 e) { + assertThat(e.getMessage()).contains("notOptional"); + } + } @AutoValue public abstract static class NullableOptionalPropertiesWithBuilder { 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 43030033fa..b74cf34117 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 @@ -372,7 +372,8 @@ public String copy(AutoValueProcessor.Property property) { String copy = String.format(copyOf, property); // Add a null guard only in cases where we are using copyOf and the property is @Nullable. - if (property.isNullable() || nullableAnnotation != null) { + // No guard for the @Nullable annotation case because from/ofNullable doesn't need it + if (property.isNullable()) { copy = String.format("(%s == null ? null : %s)", property, copy); } 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 52a8ffa8bc..97e2de0004 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 @@ -250,7 +250,7 @@ ${modifiers}class $subclass$formalTypes extends $origClass$actualTypes { ${setter.access}${builderTypeName}${builderActualTypes} ## ${setter.name}(${nullableAnnotation}$setter.parameterType $p) { - #if (!$setter.primitiveParameter && !$nullableAnnotation) + #if (!${setter.primitiveParameter} && ${nullableAnnotation.isEmpty()}) #if ($identifiers) From 226924a5b59cc847eef4c35c63352c5158574029 Mon Sep 17 00:00:00 2001 From: Kenzie Togami Date: Thu, 3 May 2018 12:46:17 -0700 Subject: [PATCH 3/5] Fix CompilationTest expected code --- .../java/com/google/auto/value/processor/CompilationTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 800505c9cc..39ce41b6cf 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 @@ -961,8 +961,8 @@ public void correctBuilder() throws Exception { " h$ *= 1000003;", " h$ ^= aNestedAutoValue.hashCode();", " h$ *= 1000003;", - " h$ ^= this.anOptionalObject.hashCode();", - " return h;", + " h$ ^= anOptionalObject.hashCode();", + " return h$;", " }", "", " @Override public Baz.Builder toBuilder() {", From 824568fd4fd3a160c90cda21b542c9c1bf91ddd9 Mon Sep 17 00:00:00 2001 From: Kenzie Togami Date: Thu, 3 May 2018 13:10:18 -0700 Subject: [PATCH 4/5] Update nullable logic for new encoding --- .../value/processor/AutoValueProcessor.java | 16 ++++++++++---- .../auto/value/processor/BuilderSpec.java | 22 +++++++++---------- .../auto/value/processor/Optionalish.java | 3 ++- 3 files changed, 24 insertions(+), 17 deletions(-) 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..e5c33d3bd5 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 @@ -371,10 +371,9 @@ private void defineVarsForType( @Override Optional nullableAnnotationForMethod( ExecutableElement propertyMethod, ImmutableList methodAnnotations) { - OptionalInt nullableAnnotationIndex = nullableAnnotationIndex(methodAnnotations); - if (nullableAnnotationIndex.isPresent()) { - ImmutableList annotations = annotationStrings(methodAnnotations); - return Optional.of(annotations.get(nullableAnnotationIndex.getAsInt())); + Optional nullableAnnotationList = nullableAnnotationIfInList(methodAnnotations); + if (nullableAnnotationList.isPresent()) { + return nullableAnnotationList; } else { List typeAnnotations = propertyMethod.getReturnType().getAnnotationMirrors(); @@ -385,6 +384,15 @@ Optional nullableAnnotationForMethod( } } + public static Optional nullableAnnotationIfInList(ImmutableList methodAnnotations) { + OptionalInt nullableAnnotationIndex = nullableAnnotationIndex(methodAnnotations); + if (nullableAnnotationIndex.isPresent()) { + ImmutableList annotations = annotationStrings(methodAnnotations); + return Optional.of(annotations.get(nullableAnnotationIndex.getAsInt())); + } + return Optional.empty(); + } + private static OptionalInt nullableAnnotationIndex(List annotations) { for (int i = 0; i < annotations.size(); i++) { if (isNullable(annotations.get(i))) { 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 b74cf34117..25212635e9 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 @@ -24,6 +24,7 @@ import com.google.auto.common.MoreTypes; import com.google.auto.value.processor.AutoValueOrOneOfProcessor.Property; import com.google.common.collect.ImmutableBiMap; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableMultimap; import com.google.common.collect.ImmutableSet; @@ -41,6 +42,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; @@ -306,20 +308,16 @@ public PropertySetter(ExecutableElement setter, TypeMirror propertyType) { this.nullableAnnotation = ""; } else { String rawTarget = TypeEncoder.encodeRaw(erasedPropertyType); - Optionalish optional = Optionalish.createIfOptional(propertyType, rawTarget); + Optionalish optional = Optionalish.createIfOptional(propertyType); String nullableAnnotation = ""; - String of = null; + String of; if (optional != null) { - for (AnnotationMirror annotationMirror : parameterElement.getAnnotationMirrors()) { - AnnotationOutput annotationOutput = new AnnotationOutput(typeSimplifier); - String annotationName = annotationOutput.sourceFormForAnnotation(annotationMirror); - if (annotationName.equals("@Nullable") || annotationName.endsWith(".Nullable")) { - of = optional.getNullable(); - nullableAnnotation = annotationName + " "; - break; - } - } - if (of == null) { + ImmutableList annotationMirrors = ImmutableList.copyOf(parameterElement.getAnnotationMirrors()); + Optional nullableAnnotationFromParam = AutoValueProcessor.nullableAnnotationIfInList(annotationMirrors); + if (nullableAnnotationFromParam.isPresent()) { + of = optional.getNullable(); + nullableAnnotation = nullableAnnotationFromParam.get() + " "; + } else { of = "of"; } } else { 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 81a3531922..d69c279e94 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 @@ -98,7 +98,8 @@ public String getEmpty() { /** * Returns a string representing the method call to obtain the nullable version of this Optional. - * This will be something like {@code "fromNullable()"} or possibly {@code "ofNullable()"}. It does not have a final semicolon. + * This will be something like {@code "fromNullable()"} or possibly {@code "ofNullable()"}. + * It does not have a final semicolon. * *

This method is public so that it can be referenced as {@code p.optional.nullable} from * templates. From 99438aee99d716b2c76e0f6bda7997ac3af04ad4 Mon Sep 17 00:00:00 2001 From: Kenzie Togami Date: Thu, 3 May 2018 13:18:56 -0700 Subject: [PATCH 5/5] Correct Optionalish.getNullable docs --- .../java/com/google/auto/value/processor/Optionalish.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) 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 d69c279e94..059a62833f 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 @@ -97,9 +97,8 @@ public String getEmpty() { } /** - * Returns a string representing the method call to obtain the nullable version of this Optional. - * This will be something like {@code "fromNullable()"} or possibly {@code "ofNullable()"}. - * It does not have a final semicolon. + * Returns a string representing the method name to call to obtain the nullable version of + * this Optional. This will be something like {@code "fromNullable"} or {@code "ofNullable"}. * *

This method is public so that it can be referenced as {@code p.optional.nullable} from * templates.