Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

bmarcaur
Copy link
Member

@bmarcaur bmarcaur commented Oct 22, 2024

Before this PR

This is an alternative variant of #2312 which attempts to fix the boxing overhead merely introduced by the shape of our API (primitive types on both sides of the abstraction). However, there was some objection to that approach due to some stylistic and API design disagreements. This alternative seeks to remedy some of those concerns by following some "prior art" from libraries we accept / use all the time - Immutables.

Immutables will generate a vararg addAll variant for a List collection of some primitive boxed type. For example, List<Double> will produce something like:

public final Builder addField(double... elements)

After this PR

This change first punches a hole through from the underlying eclipse-collections types e.g. DoubleList which has routines like public boolean addAll(double... source) which allow for direct, non-boxed, additions to the type.

Then, taking Immutables as inspiration, I wired up our codegen to leverage this abstraction. This alternative design addresses the mutability concerns as this API clearly communicates how data will be owned moving forward (i.e. copied). This implementation also addresses the concerns of having a setter that resets the type may cause order of operations bugs in the field while populating the builder.

Of Note

Yes... I am aware that the Iterable variant is a setter in the builder e.g.:

@JsonSetter(value = "primitiveItems", nulls = Nulls.SKIP, contentNulls = Nulls.FAIL)
public Builder primitiveItems(@Nonnull Iterable<Integer> primitiveItems) {
        checkNotBuilt();
        this.primitiveItems = ConjureCollections.newNonNullIntegerList(
                Preconditions.checkNotNull(primitiveItems, "primitiveItems cannot be null"));
        return this;
}

This feels like a design oversight (IMO) seeing as how primitiveItems is already initialized statically and newNonNullIntegerList simply performs a copy using addAll. Removing it from the field will be considerable work, so this will need to be inconsistent for the time being. And also previous design deficiencies cannot justify future designs.

Tangent

Sorry if this is getting off topic, but that Iterable variant seems destined to be used incorrectly. Imagine the following code:

// Has a field List<String> items
SomeConjureObject.Builder builder = SomeConjureObject.builder();
String firstToAdd = "first";
List<String> someOtherStrings = List.of(...);

// Clearly an adder
builder.items(firstToAdd);
// Adder? Setter? Setter! No longer contains firstToAdd
builder.items(someOtherStrings);

// Or how about this
// Imagine your code starts off like this
String firstToAdd = "first";
String secondToAdd = someMethod();
builder.items(firstToAdd);
builder.items(secondToAdd);

// But later the API of someMethod changes and now returns a List<String>
// the type checker is happy but now (below) is a setter and no longer
// contains firstToAdd
builder.items(secondToAdd);

Either way, necessary or not, I do not love the invalid state that is further representable on this API by adding an additional similarly named setter.

@@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is a little too specific and doesn't explain how the underlying type holds longs and those must adhere to the abstraction.

*/
// 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) {
if (addTo instanceof ConjureDoubleList) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An inelegant solution to preserve the feedback on #2307 (comment), but still capture the optimization we added. Also has some prior art in addAll.

LIST("List", false),
DOUBLE_LIST("DoubleList", true),
INTEGER_LIST("IntegerList", true),
BOOLEAN_LIST("BooleanList", true),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But also maybe not? #2390

@@ -66,7 +100,7 @@ public static MethodSpec.Builder createCollectionSetterBuilder(
field.name));
}

public static MethodSpec.Builder createOptionalSetterBuilder(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can extract this into a separate PR, but I noticed that we were overly exposing these methods which in turn was overly exposing the EnrichedField type.

@bmarcaur bmarcaur marked this pull request as ready for review October 23, 2024 15:24
@bmarcaur bmarcaur force-pushed the bmarcaurele/primitive-add-all branch from df23c89 to 5590778 Compare November 9, 2024 23:47
@@ -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();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made this a builder so I could optionally add another method and not have to duplicate logic to return lists directly.

@bmarcaur bmarcaur force-pushed the bmarcaurele/primitive-add-all branch from b021276 to 284c2ca Compare January 8, 2025 21:14
@bmarcaur bmarcaur force-pushed the bmarcaurele/primitive-add-all branch from 284c2ca to 86e4def Compare January 8, 2025 21:29
@bmarcaur bmarcaur force-pushed the bmarcaurele/primitive-add-all branch from 86e4def to a1f407a Compare January 9, 2025 02:35
@bmarcaur bmarcaur requested a review from carterkozak January 9, 2025 02:35
@bulldozer-bot bulldozer-bot bot merged commit 44a5143 into develop Jan 10, 2025
5 checks passed
@bulldozer-bot bulldozer-bot bot deleted the bmarcaurele/primitive-add-all branch January 10, 2025 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants