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

[johnlinp]

This is from the mailing list: http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067190.html

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Testing

	Linux x64	Windows x64	macOS x64
Build	✔️ (5/5 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)
Test (tier1)	✔️ (9/9 passed)	✔️ (9/9 passed)	✔️ (9/9 passed)

Issue

• JDK-8247402: Documentation for Map::compute contains confusing implementation requirements

Download

$ git fetch https://git.openjdk.java.net/jdk pull/714/head:pull/714
$ git checkout pull/714

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8247402: Documentation for Map::compute contains confusing implementation requirements

Reviewers

• Pavel Rappo (@pavelrappo - Reviewer)
• Martin Buchholz (@Martin-Buchholz - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/714/head:pull/714
$ git checkout pull/714

[bridgekeeper]

👋 Welcome back johnlinp! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@johnlinp The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (d16a35b)
• 00: Full (d058e798be18e05e637459ab1b953b038768e5fc)

[dfuch]

Hi Jon,

Can you explain what this change is about: e.g. something like:

Updates the documentation of Map::compute to match its default implementation:
The documentation of the default implementation of Map::compute was both wrong and confusing.
This change updates the documentation to match the behaviour of the implementation.

because now I am confused. I believe what you are trying to do is what I have written above. Can you confirm?
This will need a CSR.

And are you going to withdraw #451 now?

best regards,
-- daniel

[dfuch]

/csr

[openjdk]

@dfuch has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@johnlinp please create a CSR request and add link to it in JDK-8247402. This pull request cannot be integrated until the CSR request is approved.

[johnlinp]

Hi @dfuch,

Thanks for reviewing my PR. You're absolutely correct about what this PR is about. Besides, I already closed #451.

[johnlinp]

@dfuch May I ask how can I create a CSR? I checked https://wiki.openjdk.java.net/display/csr/CSR+FAQs and it says:

Q: How do I create a CSR ?
A: Do not directly create a CSR from the Create Menu. JIRA will let you do this right up until the moment you try to save it and find your typing was in vain.
Instead you should go to the target bug, select "More", and from the drop down menu select "Create CSR". This is required to properly associate the CSR with the main bug, just as is done for backports.

However, I don't have an account at https://bugs.openjdk.java.net/ yet. Therefore, I don't see the "More" button on https://bugs.openjdk.java.net/browse/JDK-8247402. Could you please tell me how do I proceed? Thank you.

[bridgekeeper]

@johnlinp This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[pavelrappo]

@johnlinp, you cannot create a CSR by yourself at the moment. Someone else will have to do that for you. Might as well be me. So here's my proposal: come up with the meat, then I'll help you with the paperwork.

For starters, have a look at existing CSRs (you don't need a JBS account for that). For example, https://bugs.openjdk.java.net/issues/?jql=issuetype%3DCSR%20and%20Subcomponent%3Djava.util%3Acollections%20

Fill in an informal CSR template inline in this thread, and we'll proceed from that point. Is that okay?

[johnlinp]

@pavelrappo Thank you for your kind help and great proposal. I'll write up a draft CSR in this thread.

[johnlinp]

@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.
2. The indents are 3 spaces, while the convention is 4 spaces.
3. 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

[pavelrappo]

The proposed CSR has a few problems that we need to resolve.

1. The Specification pseudo-code behaves differently from both the old pseudo-code and the actual implementation when newValue == null && oldValue == null and map.containsKey(key) == true.
2. The content of the Solution section seems irrelevant: aside from a couple of missing return statements the current pseudo-code is fine. We are after something else, aren't we? The bottom line is we should state the solution more clearly.
3. The Summary section differs from that of the JDK-8247402.

[pavelrappo]

The proposed CSR has a few problems that we need to resolve.

1. The Specification pseudo-code behaves differently from both the old pseudo-code and the actual implementation when newValue == null && oldValue == null and map.containsKey(key) == true.
2. The content of the Solution section seems irrelevant: aside from a couple of missing return statements the current pseudo-code is fine. We are after something else, aren't we? The bottom line is we should state the solution more clearly.
3. The Summary section differs from that of the JDK-8247402.

I read my previous reply and realized that it is confusing and contains a factual error; so let me straighten it out in this new reply rather than edit that previous one.

1. The proposed pseudo-code behaves exactly the same way as the existing pseudo-code modulo the missing return statements. (For some reason, I previously wrote that the proposed pseudo-code behaves differently from the existing pseudo-code.)
2. Both the proposed pseudo-code and the existing pseudo-code deviate from the documented behaviour (written in prose) and the actual implementation. The deviation happens when newValue == null && (oldValue = map.get(key)) == null and map.containsKey(key) == true. (That part was correct.)

Now, here's what I should have said in my previous reply. If the CSR intends to solve (2) then both the proposed pseudo-code and the Problem section must be updated; otherwise the Solution section must be updated. Put differently, either fix the diff and add one more item to that problem list, or change the solution. Otherwise the solution does not match the problem leaving the CSR in a contradictory state.

I hope this all makes sense now.

[stuart-marks]

2. Both the proposed pseudo-code and the existing pseudo-code deviate from the documented behaviour (written in prose) and the actual implementation.

Let me clarify something. The "documented behavior" is actually the API specification, the contract that applies to Map.compute and all its implementations. The pseudo-code is part of the "implementation requirements" section, whose primary responsibility is to specify what the default implementation actually does. (Of course, that implementation should also conform to the API spec.)

We've established that the pseudo-code -- either the existing or proposed version -- differs from the default implementation's actual behavior. It's thus failed its primary responsibility, and therefore that's the problem that needs to be fixed. I think it's pointless to clean up the pseudo-code if the result is still incorrect.

[pavelrappo]

@stuart-marks I thought about it before reading your reply. And since I hadn't come to a definite conclusion, I worded my response to @johnlinp as a choice.

Although in this particular case it's easy to fix the discrepancy, I think it raises a general question which applies to all (pseudo-) code snippets in normative parts of API specifications:

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

It's such a huge topic for discussion, that I don't even know where to begin; certainly, not in this PR thread. Perhaps we should start a dedicated (non-RFR/PR) thread on one of the OpenJDK mailing lists to host that discussion.

[pavelrappo]

@johnlinp In any case, you'd also need to update the surrounding prose; we need to remove this:
then returning the current value or {@code null} if absent:

[stuart-marks]

@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.

[dfuch]

@stuart-marks

Both of these inform the override vs. inherit decision.

So in this case - fixing the specification to match the default implementation seems to be the right call - as existing implementations that do not override are more probably depending on the current default behavior.

[johnlinp]

Thank you all for giving me great advice. Sounds like the conclusion is to update the documentation to match the default implementation. I'll update this PR and propose a new CSR accordingly.

[johnlinp]

@pavelrappo Please see my updated CSR below. Thanks.

Map::compute should have the implementation requirement match its default implementation

Summary

The implementation requirement of Map::compute does not match its default implementation. Besides, it has some other minor issues. We should fix it.

Problem

The documentation of the implementation requirements for Map::compute has the following problems:

1. It doesn't match its default implementation.
2. It lacks of the return statements for most of the if-else cases.
3. The indents are 3 spaces, while the convention is 4 spaces.
4. The if-else is overly complicated and can be simplified.
5. The surrounding prose contains incorrect statements.

Solution

Rewrite the documentation of Map::compute to match its default implementation and solve the above mentioned problems.

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..b30e3979259 100644
--- a/src/java.base/share/classes/java/util/Map.java
+++ b/src/java.base/share/classes/java/util/Map.java
@@ -1107,23 +1107,17 @@ public interface Map<K, V> {
      *
      * @implSpec
      * The default implementation is equivalent to performing the following
-     * steps for this {@code map}, then returning the current value or
-     * {@code null} if absent:
+     * steps for this {@code map}:
      *
      * <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.containsKey(key)) {
+     *     map.remove(key);
      * }
+     * return newValue;
      * }</pre>
      *
      * <p>The default implementation makes no guarantees about detecting if the

[pavelrappo]

@johnlinp, thanks for updating the CSR draft; it is much better now.

@stuart-marks, I think we could further improve this snippet. This if statement seems to use an optimization:

if (oldValue != null || map.containsKey(key))

I don't think we should include an optimization into the specification unless that optimization also improves readability. Is this the case here? Could this be better?

if (map.containsKey(key))

[pavelrappo]

I would even go as far as to rewrite that snippet like this:

if (newValue == null) {
    remove(key);
} else {
    put(key, newValue);
}
return newValue;

This rewrite is possible thanks to the following properties of Map.remove(Object key):

1. A call with an unmapped key has no effect.
2. A call with a mapped key has the same semantics regardless of the value that this key is mapped to.

In particular, (2) covers null values.

To me, this rewrite reads better; however, I understand that readability is subjective and that snippets used in @implSpec might be subject to additional requirements.

[stuart-marks]

@pavelrappo The intended effect of the revised snippet is sensible, but again I note that it differs from the actual default implementation. Specifically: if the map does not contain the key and newValue is null, the default implementation currently does nothing, whereas the revised snippet calls remove(key). This should have no effect on the map but a subclass might override remove and this behavior difference is observable. (The typical example of this is maintaining a counter of the number of operations. I think Effective Java uses that example in discussing subclassing.) I think the main priority here is fidelity to what the default implementation actually does -- at least, concerning operations on this -- and less on readability.

[stuart-marks]

The current snippet proposed by @johnlinp does seem to have the same behavior as the default implementation; I would avoid trying to "optimize" this. However, it does express the conditions and return value somewhat differently from the way the default implementation does. I think those differences are not significant to subclasses and are mostly stylistic. The original @implSpec snippet attempted to handle the cases separately, whereas the current proposed snippet minimizes them (while still agreeing with the implementation's behavior). I'm not too concerned about this. I think the current snippet is acceptable. Again, the main priority is agreement with the implementation.

[pavelrappo]

@pavelrappo The intended effect of the revised snippet is sensible, but again I note that it differs from the actual default implementation. Specifically: if the map does not contain the key and newValue is null, the default implementation currently does nothing, whereas the revised snippet calls remove(key). This should have no effect on the map but a subclass might override remove and this behavior difference is observable. (The typical example of this is maintaining a counter of the number of operations. I think Effective Java uses that example in discussing subclassing.) I think the main priority here is fidelity to what the default implementation actually does -- at least, concerning operations on this -- and less on readability.

Although we should really have a conversation on code snippets in API specifications, this thread is not the place for that. However, I will minimally comment on some of what you've just said.

1. If a high-fidelity copy is not enough, an identical copy is required; that suggests a JavaDoc facility for embedding portions of code into doc comments.
2. I have to note that Map.merge (a method whose semantics is very close to that of Map.compute) is specified and implemented very similarly to what my comment #1 proposed.

The current snippet proposed by @johnlinp does seem to have the same behavior as the default implementation; I would avoid trying to "optimize" this. However, it does express the conditions and return value somewhat differently from the way the default implementation does. I think those differences are not significant to subclasses and are mostly stylistic. The original @implSpec snippet attempted to handle the cases separately, whereas the current proposed snippet minimizes them (while still agreeing with the implementation's behavior). I'm not too concerned about this. I think the current snippet is acceptable. Again, the main priority is agreement with the implementation.

Perhaps there's some confusion. If anything my comment #2 was proposing to remove an optimization carried over from the default implementation.

[pavelrappo]

1. @johnlinp I've created the CSR: https://bugs.openjdk.java.net/browse/JDK-8257768
2. @dfuch, @stuart-marks, and I-cannot-seem-to-find-Martin-Buchholz's-GitHub-username: care to review that CSR?

[Martin-Buchholz]

Hello github - this is my first ever comment.

The javadoc specs for the various compute methods in all the Map classes should be maintained consistently and holistically. Sorry for not having done so years ago.

Pavel is thinking about code snippets for all of OpenJDK today, while I have been thinking about code snippets in jsr166. jsr166 code snippets have a consistent style that should also be used for Map.java, but we've had a mild case of maintainer style divergence.

[pavelrappo]

@dfuch, @stuart-marks, and @Martin-Buchholz: thanks for reviewing the CSR.
@johnlinp: I have finalized the CSR; now we are waiting for it to be approved.

[pavelrappo]

This looks good to me.

[openjdk]

@johnlinp This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: prappo, martin

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 247 new commits pushed to the master branch:

• d4282b0: 8257731: Remove excessive include of stubRoutines.hpp
• 80dac5a: 8257912: Convert enum iteration to use range-based for loops
• 164c55b: 8258056: jdk/javadoc/doclet/testHtmlTableTags/TestHtmlTableTags.java fails against jdk17
• 42264b2: 8257971: (fs) Remove unused code from WindowsPath.subpath(begin, end)
• 3342eca: 8258054: runtime/sealedClasses/GetPermittedSubclassesTest.java fails w/ jdk17
• f574056: 8256424: Move ciSymbol::symbol_name() to ciSymbols::symbol_name()
• 1e5e790: 8258018: Remove arrayOop.inline.hpp
• 6693611: 8253797: [cgroups v2] Account for the fact that swap accounting is disabled on some systems
• 6be1f56: 8257450: Start of release updates for JDK 17
• d163c6f: 8258015: [JVMCI] JVMCI_lock shouldn't be held while initializing box classes
• ... and 237 more: https://git.openjdk.java.net/jdk/compare/1241f800023996d33b39a2b881b2bf7e3b7201c3...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@pavelrappo, @Martin-Buchholz) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[pavelrappo]

/sponsor

[openjdk]

@pavelrappo The change author (@johnlinp) must issue an integrate command before the integration can be sponsored.

[johnlinp]

Thank you all for the review and discussion. This is my first time contributing code in jdk, and it's a really nice experience.

[johnlinp]

/integrate

[openjdk]

@johnlinp
Your change (at version d16a35b) is now ready to be sponsored by a Committer.

[pavelrappo]

/sponsor

[openjdk]

@pavelrappo @johnlinp Since your change was applied there have been 247 commits pushed to the master branch:

• d4282b0: 8257731: Remove excessive include of stubRoutines.hpp
• 80dac5a: 8257912: Convert enum iteration to use range-based for loops
• 164c55b: 8258056: jdk/javadoc/doclet/testHtmlTableTags/TestHtmlTableTags.java fails against jdk17
• 42264b2: 8257971: (fs) Remove unused code from WindowsPath.subpath(begin, end)
• 3342eca: 8258054: runtime/sealedClasses/GetPermittedSubclassesTest.java fails w/ jdk17
• f574056: 8256424: Move ciSymbol::symbol_name() to ciSymbols::symbol_name()
• 1e5e790: 8258018: Remove arrayOop.inline.hpp
• 6693611: 8253797: [cgroups v2] Account for the fact that swap accounting is disabled on some systems
• 6be1f56: 8257450: Start of release updates for JDK 17
• d163c6f: 8258015: [JVMCI] JVMCI_lock shouldn't be held while initializing box classes
• ... and 237 more: https://git.openjdk.java.net/jdk/compare/1241f800023996d33b39a2b881b2bf7e3b7201c3...master

Your commit was automatically rebased without conflicts.

Pushed as commit 37dc675.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.