spotBugs and JDK-8194978: Javac produces dead code for try-with-resource
Enrico Olivelli
eolivelli at gmail.com
Mon Jan 28 11:37:11 UTC 2019
Vicente
I see your concerns, I just wanted to try to suggest an alternative.
I guess it will be up to spotbugs team to solve the issue somehow
Thank you very much
Enrico
Il giorno lun 28 gen 2019 alle ore 12:33 Vicente Romero
<vicente.romero at oracle.com> ha scritto:
>
> Hi,
>
> On 1/25/19 3:44 PM, Enrico Olivelli wrote:
>
> Up
>
> Regards
> Enrico
>
> Il 10 gen 2019 8:13 PM, "Enrico Olivelli" <eolivelli at gmail.com> ha scritto:
>>
>> Kindly pinging on this..
>>
>> Cheers
>> Enrico
>>
>> Il lun 7 gen 2019, 14:28 Enrico Olivelli <eolivelli at gmail.com> ha scritto:
>>>
>>> Two questions/ideas:
>>> 1) Wouldn't it be possible for javac to annotate such code blocks and
>>> write into the class file that that code is "synthetic" ? I really
>>> don't know where to store such flags, but maybe someone could
>>> elaborate more this idea
>
>
> I don't think that this is a good idea. There is no perfect fix in this area as we have discussed. Adding annotations to blocks doesn't seems like the right direction here
>
>>> 2) Why isn't javac adding the "$closeResource" method ?
>
>
> that method is not being generated anymore in recent versions of javac
>
>>>
>>> Enrico
>
>
> Vicente
>
>>>
>>> Il giorno ven 9 nov 2018 alle ore 23:01 Alex Buckley
>>> <alex.buckley at oracle.com> ha scritto:
>>> >
>>> > 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
>>> > >>
>>> > >
>>
>> --
>>
>>
>> -- Enrico Olivelli
>
>
More information about the compiler-dev
mailing list