RFR: 8325304: Several classes in java.util.jar and java.util.zip don't specify the behaviour for null arguments [v5]

[Jaikiran Pai]

On Wed, 7 Feb 2024 09:44:25 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   make <p> usage consistent with other similar usages in the file
>
> src/java.base/share/classes/java/util/zip/CheckedOutputStream.java line 48:
> 
>> 46:      * value to either {@code out} or {@code cksum} will cause
>> 47:      * a {@link NullPointerException} to be thrown from the
>> 48:      * {@code write} methods of this {@code CheckedOutputStream}.
> 
> What is the reason for specifying the NPE in the method description rather than a throws?

These 2 classes, the `CheckedInputStream` and the `CheckedOutputStream` are slightly different from the rest of the classes in this changeset. This javadoc here is for the constructor of the `CheckedInputStream`. The implementation of the constructor just blindly assigns the passed argument to an internal member field and doesn't do any null check on the passed arguments (any subsequent usage of these instance fields within that class, then leads to a NullPointerException). So the constructor isn't throwing any `NullPointerException` here and thus adding a `@throws` here would be incorrect. In theory, we could just change the implementation of this constructor to throw a `NullPointerException` if either of these arguments were null, but that might mean a potential breakage of some weird usage of the CheckedInputStream. So I decided to add the `NullPointerException` detail to the constructor description.

Do you think we should instead do something like this for this class and the `CheckedOutputStream` class:


diff --git a/src/java.base/share/classes/java/util/zip/CheckedInputStream.java b/src/java.base/share/classes/java/util/zip/CheckedInputStream.java
index 10f72b416d1..76cba26986e 100644
--- a/src/java.base/share/classes/java/util/zip/CheckedInputStream.java
+++ b/src/java.base/share/classes/java/util/zip/CheckedInputStream.java
@@ -41,10 +41,7 @@ public class CheckedInputStream extends FilterInputStream {
     private final Checksum cksum;
 
     /**
-     * Creates an input stream using the specified Checksum. A null
-     * value to either {@code in} or {@code cksum} will cause
-     * a {@link NullPointerException} to be thrown from the
-     * {@code read} methods of this {@code CheckedInputStream}.
+     * Creates an input stream using the specified Checksum.
      * @param in the input stream
      * @param cksum the Checksum
      */
@@ -57,6 +54,8 @@ public CheckedInputStream(InputStream in, Checksum cksum) {
      * Reads a byte. Will block if no input is available.
      * @return the byte read, or -1 if the end of the stream is reached.
      * @throws    IOException if an I/O error has occurred
+     * @throws    NullPointerException if this {@code CheckedInputStream} was
+     *            constructed with a {@code null} value for {@code in} or {@code cksum}
      */
     public int read() throws IOException {
         int b = in.read();
@@ -75,7 +74,9 @@ public int read() throws IOException {
      * @param len the maximum number of bytes read
      * @return    the actual number of bytes read, or -1 if the end
      *            of the stream is reached.
-     * @throws     NullPointerException If {@code buf} is {@code null}.
+     * @throws     NullPointerException If {@code buf} is {@code null} or
+     *             if this {@code CheckedInputStream} was
+     *             constructed with a {@code null} value for {@code in} or {@code cksum}.
      * @throws     IndexOutOfBoundsException If {@code off} is negative,
      * {@code len} is negative, or {@code len} is greater than
      * {@code buf.length - off}

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17728#discussion_r1481237216