efficient primitive item addition

palantir · Nov 25, 2024 · b021276 · b021276
1 parent ec5769c
commit b021276
Show file tree

Hide file tree

Showing 10 changed files with 150 additions and 24 deletions.
diff --git a/changelog/@unreleased/pr-2389.v2.yml b/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
diff --git a/conjure-java-core/src/integrationInput/java/com/palantir/product/ListExample.java b/conjure-java-core/src/integrationInput/java/com/palantir/product/ListExample.java
diff --git a/conjure-java-core/src/integrationInput/java/com/palantir/product/SafeLongExample.java b/conjure-java-core/src/integrationInput/java/com/palantir/product/SafeLongExample.java
diff --git a/...-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderAuxiliarySettersUtils.java b/...-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) {

diff --git a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderGenerator.java b/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,31 @@ 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),
+        SAFE_LONG_LIST("SafeLongList", true),
+        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 {

diff --git a/conjure-lib/src/main/java/com/palantir/conjure/java/lib/SafeLong.java b/conjure-lib/src/main/java/com/palantir/conjure/java/lib/SafeLong.java
@@ -40,7 +40,7 @@ public long longValue() {
         return longValue;
     }
 
-    private static long check(long value) {
+    public static long check(long value) {
         Preconditions.checkArgument(
                 MIN_SAFE_VALUE <= value && value <= MAX_SAFE_VALUE,
                 "number must be safely representable in javascript i.e. "

diff --git a/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureCollections.java b/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureCollections.java
@@ -19,6 +19,7 @@
 import com.palantir.conjure.java.lib.SafeLong;
 import com.palantir.logsafe.Preconditions;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.LinkedHashSet;
@@ -170,8 +171,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);
+            }
         }
     }
 
@@ -195,8 +200,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);
+            }
         }
     }
 
@@ -235,8 +244,12 @@ public static List<SafeLong> newNonNullSafeLongList(Iterable<SafeLong> iterable)
 
     // This method modifies a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set
     public static void addAllToSafeLongList(Collection<SafeLong> addTo, long[] elementsToAdd) {
-        for (long el : elementsToAdd) {
-            addTo.add(SafeLong.of(el));
+        if (addTo instanceof ConjureSafeLongList) {
+            ((ConjureSafeLongList) addTo).addAll(elementsToAdd);
+        } else {
+            for (long el : elementsToAdd) {
+                addTo.add(SafeLong.of(el));
+            }
         }
     }
 }
diff --git a/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureDoubleList.java b/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureDoubleList.java
@@ -55,6 +55,10 @@ public boolean addAll(int index, Collection<? extends Double> collection) {
         return delegate.addAllAtIndex(index, target);
     }
 
+    public void addAll(double... source) {
+        this.delegate.addAll(source);
+    }
+
     @Override
     public Double remove(int index) {
         return delegate.removeAtIndex(index);

diff --git a/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureIntegerList.java b/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureIntegerList.java
@@ -55,6 +55,10 @@ public boolean addAll(int index, Collection<? extends Integer> collection) {
         return delegate.addAllAtIndex(index, target);
     }
 
+    public void addAll(int... source) {
+        this.delegate.addAll(source);
+    }
+
     @Override
     public Integer remove(int index) {
         return delegate.removeAtIndex(index);

diff --git a/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureSafeLongList.java b/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureSafeLongList.java
@@ -56,6 +56,14 @@ public boolean addAll(int index, Collection<? extends SafeLong> collection) {
         return delegate.addAllAtIndex(index, target);
     }
 
+    public void addAll(long... source) {
+        for (long value : source) {
+            // Doesn't use SafeLong creation because this causes unnecessary boxing
+            SafeLong.check(value);
+        }
+        this.delegate.addAll(source);
+    }
+
     @Override
     public SafeLong remove(int index) {
         return SafeLong.of(delegate.removeAtIndex(index));