Search code examples
javajava-streamcollectors

Why does Collectors.toUnmodifiableList use a type check of the intermediate accumulator?


I see the following code (JDK 16, JDK 17):

    public static <T>
    Collector<T, ?, List<T>> toUnmodifiableList() {
        return new CollectorImpl<>(ArrayList::new, List::add,
                                   (left, right) -> { left.addAll(right); return left; },
                                   list -> {
                                       if (list.getClass() == ArrayList.class) { // ensure it's trusted
                                           return SharedSecrets.getJavaUtilCollectionAccess()
                                                               .listFromTrustedArray(list.toArray());
                                       } else {
                                           throw new IllegalArgumentException();
                                       }
                                   },
                                   CH_NOID);
    }

What's the point of using if (list.getClass() == ArrayList.class) { check? Why not simply make this code as simple as

return new CollectorImpl<>(ArrayList::new, ArrayList::add,                                          
                           (left, right) -> { left.addAll(right); return left; },              
                           list -> SharedSecrets.getJavaUtilCollectionAccess().listFromTrustedArray(list.toArray()),                                                                  
                           CH_NOID);                                                           

By replacing List::add with ArrayList::add we're providing the most specific method handle of the class, not an interface. The last code uses an assumption that the accumulator instance will be passed to the 'accumulator', the 'combiner' and the 'finisher' parameters as an ArrayList instance (actually, it will, since CollectorImpl uses the same invariant generic variable A for all parameters). I don't see any difference between an IllegalArgumentException and a ClassCastException except the code complexity and an additional check in the finisher.


Solution

  • The change from

    Collector<T, ?, List<T>> toUnmodifiableList() {
        return new CollectorImpl<>((Supplier<List<T>>) ArrayList::new, List::add,
                                   (left, right) -> { left.addAll(right); return left; },
                                   list -> (List<T>)List.of(list.toArray()),
                                   CH_NOID);
    }
    

    to

    public static <T>
    Collector<T, ?, List<T>> toUnmodifiableList() {
        return new CollectorImpl<>(ArrayList::new, List::add,
                                   (left, right) -> { left.addAll(right); return left; },
                                   list -> {
                                       if (list.getClass() == ArrayList.class) { // ensure it's trusted
                                           return SharedSecrets.getJavaUtilCollectionAccess()
                                                               .listFromTrustedArray(list.toArray());
                                       } else {
                                           throw new IllegalArgumentException();
                                       }
                                   },
                                   CH_NOID);
    }
    

    happened in two steps. The step in between was

    Collector<T, ?, List<T>> toUnmodifiableList() {
        return new CollectorImpl<>((Supplier<List<T>>) ArrayList::new, List::add,
                                   (left, right) -> { left.addAll(right); return left; },
                                   list -> (List<T>)SharedSecrets.getJavaUtilCollectionAccess()
                                                                 .listFromTrustedArray(list.toArray()),
                                   CH_NOID);
    }
    

    This first step was introduced for performance reasons (https://bugs.openjdk.java.net/browse/JDK-8156071) and later some noticed that this leads to a security problem (https://bugs.openjdk.java.net/browse/JDK-8254090, https://github.com/openjdk/jdk/pull/449#issuecomment-704001706) and therefore the second change was done.