JDK-8313644 : ConcurrentMap should use {@inheritDoc} consistently
  • Type: Bug
  • Component: core-libs
  • Sub-Component: java.util.concurrent
  • Priority: P4
  • Status: Open
  • Resolution: Unresolved
  • Submitted: 2023-08-02
  • Updated: 2023-08-02
Related Reports
Relates :  
JDK-8159527 - Collections mutator methods should all be marked as optional operations
Description
Some of its methods use the inheritDoc tag and some do not. In particular the following methods do not, and their wording differs only slightly from the corresponding wording that could be inherited from the Map interface:

putIfAbsent()
remove(key, value)
replace(key, value)
replace(key, oldValue, newValue)

Sometimes the ConcurrentMap doc includes a code snippet and additional text such as "except that the action is performed atomically." This can remain and augment the inherited doc.

Note that JDK-8159527 will add the "(optional operation)" wording to some mutator methods where it was missing. This wording will be inherited by the existing ConcurrentMap methods where inheritDoc is already used; it will be inherited by more if usage of inheritDoc is expanded.

It's not clear to me that mutator methods of a ConcurrentMap are in fact optional. If we want to remove the "(optional operation)" wording from ConcurrentMap, we'll need to investigate doing something other than adding inheritDoc tags.
Comments
There are definitely tradeoffs regarding inheritance and inheritDoc. In this context I'm mainly talking about the main body of the method spec. For Map and ConcurrentMap, the history here is that ConcurrentMap was added in 1.5 and some of its methods were promoted to be default methods of Map in Java 8. Unfortunately because of ... entropy ... some of the method docs ended up using inheritDoc and some did not. I admit I was thinking they should all be changed to use inheritDoc. However, that might end up being strange because many of them will end up with "(optional operation)" which I'm adding as part of JDK-8159527. It would be kind of strange for a ConcurrentMap implementation not to implement something like computeIfAbsent, since those composite operations were the reason for introducing ConcurrentMap in the first place! So perhaps it would be preferable for ConcurrentMap to consistently *avoid* using inheritDoc, and instead just copy the wording but omit (optional operation). That would be OK with me too. Regarding ConcurrentMap.replace(): > UnsupportedOperationException - if the put operation is not supported by this map Yes I agree this is wrong. I do think it was probably written with the default implementation in mind. There are other examples of this, such as List.sort() saying > UnsupportedOperationException - if the list's list-iterator does not support the set operation which is also quite wrong. The standard wording for "throws UOE" seems to be something like "if the <op> operation is not supported by this <collection>". (There are a couple variations on this that seem acceptable though.) I've gone through the collections interfaces and cleaned up the ones that incorrectly refer to a particular method that might be used as part of an implementation. See my PR for JDK-8159527 here: https://github.com/openjdk/jdk/pull/15127 Some of the UOE docs use inheritDoc and some use copied boilerplate text. Oh well, I'll leave that for another day.
02-08-2023
Like I always say, doc comment inheritance is surprisingly difficult and subtle.. --- Often there's a trade-off between copy-pasta duplication and brittle inheritance. I'm thinking now that copy-pasta is better than doc inheritance when the doc element being inherited has (in principle) a refined spec in the subclass. So, use doc inheritance for e.g. spec of ClassCastException, but not for the main method doc. --- I can imagine ConcurrentMaps that disallow plain put() but allow replace() via the principle that all map update methods should be forced to consider any previous value, which makes the spec for ... https://download.java.net/java/early_access/jdk22/docs/api/java.base/java/util/concurrent/ConcurrentMap.html#replace(K,V) UnsupportedOperationException - if the put operation is not supported by this map ... terribly wrong. Probably UOE spec should not refer to the name of some other method! The UOE spec was perhaps (wrongly!) influenced by the default implementation of Map#replace
02-08-2023
[~martin] Any thoughts on this?
02-08-2023