specification for null handling in String, StringBuilder, etc.

[Jim Gish]

On 06/12/2012 01:40 AM, David Holmes wrote:
> Hi Jim,
>
> On 12/06/2012 7:59 AM, Jim Gish wrote:
>> While triaging outstanding String bugs, I was looking at 4247235, "(spec
>> str) StringBuffer.insert(int, char[]) specification is inconsistent"
>>
>> Although the description is out of date w.r.t. the current code, I did
>> find what I would consider weaknesses (maybe even holes) in the specs
>> relative to this issue.
>>
>> It appears that the common practice is to spec checked exceptions but
>> not unchecked exceptions (which I understand). For example, in the case
>> mentioned the spec indicates that StringIndexOutOfBoundsException is
>> thrown if the offset is invalid, and is silent about the consequences of
>> the char[] being null. In the latter case, it throws
>> NullPointerException, but the str == null is not checked, rather it
>> simply tries to access str.length and fails if str is null.
>>
>> My personal feeling is that pre-conditions should be explicitly checked
>> for and be spec'd.
>
> This is very, very common in the core libraries. Rather than document 
> every single method where a null parameter triggers NPE they are often 
> covered (directly or indirectly) by blanket statements of the form 
> "unless otherwise stated ...".

For String -- that's fine in that it is covered, but for StringBuilder 
and StringBuffer, it's not.  In fact, the constructors indicate an NPE 
will be thrown for null args, as do some of the methods (e.g.  getchars, 
indexOf,..l).  However, the insert method for char[] are silent.  I 
don't like the inconsistency.  Furthermore, I don't think the user 
should have to go read the blanket statement for String (or some other 
/different /class), when all we have to say is that either "str - a 
non-null character array" or "Throws NPE - if the str is null" (as was 
suggested by Stephen).

I recommend we go with documenting the parameters are required to be 
non-null -- more of a complete contract specification -- for now and 
transition to @NotNull if/when JSR 305 is approved.

Jim
>
> I'm strongly of the opinion that people should be reading the complete 
> specification for any type they use, not just individual methods. So I 
> don't have a problem with not documenting this on each method - there 
> are likely to be far more significant misunderstandings of behaviour 
> if you don't read all the docs. But I understand others will see this 
> from the other side.
>
> Also note that the handling of unchecked exceptions by JavaDoc 
> complicates things because overriding implementations need to re-state 
> that they throw the NPE (or whatever it may be), using @inheritDoc, 
> even if there is no change from the superclass or interface 
> specification of the method.
>
> David
> -----
>
>
>> One might argue that the spec is complete, because it says that "The
>> overall effect is exactly as if the second argument were converted to a
>> string by String.valueOf( char[] ),..." If you look at the class javadoc
>> for String there is a statement that "Unless otherwise noted, passing a
>> null argument to a constructor or method in this class will cause a ||
>> <http://docs.oracle.com/javase/7/docs/api/java/lang/NullPointerException.html>|NullPointerException| 
>>
>> to be thrown." However, if the user simply looks at the javadoc for
>> String.valueOf( char[] ) nothing is said about null handling there. The
>> user would have to have read the class javadoc to catch the reference to
>> NullPointerException. Seems like an unreasonably indirect chain to have
>> to follow, when all we'd have to say is that the original insert method
>> throws NPE if the char[] is null.
>>
>> I suggest we improve the readability here (and in related places) and
>> tell the user straight off the effect of passing null.
>>
>> Cheers
>> Jim Gish
>>
>>