From d5e4f555ec8cd715b1c4a920aaed435d2118b5f0 Mon Sep 17 00:00:00 2001 From: emcmanus Date: Tue, 21 May 2019 12:26:48 -0700 Subject: [PATCH] Allow @AutoOneOf properties to be void. An abstract void method means that the associated kind has no value. The factory method has no parameter and calling the implementation of the void method does nothing unless the instance is of another kind. RELNOTES=Allow @AutoOneOf properties to be void. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=249301961 --- .../com/google/auto/value/AutoOneOfTest.java | 83 +++++++++++++++++ .../value/processor/AutoOneOfProcessor.java | 9 ++ .../processor/AutoOneOfTemplateVars.java | 3 + .../processor/AutoValueOrOneOfProcessor.java | 9 +- .../google/auto/value/processor/autooneof.vm | 89 +++++++++++++++--- .../processor/AutoOneOfCompilationTest.java | 92 +++++++++++-------- value/userguide/howto.md | 45 ++++++++- 7 files changed, 275 insertions(+), 55 deletions(-) diff --git a/value/src/it/functional/src/test/java/com/google/auto/value/AutoOneOfTest.java b/value/src/it/functional/src/test/java/com/google/auto/value/AutoOneOfTest.java index 5d35e7417e..fae96f4c64 100644 --- a/value/src/it/functional/src/test/java/com/google/auto/value/AutoOneOfTest.java +++ b/value/src/it/functional/src/test/java/com/google/auto/value/AutoOneOfTest.java @@ -19,6 +19,10 @@ import static org.junit.Assert.fail; import com.google.common.testing.EqualsTester; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.ObjectInputStream; +import java.io.ObjectOutputStream; import java.io.Serializable; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; @@ -518,4 +522,83 @@ public void classAnnotationsCopiedIfCopyAnnotations() { assertThat(ace.getClass().isAnnotationPresent(CopyTest.class)).isTrue(); assertThat(ace.getClass().getAnnotation(CopyTest.class).value()).isEqualTo(23); } + + @AutoOneOf(MaybeEmpty.Kind.class) + public abstract static class MaybeEmpty implements Serializable { + public enum Kind { + EMPTY, STRING, + } + + public abstract Kind getKind(); + + public abstract void empty(); + + public abstract String string(); + + public static MaybeEmpty ofEmpty() { + return AutoOneOf_AutoOneOfTest_MaybeEmpty.empty(); + } + + public static MaybeEmpty ofString(String s) { + return AutoOneOf_AutoOneOfTest_MaybeEmpty.string(s); + } + } + + @Test + public void voidPropertyIsSingleton() { + MaybeEmpty empty1 = MaybeEmpty.ofEmpty(); + MaybeEmpty empty2 = MaybeEmpty.ofEmpty(); + assertThat(empty1).isSameInstanceAs(empty2); + } + + @Test + public void voidPropertyRemainsSingletonWhenDeserialized() throws Exception { + MaybeEmpty empty1 = MaybeEmpty.ofEmpty(); + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + // We're still compiling this with -source 6, so we can't use try-with-resources. + ObjectOutputStream dos = new ObjectOutputStream(baos); + dos.writeObject(empty1); + dos.close(); + ByteArrayInputStream bais = new ByteArrayInputStream(baos.toByteArray()); + ObjectInputStream ois = new ObjectInputStream(bais); + MaybeEmpty empty2 = (MaybeEmpty) ois.readObject(); + assertThat(empty2).isSameInstanceAs(empty1); + } + + @Test + public void voidPropertyToString() { + MaybeEmpty empty = MaybeEmpty.ofEmpty(); + assertThat(empty.toString()).isEqualTo("MaybeEmpty{empty}"); + } + + @Test + public void voidPropertyHashCodeIsIdentity() { + MaybeEmpty empty = MaybeEmpty.ofEmpty(); + assertThat(empty.hashCode()).isEqualTo(System.identityHashCode(empty)); + } + + @Test + public void voidPropertyGetterDoesNothing() { + MaybeEmpty empty = MaybeEmpty.ofEmpty(); + empty.empty(); + } + + @Test + public void voidPropertyNotEqualToNonVoid() { + MaybeEmpty empty = MaybeEmpty.ofEmpty(); + MaybeEmpty notEmpty = MaybeEmpty.ofString("foo"); + assertThat(empty).isNotEqualTo(notEmpty); + assertThat(notEmpty).isNotEqualTo(empty); + } + + @Test + public void voidPropertyWrongType() { + MaybeEmpty notEmpty = MaybeEmpty.ofString("foo"); + try { + notEmpty.empty(); + fail(); + } catch (UnsupportedOperationException e) { + assertThat(e).hasMessageThat().containsMatch("(?i:string)"); + } + } } 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 1dc3be35aa..79b6ba0bb7 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 @@ -63,6 +63,11 @@ public AutoOneOfProcessor() { super(AUTO_ONE_OF_NAME); } + @Override + boolean propertiesCanBeVoid() { + return true; + } + @Override void processType(TypeElement autoOneOfType) { if (autoOneOfType.getKind() != ElementKind.CLASS) { @@ -255,6 +260,10 @@ private void defineVarsForType( propertySet(type, propertyMethods, ImmutableListMultimap.of(), ImmutableListMultimap.of()); vars.kindGetter = kindGetter.getSimpleName().toString(); vars.kindType = TypeEncoder.encode(kindGetter.getReturnType()); + TypeElement javaIoSerializable = elementUtils().getTypeElement("java.io.Serializable"); + vars.serializable = + javaIoSerializable != null // just in case + && typeUtils().isAssignable(type.asType(), javaIoSerializable.asType()); } @Override diff --git a/value/src/main/java/com/google/auto/value/processor/AutoOneOfTemplateVars.java b/value/src/main/java/com/google/auto/value/processor/AutoOneOfTemplateVars.java index 753102b930..1e15771f20 100644 --- a/value/src/main/java/com/google/auto/value/processor/AutoOneOfTemplateVars.java +++ b/value/src/main/java/com/google/auto/value/processor/AutoOneOfTemplateVars.java @@ -44,6 +44,9 @@ class AutoOneOfTemplateVars extends AutoValueOrOneOfTemplateVars { /** Maps property names like {@code dog} to enum constants like {@code DOG}. */ Map propertyToKind; + /** True if this {@code @AutoOneOf} class is Serializable. */ + Boolean serializable; + private static final Template TEMPLATE = parsedTemplateForResource("autooneof.vm"); @Override 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 2ac21a37e3..03fb9b8ba9 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 @@ -719,11 +719,11 @@ static ImmutableSet abstractMethodsIn( * Returns the subset of property methods in the given set of abstract methods. A property method * has no arguments, is not void, and is not {@code hashCode()} or {@code toString()}. */ - static ImmutableSet propertyMethodsIn(Set abstractMethods) { + ImmutableSet propertyMethodsIn(Set abstractMethods) { ImmutableSet.Builder properties = ImmutableSet.builder(); for (ExecutableElement method : abstractMethods) { if (method.getParameters().isEmpty() - && method.getReturnType().getKind() != TypeKind.VOID + && (method.getReturnType().getKind() != TypeKind.VOID || propertiesCanBeVoid()) && objectMethodToOverride(method) == ObjectMethod.NONE) { properties.add(method); } @@ -731,6 +731,11 @@ && objectMethodToOverride(method) == ObjectMethod.NONE) { return properties.build(); } + /** True if void properties are allowed. */ + boolean propertiesCanBeVoid() { + return false; + } + /** * Checks that the return type of the given property method is allowed. Currently, this means that * it cannot be an array, unless it is a primitive array. diff --git a/value/src/main/java/com/google/auto/value/processor/autooneof.vm b/value/src/main/java/com/google/auto/value/processor/autooneof.vm index be71ad0f8e..1bf46e1a38 100644 --- a/value/src/main/java/com/google/auto/value/processor/autooneof.vm +++ b/value/src/main/java/com/google/auto/value/processor/autooneof.vm @@ -46,6 +46,14 @@ final class $generatedClass { ## Factory methods. #foreach ($p in $props) + #if ($p.type == "void") + + static $origClass$wildcardTypes $p() { + return Impl_${p}.INSTANCE; + } + + #else + ## If the @AutoOneOf type is TaskResult, then we might have here: ## static TaskResult value(V value) { ## return new Impl_value(value); @@ -55,17 +63,19 @@ final class $generatedClass { static $formalTypes $origClass$actualTypes $p($p.type $p) { - #if (!$p.kind.primitive) + #if (!$p.kind.primitive) if ($p == null) { throw new NullPointerException(); } - #end + #end return new Impl_$p$actualTypes($p); } + #end + #end #foreach ($a in $annotations) @@ -99,15 +109,63 @@ final class $generatedClass { // Implementation when the contained property is "${p}". private static final class Impl_$p$formalTypes extends Parent_$actualTypes { - private final $p.type $p; - Impl_$p($p.type $p) { - this.$p = $p; + #if ($p.type == "void") + + // There is only one instance of this class. + static final Impl_$p$wildcardTypes INSTANCE = new ## + #if ($wildcardTypes == "") Impl_$p() #else Impl_$p<>() #end; + + private Impl_$p() {} + + @Override + public void ${p.getter}() {} + + #if ($serializable) + + private Object readResolve() { + return INSTANCE; } + #end + + #if ($toString) + @Override - public $kindType ${kindGetter}() { - return ${kindType}.$propertyToKind[$p.name]; + public String toString() { + return "${simpleClassName}{$p.name}"; + } + + #end + + ## The implementations of equals and hashCode are equivalent to the ones + ## we inherit from Object. We only need to define them if they're redeclared + ## as abstract in an ancestor class. But currently we define them always. + + #if ($equals) + + @Override + public boolean equals($equalsParameterType x) { + return x == this; + } + + #end + + #if ($hashCode) + + @Override + public int hashCode() { + return System.identityHashCode(this); + } + + #end + + #else + + private final $p.type $p; + + Impl_$p($p.type $p) { + this.$p = $p; } @Override @@ -115,16 +173,16 @@ final class $generatedClass { return $p; } - #if ($toString) + #if ($toString) @Override public String toString() { return "${simpleClassName}{$p.name=" + this.$p + "}"; } - #end + #end - #if ($equals) + #if ($equals) @Override public boolean equals($equalsParameterType x) { @@ -137,17 +195,24 @@ final class $generatedClass { } } - #end + #end - #if ($hashCode) + #if ($hashCode) @Override public int hashCode() { return #hashCodeExpression($p); } + #end + #end + @Override + public $kindType ${kindGetter}() { + return ${kindType}.$propertyToKind[$p.name]; + } + } #end diff --git a/value/src/test/java/com/google/auto/value/processor/AutoOneOfCompilationTest.java b/value/src/test/java/com/google/auto/value/processor/AutoOneOfCompilationTest.java index 71d43058dc..cf5a75e978 100644 --- a/value/src/test/java/com/google/auto/value/processor/AutoOneOfCompilationTest.java +++ b/value/src/test/java/com/google/auto/value/processor/AutoOneOfCompilationTest.java @@ -44,11 +44,12 @@ public void success() { "", "@AutoOneOf(TaskResult.Kind.class)", "public abstract class TaskResult {", - " public enum Kind {VALUE, EXCEPTION}", + " public enum Kind {VALUE, EXCEPTION, EMPTY}", " public abstract Kind getKind();", "", " public abstract V value();", " public abstract Throwable exception();", + " public abstract void empty();", "", " public static TaskResult value(V value) {", " return AutoOneOf_TaskResult.value(value);", @@ -83,6 +84,10 @@ public void success() { " return new Impl_exception(exception);", " }", "", + " static TaskResult empty() {", + " return Impl_empty.INSTANCE;", + " }", + "", " // Parent class that each implementation will inherit from.", " private abstract static class Parent_ " + "extends TaskResult {", @@ -95,6 +100,11 @@ public void success() { " public Throwable exception() {", " throw new UnsupportedOperationException(getKind().toString());", " }", + "", + " @Override", + " public void empty() {", + " throw new UnsupportedOperationException(getKind().toString());", + " }", " }", "", " // Implementation when the contained property is \"value\".", @@ -107,11 +117,6 @@ public void success() { " }", "", " @Override", - " public TaskResult.Kind getKind() {", - " return TaskResult.Kind.VALUE;", - " }", - "", - " @Override", " public V value() {", " return value;", " }", @@ -136,6 +141,11 @@ public void success() { " public int hashCode() {", " return value.hashCode();", " }", + "", + " @Override", + " public TaskResult.Kind getKind() {", + " return TaskResult.Kind.VALUE;", + " }", " }", "", " // Implementation when the contained property is \"exception\".", @@ -148,11 +158,6 @@ public void success() { " }", "", " @Override", - " public TaskResult.Kind getKind() {", - " return TaskResult.Kind.EXCEPTION;", - " }", - "", - " @Override", " public Throwable exception() {", " return exception;", " }", @@ -177,6 +182,42 @@ public void success() { " public int hashCode() {", " return exception.hashCode();", " }", + "", + " @Override", + " public TaskResult.Kind getKind() {", + " return TaskResult.Kind.EXCEPTION;", + " }", + " }", + "", + " // Implementation when the contained property is \"empty\".", + " private static final class Impl_empty " + + "extends Parent_ {", + " static final Impl_empty INSTANCE = new Impl_empty<>();", + "", + " private Impl_empty() {}", + "", + " @Override", + " public void empty() {}", + "", + " @Override", + " public String toString() {", + " return \"TaskResult{empty}\";", + " }", + "", + " @Override", + " public boolean equals(Object x) {", + " return x == this;", + " }", + "", + " @Override", + " public int hashCode() {", + " return System.identityHashCode(this);", + " }", + "", + " @Override", + " public TaskResult.Kind getKind() {", + " return TaskResult.Kind.EMPTY;", + " }", " }"); Compilation compilation = javac() @@ -320,35 +361,6 @@ public void enumExtraCase() { .onLineContaining("GERBIL"); } - @Test - public void abstractVoidMethod() { - JavaFileObject javaFileObject = - JavaFileObjects.forSourceLines( - "foo.bar.Pet", - "package foo.bar;", - "", - "import com.google.auto.value.AutoOneOf;", - "", - "@AutoOneOf(Pet.Kind.class)", - "public abstract class Pet {", - " public enum Kind {", - " DOG,", - " CAT,", - " }", - " public abstract Kind getKind();", - " public abstract String dog();", - " public abstract String cat();", - " public abstract void frob();", - "}"); - Compilation compilation = - javac().withProcessors(new AutoOneOfProcessor()).compile(javaFileObject); - assertThat(compilation) - .hadWarningContaining( - "Abstract methods in @AutoOneOf classes must be non-void with no parameters") - .inFile(javaFileObject) - .onLineContaining("frob"); - } - @Test public void cantBeNullable() { JavaFileObject javaFileObject = diff --git a/value/userguide/howto.md b/value/userguide/howto.md index b075ec01ba..a04f43d162 100644 --- a/value/userguide/howto.md +++ b/value/userguide/howto.md @@ -536,9 +536,52 @@ factory method for each property, with the same name. In the example, the `STRING` value in the enum corresponds to the `string()` property and to the `AutoOneOf_StringOrInteger.string` factory method. +Properties in an `@AutoOneOf` class can be `void` to indicate that the +corresponding variant has no data. In that case, the factory method for that +variant has no parameters: + +```java +@AutoOneOf(Transform.Kind.class) +public abstract class Transform { + public enum Kind {NONE, CIRCLE_CROP, BLUR} + public abstract Kind getKind(); + + abstract void none(); + + abstract void circleCrop(); + + public abstract BlurTransformParameters blur(); + + public static Transform ofNone() { + return AutoOneOf_Transform.none(); + } + + public static Transform ofCircleCrop() { + return AutoOneOf_Transform.circleCrop(); + } + + public static Transform ofBlur(BlurTransformParmeters params) {} + return AutoOneOf_Transform.blur(params); + } +} +``` + +Here, the `NONE` and `CIRCLE_CROP` variants have no associated data but are +distinct from each other. The `BLUR` variant does have data. The `none()` +and `circleCrop()` methods are package-private; they must exist to configure +`@AutoOneOf`, but calling them is not very useful. (It does nothing if the +instance is of the correct variant, or throws an exception otherwise.) + +The `AutoOneOf_Transform.none()` and `AutoOneOf_Transform.circleCrop()` methods +return the same instance every time they are called. + +If one of the `void` variants means "none", consider using an `Optional` or +a `@Nullable Transform` instead of that variant. + Properties in an `@AutoOneOf` class cannot be null. Instead of a `StringOrInteger` with a `@Nullable String`, you probably want a -`@Nullable StringOrInteger` or an `Optional`. +`@Nullable StringOrInteger` or an `Optional`, or an empty +variant as just described. ## ... copy annotations from a class/method to the implemented class/method/field?