biddyweb / checker-framework

Automatically exported from code.google.com/p/checker-framework
Other
0 stars 1 forks source link

parseInt should accept nullable strings #287

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Some minor feedback on the jdk annotations:

java.lang.Integer#parseInt should probably accept nullable strings, since it 
guards against null input and throws a NumberFormatException. Requiring that 
the input to parseInt is non-null will never prevent a NullPointerException, 
and is insufficient to prevent NumberFormatExceptions.

Original issue reported on code.google.com by cus...@google.com on 5 Dec 2013 at 3:52

GoogleCodeExporter commented 9 years ago
Liam-

Thanks for raising this issue.  Your analysis of the code is exactly correct.  
Nonetheless, I am going to leave the JDK annotation as is.

The reason is that it is never useful or desirable to pass null to parseInt.  
Whether the exception thrown is NullPointerException or 
IllegalArgumentException or something different is secondary to specifying the 
correct use of the method.  Section 2.4.3, "Annotations indicate normal 
behavior", of the manual discusses this issue and uses Double.valueOf as an 
example:  
http://types.cs.washington.edu/checker-framework/current/checkers-manual.html#an
notate-normal-behavior

Please let me know if this doesn't make sense or I have mis-analyzed the 
situation.

Original comment by michael.ernst@gmail.com on 5 Dec 2013 at 1:41

GoogleCodeExporter commented 9 years ago
That makes sense, thanks for the explanation. My argument is the same one that 
you refute in the manual, so if I had seen that section I wouldn't have 
bothered you with my question.

The only comment I have is that the definition of 'normal' behaviour is 
subjective. We have some code that converts a nullable string into an integer, 
and returns a default value if the string is not well-formed. I may continue 
using an annotated parseInt to avoid rewriting the following snippet:

try {
  return Integer.parseInt(maybeNullString);
} catch (NumberFormatException e) {
  return 0;
}

as:

if (maybeNullString == null) {
  return 0;
}
try {
  return Integer.parseInt(maybeNullString);
} catch (NumberFormatException e) {
  return 0;
}

Original comment by cus...@google.com on 5 Dec 2013 at 3:25

GoogleCodeExporter commented 9 years ago
I agree you don't want to have to rewrite the code.

My personal preference -- if possible in your situation -- would be to abstract 
the code into a method called parseIntSafely() or parseIntOrNull, and suppress 
the warning on that method.

Original comment by michael.ernst@gmail.com on 5 Dec 2013 at 4:24