[Feature] Non-boxing 'addAll' Methods for Primitive Types (#2389)

changelog/@unreleased/pr-2389.v2.yml

@@ -0,0 +1,5 @@
+type: feature
+feature:
+  description: '[Feature] Non-boxing ''addAll'' Methods for Primitive Types'
+  links:
+  - https://github.com/palantir/conjure-java/pull/2389

conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderAuxiliarySettersUtils.java

@@ -17,6 +17,7 @@
 package com.palantir.conjure.java.types;
 
 import com.palantir.conjure.java.ConjureAnnotations;
+import com.palantir.conjure.java.lib.SafeLong;
 import com.palantir.conjure.java.types.BeanGenerator.EnrichedField;
 import com.palantir.conjure.java.util.Javadoc;
 import com.palantir.conjure.java.util.Primitives;
@@ -28,6 +29,7 @@
 import com.palantir.conjure.spec.PrimitiveType;
 import com.palantir.conjure.spec.Type;
 import com.palantir.conjure.visitor.TypeVisitor;
+import com.palantir.javapoet.ArrayTypeName;
 import com.palantir.javapoet.ClassName;
 import com.palantir.javapoet.CodeBlock;
 import com.palantir.javapoet.FieldSpec;
@@ -39,11 +41,43 @@
 import javax.lang.model.element.Modifier;
 import org.apache.commons.lang3.StringUtils;
 
-public final class BeanBuilderAuxiliarySettersUtils {
+final class BeanBuilderAuxiliarySettersUtils {
 
     private BeanBuilderAuxiliarySettersUtils() {}
 
-    public static MethodSpec.Builder createCollectionSetterBuilder(
+    static MethodSpec.Builder createPrimitiveCollectionSetterBuilder(
+            EnrichedField enriched, TypeMapper typeMapper, ClassName returnClass, SafetyEvaluator safetyEvaluator) {
+        FieldSpec field = enriched.poetSpec();
+        FieldDefinition definition = enriched.conjureDef();
+        TypeName innerTypeName = extractInnerTypeFromList(definition, typeMapper, safetyEvaluator);
+
+        return MethodSpec.methodBuilder("addAll" + StringUtils.capitalize(field.name()))
+                .addJavadoc(Javadoc.render(definition.getDocs(), definition.getDeprecated())
+                        .map(rendered -> CodeBlock.of("$L", rendered))
+                        .orElseGet(() -> CodeBlock.builder().build()))
+                .addAnnotations(ConjureAnnotations.deprecation(definition.getDeprecated()))
+                .addModifiers(Modifier.PUBLIC)
+                // Forces the array argument to instead be variadic
+                .varargs()
+                .addParameter(Parameters.nonnullParameter(ArrayTypeName.of(innerTypeName), field.name()))
+                .returns(returnClass);
+    }
+
+    private static TypeName extractInnerTypeFromList(
+            FieldDefinition conjureDef, TypeMapper typeMapper, SafetyEvaluator safetyEvaluator) {
+        Type innerType = conjureDef.getType().accept(TypeVisitor.LIST).getItemType();
+        TypeName boxedTypeName = typeMapper.getClassName(innerType);
+
+        // SafeLong is just a special case of long
+        if (boxedTypeName.equals(ClassName.get(SafeLong.class))) {
+            return ConjureAnnotations.withSafety(TypeName.LONG, safetyEvaluator.getUsageTimeSafety(conjureDef));
+        } else {
+            return ConjureAnnotations.withSafety(
+                    Primitives.unbox(boxedTypeName), safetyEvaluator.getUsageTimeSafety(conjureDef));
+        }
+    }
+
+    static MethodSpec.Builder createCollectionSetterBuilder(
             String prefix,
             EnrichedField enriched,
             TypeMapper typeMapper,
@@ -66,7 +100,7 @@ public static MethodSpec.Builder createCollectionSetterBuilder(
                         field.name()));
     }
 
-    public static MethodSpec.Builder createOptionalSetterBuilder(
+    static MethodSpec.Builder createOptionalSetterBuilder(
             EnrichedField enriched, TypeMapper typeMapper, ClassName returnClass, SafetyEvaluator safetyEvaluator) {
         FieldSpec field = enriched.poetSpec();
         OptionalType type = enriched.conjureDef().getType().accept(TypeVisitor.OPTIONAL);
@@ -78,7 +112,7 @@ public static MethodSpec.Builder createOptionalSetterBuilder(
                         field.name()));
     }
 
-    public static MethodSpec.Builder createItemSetterBuilder(
+    static MethodSpec.Builder createItemSetterBuilder(
             EnrichedField enriched,
             Type itemType,
             TypeMapper typeMapper,
@@ -89,15 +123,15 @@ public static MethodSpec.Builder createItemSetterBuilder(
                 .addParameter(ConjureAnnotations.withSafety(typeMapper.getClassName(itemType), safety), field.name());
     }
 
-    public static MethodSpec.Builder createMapSetterBuilder(
+    static MethodSpec.Builder createMapSetterBuilder(
             EnrichedField enriched, TypeMapper typeMapper, ClassName returnClass) {
         MapType type = enriched.conjureDef().getType().accept(TypeVisitor.MAP);
         return publicSetter(enriched, returnClass)
                 .addParameter(typeMapper.getClassName(type.getKeyType()), "key")
                 .addParameter(typeMapper.getClassName(type.getValueType()), "value");
     }
 
-    public static MethodSpec.Builder publicSetter(EnrichedField enriched, ClassName returnClass) {
+    static MethodSpec.Builder publicSetter(EnrichedField enriched, ClassName returnClass) {
         FieldDefinition definition = enriched.conjureDef();
         return MethodSpec.methodBuilder(enriched.poetSpec().name())
                 .addJavadoc(Javadoc.render(definition.getDocs(), definition.getDeprecated())
@@ -108,7 +142,7 @@ public static MethodSpec.Builder publicSetter(EnrichedField enriched, ClassName
                 .returns(returnClass);
     }
 
-    public static TypeName widenParameterIfPossible(
+    static TypeName widenParameterIfPossible(
             TypeName current, Type type, TypeMapper typeMapper, Optional<LogSafety> safety) {
         if (type.accept(TypeVisitor.IS_LIST)) {
             Type innerType = type.accept(TypeVisitor.LIST).getItemType();
@@ -147,7 +181,7 @@ public static TypeName widenParameterIfPossible(
 
     // we want to widen containers of anything that's not a primitive, a conjure reference or an optional
     // since we know all of those are final.
-    public static boolean isWidenableContainedType(Type containedType) {
+    static boolean isWidenableContainedType(Type containedType) {
         return containedType.accept(new DefaultTypeVisitor<Boolean>() {
             @Override
             public Boolean visitPrimitive(PrimitiveType value) {

conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderGenerator.java

@@ -671,6 +671,26 @@ private MethodSpec createCollectionSetter(String prefix, EnrichedField enriched,
                 .build();
     }
 
+    private MethodSpec createPrimitiveCollectionSetter(EnrichedField enriched, boolean override) {
+        FieldSpec field = enriched.poetSpec();
+        Type type = enriched.conjureDef().getType();
+
+        CollectionType collectionType = getCollectionType(type);
+        return BeanBuilderAuxiliarySettersUtils.createPrimitiveCollectionSetterBuilder(
+                        enriched, typeMapper, builderClass, safetyEvaluator)
+                .addAnnotations(ConjureAnnotations.override(override))
+                .addCode(verifyNotBuilt())
+                .addCode(CodeBlocks.statement(
+                        "$1T.addAllTo$2L(this.$3N, $4L)",
+                        ConjureCollections.class,
+                        collectionType.getConjureCollectionType().getCollectionName(),
+                        field.name(),
+                        Expressions.requireNonNull(
+                                field.name(), enriched.fieldName().get() + " cannot be null")))
+                .addStatement("return this")
+                .build();
+    }
+
     @SuppressWarnings("checkstyle:CyclomaticComplexity")
     private CodeBlock typeAwareAssignment(EnrichedField enriched, Type type, boolean shouldClearFirst) {
         FieldSpec spec = enriched.poetSpec();
@@ -767,10 +787,19 @@ private List<MethodSpec> createAuxiliarySetters(EnrichedField enriched, boolean
         Type type = enriched.conjureDef().getType();
         Optional<LogSafety> safety = safetyEvaluator.getUsageTimeSafety(enriched.conjureDef());
 
+        ImmutableList.Builder<MethodSpec> builder = ImmutableList.builder();
+
         if (type.accept(TypeVisitor.IS_LIST)) {
-            return ImmutableList.of(
+            CollectionType collectionType = getCollectionType(type);
+            if (collectionType.getConjureCollectionType().isPrimitiveCollection()
+                    && collectionType.useNonNullFactory()) {
+                builder.add(createPrimitiveCollectionSetter(enriched, override));
+            }
+
+            builder.add(
                     createCollectionSetter("addAll", enriched, override),
                     createItemSetter(enriched, type.accept(TypeVisitor.LIST).getItemType(), override, safety));
+            return builder.build();
         }
 
         if (type.accept(TypeVisitor.IS_SET)) {
@@ -822,7 +851,9 @@ private CodeBlock optionalAssignmentStatement(EnrichedField enriched, OptionalTy
     private static final EnumSet<PrimitiveType.Value> OPTIONAL_PRIMITIVES =
             EnumSet.of(PrimitiveType.Value.INTEGER, PrimitiveType.Value.DOUBLE, PrimitiveType.Value.BOOLEAN);
 
-    /** Check if the optionalType contains a primitive boolean, double or integer. */
+    /**
+     * Check if the optionalType contains a primitive boolean, double or integer.
+     */
     private boolean isPrimitiveOptional(OptionalType optionalType) {
         return optionalType.getItemType().accept(TypeVisitor.IS_PRIMITIVE)
                 && OPTIONAL_PRIMITIVES.contains(
@@ -1041,25 +1072,38 @@ public boolean useNonNullFactory() {
     }
 
     private enum ConjureCollectionType {
-        LIST("List"),
-        DOUBLE_LIST("DoubleList"),
-        INTEGER_LIST("IntegerList"),
+        LIST("List", false),
+        DOUBLE_LIST("DoubleList", true),
+        INTEGER_LIST("IntegerList", true),
         // Eclipse has a BooleanList type, but this use case implies
         // bit mask and it doesn't serialize efficiently as a collection
         // so let's just use the "naive" boxed collection
-        BOOLEAN_LIST("List"),
-        SAFE_LONG_LIST("SafeLongList"),
-        SET("Set");
+        BOOLEAN_LIST("List", false),
+        // SafeLong is unique in this list. While it is technically backed with a long
+        // its logical limitations are captured in the boxed type SafeLong. Meaning,
+        // you must either expose this "implementation detail" on the public API or
+        // accept that you cannot optimize away the boxing. For now, given the focus
+        // on doubles, let's delay this optimization and have it as a separate discussion.
+        // Technically this type could be optimized at rest, but that would require a more
+        // complex enum to represent this trinary. So for now this is disabled.
+        SAFE_LONG_LIST("SafeLongList", false),
+        SET("Set", false);
 
         private final String collectionName;
+        private final Boolean primitiveCollection;
 
-        ConjureCollectionType(String collectionName) {
+        ConjureCollectionType(String collectionName, boolean primitiveCollection) {
             this.collectionName = collectionName;
+            this.primitiveCollection = primitiveCollection;
         }
 
         public String getCollectionName() {
             return collectionName;
         }
+
+        public Boolean isPrimitiveCollection() {
+            return primitiveCollection;
+        }
     }
 
     private enum ConjureCollectionNullHandlingMode {

conjure-lib/src/main/java/com/palantir/conjure/java/lib/SafeLong.java

@@ -26,9 +26,6 @@ public final class SafeLong implements Comparable<SafeLong> {
     private static final long MIN_SAFE_VALUE = -(1L << 53) + 1;
     private static final long MAX_SAFE_VALUE = (1L << 53) - 1;
 
-    public static final SafeLong MAX_VALUE = SafeLong.of(MAX_SAFE_VALUE);
-    public static final SafeLong MIN_VALUE = SafeLong.of(MIN_SAFE_VALUE);
-
     private final long longValue;
 
     private SafeLong(long longValue) {

conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureCollections.java

@@ -195,8 +195,12 @@ public static List<Double> newNonNullDoubleList(Iterable<Double> iterable) {
 
     // This method modifies a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set
     public static void addAllToDoubleList(Collection<Double> addTo, double[] elementsToAdd) {
-        for (double el : elementsToAdd) {
-            addTo.add(el);
+        if (addTo instanceof ConjureDoubleList) {
+            ((ConjureDoubleList) addTo).addAll(elementsToAdd);
+        } else {
+            for (double el : elementsToAdd) {
+                addTo.add(el);
+            }
         }
     }
 
@@ -220,8 +224,12 @@ public static List<Integer> newNonNullIntegerList(Iterable<Integer> iterable) {
 
     // This method modifies a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set
     public static void addAllToIntegerList(Collection<Integer> addTo, int[] elementsToAdd) {
-        for (int el : elementsToAdd) {
-            addTo.add(el);
+        if (addTo instanceof ConjureIntegerList) {
+            ((ConjureIntegerList) addTo).addAll(elementsToAdd);
+        } else {
+            for (int el : elementsToAdd) {
+                addTo.add(el);
+            }
         }
     }
 

conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureDoubleList.java

@@ -56,6 +56,10 @@ public boolean addAll(int index, Collection<? extends Double> collection) {
         return delegate.addAllAtIndex(index, target);
     }
 
+    void addAll(double[] source) {
+        this.delegate.addAll(source);
+    }
+
     @Override
     public Double remove(int index) {
         return delegate.removeAtIndex(index);

conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureIntegerList.java

@@ -56,6 +56,10 @@ public boolean addAll(int index, Collection<? extends Integer> collection) {
         return delegate.addAllAtIndex(index, target);
     }
 
+    void addAll(int[] source) {
+        this.delegate.addAll(source);
+    }
+
     @Override
     public Integer remove(int index) {
         return delegate.removeAtIndex(index);