RFR: 8021591 : (s) Additional explicit null checks

[Mike Duigou]

On Aug 19 2013, at 15:35 , Martin Buchholz wrote:

> My feeling is that the JDK specs have been creeping in the direction of excessive pedantry and doc lawyerism.  I think it's overall a benefit of Java that its documentation is more readable (at the cost of being a little less precise) than the typical ISO spec.

There is a definite tension here. We would like to keep the documentation and specification as readable as possible while still being sufficiently exacting so that behaviour of an API can be correctly predicted by a reader. It goes further than that though because Oracle employs an entire group of engineers who examine the JDK API javadocs looking for normative statements and then write tests to confirm that implementations conform to the API documentation/specification.  The number and quality of tests they provide to ensure conformance has been steadily increasing (and accelerating). Is this a good thing? To me it seems so. When I hear that people encounter problems (other than performance) when switching among Vector<->ArrayList<->LinkedList<->CopyOnWriteArrayList or HashMap<->ConcurrentHashMap or TreeSet<->ConcurrentSkipListSet because of arbitrary corner case differences between implementations I become smy

> 
> So we usually say "returns true if the foo got frozzled" instead of the more precise "returns true if and only if the foo got frozzled".
> It's "obvious" that an NPE is thrown on an attempt to derefence a null arg, which might not happen in some corner case.  I think it's overall a detriment if either extra null checks are inserted or the spec is made more precise but less readable.

Unfortunately the need for NPE documentation is something that seems to have recently gotten worse. It seems to becoming accepted that it is necessary to document what happens when a null reference is passed even when the parameter provides no specific mention of null handling. I blame the plethora of APIs that for inadequate reasons use "null" as their universal solvent--absent, "don't care", default, unknown, maybe-it-works. This could include some JDK APIs such as use of null Comparator to mean "natural order". I remain hopeful that the JSR 305 annotations will reduce some of the burden of documenting null handling. 

> Consider the Throws spec for 
> http://download.java.net/jdk8/docs/api/java/util/TreeMap.html#containsKey(java.lang.Object)
> 
> ClassCastException - if the specified key cannot be compared with the keys currently in the map
> 
> That's incomplete and imprecise, but don't pessimize by adding code to compare the arg with every key in the map!  (which is the equivalent of what is happening here)

Yes, more precise would be to say "could not be compared to some key currently already in the map", but the intent was clear.

On the opposite side we have regular requests (https://bugs.openjdk.java.net/browse/JDK-7014079 plus duplicates it references) to "fix" the specification of HashSet contains() so that it mentions it considers the hashCode() value in addition to equals(). The belief expressed by these reporters is generally that if the JDK developers had fully specified the contract in the HashSet.contains() method along with it's actual behaviour then they would not have used the class incorrectly (generally missing or incorrect equals()/hashCode()).

> A vote from me for maintaining the already high level of pedantry we have, but not raising it any further.

I always imagine it's more like limbo. ;-)

Mike

> 
> 
> 
> On Mon, Aug 19, 2013 at 3:15 PM, Mike Duigou <mike.duigou at oracle.com> wrote:
> 
> On Aug 1 2013, at 08:57 , Alan Bateman wrote:
> 
> > On 26/07/2013 16:31, Mike Duigou wrote:
> >> Hello all;
> >>
> >> This patch adds some missing checks for null that, according to interface contract, should be throwing NPE. It also improves the existing tests to check for these cases.
> >>
> >> http://cr.openjdk.java.net/~mduigou/JDK-8021591/0/webrev/
> >>
> >> The changes to src/share/classes/java/util/concurrent/ConcurrentHashMap.java will be synchronized separately with the jsr166 workspace. They are part of this review to avoid test failures.
> >>
> >> Mike
> > As retainAll and removeAll are long standing methods, are there are cases where we might now throw NPE when we didn't previously?
> 
> Yes.
> 
> retainAll(null) and removeAll(null) will more consistently throw NPE. Previously the NPE was not thrown by some collections when the collection they were empty.
> 
> > I'm just wondering if any of these need to be looked at more closely, minimally to get into release/compatibility notes.
> 
> I will definitely tag this issue for release notes mention.
> 
> > -Alan
> 
>