ImmutableList.builder(Collection) or ImmutableList.toBuilder()

[2is10]

My employer’s codebase currently has 130 occurrences of this pattern:

ImmutableList.<ItemType>builder().addAll(items)

That pattern can be useful for appending items and/or collections.

The need for an explicit type argument is unfortunate; it’s nearly always the same as that of items. Better would be:

ImmutableList.builder(items)

Even more concise, but not as broadly applicable would be:

items.toBuilder()

Would you consider either to be worthwhile?

[cpovirk]

Dumping some notes from a chat that @eamonnmcmanus and I were having, just so that we don't lose them. (I see that Éamonn assigned the issue to himself, but I'm not committing him to doing anything in particular :))

AutoValue would use this: https://github.com/google/auto/blob/ce33eafc196b0c405654a7c534a8836cc1c26c91/value/src/main/java/com/google/auto/value/processor/PropertyBuilderClassifier.java#L448

We likewise see a lot of ImmutableList.<ItemType>builder().addAll(items) in our codebase. Additionally, we've gotten requests for "persistent-collection" APIs like anImmutableList.plus(element), which have similar uses but which seem overall slightly less useful than your suggestion: Your suggestion, while it produces longer code and can't support removals, supports multiple subsequent additions efficiently and (more importantly) is clearer about its performance characteristics.

We feel that some occurrences of ImmutableList.<ItemType>builder().addAll(items) would still be "better" in their existing form, but this is all kind of squishy. For example, if there are two successive addAll calls, then it could be "better" to keep that symmetry, rather than to have ImmutableList.builder(fooItems).addAll(barItems). Now, adding the proposed API doesn't force anyone to break that symmetry. Still, it may encourage it, and it can definitely encourage bikeshedding of the sort I am engaging in here :)

There are similarities here to another discussion from when we designed our builders years ago: We could have avoided the need for type arguments in many cases (and made the code shorter in general) by providing ImmutableMap.builderWith(k1, v1).and(k2, v2).build(k3, v3). But we decided that we placed more value on having every entry insertion look the same (ImmutableMap.<Foo>builder().put(...).put(...).put(...).build()). Granted, that may be less important in this case: In the case of a static ImmutableMap built with a chain of put calls, it's easy to imagine that someone might remove the first entry (or last entry) in the future. But in the case of this new proposed API, it feels more like the first collection is typically going to be "special," as if we are "modifying" that collection and not as if it's just one of n entries of equal importance. Thus, as long as the builder usage continues to exist, we'd expect it to continue to include that first collection. And so there's no need for the API design to optimize for removing that collection from the builder chain someday.

All that said, we feel (for reasons that we can't put our finger on) that fooItems.toBuilder().addAll(barItems).build() is somehow less offensive on symmetry grounds.

Note, though, that toBuilder() obviously requires that fooItems already be an ImmutableList. (Likely this requirement rules out many of the possible users that our searches turned up.)

toBuilder() also raises questions about subtypes: If my method receives an ImmutableSet, then I might expect toBuilder() to return a plain ImmutableSet.Builder. But what if the set is actually an ImmutableSortedSet? Either we'd need for ImmutableSortedSet.toBuilder() to return a plain ImmutableSet.Builder (which is strange... and might even be able to lead to exceptions from toBuilder().build() if the set contains duplicates according to equals? [edit: and which presumably does not help AutoValue]), or callers would need to be aware that they might not get the kind of builder that they'd naively expect. (Contrast to ImmutableSet.copyOf(setThatHappensToBeAnImmutableSortedSet), which I believe returns a plain ImmutableSet rather than the original ImmutableSortedSet.)

Currently, that can come up in even more surprising ways: ImmutableMap.of(k, v) currently returns an ImmutableBiMap:

https://github.com/google/guava/blob/70d571b10a95e1579a473dfefa981ab5fcb6fcc3/guava/src/com/google/common/collect/ImmutableMap.java#L129-L130

Thus, ImmutableMap.of(k, v).toBuilder() might give you an ImmutableBiMap.Builder. But we could at least rework things to avoid that scenario.

[wyyl1]

Support this writing

ImmutableList.of(items);

[jbduncan]

@wyyl1 Have you tried the following?

ImmutableList.copyOf(items);

[wyyl1]

@jbduncan Thank you for your advice ! I misunderstood before .

import com.google.common.collect.ImmutableList;
import org.junit.jupiter.api.Test;

import java.util.ArrayList;
import java.util.List;
import java.util.stream.IntStream;

import static org.assertj.core.api.AssertionsForClassTypes.assertThat;

public class ImmutableTest {
    @Test
    void copy_items_and_get_the_same_items() {
        ImmutableList<Integer> immutableItems = ImmutableList.copyOf(items());

        assertThat(immutableItems).asList().hasSize(3);
        IntStream.rangeClosed(0, 2).forEach(i -> assertThat(immutableItems.get(i)).isEqualTo(i));
    }

    @Test
    void packaged_items_and_get_the_new_list() {
        ImmutableList<List<Integer>> list = ImmutableList.of(items());

        assertThat(list).asList().hasSize(1);
    }

    private List<Integer> items() {
        List<Integer> items = new ArrayList<>();
        items.add(0);
        items.add(1);
        items.add(2);

        return items;
    }
}