From d9c0593d3359b2aecf04f9b9fef51c091009d73e Mon Sep 17 00:00:00 2001 From: cushon Date: Thu, 16 May 2019 10:48:16 -0700 Subject: [PATCH 1/7] Don't compare TypeMirrors using Object#equals They don't override Object#equals and are not interned, so reference equality is not useful. This was causing unnecessary duplication of Provider fields in AutoFactories. RELNOTES=n/a ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=248752047 --- .../auto/factory/processor/FactoryWriter.java | 2 +- .../java/com/google/auto/factory/processor/Key.java | 9 ++++++--- .../expected/CheckerFrameworkNullableFactory.java | 13 +++++-------- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/factory/src/main/java/com/google/auto/factory/processor/FactoryWriter.java b/factory/src/main/java/com/google/auto/factory/processor/FactoryWriter.java index 03bd420dc1..5ceb82ebe5 100644 --- a/factory/src/main/java/com/google/auto/factory/processor/FactoryWriter.java +++ b/factory/src/main/java/com/google/auto/factory/processor/FactoryWriter.java @@ -106,7 +106,7 @@ private void addConstructorAndProviderFields( Iterator providerFields = descriptor.providers().values().iterator(); for (int argumentIndex = 1; providerFields.hasNext(); argumentIndex++) { ProviderField provider = providerFields.next(); - TypeName typeName = TypeName.get(provider.key().type()).box(); + TypeName typeName = TypeName.get(provider.key().type().get()).box(); TypeName providerType = ParameterizedTypeName.get(ClassName.get(Provider.class), typeName); factory.addField(providerType, provider.name(), PRIVATE, FINAL); if (provider.key().qualifier().isPresent()) { diff --git a/factory/src/main/java/com/google/auto/factory/processor/Key.java b/factory/src/main/java/com/google/auto/factory/processor/Key.java index e1c1c2960d..04bd4f3795 100644 --- a/factory/src/main/java/com/google/auto/factory/processor/Key.java +++ b/factory/src/main/java/com/google/auto/factory/processor/Key.java @@ -38,9 +38,11 @@ * @author Gregory Kick */ @AutoValue +// TODO(ronshapiro): reuse dagger.model.Key? abstract class Key { - abstract TypeMirror type(); + abstract Equivalence.Wrapper type(); + abstract Optional> qualifierWrapper(); Optional qualifier() { @@ -80,7 +82,8 @@ static Key create( ? MoreTypes.asDeclared(type).getTypeArguments().get(0) : boxedType(type, types); return new AutoValue_Key( - keyType, wrapOptionalInEquivalence(AnnotationMirrors.equivalence(), qualifier)); + MoreTypes.equivalence().wrap(keyType), + wrapOptionalInEquivalence(AnnotationMirrors.equivalence(), qualifier)); } /** @@ -95,7 +98,7 @@ private static TypeMirror boxedType(TypeMirror type, Types types) { @Override public String toString() { - String typeQualifiedName = MoreTypes.asTypeElement(type()).toString(); + String typeQualifiedName = MoreTypes.asTypeElement(type().get()).toString(); return qualifier().isPresent() ? qualifier().get() + "/" + typeQualifiedName : typeQualifiedName; diff --git a/factory/src/test/resources/expected/CheckerFrameworkNullableFactory.java b/factory/src/test/resources/expected/CheckerFrameworkNullableFactory.java index bea01e982b..79175c7e8e 100644 --- a/factory/src/test/resources/expected/CheckerFrameworkNullableFactory.java +++ b/factory/src/test/resources/expected/CheckerFrameworkNullableFactory.java @@ -27,24 +27,21 @@ ) final class CheckerFrameworkNullableFactory { - private final Provider providedNullableDeclProvider; - private final Provider providedNullableTypeProvider; + private final Provider java_lang_StringProvider; @Inject CheckerFrameworkNullableFactory( - Provider providedNullableDeclProvider, - Provider providedNullableTypeProvider) { - this.providedNullableDeclProvider = checkNotNull(providedNullableDeclProvider, 1); - this.providedNullableTypeProvider = checkNotNull(providedNullableTypeProvider, 2); + Provider java_lang_StringProvider) { + this.java_lang_StringProvider = checkNotNull(java_lang_StringProvider, 1); } CheckerFrameworkNullable create( @NullableDecl String nullableDecl, @NullableType String nullableType) { return new CheckerFrameworkNullable( nullableDecl, - providedNullableDeclProvider.get(), + java_lang_StringProvider.get(), nullableType, - providedNullableTypeProvider.get()); + java_lang_StringProvider.get()); } private static T checkNotNull(T reference, int argumentIndex) { From c148ee4c6cc332e1115a1fc488fae85d9eb13162 Mon Sep 17 00:00:00 2001 From: ronshapiro Date: Fri, 17 May 2019 08:13:35 -0700 Subject: [PATCH 2/7] Add an originating element for AutoFactory types RELNOTES=n/a ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=248722215 --- .../com/google/auto/factory/processor/FactoryDescriptor.java | 4 ++++ .../java/com/google/auto/factory/processor/FactoryWriter.java | 4 +++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/factory/src/main/java/com/google/auto/factory/processor/FactoryDescriptor.java b/factory/src/main/java/com/google/auto/factory/processor/FactoryDescriptor.java index 8815ce5ea6..2ffad953d5 100644 --- a/factory/src/main/java/com/google/auto/factory/processor/FactoryDescriptor.java +++ b/factory/src/main/java/com/google/auto/factory/processor/FactoryDescriptor.java @@ -56,6 +56,10 @@ public boolean matches(char c) { abstract boolean allowSubclasses(); abstract ImmutableMap providers(); + final AutoFactoryDeclaration declaration() { + return Iterables.getFirst(methodDescriptors(), null).declaration(); + } + private static class UniqueNameSet { private final Set uniqueNames = new HashSet(); diff --git a/factory/src/main/java/com/google/auto/factory/processor/FactoryWriter.java b/factory/src/main/java/com/google/auto/factory/processor/FactoryWriter.java index 5ceb82ebe5..61058470c3 100644 --- a/factory/src/main/java/com/google/auto/factory/processor/FactoryWriter.java +++ b/factory/src/main/java/com/google/auto/factory/processor/FactoryWriter.java @@ -67,7 +67,9 @@ final class FactoryWriter { void writeFactory(final FactoryDescriptor descriptor) throws IOException { String factoryName = getSimpleName(descriptor.name()).toString(); - TypeSpec.Builder factory = classBuilder(factoryName); + TypeSpec.Builder factory = + classBuilder(factoryName) + .addOriginatingElement(descriptor.declaration().targetType()); generatedAnnotationSpec( elements, sourceVersion, From 0b58d0428800f186e8430a9f51673c2fa5383744 Mon Sep 17 00:00:00 2001 From: ronshapiro Date: Fri, 17 May 2019 12:51:20 -0700 Subject: [PATCH 3/7] Make AutoFactory an isolating annotation processor in Gradle RELNOTES=Gradle: `@AutoFactory` is now an isolating annotation processor ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=248771150 --- factory/pom.xml | 6 ++++++ .../google/auto/factory/processor/AutoFactoryProcessor.java | 3 +++ 2 files changed, 9 insertions(+) diff --git a/factory/pom.xml b/factory/pom.xml index faf9bb1126..9ffc8ac308 100644 --- a/factory/pom.xml +++ b/factory/pom.xml @@ -82,6 +82,12 @@ 1.0-rc4 provided + + net.ltgt.gradle.incap + incap + 0.2 + provided + com.google.googlejavaformat google-java-format diff --git a/factory/src/main/java/com/google/auto/factory/processor/AutoFactoryProcessor.java b/factory/src/main/java/com/google/auto/factory/processor/AutoFactoryProcessor.java index 38ef55fa7b..7c84249188 100644 --- a/factory/src/main/java/com/google/auto/factory/processor/AutoFactoryProcessor.java +++ b/factory/src/main/java/com/google/auto/factory/processor/AutoFactoryProcessor.java @@ -51,12 +51,15 @@ import javax.lang.model.util.Elements; import javax.lang.model.util.Types; import javax.tools.Diagnostic.Kind; +import net.ltgt.gradle.incap.IncrementalAnnotationProcessor; +import net.ltgt.gradle.incap.IncrementalAnnotationProcessorType; /** * The annotation processor that generates factories for {@link AutoFactory} annotations. * * @author Gregory Kick */ +@IncrementalAnnotationProcessor(IncrementalAnnotationProcessorType.ISOLATING) @AutoService(Processor.class) public final class AutoFactoryProcessor extends AbstractProcessor { private FactoryDescriptorGenerator factoryDescriptorGenerator; From d5e4f555ec8cd715b1c4a920aaed435d2118b5f0 Mon Sep 17 00:00:00 2001 From: emcmanus Date: Tue, 21 May 2019 12:26:48 -0700 Subject: [PATCH 4/7] 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? From e0c631a59355c4fc86f0d052acd2d63604215fa8 Mon Sep 17 00:00:00 2001 From: emcmanus Date: Wed, 22 May 2019 10:42:15 -0700 Subject: [PATCH 5/7] Improve the logic for setter type conversion. Setter type conversion is when for example we have a property `Optional foo()` or `ImmutableList bar()` and a setter like `setFoo(String x)` or `setBar(String[] x)`. The generated setter will need to do `Optional.of(x)` or `ImmutableList.copyOf(x)`. Previously the logic to do this was split into two places: (a) where we determined whether a conversion was possible, and (b) where we generated the code. In (b) we had to guess the name of the copy method (`of` or `copyOf` for example) even though we knew it in (a). Now we arrange for the information about the method name to be copied from (a) to (b). This change will allow us to add logic to support ImmutableSortedSet.copyOfSorted and ImmutableSortedMap.copyOfSorted. RELNOTES=n/a ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=249476752 --- .../processor/BuilderMethodClassifier.java | 165 ++++++++++-------- .../auto/value/processor/BuilderSpec.java | 65 ++----- .../processor/AutoValueCompilationTest.java | 5 +- 3 files changed, 108 insertions(+), 127 deletions(-) diff --git a/value/src/main/java/com/google/auto/value/processor/BuilderMethodClassifier.java b/value/src/main/java/com/google/auto/value/processor/BuilderMethodClassifier.java index 8169f8e4da..ca20f0b87c 100644 --- a/value/src/main/java/com/google/auto/value/processor/BuilderMethodClassifier.java +++ b/value/src/main/java/com/google/auto/value/processor/BuilderMethodClassifier.java @@ -19,6 +19,7 @@ import com.google.auto.common.MoreElements; import com.google.auto.common.MoreTypes; +import com.google.auto.value.processor.BuilderSpec.PropertySetter; import com.google.auto.value.processor.PropertyBuilderClassifier.PropertyBuilder; import com.google.common.base.Equivalence; import com.google.common.collect.ImmutableBiMap; @@ -26,6 +27,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableMultimap; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; import com.google.common.collect.LinkedListMultimap; import com.google.common.collect.Multimap; import java.util.LinkedHashMap; @@ -33,10 +35,12 @@ import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.function.Function; import javax.annotation.processing.ProcessingEnvironment; import javax.lang.model.element.ExecutableElement; import javax.lang.model.element.Modifier; import javax.lang.model.element.TypeElement; +import javax.lang.model.element.VariableElement; import javax.lang.model.type.DeclaredType; import javax.lang.model.type.ExecutableType; import javax.lang.model.type.TypeKind; @@ -65,9 +69,9 @@ class BuilderMethodClassifier { private final Set buildMethods = new LinkedHashSet<>(); private final Map builderGetters = new LinkedHashMap<>(); private final Map propertyNameToPropertyBuilder = new LinkedHashMap<>(); - private final Multimap propertyNameToPrefixedSetters = + private final Multimap propertyNameToPrefixedSetters = LinkedListMultimap.create(); - private final Multimap propertyNameToUnprefixedSetters = + private final Multimap propertyNameToUnprefixedSetters = LinkedListMultimap.create(); private final EclipseHack eclipseHack; @@ -141,10 +145,10 @@ static Optional classify( * Returns a multimap from the name of a property to the methods that set it. If the property is * defined by an abstract method in the {@code @AutoValue} class called {@code foo()} or {@code * getFoo()} then the name of the property is {@code foo} and there will be an entry in the map - * where the key is {@code "foo"} and the value is a method in the builder called {@code foo} or - * {@code setFoo}. + * where the key is {@code "foo"} and the value describes a method in the builder called + * {@code foo} or {@code setFoo}. */ - ImmutableMultimap propertyNameToSetters() { + ImmutableMultimap propertyNameToSetters() { return ImmutableMultimap.copyOf( settersPrefixed ? propertyNameToPrefixedSetters : propertyNameToUnprefixedSetters); } @@ -182,7 +186,7 @@ private boolean classifyMethods( if (errorReporter.errorCount() > startErrorCount) { return false; } - Multimap propertyNameToSetter; + Multimap propertyNameToSetter; if (propertyNameToPrefixedSetters.isEmpty()) { propertyNameToSetter = propertyNameToUnprefixedSetters; this.settersPrefixed = false; @@ -192,7 +196,7 @@ private boolean classifyMethods( } else { errorReporter.reportError( "If any setter methods use the setFoo convention then all must", - propertyNameToUnprefixedSetters.values().iterator().next()); + propertyNameToUnprefixedSetters.values().iterator().next().getSetter()); return false; } getterToPropertyName.forEach( @@ -254,16 +258,15 @@ private void classifyMethod(ExecutableElement method) { * {@code @AutoValue} class; or it can be a property builder, like {@code * ImmutableList.Builder foosBuilder()} for the property defined by {@code * ImmutableList foos()} or {@code getFoos()}. - * - * @return true if the method was successfully classified, false if an error has been reported. */ - private boolean classifyMethodNoArgs(ExecutableElement method) { + private void classifyMethodNoArgs(ExecutableElement method) { String methodName = method.getSimpleName().toString(); TypeMirror returnType = builderMethodReturnType(method); ExecutableElement getter = getterNameToGetter.get(methodName); if (getter != null) { - return classifyGetter(method, getter); + classifyGetter(method, getter); + return; } if (methodName.endsWith("Builder")) { @@ -276,29 +279,25 @@ private boolean classifyMethodNoArgs(ExecutableElement method) { propertyBuilderClassifier.makePropertyBuilder(method, property); if (propertyBuilder.isPresent()) { propertyNameToPropertyBuilder.put(property, propertyBuilder.get()); - return true; - } else { - return false; } + return; } } if (TYPE_EQUIVALENCE.equivalent(returnType, autoValueClass.asType())) { buildMethods.add(method); - return true; + } else { + String error = + String.format( + "Method without arguments should be a build method returning %1$s%2$s," + + " or a getter method with the same name and type as a getter method of %1$s," + + " or fooBuilder() where foo() or getFoo() is a getter method of %1$s", + autoValueClass, typeParamsString()); + errorReporter.reportError(error, method); } - - String error = - String.format( - "Method without arguments should be a build method returning %1$s%2$s" - + " or a getter method with the same name and type as a getter method of %1$s", - autoValueClass, typeParamsString()); - errorReporter.reportError(error, method); - return false; } - private boolean classifyGetter( - ExecutableElement builderGetter, ExecutableElement originalGetter) { + private void classifyGetter(ExecutableElement builderGetter, ExecutableElement originalGetter) { String propertyName = getterToPropertyName.get(originalGetter); TypeMirror originalGetterType = getterToPropertyType.get(originalGetter); TypeMirror builderGetterType = builderMethodReturnType(builderGetter); @@ -307,7 +306,7 @@ private boolean classifyGetter( builderGetters.put( propertyName, new BuilderSpec.PropertyGetter(builderGetter, builderGetterTypeString, null)); - return true; + return; } Optionalish optional = Optionalish.createIfOptional(builderGetterType); if (optional != null) { @@ -324,7 +323,7 @@ private boolean classifyGetter( builderGetters.put( propertyName, new BuilderSpec.PropertyGetter(builderGetter, builderGetterTypeString, optional)); - return true; + return; } } String error = @@ -333,22 +332,19 @@ private boolean classifyGetter( + "or an Optional wrapping of %3$s", autoValueClass, builderGetterType, originalGetterType); errorReporter.reportError(error, builderGetter); - return false; } /** * Classifies a method given that it has one argument. Currently, a method with one argument can * only be a setter, meaning that it must look like {@code foo(T)} or {@code setFoo(T)}, where the * {@code AutoValue} class has a property called {@code foo} of type {@code T}. - * - * @return true if the method was successfully classified, false if an error has been reported. */ - private boolean classifyMethodOneArg(ExecutableElement method) { + private void classifyMethodOneArg(ExecutableElement method) { String methodName = method.getSimpleName().toString(); Map propertyNameToGetter = getterToPropertyName.inverse(); String propertyName = null; ExecutableElement valueGetter = propertyNameToGetter.get(methodName); - Multimap propertyNameToSetters = null; + Multimap propertyNameToSetters = null; if (valueGetter != null) { propertyNameToSetters = propertyNameToUnprefixedSetters; propertyName = methodName; @@ -373,18 +369,21 @@ private boolean classifyMethodOneArg(ExecutableElement method) { errorReporter.reportError( "Method does not correspond to a property of " + autoValueClass, method); checkForFailedJavaBean(method); - return false; + return; } - if (!checkSetterParameter(valueGetter, method)) { - return false; - } else if (!TYPE_EQUIVALENCE.equivalent( - builderMethodReturnType(method), builderType.asType())) { - errorReporter.reportError( - "Setter methods must return " + builderType + typeParamsString(), method); - return false; - } else { - propertyNameToSetters.put(propertyName, method); - return true; + Optional> function = getSetterFunction(valueGetter, method); + if (function.isPresent()) { + DeclaredType builderTypeMirror = MoreTypes.asDeclared(builderType.asType()); + ExecutableType methodMirror = + MoreTypes.asExecutable(typeUtils.asMemberOf(builderTypeMirror, method)); + if (TYPE_EQUIVALENCE.equivalent(methodMirror.getReturnType(), builderType.asType())) { + TypeMirror parameterType = Iterables.getOnlyElement(methodMirror.getParameterTypes()); + propertyNameToSetters.put( + propertyName, new PropertySetter(method, parameterType, function.get())); + } else { + errorReporter.reportError( + "Setter methods must return " + builderType + typeParamsString(), method); + } } } @@ -408,50 +407,53 @@ private void checkForFailedJavaBean(ExecutableElement rejectedSetter) { } /** - * Checks that the given setter method has a parameter type that is compatible with the return - * type of the given getter. Compatible means either that it is the same, or that it is a type - * that can be copied using a method like {@code ImmutableList.copyOf} or {@code Optional.of}. - * - * @return true if the types correspond, false if an error has been reported. + * Returns an {@code Optional} describing how to convert a value from the setter's parameter type + * to the getter's return type, or {@code Optional.empty()} if the conversion isn't possible. An + * error will have been reported in the latter case. We can convert if they are already the same + * type, when the returned function will be the identity; or if the setter type can be copied + * using a method like {@code ImmutableList.copyOf} or {@code Optional.of}, when the returned + * function will be something like {@code s -> "Optional.of(" + s + ")"}. */ - private boolean checkSetterParameter(ExecutableElement valueGetter, ExecutableElement setter) { + private Optional> getSetterFunction( + ExecutableElement valueGetter, ExecutableElement setter) { TypeMirror targetType = getterToPropertyType.get(valueGetter); ExecutableType finalSetter = MoreTypes.asExecutable( typeUtils.asMemberOf(MoreTypes.asDeclared(builderType.asType()), setter)); TypeMirror parameterType = finalSetter.getParameterTypes().get(0); if (typeUtils.isSameType(parameterType, targetType)) { - return true; + return Optional.of(s -> s); } // Parameter type is not equal to property type, but might be convertible with copyOf. - ImmutableList copyOfMethods = copyOfMethods(targetType); + ImmutableList copyOfMethods = copyOfMethods(targetType, setter); if (!copyOfMethods.isEmpty()) { - return canMakeCopyUsing(copyOfMethods, valueGetter, setter); + return getConvertingSetterFunction(copyOfMethods, valueGetter, setter); } String error = String.format( "Parameter type %s of setter method should be %s to match getter %s.%s", parameterType, targetType, autoValueClass, valueGetter.getSimpleName()); errorReporter.reportError(error, setter); - return false; + return Optional.empty(); } /** - * Checks that the given setter method has a parameter type that can be copied to the return type - * of the given getter using one of the given {@code copyOf} methods. - * - * @return true if the copy can be made, false if an error has been reported. + * Returns an {@code Optional} describing how to convert a value from the setter's parameter type + * to the getter's return type using one of the given methods, or {@code Optional.empty()} if the + * conversion isn't possible. An error will have been reported in the latter case. */ - private boolean canMakeCopyUsing( + private Optional> getConvertingSetterFunction( ImmutableList copyOfMethods, ExecutableElement valueGetter, ExecutableElement setter) { DeclaredType targetType = MoreTypes.asDeclared(getterToPropertyType.get(valueGetter)); TypeMirror parameterType = setter.getParameters().get(0).asType(); for (ExecutableElement copyOfMethod : copyOfMethods) { - if (canMakeCopyUsing(copyOfMethod, targetType, parameterType)) { - return true; + Optional> function = + getConvertingSetterFunction(copyOfMethod, targetType, parameterType); + if (function.isPresent()) { + return function; } } String targetTypeSimpleName = targetType.asElement().getSimpleName().toString(); @@ -466,15 +468,16 @@ private boolean canMakeCopyUsing( copyOfMethods.get(0).getSimpleName(), targetType); errorReporter.reportError(error, setter); - return false; + return Optional.empty(); } /** - * Returns true if {@code copyOfMethod} can be used to copy the {@code parameterType} to the - * {@code targetType}. For example, we might have a property of type {@code ImmutableSet} and - * our setter has a parameter of type {@code Set}. Can we use {@code - * ImmutableSet ImmutableSet.copyOf(Collection)} to set the property? What about - * {@code ImmutableSet ImmutableSet.copyOf(E[])}? + * Returns an {@code Optional} containing a function to use {@code copyOfMethod} to copy the + * {@code parameterType} to the {@code targetType}, or {@code Optional.empty()} if the method + * can't be used. For example, we might have a property of type {@code ImmutableSet} and our + * setter has a parameter of type {@code Set}. Can we use {@code ImmutableSet + * ImmutableSet.copyOf(Collection)} to set the property? What about {@code + * ImmutableSet ImmutableSet.copyOf(E[])}? * *

