RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements

[Stuart Marks]

On Wed, 25 Nov 2020 16:39:20 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> @pavelrappo Please see my proposed CSR below. Thank you.
>> 
>> # Map::compute should have a clearer implementation requirement.
>> 
>> ## Summary
>> 
>> java.util.Map::compute should have a clearer implementation requirement in its documentation.
>> 
>> ## Problem
>> 
>> The documentation of the implementation requirements for Map::compute has the following problems:
>> 1. It lacks of return statements for most of the if-else cases.
>> 1. The indents are 3 spaces, while the convention is 4 spaces.
>> 1. The if-else is overly complicated and can be simplified.
>> 
>> ## Solution
>> 
>> Rewrite the documentation of Map::compute to match its default implementation.
>> 
>> ## Specification
>> 
>> diff --git a/src/java.base/share/classes/java/util/Map.java b/src/java.base/share/classes/java/util/Map.java
>> index b1de34b42a5..c3118a90581 100644
>> --- a/src/java.base/share/classes/java/util/Map.java
>> +++ b/src/java.base/share/classes/java/util/Map.java
>> @@ -1113,17 +1113,12 @@ public interface Map<K, V> {
>>       * <pre> {@code
>>       * V oldValue = map.get(key);
>>       * V newValue = remappingFunction.apply(key, oldValue);
>> -     * if (oldValue != null) {
>> -     *    if (newValue != null)
>> -     *       map.put(key, newValue);
>> -     *    else
>> -     *       map.remove(key);
>> -     * } else {
>> -     *    if (newValue != null)
>> -     *       map.put(key, newValue);
>> -     *    else
>> -     *       return null;
>> +     * if (newValue != null) {
>> +     *     map.put(key, newValue);
>> +     * } else if (oldValue != null) {
>> +     *     map.remove(key);
>>       * }
>> +     * return newValue;
>>       * }</pre>
>>       *
>>       * <p>The default implementation makes no guarantees about detecting if the
>
> @johnlinp In any case, you'd also need to update the surrounding prose; we need to remove this:
>  returning the current value or {@code null} if absent:```

@pavelrappo 

> What is the required level of fidelity particular (pseudo-) code has to have?

It's potentially a large discussion, one that could be had in the context of my JEP draft http://openjdk.java.net/jeps/8068562 . However, speaking practically, it's possible to focus the discussion fairly concisely: the main responsibility of the `@implSpec` ("Implementation Requirements") section is to give implementors of subclasses enough information to decide whether to inherit the implementation or to override it, and if they override it, what behavior they can expect if they were to call `super.compute`.

In this case, a null-value-tolerating Map implementation needs to know that the default implementation calls `remove` in the particular case that you mentioned. A concurrent Map implementation will also need to know that the default implementation calls `get(key)` and `containsKey(key)` at different times, potentially leading to a race condition. Both of these inform the override vs. inherit decision.

-------------

PR: https://git.openjdk.java.net/jdk/pull/714