Resource leak checker does not seem to honor try-with-resources in some cases

[jacek-lewandowski]

I have the following method:

    static List<File> list(@Owning DirectoryStream<Path> stream) throws IOException
    {
        try
        {
            return StreamSupport.stream(stream.spliterator(), false)
                                .map(File::new)
                                .filter((f) -> !f.isDirectory())
                                .collect(Collectors.toList());
        }
        finally
        {
            stream.close();
        }
    }

It passes the analysis, however if I refactor it into:

    static List<File> list(@Owning DirectoryStream<Path> stream) throws IOException
    {
        try (stream)
        {
            return StreamSupport.stream(stream.spliterator(), false)
                                .map(File::new)
                                .filter((f) -> !f.isDirectory())
                                .collect(Collectors.toList());
        }
    }

the analysis fails with required.method.not.called - is it possible that it automatically detects the close method was called on the parameter?

[kelloggm]

I suspect that there is an issue with our code for handling try-with-resources: all of the examples in our test suite currently involve allocating a new local variable in the try, not using a formal.

[kelloggm]

I did a bit of investigation into this, and it is probably going to be difficult to fix. The core problems are the following:

• we currently handle try-with-resources in the last stage of the analysis (i.e., the consistency analyzer). It operates on the CFG, and checks for the RESOURCE_VARIABLE ElementKind on the LHS of assignment nodes. This works for code that declares a new, local resource variable, but not for code that handles parameters like this example.
• the Checker Framework's CFG doesn't have a node kind for the resources within a try-with-resources block: they are scanned as-is and seem to be indistinguishable at the CFG level. See CFGTranslationPhaseOne.java:3538.
• the code we have in the MustCallAnnotatedTypeFactory to try to handle try-with-resources (around line 420) doesn't seem to do anything. Removing it doesn't change the results of the tests.
• the other obvious place to fix this is in MustCallTransfer, but then we face the same problem we'd face in the consistency analyzer: there is no CFG marker for "try resource", so we don't know when to remove close()
• the CF's Tree visitors don't visit TryTrees, since they are untyped, anyway. So, even if we wanted to do this in the type factory, I don't think we could.

Here's a simplified test case:

// test case for https://github.com/typetools/checker-framework/issues/6037

import java.net.Socket;
import org.checkerframework.checker.mustcall.qual.Owning;

public class TryWithResourcesParam {
  public void test(@Owning Socket s) {
    try (s) {

    } catch (Exception e) {

    }
  }
}

[msridhar]

@kelloggm it seems we could solve this problem if we exposed the right info in the CFG, is that right? I think it is appropriate information to expose. Arguably the CFG is incomplete right now since it does not expose this information, nor does it contain an explicit close() call in a finally block or some similar modeling.

[msridhar]

A slightly analogous case might be synchronized blocks, which are handled here. We could add a similar TryWithResourcesNode at the start of the corresponding block. I'm not sure we need a special node at the end of the block.

[kelloggm]

it seems we could solve this problem if we exposed the right info in the CFG, is that right?

Yes, I think that would be the easiest way to solve this problem. It's unfortunate, but I don't think there's any way for us to fix this at the RLC level without changing the framework itself.

[jacek-lewandowski]

hey, is there any chance that it will be fixed in the upcoming July release?

[msridhar]

@jacek-lewandowski I think a fix for the July release is unlikely at this point. I think @mernst was planning on making the requisite changes to the dataflow library. After that we will have to update the Resource Leak Checker based on the newly-exposed information. The latter step shouldn't take long, but given the upcoming release date, my expectation is that it won't be done in time. Once this is fixed, we can consider a patch release if it is causing a serious inconvenience for you.

[mernst]

Thanks for the bug report! I will change the dataflow library to address this. However, I will not be able to do so in time for the August 1 release.