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

[Peter Levart]

On 04/25/2014 03:43 PM, Chris Hegarty wrote:
>
> On 25/04/14 13:12, Paul Sandoz wrote:
>>
>> On Apr 23, 2014, at 11:06 AM, Peter Levart <peter.levart at gmail.com> 
>> wrote:
>>
>>> Hi,
>>>
>>> I propose a patch for:
>>>
>>>     https://bugs.openjdk.java.net/browse/JDK-8040892
>>>
>>
>> +1, nice use of putIfAbsent. Just double checking, did you run the 
>> stream unit tests?
>
> Agreed. This looks good to me.
>
> Trivially, is this comment, in uniqKeysMapMerger, left over from a 
> previous cut'n'paste ?
>
>   144      * {@link Map#merge(Object, Object, BiFunction) Map.merge()}
>
> -Chris.

Hi Chris,

Thanks for spotting the left over line. I'll remove it.

Regards, Peter

>
>>
>> I think key step was to separate map merge and accumulate functions 
>> for the toMap* methods not accepting a value merge function. Now that 
>> is the case one could still use merge, but that would be less 
>> efficient than putIfAbsent since that does not require a lambda 
>> capturing the key.
>>
>>
>> That fix caused me to think that we should update the value merge 
>> function accepting toMap* methods for the case when the merge 
>> function returns null, as removing an entry would be most undesirable.
>>
>> A simple fix would be to throw an NPE if the merge function returns 
>> null, if it is acceptable to induce a side-effect of a removal, which 
>> seems reasonable given a partial result would any be returned if a 
>> null check is performed on the result of the merge function, 
>> otherwise that merge function requires wrapping e.g. 
>> mergeFunction.andThen(Objects::requireNonNull).
>>
>> Paul.
>>
>>
>>> Here's the webrev:
>>>
>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/Collectors.duplicateKey/webrev.02/ 
>>>
>>>
>>> There has already been a discussion on the lambda-dev about this bug:
>>>
>>> http://mail.openjdk.java.net/pipermail/lambda-dev/2014-April/012005.html 
>>>
>>>
>>>
>>> Initially a fix has been proposed which used a private 
>>> RuntimeException subclass thrown by the merger function in order to 
>>> transport the values that attempted to be merged out of Map.merge() 
>>> method to an outer context where they could be combined with the key 
>>> to produce a friendlier "duplicate key" message. It turns out that 
>>> there is an alternative approach that doesn't require exception 
>>> acrobatics - using Map.putIfAbsent() instead of Map.merge(). I 
>>> checked both methods in Map defaults and in HashMap implementations 
>>> for possible semantic differences. Same behaviour can be achieved by 
>>> requiring that the value passed to Map.putIfAbsent(key, value) is 
>>> non-null and throwing NPE if it isn't.
>>>
>>>
>>> Regards, Peter