From 668c2dcff5f1ba1c0c41ecf33ef57c5a6c016220 Mon Sep 17 00:00:00 2001 From: Alasia Delgado Date: Mon, 13 Nov 2023 15:05:15 -0800 Subject: [PATCH] AutoValue support for very large classes. This change makes it so that if the AutoValue class defines a Builder method, the builder will be passed to the constructor. Effectively working around the java 255 method-arg limit. This is instead of defining a constructor where every field in the autovalue is an argument in it. This change will only apply to autovalues that don't have extensions: in which case the constructor is private and we can be assured that updating it shouldn't be problematic for current cases. RELNOTES=Support for AutoValues with +255 properties. PiperOrigin-RevId: 582096324 --- .../AutoValueOrBuilderTemplateVars.java | 6 ++ .../value/processor/AutoValueProcessor.java | 5 ++ .../google/auto/value/processor/autovalue.vm | 41 ++++++----- .../google/auto/value/processor/builder.vm | 16 +++-- .../processor/AutoValueCompilationTest.java | 69 +++++++------------ 5 files changed, 72 insertions(+), 65 deletions(-) diff --git a/value/src/main/java/com/google/auto/value/processor/AutoValueOrBuilderTemplateVars.java b/value/src/main/java/com/google/auto/value/processor/AutoValueOrBuilderTemplateVars.java index b2aad04109..664f0a414a 100644 --- a/value/src/main/java/com/google/auto/value/processor/AutoValueOrBuilderTemplateVars.java +++ b/value/src/main/java/com/google/auto/value/processor/AutoValueOrBuilderTemplateVars.java @@ -153,4 +153,10 @@ abstract class AutoValueOrBuilderTemplateVars extends AutoValueishTemplateVars { * subclass, followed by a space if they are not empty. */ String builderClassModifiers = ""; + + /** + * Set if the code should generate a constructor that takes a Builder as an argument instead of + * the usual per-field constructor. + */ + Boolean shouldGenerateBuilderConstructor = false; } 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 0206dcd725..a4800d3a8f 100644 --- a/value/src/main/java/com/google/auto/value/processor/AutoValueProcessor.java +++ b/value/src/main/java/com/google/auto/value/processor/AutoValueProcessor.java @@ -45,6 +45,7 @@ import javax.annotation.processing.ProcessingEnvironment; import javax.annotation.processing.Processor; import javax.annotation.processing.SupportedAnnotationTypes; +import javax.lang.model.SourceVersion; import javax.lang.model.element.AnnotationMirror; import javax.lang.model.element.ExecutableElement; import javax.lang.model.element.TypeElement; @@ -289,6 +290,10 @@ void processType(TypeElement type) { vars.subclass = TypeSimplifier.simpleNameOf(subclass); vars.finalSubclass = finalSubclass; vars.isFinal = (subclassDepth == 0); + vars.shouldGenerateBuilderConstructor = + applicableExtensions.isEmpty() + && builder.isPresent() + && processingEnv.getSourceVersion().compareTo(SourceVersion.RELEASE_8) > 0; vars.modifiers = vars.isFinal ? "final " : "abstract "; vars.builderClassModifiers = consumedBuilderMethods.isEmpty() 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 ec0910c2a0..33ab6b13b8 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 @@ -62,31 +62,40 @@ ${modifiers}class $subclass$formalTypes extends $origClass$actualTypes { #if ($isFinal && $builderTypeName != "") private ## #end +#if ($shouldGenerateBuilderConstructor) + $subclass(${builderName}${actualTypes} builder) { + #foreach ($p in $props) + #if ($p.nullable || $p.kind.primitive) + this.$p = builder.$p; + #else + this.$p = `java.util.Objects`.requireNonNull(builder.$p); + #end + #end + } +#else $subclass( -#foreach ($p in $props) - - ${p.nullableAnnotation}$p.type $p #if ($foreach.hasNext) , #end -#end ) { -#foreach ($p in $props) - #if (!$p.kind.primitive && !$p.nullable && ($builderTypeName == "" || !$isFinal)) - ## We don't need a null check if the type is primitive or @Nullable. We also don't need it - ## if there is a builder, since the build() method will check for us. However, if there is a - ## builder but there are also extensions (!$isFinal) then we can't omit the null check because - ## the constructor is called from the extension code. + #foreach ($p in $props) + ${p.nullableAnnotation}$p.type $p #if ($foreach.hasNext) , #end + #end) { + #foreach ($p in $props) + #if (!$p.kind.primitive && !$p.nullable && ($builderTypeName == "" || !$isFinal)) + ## We don't need a null check if the type is primitive or @Nullable. We also don't need it + ## if there is a builder, since the build() method will check for us. However, if there is a + ## builder but there are also extensions (!$isFinal) then we can't omit the null check because + ## the constructor is called from the extension code. - #if ($identifiers) + #if ($identifiers) if ($p == null) { throw new NullPointerException("Null $p.name"); } - #else + #else `java.util.Objects`.requireNonNull($p); + #end #end - - #end - this.$p = $p; -#end + #end } +#end ## Property getters diff --git a/value/src/main/java/com/google/auto/value/processor/builder.vm b/value/src/main/java/com/google/auto/value/processor/builder.vm index 30989b7843..e5313386c5 100644 --- a/value/src/main/java/com/google/auto/value/processor/builder.vm +++ b/value/src/main/java/com/google/auto/value/processor/builder.vm @@ -274,11 +274,15 @@ ${builderClassModifiers}class ${builderName}${builderFormalTypes} ## #end - #if ($builtType != "void") return #end ${build}( -#foreach ($p in $props) - - this.$p #if ($foreach.hasNext) , #end -#end - $builderRequiredProperties.defaultedBitmaskParameters ); + #if ($builtType != "void") return #end + #if ($shouldGenerateBuilderConstructor) + ${build}(this); + #else + ${build}( + #foreach ($p in $props) + this.$p #if ($foreach.hasNext) , #end + #end + $builderRequiredProperties.defaultedBitmaskParameters ); + #end } } 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 e4224913cf..fa5fcfc0e7 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 @@ -61,6 +61,11 @@ public class AutoValueCompilationTest { private boolean typeAnnotationsWork = Double.parseDouble(JAVA_SPECIFICATION_VERSION.value()) >= 9.0; + // Sadly we can't rely on JDK 8 to handle accessing the private methods of the builder. + // So skip the test unless we are on at least JDK 9. + private boolean constructorUsesBuilderFields = + Double.parseDouble(JAVA_SPECIFICATION_VERSION.value()) >= 9.0; + @Test public void simpleSuccess() { // Positive test case that ensures we generate the expected code for at least one case. @@ -1066,6 +1071,7 @@ public void nullablePrimitive() { @Test public void correctBuilder() { + assume().that(constructorUsesBuilderFields).isTrue(); JavaFileObject javaFileObject = JavaFileObjects.forSourceLines( "foo.bar.Baz", @@ -1155,6 +1161,7 @@ public void correctBuilder() { "import java.util.Arrays;", "import java.util.List;", "import java.util.Map;", + "import java.util.Objects;", sorted( GeneratedImport.importGeneratedAnnotationType(), "import javax.annotation.Nullable;"), @@ -1170,21 +1177,14 @@ public void correctBuilder() { " private final Optional anOptionalString;", " private final NestedAutoValue aNestedAutoValue;", "", - " private AutoValue_Baz(", - " int anInt,", - " byte[] aByteArray,", - " @Nullable int[] aNullableIntArray,", - " List aList,", - " ImmutableMap anImmutableMap,", - " Optional anOptionalString,", - " NestedAutoValue aNestedAutoValue) {", - " this.anInt = anInt;", - " this.aByteArray = aByteArray;", - " this.aNullableIntArray = aNullableIntArray;", - " this.aList = aList;", - " this.anImmutableMap = anImmutableMap;", - " this.anOptionalString = anOptionalString;", - " this.aNestedAutoValue = aNestedAutoValue;", + " private AutoValue_Baz(Builder builder) {", + " this.anInt = builder.anInt;", + " this.aByteArray = Objects.requireNonNull(builder.aByteArray);", + " this.aNullableIntArray = builder.aNullableIntArray;", + " this.aList = Objects.requireNonNull(builder.aList);", + " this.anImmutableMap = Objects.requireNonNull(builder.anImmutableMap);", + " this.anOptionalString = Objects.requireNonNull(builder.anOptionalString);", + " this.aNestedAutoValue = Objects.requireNonNull(builder.aNestedAutoValue);", " }", "", " @Override public int anInt() {", @@ -1439,14 +1439,7 @@ public void correctBuilder() { " }", " throw new IllegalStateException(\"Missing required properties:\" + missing);", " }", - " return new AutoValue_Baz(", - " this.anInt,", - " this.aByteArray,", - " this.aNullableIntArray,", - " this.aList,", - " this.anImmutableMap,", - " this.anOptionalString,", - " this.aNestedAutoValue);", + " return new AutoValue_Baz(this);", " }", " }", "}"); @@ -1465,6 +1458,7 @@ public void correctBuilder() { @Test public void builderWithNullableTypeAnnotation() { assume().that(typeAnnotationsWork).isTrue(); + assume().that(constructorUsesBuilderFields).isTrue(); JavaFileObject javaFileObject = JavaFileObjects.forSourceLines( "foo.bar.Baz", @@ -1518,6 +1512,7 @@ public void builderWithNullableTypeAnnotation() { "import java.util.Arrays;", "import java.util.List;", "import java.util.Map;", + "import java.util.Objects;", sorted( GeneratedImport.importGeneratedAnnotationType(), "import org.checkerframework.checker.nullness.qual.Nullable;"), @@ -1531,19 +1526,13 @@ public void builderWithNullableTypeAnnotation() { " private final ImmutableMap anImmutableMap;", " private final Optional anOptionalString;", "", - " private AutoValue_Baz(", - " int anInt,", - " byte[] aByteArray,", - " int @Nullable [] aNullableIntArray,", - " List aList,", - " ImmutableMap anImmutableMap,", - " Optional anOptionalString) {", - " this.anInt = anInt;", - " this.aByteArray = aByteArray;", - " this.aNullableIntArray = aNullableIntArray;", - " this.aList = aList;", - " this.anImmutableMap = anImmutableMap;", - " this.anOptionalString = anOptionalString;", + " private AutoValue_Baz(Builder builder) {", + " this.anInt = builder.anInt;", + " this.aByteArray = Objects.requireNonNull(builder.aByteArray);", + " this.aNullableIntArray = builder.aNullableIntArray;", + " this.aList = Objects.requireNonNull(builder.aList);", + " this.anImmutableMap = Objects.requireNonNull(builder.anImmutableMap);", + " this.anOptionalString = Objects.requireNonNull(builder.anOptionalString);", " }", "", " @Override public int anInt() {", @@ -1733,13 +1722,7 @@ public void builderWithNullableTypeAnnotation() { " }", " throw new IllegalStateException(\"Missing required properties:\" + missing);", " }", - " return new AutoValue_Baz(", - " this.anInt,", - " this.aByteArray,", - " this.aNullableIntArray,", - " this.aList,", - " this.anImmutableMap,", - " this.anOptionalString);", + " return new AutoValue_Baz(this);", " }", " }", "}");