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

[Peter Levart]

On 04/28/2014 10:20 AM, Paul Sandoz wrote:
> 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.

You could use the null return from Map.merge() as a signal to throw NPE, 
but this is only 100% safe in to*Map() methods that don't take a 
mapSupplier.

Collections.toConcurrentMap(...., Supplier<M> mapSupplier) could be 
abused by someone knowing that the supplier is called exactly once and 
that the returned map is the same instance returned from the supplier. 
For example:


         ConcurrentMap<Integer, String> map = new ConcurrentHashMap<>();

         Stream<Integer> s1 = Stream.of(1, 2, 3);
         Stream<Integer> s2 = Stream.of(4, 5, 6);

         s1.collect(toConcurrentMap(e -> e, e -> "" + e, (v1, v2) -> v1, 
() -> map));
         s2.collect(toConcurrentMap(e -> e, e -> "" + e, (v1, v2) -> v1, 
() -> map));

         System.out.println(map);


...in that case a null return from mergeFunction could be observed in 
the result.

Regards, Peter

>
> Paul.