False potential resource leak for java.nio.file.Path.getFileSystem()

[basilevs]

Test steps

In JDT create a method:

public void reproducer() {
	java.nio.file.FileSystem filesystemShouldNotBeclosed = java.nio.file.Path.of(".").getFileSystem();
	filesystemShouldNotBeclosed.getSeparator();
}

Expected result

No warnings

Actual result

Potential resource leak: 'filesystemShouldNotBeclosed' may not be closed

This seems like a major flaw of NIO type system, and deserves an explicit analysis exception.

[stephan-herrmann]

ecj already maintains a list of "resource-free closeables", which resulted from a request by Brian Goetz when they made Stream extend AutoCloseable. A stream can be closed, but it's not a problem if it isn't, and we agreed that it's OK if static analysis uses this specific background knowledge.

If I understand this request correctly, class java.nio.file.FileSystem should be put in the same bucket, right? "Resource-free" isn't fully correct for FileSystem, but being long-lived a FileSystem is surely not a good candidate for try-with-resources or such.

Clearly ecj cannot capture all such may-be-closed types in the Java universe in this list. That's why users have the option to annotated such classes as @NotOwning. Unfortunately, I haven't yet found the time to finalize #2258, which would put the solution also for FileSystem into your hands.

[basilevs]

@stephan-herrmann ZipFileSystem is a candidate for try-with-resources by itself, it has resources and is short-lived. However, the reference returned from Path.getFileSystem() is semantically non-owning and is more likely to be long-lived FS.

Ideally, a method's (Path.getFileSystem()) return value should be exempt from analysis and not a type.

[stephan-herrmann]

@stephan-herrmann ZipFileSystem is a candidate for try-with-resources by itself, but the reference returned from Path.getFileSystem() is semantically non-owning.

Thanks, so some filesystems should be closed while others shouldn't. If we know which ones fall in which category this is not unlike what we have in the hierarchy of, e.g., Reader: see FileReader vs StringReader.

Ideally, a method's (Path.getFileSystem()) return value should be exempt from analysis and not a type.

We could express that with an external @NotOwning on that method, but then things get murky when looking at ZipPath.getFileSystem().

With that the class based solution looks cleaner to me. We could say that FileSystem is generally exempted from the analysis, but subclasses ZipFileSystem and JrtFileSystem would be exceptions from the exception (i.e., treated as regular closeable resources). And when anybody comes up with new implementations of FileSystem then they may freely choose between an explicit @Owning or @NotOwning on the class.

Anyway, for the analysis to distinguish kinds of FileSystems, the concrete type must be used in the program. But that restriction applies to class-based and method-based approaches alike.

[basilevs]

I'll leave that up to you, but IMO the result of Path.getFileSystem() should not be closed irrespective of its actual type. The code that allocated the FS should close it and this particular instance of Path never does - it is owned by FS, not he other way around.

Example of short-lived FileSystem use:

import java.util.*;
import java.net.URI;
import java.nio.file.Path;
import java.nio.file.*;

public class ZipFSPUser {
    public static void main(String [] args) throws Throwable {
        Map<String, String> env = new HashMap<>(); 
        env.put("create", "true");
        // locate file system by using the syntax 
        // defined in java.net.JarURLConnection
        URI uri = URI.create("jar:file:/codeSamples/zipfs/zipfstest.zip");
        
        try (FileSystem zipfs = FileSystems.newFileSystem(uri, env)) {
            Path externalTxtFile = Paths.get("/codeSamples/zipfs/SomeTextFile.txt");
            externalTxtFile.getFileSystem().close(); // logical error, breaks code below
            Path pathInZipfile = zipfs.getPath("/SomeTextFile.txt");          
            // copy a file into the zip file
            Files.copy( externalTxtFile,pathInZipfile, 
                    StandardCopyOption.REPLACE_EXISTING ); 
        } 
    }
}

Note how there are multiple instances of Path object within the try block and each has a reference to FileSystem. Yet, none of them should close the FS, because it would break subsequent FS reads.