Poor Javadoc of Map default methods [Re: RFR: 8029055: Map.merge must refuse null values]

[Martin Buchholz]

I haven't looked in depth, but I agree with Stephen's analysis.  This API
and its javadoc needs work.

E.g. It's not clear that the purpose of Map.compute is to *update* the
mapping for key in the map.

Getting these right is hard; you have the "null" value/sentinel confusion
and the confusion that a single method may (or may not) either add or
remove a mapping for key.

Instead of "The default implementation makes no guarantees about
synchronization or atomicity properties of this method."  we should boldly
say that the default implementation is *not* atomic, even if the underlying
map is.


On Tue, Nov 26, 2013 at 5:20 AM, Stephen Colebourne <scolebourne at joda.org>wrote:

> I took a quick look, but jumped back in horror at the start of the
> Javadoc for the new methods in Map. A Javadoc description should start
> with the positive, not the negative. I had to read the code to figure
> out what they are supposed to do. Here are the four poor Javadocs and
> some proposed enhacements:
>
> merge()
> "If the specified key is not already associated with ..."
> should be
> "Updates the value for an existing key merging the existing value with
> the specified value."
> "<p>"
> "....more detailed user-level explanation..."
> "<p>"
> "....now the more spec/edge case part..."
> ("if" is a terrible way to start the docs. Say what it does!)
>
>
> computeIfAbsent()
> "If the specified key is not already associated with a value (or is
> mapped..."
> should be
> "Associates the key with a value computed on demand if the key is not
> currently present."
> "<p>"
> "....more detailed user-level explanation..."
> "<p>"
> "....now the more spec/edge case part..."
>
>
> computeIfPresent()
> "If the value for the specified key is present and non-null,"
> should be
> "Updates the value for an existing key."
> "<p>"
> "....more detailed user-level explanation..."
> "<p>"
> "....now the more spec/edge case part..."
>
>
> compute()
> "Attempts to compute a mapping for the specified key..."
> should be
> "Updates the value for a key, adding a new mapping if necessary."
> "<p>"
> "....more detailed user-level explanation..."
> "<p>"
> "....now the more spec/edge case part..."
> (attempts is negative, not positive)
>
> Similar problems seem to exist for "putIfAbsent".
>
> also...
>
> I have to say that I don't overly like the method name.
> "map.merge(...)" sounds like a bulk operation affecting the whole map
> (ie. like putAll). Assuming that the method cannot be renamed, it
> could be improved just by changing the method parameter names.:
>
> default V merge(K key, V valueToMerge,
>     BiFunction<? super V, ? super V, ? extends V> mergeFunction) {
>
> "valueToMerge" implies that action will occur to the parameter.
> "mergeFunction" is much more meaningful that "remapping Function".
>
> Similar parameter name change, "mappingFunction" -> "valueCreationFunction"
> default V computeIfAbsent(
>   K key,
>   Function<? super K, ? extends V> valueCreationFunction) {
>
> Similar parameter name change, "remappingFunction" -> "valueUpdateFunction"
> default V computeIfPresent(
>   K key,
>   BiFunction<? super K, ? super V, ? extends V> valueUpdateFunction) {
>
> Similar parameter name change, "remappingFunction" -> "valueUpdateFunction"
> default V compute(
>   K key,
>   BiFunction<? super K, ? super V, ? extends V> valueUpdateFunction) {
>
>
> also...
> Can you use HashSet::new in the example code?
>
>
> In general, all these new methods are written in a spec style, rather
> than a user friendly style, and I'm afraid I don't think thats
> sufficient.
> Stephen
>
>
>
> On 26 November 2013 04:32, Mike Duigou <mike.duigou at oracle.com> wrote:
> >
> > On Nov 24 2013, at 16:31 , 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.
> >
> > I feel very uncomfortable the status quo of with null being ignored,
> used for a sentinel and also as value. The relations between null and
> values in this method are just too complicated.
> >
> > Currently:
> >
> > - The provided value may be either null or non-null. Is null a legal
> value? It depends upon:
> >         - Is there an existing value?
> >         - Does the Map allow null values?
> >         - Does the function allow null values?
> > - Existing null values are treated as absent.
> >         - If a null value is passed should we remove this mapping or add
> it to the map?
> >               - null might not be accepted by the map
> >               - The associated value would still be regarded as absent
> for future operations.
> > - The function may return null to signal "remove".
> >
> > In particular I dislike adding a null entry to the map if there is no
> current mapping (or mapped to null). It seems like it should be invariant
> whether we end up with an associated value. If null is used to signify
> "remove" then map.contains(key) will return variable results depending upon
> the initial state. Having the behaviour vary based upon whether the map
> allows null values would be even worse.
> >
> > So I would like to suggest that we require value to be non-null. I have
> provisionally updated the spec and impls accordingly.
> >
> >> The parenthesized part is wrong.
> >
> > I think that's overzealous copying from compute(). I have removed it.
> >
> >>
> >> Also you have changed the method implementation not just the implDoc so
> the bug synopsis is somewhat misleading.
> >
> > I will correct this. More changes were made than I originally expected.
> New synopsis will be "Map.merge implementations should refuse null value
> param"
> >
> > I have updated the webrev.
> >
> > http://cr.openjdk.java.net/~mduigou/JDK-8029055/1/webrev/
> >
> > Thanks,
> >
> > Mike
> >
> >>
> >> 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
> >>>>
> >>>
> >
>