RFR: 8029055: Map.merge @implDoc is incorrect.

[Louis Wasserman]

FWIW, I would tend to expect

map.merge(key, value, remappingFunction)

to be equivalent to

map.compute(key, (k, oldValue) ->
   (oldValue == null) ? value : remappingFunction.apply(oldValue, value));

...in which case the mapping would be removed.


On Sun, Nov 24, 2013 at 7:31 PM, David Holmes <david.holmes at oracle.com>wrote:

> Hi Mike,
>
> There is still no clear spec for what should happen if the param value is
> null. To me the spec implies that null should become the new value for the
> mapping, but the code actually removes the mapping.
>
> There is also a further error in this spec. First we have:
>
> 1112      * If the specified key is not already associated with a value or
> is
> 1113      * associated with null, associates it with the given value.
> 1114      * Otherwise, replaces the value with the results of the given
> 1115      * remapping function
>
> but then we have
>
> 1124      * <p>If the function returns {@code null}, the mapping is
> removed (or
> 1125      * remains absent if initially absent).
>
> The parenthesized part is wrong. If the mapping was initially absent then
> the function is never even examined so we don't know if it returns null or
> not. But let us assume that it would have returned null. If the mapping was
> initially absent then we create a new mapping from key->value, hence
> regardless of whether the function would return null, the mapping does not
> remain absent if initially absent.
>
> Also you have changed the method implementation not just the implDoc so
> the bug synopsis is somewhat misleading.
>
> Thanks,
> David
>
> On 23/11/2013 10:25 AM, Mike Duigou wrote:
>
>> We'll be using https://bugs.openjdk.java.net/browse/JDK-8029055 for this
>> issue.
>>
>> I've posted a webrev here:
>>
>> http://cr.openjdk.java.net/~mduigou/JDK-8029055/0/webrev/
>>
>> There is an identical change in ConcurrentMap's merge().
>>
>> Mike
>>
>> On Nov 22 2013, at 16:01 , Henry Jen <henry.jen at oracle.com> wrote:
>>
>>
>>> On 11/21/2013 06:33 PM, David Holmes wrote:
>>>
>>>> On 22/11/2013 5:02 AM, Louis Wasserman wrote:
>>>>
>>>>> While I agree that case should be specified, I'm not certain I follow
>>>>> why
>>>>> that's what's going on here.
>>>>>
>>>>> The weird condition is that if oldValue is null, not value; oldValue
>>>>> is the
>>>>> old result of map.get(key).  The Javadoc seems not just unspecified,
>>>>> but
>>>>> actively wrong.  Here's a worked example:
>>>>>
>>>>> Map<String, Integer> map = new HashMap<>();
>>>>> map.merge("foo", 3, Integer::plus);
>>>>> Integer fooValue = map.get("foo");
>>>>>
>>>>> Going through the Javadoc-specified default implementation:
>>>>>
>>>>>    V oldValue = map.get(key); // oldValue is null
>>>>>    V newValue = (oldValue == null) ? value :
>>>>>                 remappingFunction.apply(oldValue, value);
>>>>>       // newValue is set to value, which is 3
>>>>>    if (newValue == null) // newValue is nonnull, branch not taken
>>>>>        map.remove(key);
>>>>>    else if (oldValue == null) // oldValue is null, branch taken
>>>>>        map.remove(key); // removes the entry from the map
>>>>>    else // else if was already triggered, branch not taken
>>>>>        map.put(key, newValue);
>>>>>
>>>>>
>>> Seems like a document bug to me, we should fix this @implSpec.
>>>
>>> Cheers,
>>> Henry
>>>
>>>
>>


-- 
Louis Wasserman