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

[Vicente Romero]

Hi,

Sorry for the late reply. My evaluation on this issue is that javac is 
generating code according to the spec, see JLS11 14.20.3.1 [1]. There is 
an explicit check for null on the resource before invoking the close() 
method. This is the null check javac is generating. Fix for JDK-8194978 
went a bit further trying to eliminate dead code for some cases for 
which javac could statically determine that the resource would be 
different from null. In particular if the resource was initialized with 
a new class expression. It could be argued that we could try to analyze 
the body and if we find out that a NPE must be thrown in the body of the 
try, then the null check on the resource would be deemed unnecessary. 
But then we can get to an implementation that will be out of sync with 
the spec plus it probably won't cover all cases.

Thanks,
Vicente

[1] 
https://docs.oracle.com/javase/specs/jls/se11/html/jls-14.html#jls-14.20.3.1

On 11/9/18 10:22 AM, Ismael Juma wrote:
> Any comments on this? Many people are disabling spotBugs when 
> compiling with Java 11 due to this issue:
>
> https://github.com/spotbugs/spotbugs/issues/756
>
> Ismael
>
> On Thu, Sep 13, 2018 at 10:05 PM Ismael Juma <mlists at juma.me.uk 
> <mailto:mlists at juma.me.uk>> 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
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20181109/f7fbed4d/attachment-0001.html>