spotBugs and JDK-8194978: Javac produces dead code for try-with-resource

[Jonathan Gibbons]

Ismael,

What about the case where the initialization of the variable 
unexpectedly returns null?

Yes, you'll get an NPE when you invoke a method, but the finally block 
will still get executed (JLS11 14.20.2)

The null check in the finally block protects against trying to call 
close on a variable whose value is null.

-- Jon


On 09/13/2018 10:05 PM, Ismael Juma wrote:
> Hi all,
>
> JDK-8194978 introduced some changes to the bytecode generated by javac 
> for the try with resource construct. In the following code, it seems 
> to generate a null check on a reference after invoking a method on it:
>
>     public static void readFileAsString(String path) throws IOException {
>         try (FileChannel fc = FileChannel.open(Paths.get(path))) {
>             fc.size();
>         }
>
>     }
>
> In line 16 to 22 of the bytecode, it looks like we check for null 
> after calling a method on the fc reference:
>
>       16: aload_1
>       17: invokevirtual #6                  // Method 
> java/nio/channels/FileChannel.size:()J
>       20: pop2
>       21: aload_1
>       22: ifnull        52
>       25: aload_1
>       26: invokevirtual #7                  // Method 
> java/nio/channels/FileChannel.close:()V
>
> Is this intentional? I ask because this pattern triggers a spotBugs 
> warning since it often implies a bug in user's code:
>
> RCN | Nullcheck of fc at line 10 of value previously dereferenced in 
> TryTest.readFileAsString(String, Charset)
>
> Note that this works fine in Java versions older than Java 11. Since 
> this spotBugs warning is generally useful, it would be handy if javac 
> did not trigger it. Alternatively, if there's a good way to detect 
> the code that was generated by javac, spotBugs could be updated to 
> ignore it. For reference, this was discussed in the spotBugs issue 
> tracker:
>
> https://github.com/spotbugs/spotbugs/issues/756
>
> And method bytecode in full:
>
>   public static void readFileAsString(java.lang.String) throws 
> java.io.IOException;
>     Code:
>        0: aload_0
>        1: iconst_0
>        2: anewarray     #2                  // class java/lang/String
>        5: invokestatic  #3                  // Method 
> java/nio/file/Paths.get:(Ljava/lang/String;[Ljava/lang/String;)Ljava/nio/file/Path;
>        8: iconst_0
>        9: anewarray     #4                  // class 
> java/nio/file/OpenOption
>       12: invokestatic  #5                  // Method 
> java/nio/channels/FileChannel.open:(Ljava/nio/file/Path;[Ljava/nio/file/OpenOption;)Ljava/nio/channels/FileChannel;
>       15: astore_1
>       16: aload_1
>       17: invokevirtual #6                  // Method 
> java/nio/channels/FileChannel.size:()J
>       20: pop2
>       21: aload_1
>       22: ifnull        52
>       25: aload_1
>       26: invokevirtual #7                  // Method 
> java/nio/channels/FileChannel.close:()V
>       29: goto          52
>       32: astore_2
>       33: aload_1
>       34: ifnull        50
>       37: aload_1
>       38: invokevirtual #7                  // Method 
> java/nio/channels/FileChannel.close:()V
>       41: goto          50
>       44: astore_3
>       45: aload_2
>       46: aload_3
>       47: invokevirtual #9                  // Method 
> java/lang/Throwable.addSuppressed:(Ljava/lang/Throwable;)V
>       50: aload_2
>       51: athrow
>       52: return
>     Exception table:
>        from    to  target type
>           16    21    32   Class java/lang/Throwable
>           37    41    44   Class java/lang/Throwable
>
> Thanks,
> Ismael