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

[Alex Buckley]

On 11/9/2018 12:31 PM, Vicente Romero wrote:
> 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.

I confirm that the explicit null check on the resource variable is 
required, per the translation in 
https://docs.oracle.com/javase/specs/jls/se11/html/jls-14.html#jls-14.20.3.1-140 
-- it has been specified that way since JLS7.

> 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.

I confirm it's acceptable to not perform the null check in generated 
code if you can prove the resource variable is non-null. Frankly, 
though, there is so much that can occur in a "new class expression" -- 
`new Outer(a(b(c)))<>.new <String>Inner(d(e(f)))` -- that I would be 
wary of trying to prove anything.

> 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.

Right, let's not be too clever.

Alex

> 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
>>
>