RFR 9: JDK-8040892 Incorrect message in Exception thrown by Collectors.toMap(Function, Function)

[Paul Sandoz]

On Apr 25, 2014, at 7:31 PM, Peter Levart <peter.levart at gmail.com> wrote:
> Hi Paul,
> 
> To make any sense of null return from the mergeFunction, which could mean that an entry with that key is absent from resulting map, such Collector would have to keep some more state - the collecting map (which is also returned at the end) and the set of removed keys. For example:
> 
> 
>     public static <T, K, U, M extends Map<K, U>>
>     Collector<T, ?, M> toMap(Function<? super T, ? extends K> keyMapper,
>                              Function<? super T, ? extends U> valueMapper,
>                              BinaryOperator<U> mergeFunction,
>                              Supplier<M> mapSupplier) {
>         
>         class State {
>             final M map = mapSupplier.get();
>             final Set<K> removedKeys = new HashSet<>(); 
>             M map() { return map; }
>         }
>         
>         BiConsumer<State, T> accumulator = (state, element) -> {
>                 K k = keyMapper.apply(element);
>                 if (!state.removedKeys.contains(k)) {
>                     U u = state.map.merge(k, valueMapper.apply(element), mergeFunction);
>                     if (u == null) state.removedKeys.add(k);
>                 }
>         };
>         
>         BinaryOperator<State> merger = (state1, state2) -> {
>             state1.map.keySet().removeAll(state2.removedKeys);
>             state2.map.keySet().removeAll(state1.removedKeys);
>             state1.removedKeys.addAll(state2.removedKeys);
>             for (Map.Entry<K,U> e : state2.map.entrySet()) {
>                 U u = state1.map.merge(e.getKey(), e.getValue(), mergeFunction);
>                 if (u == null) state1.removedKeys.add(e.getKey());
>             }
>             return state1;
>         };
>         
>         return new CollectorImpl<>(State::new, accumulator, merger, State::map, CH_ID);
>     }
> 
> 
> ...but the concurrent variant would be tricky to implement.
> 

Yes, i suspect a finishing stage would be required to clean up the map. 

The simplest thing to do is throw an NPE if Map.merge returns null. It would be rather surprising for partial map value merging to have a global subtractive effect (esp. if that could occur non-determinisitically when encounter order of processing values is not important).

Collectors.toMap and groupingBy are already null-hostile, Map.merge (and your update using putIfAbsent preserving the same merge behaviour) will throw an NPE on a null input value.

Plus I now realize that throwing an NPE after removal has occurred is OK since that side-effect should never be observed as resulting map will not be returned to the caller.

Paul.