[java] UseTryWithResources false positive when AutoCloseable helper used

[amarkevich]

Affects PMD Version:

maven-pmd-plugin 3.13.0 (pmd 6.21.0)

Rule: UseTryWithResources

Description:

PMD Failure: UseTryWithResources when resource is not AutoCloseable and closed via external class which implement AutoCloseable

Code Sample demonstrating the issue:

public class PMDUseTryWithResources {

    static class ResourceCloser implements AutoCloseable {
        public static void close(NotAutoCloseable notAutoCloseable) {
            notAutoCloseable.close();
        }

        @Override
        public void close() throws Exception {
            // nothing
        }
    }

    interface NotAutoCloseable {
        void close();
    }

    public void useTryWithResources() {
        NotAutoCloseable notAutoCloseable = null;
        try {
            notAutoCloseable = new NotAutoCloseable() {
                @Override
                public void close() {
                }
            };
        } finally {
            if (notAutoCloseable != null) {
                ResourceCloser.close(notAutoCloseable);
            }
        }
    }
}

Steps to reproduce:

1. mvn install

Running PMD through: Maven

[adangel]

Maybe the problem is, that your helper class ResourceCloser implements unnecessarily AutoClosable.

The rule searches for any method call with the name close, regardless whether it's static or not. So it finds ResourceCloser.close(notAutoCloseable);and tells you, you should use try-with-resources for ResourceCloser...

[amarkevich]

Actually AutoClosable logic is used for another case.
Expected behavior: do not report a problem when static 'close' method used

[adangel]

Expected behavior: do not report a problem when static 'close' method used

I actually disagree - if the thing, that is being closed, is AutoCloseable, then one should use try-with-resources. That's the point. If you use IOUtils.closeQuietly(resource), the rule should still trigger, even though the close method is static.

We have two cases:

1. Non-static close methods: resource.close() - if resource is of type AutoCloseable, then the rule should trigger. In that case, there are no arguments to the close method. If the resource is not a autocloseable resource, then the close call should be ignored.

2. Static close (helper) methods: ResourceClose.close(resource). In that case, it doesn't matter, whether ResourceClose is AutoCloseable or not. What matters is only, whether the (first) argument is AutoCloseable - in that case "resource". If the resource is AutoCloseable, then the rule should trigger. If it isn't, then the close call should be ignored.

Currently, the rule always checks both for the type "AutoCloseable" - the thing, on which the close method is called (might be the resource or in static case the helper class) and also (if present) the argument.

The rule should probably be changed to check the argument - if present (and assume, it's a static method call) and otherwise check the method call target.

The rule is a XPath rule:

pmd/pmd-java/src/main/resources/category/java/bestpractices.xml

Lines 1839 to 1845 in 9ed79f4

	//TryStatement[FinallyStatement//Name[
	tokenize(@Image, '\.')[last()] = $closeMethods
	][
	pmd-java:typeIs('java.lang.AutoCloseable')
	or
	../../PrimarySuffix/Arguments//PrimaryPrefix[pmd-java:typeIs('java.lang.AutoCloseable')]
	]]

The type check in line 1842 should probably only be done, if there are no arguments.

[adangel]

This has been fixed with PMD 7.0.0-rc1.