The example here is deliberately complicated, in that it has a type parameter of its own, * presumably because the {@code @AutoValue} class is {@code Foo}. One subtle point is that the @@ -488,10 +491,10 @@ private boolean canMakeCopyUsing( * examples. * @param targetType the type of the property to be set, {@code ImmutableSet} in the example. * @param parameterType the type of the setter parameter, {@code Set} in the example. - * @return true if the method can be used to make the right type of result from the given - * parameter type. + * @return a function that maps a string parameter to a method call using that parameter. For + * example it might map {@code foo} to {@code ImmutableList.copyOf(foo)}. */ - private boolean canMakeCopyUsing( + private Optional> getConvertingSetterFunction( ExecutableElement copyOfMethod, DeclaredType targetType, TypeMirror parameterType) { // We have a parameter type, for example Set, and we want to know if it can be // passed to the given copyOf method, which might for example be one of these methods from @@ -504,8 +507,12 @@ private boolean canMakeCopyUsing( // are static. So even if our target type is ImmutableSet, if we ask what the type of // copyOf is in ImmutableSet it will still tell us Optional (T). // Instead, we do the variable substitutions ourselves. - return TypeVariables.canAssignStaticMethodResult( - copyOfMethod, parameterType, targetType, typeUtils); + if (TypeVariables.canAssignStaticMethodResult( + copyOfMethod, parameterType, targetType, typeUtils)) { + String method = TypeEncoder.encodeRaw(targetType) + "." + copyOfMethod.getSimpleName(); + return Optional.of(s -> method + "(" + s + ")"); + } + return Optional.empty(); } /** @@ -514,11 +521,23 @@ private boolean canMakeCopyUsing( * least one such method, but we will also accept other classes with an appropriate method, such * as {@link java.util.EnumSet}. */ - private ImmutableList copyOfMethods(TypeMirror targetType) { + private ImmutableList copyOfMethods( + TypeMirror targetType, ExecutableElement setter) { if (!targetType.getKind().equals(TypeKind.DECLARED)) { return ImmutableList.of(); } - String copyOf = Optionalish.isOptional(targetType) ? "of" : "copyOf"; + String copyOf; + Optionalish optionalish = Optionalish.createIfOptional(targetType); + if (optionalish == null) { + copyOf = "copyOf"; + } else { + VariableElement parameterElement = Iterables.getOnlyElement(setter.getParameters()); + boolean nullable = + AutoValueOrOneOfProcessor.nullableAnnotationFor( + parameterElement, parameterElement.asType()) + .isPresent(); + copyOf = nullable ? optionalish.ofNullable() : "of"; + } TypeElement targetTypeElement = MoreElements.asType(typeUtils.asElement(targetType)); ImmutableList.Builder copyOfMethods = ImmutableList.builder(); for (ExecutableElement method : 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 a9c464a81d..9cebcad16b 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 @@ -25,13 +25,13 @@ import com.google.auto.value.processor.AutoValueOrOneOfProcessor.Property; import com.google.common.collect.ImmutableBiMap; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableMultimap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import java.util.LinkedHashSet; import java.util.List; import java.util.Optional; import java.util.Set; +import java.util.function.Function; import javax.annotation.processing.ProcessingEnvironment; import javax.lang.model.element.Element; import javax.lang.model.element.ElementKind; @@ -41,7 +41,6 @@ import javax.lang.model.element.TypeParameterElement; import javax.lang.model.element.VariableElement; import javax.lang.model.type.DeclaredType; -import javax.lang.model.type.ExecutableType; import javax.lang.model.type.TypeKind; import javax.lang.model.type.TypeMirror; import javax.lang.model.util.ElementFilter; @@ -214,23 +213,7 @@ void defineVars( vars.builderActualTypes = TypeSimplifier.actualTypeParametersString(builderTypeElement); vars.buildMethod = Optional.of(new SimpleMethod(buildMethod)); vars.builderGetters = classifier.builderGetters(); - - DeclaredType builderTypeMirror = MoreTypes.asDeclared(builderTypeElement.asType()); - ImmutableMultimap.Builder setterBuilder = ImmutableMultimap.builder(); - classifier.propertyNameToSetters().forEach( - (property, setter) -> { - ExecutableElement getter = getterToPropertyName.inverse().get(property); - // Get the effective parameter type, which might need asMemberOf in cases where the method - // is inherited from a generic ancestor type and references type parameters. - ExecutableType setterMirror = MoreTypes.asExecutable( - processingEnv.getTypeUtils().asMemberOf(builderTypeMirror, setter)); - TypeMirror parameterType = Iterables.getOnlyElement(setterMirror.getParameterTypes()); - TypeMirror propertyType = getterToPropertyType.get(getter); - PropertySetter propertySetter = new PropertySetter( - setter, parameterType, propertyType, processingEnv.getTypeUtils()); - setterBuilder.put(property, propertySetter); - }); - vars.builderSetters = setterBuilder.build(); + vars.builderSetters = classifier.propertyNameToSetters(); vars.builderPropertyBuilders = ImmutableMap.copyOf(classifier.propertyNameToPropertyBuilder()); @@ -304,18 +287,18 @@ public Optionalish getOptional() { * with a type that can be copied to {@code T} through {@code Optional.of}. */ public static class PropertySetter { + private final ExecutableElement setter; private final String access; private final String name; private final String parameterTypeString; private final boolean primitiveParameter; private final String nullableAnnotation; - private final String copyOf; + private final Function copyFunction; PropertySetter( - ExecutableElement setter, - TypeMirror parameterType, - TypeMirror propertyType, - Types typeUtils) { + ExecutableElement setter, TypeMirror parameterType, Function copyFunction) { + this.setter = setter; + this.copyFunction = copyFunction; this.access = SimpleMethod.access(setter); this.name = setter.getSimpleName().toString(); primitiveParameter = parameterType.getKind().isPrimitive(); @@ -324,8 +307,10 @@ public static class PropertySetter { Optional maybeNullable = AutoValueOrOneOfProcessor.nullableAnnotationFor(parameterElement, parameterType); this.nullableAnnotation = maybeNullable.orElse(""); - boolean nullable = maybeNullable.isPresent(); - this.copyOf = copyOfString(propertyType, parameterType, typeUtils, nullable); + } + + ExecutableElement getSetter() { + return setter; } private static String parameterTypeString(ExecutableElement setter, TypeMirror parameterType) { @@ -342,26 +327,6 @@ private static String parameterTypeString(ExecutableElement setter, TypeMirror p } } - private String copyOfString( - TypeMirror propertyType, TypeMirror parameterType, Types typeUtils, boolean nullable) { - boolean sameType = typeUtils.isAssignable(parameterType, propertyType); - if (sameType) { - return null; - } - TypeMirror erasedPropertyType = typeUtils.erasure(propertyType); - 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; } @@ -383,14 +348,10 @@ public String getNullableAnnotation() { } public String copy(AutoValueProcessor.Property property) { - if (copyOf == null) { - return property.toString(); - } - - String copy = String.format(copyOf, property); + String copy = copyFunction.apply(property.toString()); // Add a null guard only in cases where we are using copyOf and the property is @Nullable. - if (property.isNullable()) { + if (property.isNullable() && !copy.equals(property.toString())) { copy = String.format("(%s == null ? null : %s)", property, copy); } diff --git a/value/src/test/java/com/google/auto/value/processor/AutoValueCompilationTest.java b/value/src/test/java/com/google/auto/value/processor/AutoValueCompilationTest.java index a3893ed1a4..32edfac4a1 100644 --- a/value/src/test/java/com/google/auto/value/processor/AutoValueCompilationTest.java +++ b/value/src/test/java/com/google/auto/value/processor/AutoValueCompilationTest.java @@ -2154,8 +2154,9 @@ public void autoValueBuilderAlienMethod0() { .compile(javaFileObject); assertThat(compilation) .hadErrorContaining( - "Method without arguments should be a build method returning foo.bar.Baz" - + " or a getter method with the same name and type as a getter method of foo.bar.Baz") + "Method without arguments should be a build method returning foo.bar.Baz, or a getter" + + " method with the same name and type as a getter method of foo.bar.Baz, or" + + " fooBuilder() where foo() or getFoo() is a getter method of foo.bar.Baz") .inFile(javaFileObject) .onLine(12); } From 7564c4c6a5c5a6dee14fb2b3771d21fc9de2b151 Mon Sep 17 00:00:00 2001 From: emcmanus Date: Thu, 23 May 2019 06:39:58 -0700 Subject: [PATCH 6/7] Better support for ImmutableSortedSet and ImmutableSortedMap. Use .copyOfSorted when setting from a SortedSet or SortedMap. Use .naturalOrder when constructing a builder. Fixes https://github.com/google/auto/issues/666. RELNOTES=Use ImmutableSortedSet.copyOfSorted and .naturalOrder where appropriate. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=249632356 --- .../com/google/auto/value/AutoValueTest.java | 91 ++++++++++++++++++- .../processor/BuilderMethodClassifier.java | 20 ++-- .../processor/PropertyBuilderClassifier.java | 7 +- 3 files changed, 101 insertions(+), 17 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 233ab537c8..567a1e2263 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 @@ -31,6 +31,8 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableSortedMap; +import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.ImmutableTable; import com.google.common.testing.EqualsTester; import com.google.common.testing.SerializableTester; @@ -53,7 +55,12 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.NavigableMap; +import java.util.NavigableSet; import java.util.NoSuchElementException; +import java.util.SortedMap; +import java.util.TreeMap; +import java.util.TreeSet; import javax.annotation.Nullable; import org.junit.BeforeClass; import org.junit.Test; @@ -2220,15 +2227,89 @@ public void testBuilderWithCopyingSetters() { BuilderWithCopyingSetters.Builder builder = BuilderWithCopyingSetters.builder(23); BuilderWithCopyingSetters a = builder.setThings(ImmutableSet.of(1, 2)).build(); - assertEquals(ImmutableSet.of(1, 2), a.things()); - assertEquals(ImmutableList.of(17, 23.0), a.numbers()); - assertEquals(ImmutableMap.of("foo", 23), a.map()); + assertThat(a.things()).containsExactly(1, 2); + assertThat(a.numbers()).containsExactly(17, 23.0).inOrder(); + assertThat(a.map()).containsExactly("foo", 23); BuilderWithCopyingSetters b = builder.setThings(Arrays.asList(1, 2)).build(); - assertEquals(a, b); + assertThat(b).isEqualTo(a); BuilderWithCopyingSetters c = builder.setThings(1, 2).build(); - assertEquals(a, c); + assertThat(c).isEqualTo(a); + } + + @AutoValue + public abstract static class BuilderWithImmutableSorted> { + public abstract ImmutableSortedSet sortedSet(); + + public abstract ImmutableSortedMap sortedMap(); + + public static > Builder builder() { + return new AutoValue_AutoValueTest_BuilderWithImmutableSorted.Builder() + .setSortedSet(new TreeSet()) + .setSortedMap(new TreeMap()); + } + + @AutoValue.Builder + public interface Builder> { + @SuppressWarnings("unchecked") + Builder setSortedSet(T... x); + + Builder setSortedSet(NavigableSet x); + + ImmutableSortedSet.Builder sortedSetBuilder(); + + Builder setSortedMap(SortedMap x); + + Builder setSortedMap(NavigableMap x); + + ImmutableSortedMap.Builder sortedMapBuilder(); + + BuilderWithImmutableSorted build(); + } + } + + @Test + public void testBuilderWithImmutableSorted_Varargs() { + BuilderWithImmutableSorted x = + BuilderWithImmutableSorted.builder().setSortedSet("foo", "bar", "baz").build(); + assertThat(x.sortedSet()).containsExactly("bar", "baz", "foo").inOrder(); + } + + @Test + public void testBuilderWithImmutableSorted_SetSet() { + BuilderWithImmutableSorted x = + BuilderWithImmutableSorted.builder() + .setSortedSet(new TreeSet(String.CASE_INSENSITIVE_ORDER)) + .build(); + assertThat(x.sortedSet().comparator()).isEqualTo(String.CASE_INSENSITIVE_ORDER); + } + + @Test + public void testBuilderWithImmutableSorted_SetMap() { + BuilderWithImmutableSorted x = + BuilderWithImmutableSorted.builder() + .setSortedMap(new TreeMap(String.CASE_INSENSITIVE_ORDER)) + .build(); + assertThat(x.sortedMap().comparator()).isEqualTo(String.CASE_INSENSITIVE_ORDER); + } + + @Test + public void testBuilderWithImmutableSorted_SetCollectionBuilder() { + BuilderWithImmutableSorted.Builder builder = + BuilderWithImmutableSorted.builder(); + builder.sortedSetBuilder().add("is", "ea", "id"); + BuilderWithImmutableSorted x = builder.build(); + assertThat(x.sortedSet()).containsExactly("ea", "id", "is").inOrder(); + } + + @Test + public void testBuilderWithImmutableSorted_MapCollectionBuilder() { + BuilderWithImmutableSorted.Builder builder = + BuilderWithImmutableSorted.builder(); + builder.sortedMapBuilder().put("two", 2).put("one", 1); + BuilderWithImmutableSorted x = builder.build(); + assertThat(x.sortedMap()).containsExactly("one", 1, "two", 2).inOrder(); } @AutoValue diff --git a/value/src/main/java/com/google/auto/value/processor/BuilderMethodClassifier.java b/value/src/main/java/com/google/auto/value/processor/BuilderMethodClassifier.java index ca20f0b87c..94e26ef0ab 100644 --- a/value/src/main/java/com/google/auto/value/processor/BuilderMethodClassifier.java +++ b/value/src/main/java/com/google/auto/value/processor/BuilderMethodClassifier.java @@ -526,26 +526,28 @@ private ImmutableList copyOfMethods( if (!targetType.getKind().equals(TypeKind.DECLARED)) { return ImmutableList.of(); } - String copyOf; + ImmutableSet copyOfNames; Optionalish optionalish = Optionalish.createIfOptional(targetType); if (optionalish == null) { - copyOf = "copyOf"; + copyOfNames = ImmutableSet.of("copyOfSorted", "copyOf"); } else { VariableElement parameterElement = Iterables.getOnlyElement(setter.getParameters()); boolean nullable = AutoValueOrOneOfProcessor.nullableAnnotationFor( parameterElement, parameterElement.asType()) .isPresent(); - copyOf = nullable ? optionalish.ofNullable() : "of"; + copyOfNames = ImmutableSet.of(nullable ? optionalish.ofNullable() : "of"); } TypeElement targetTypeElement = MoreElements.asType(typeUtils.asElement(targetType)); ImmutableList.Builder copyOfMethods = ImmutableList.builder(); - for (ExecutableElement method : - ElementFilter.methodsIn(targetTypeElement.getEnclosedElements())) { - if (method.getSimpleName().contentEquals(copyOf) - && method.getParameters().size() == 1 - && method.getModifiers().contains(Modifier.STATIC)) { - copyOfMethods.add(method); + for (String copyOfName : copyOfNames) { + for (ExecutableElement method : + ElementFilter.methodsIn(targetTypeElement.getEnclosedElements())) { + if (method.getSimpleName().contentEquals(copyOfName) + && method.getParameters().size() == 1 + && method.getModifiers().contains(Modifier.STATIC)) { + copyOfMethods.add(method); + } } } return copyOfMethods.build(); diff --git a/value/src/main/java/com/google/auto/value/processor/PropertyBuilderClassifier.java b/value/src/main/java/com/google/auto/value/processor/PropertyBuilderClassifier.java index 520a8be507..41080730d5 100644 --- a/value/src/main/java/com/google/auto/value/processor/PropertyBuilderClassifier.java +++ b/value/src/main/java/com/google/auto/value/processor/PropertyBuilderClassifier.java @@ -186,7 +186,8 @@ public String getCopyAll() { // (1) It must have an instance method called `build()` that returns `Bar`. If the type of // `bar()` is `Bar` then the type of `build()` must be `Bar`. // (2) `BarBuilder` must have a public no-arg constructor, or `Bar` must have a static method - // `builder()` or `newBuilder()` that returns `BarBuilder`. + // `naturalOrder(), `builder()`, or `newBuilder()` that returns `BarBuilder`. The + // `naturalOrder()` case is specifically for ImmutableSortedSet and ImmutableSortedMap. // (3) `Bar` must have an instance method `BarBuilder toBuilder()`, or `BarBuilder` must be a // Guava immutable builder like `ImmutableSet.Builder`. (See TODO below for relaxing the // requirement on having a `toBuilder()`. @@ -324,10 +325,10 @@ Optional makePropertyBuilder(ExecutableElement method, String p } private static final ImmutableSet BUILDER_METHOD_NAMES = - ImmutableSet.of("builder", "newBuilder"); + ImmutableSet.of("naturalOrder", "builder", "newBuilder"); // (2) `BarBuilder must have a public no-arg constructor, or `Bar` must have a visible static - // method `builder()` or `newBuilder()` that returns `BarBuilder`. + // method `naturalOrder(), `builder()`, or `newBuilder()` that returns `BarBuilder`. private Optional builderMaker( Map barNoArgMethods, TypeElement barBuilderTypeElement) { for (String builderMethodName : BUILDER_METHOD_NAMES) { From 6c9bd9fdbc46a4c7ac01b4d2accd7571dd144338 Mon Sep 17 00:00:00 2001 From: cpovirk Date: Fri, 24 May 2019 11:38:17 -0700 Subject: [PATCH 7/7] Migrate Correspondence subclasses to instead call Correspondence.from. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=249869914 --- .../common/BasicAnnotationProcessorTest.java | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/common/src/test/java/com/google/auto/common/BasicAnnotationProcessorTest.java b/common/src/test/java/com/google/auto/common/BasicAnnotationProcessorTest.java index d617258d36..0cf1075d08 100644 --- a/common/src/test/java/com/google/auto/common/BasicAnnotationProcessorTest.java +++ b/common/src/test/java/com/google/auto/common/BasicAnnotationProcessorTest.java @@ -307,18 +307,10 @@ public void properlyDefersProcessing_rejectsElement() { private static Correspondence, SetMultimap> setMultimapValuesByString() { - return new Correspondence, SetMultimap>() { - @Override - public boolean compare(SetMultimap actual, SetMultimap expected) { - return ImmutableSetMultimap.copyOf(transformValues(actual, Object::toString)) - .equals(expected); - } - - @Override - public String toString() { - return "is equivalent comparing multimap values by `toString()` to"; - } - }; + return Correspondence.from( + (actual, expected) -> + ImmutableSetMultimap.copyOf(transformValues(actual, Object::toString)).equals(expected), + "is equivalent comparing multimap values by `toString()` to"); } @Test public void reportsMissingType() {