Re: specification for null handling in String, StringBuilder, etc.
On 06/12/2012 07: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 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 And I see no problem to check NPE with a str.length if there is a comment saying something like 'implicit NPE check'. Rémi - 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
Re: specification for null handling in String, StringBuilder, etc.
On 06/12/2012 07:40 AM, David Holmes wrote:
On 12/06/2012 7:59 AM, Jim Gish wrote:
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
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.
In JSR-310 and my other projects (OpenGamma, Joda-*) I explicitly
document null handling in a fairly non-invasive way, eg
https://github.com/ThreeTen/threeten/blob/master/src/main/java/javax/time/ZonedDateTime.java#L276
* @param foo the foo thing, not null
* @param bar the bar thing, null treated as a default bar
* @param baz the baz thing, may be null
* @return the foo-bar-baz result, not null
*/
public static FooBarBaz of(Foo foo, Bar bar, Baz baz) {
The rule I use is to add a clause to the end of the @param that
defines the null behaviour:
- , not null - this parameter does not accept null, null will throw
NPE or be undefined
- , may be null - this parameter does accept null
- , null ... - explain what null does
Similar explanations for @return:
- , not null - this method will never return null
- , may be null - this method may return null
- , null ... - explain when null returned, eg. , null if not found
This approach works well as a common pattern that developers can
understand without further reading or learning. It provides much more
data on null-handling, and in the location that the developer actually
wants it. I would love to see this approach more generally in the JDK
(and I suspect you might be able to get JUG hackathons to support
adding the text in to core Java classes)
Stephen
Re: specification for null handling in String, StringBuilder, etc.
On 12/06/2012 06:40, David Holmes wrote:
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
Right, @throws NullPointerException can be considered clutter so it's
common to see a blanket statement in the class or package description.
In the case of String it already has:
* p Unless otherwise noted, passing a ttnull/tt argument to a
constructor
* or method in this class will cause a {@link NullPointerException} to be
* thrown.
This was added via 4703640 a long time ago.
Clearly a blanket statement like this doesn't make sense in all cases as
there will be APIs that define many methods that allow null.
-Alan
Re: specification for null handling in String, StringBuilder, etc.
On Tue, Jun 12, 2012 at 9:04 AM, Alan Bateman alan.bate...@oracle.com wrote:
On 12/06/2012 06:40, David Holmes wrote:
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
Right, @throws NullPointerException can be considered clutter so it's common
to see a blanket statement in the class or package description. In the case
of String it already has:
* p Unless otherwise noted, passing a ttnull/tt argument to a
constructor
* or method in this class will cause a {@link NullPointerException} to be
* thrown.
This was added via 4703640 a long time ago.
Clearly a blanket statement like this doesn't make sense in all cases as
there will be APIs that define many methods that allow null.
-Alan
Sounds like a job for JSR-305 annotations if/when those are ever approved.
Thanks,
Dave
Re: specification for null handling in String, StringBuilder, etc.
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
Re: specification for null handling in String, StringBuilder, etc.
On 6/12/2012 6:04 AM, Alan Bateman wrote:
On 12/06/2012 06:40, David Holmes wrote:
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
Right, @throws NullPointerException can be considered clutter so it's
common to see a blanket statement in the class or package description.
In the case of String it already has:
* p Unless otherwise noted, passing a ttnull/tt argument to a
constructor
* or method in this class will cause a {@link NullPointerException}
to be
* thrown.
This was added via 4703640 a long time ago.
Clearly a blanket statement like this doesn't make sense in all cases
as there will be APIs that define many methods that allow null.
-Alan
Yes, in most cases lots of explicit @throws NullPointerException if foo
is null is noise and should be avoided as a documentation pattern. No
one should be that surprised if a method nulls a NPE after having a null
passed to it!
My preferred long-term solution here would be to use annotations to
documentation the null-handling behavior, including class-level
defaults, which would then allow tools to check null-handling of callers.
-Joe
Re: specification for null handling in String, StringBuilder, etc.
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 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
