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.
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.