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

[Peter Levart]

Hi,

I propose a patch for:

     https://bugs.openjdk.java.net/browse/JDK-8040892

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


On 04/22/2014 02:44 PM, Paul Sandoz wrote:
> Thanks Peter.
>
> FWIW i would have used the same exception throwing trick :-)
>
> Originally throwingMerger was public, but we hesitated about it's utility and made it private, glad we did that now.
>
>
>> Hmmm, if Brian approves using this spare Exception subclass, I suggest
>> extracting the creation of the accumulator into its own method.
>>
> Rather than the MergedRefusedException overriding fillInStackTrace it may be better to defer to the super constructor:
>
>    MergeRefusedException(Object u, Object v) {
>       super(null, null, false, false)
>       this.u = u;
>       this.v = v;
>    }
>
> as that sets up the correct conditions for the state of the fields.
>
> Also since this is only relevant for the to*Map functions when no merger is explicitly provided it is tempting to rejig the implementation of those to create a Collector and wrap the existing mapMerger function, thus limiting the scope of the exception to where it is used.
>
> I have a slight concern that for large values the exception could hold large string values, but i don't think we try very hard generally in the JDK to use a form of pretty truncated printing...
>
>
> Peter, i believe you have committer rights? Perhaps you could submit on core-libs for review?
>
> Paul.
>