Merge branch 'pr-1968'

2019-08-18 01:28:07 -03:00
commit febf512e9c
@@ -28,6 +28,14 @@ about the usage and features of the rule designer.
    be entirely wrong. Instead of declaring this as a field and synchronize access to use it from multiple threads,
    a new instance should be created when needed. This rule is also active when using java's quickstart ruleset.

+#### Modified Rules
+
+*   The Java rule {% rule "java/errorprone/CloseResource" %} (`java-errorprone`) now ignores by default instances
+    of `java.util.stream.Stream`. These streams are `AutoCloseable`, but most streams are backed by collections,
+    arrays, or generating functions, which require no special resource management. However, there are some exceptions:
+    The stream returned by `Files::lines(Path)` is backed by a actual file and needs to be closed. These instances
+    won't be found by default by the rule anymore.
+
 ### Fixed Issues

 *   all
@@ -36,6 +44,10 @@ about the usage and features of the rule designer.
    *   [#1862](https://github.com/pmd/pmd/issues/1862): \[java] New rule for MessageDigest.getInstance
 *   java-codestyle
    *   [#1951](https://github.com/pmd/pmd/issues/1951): \[java] UnnecessaryFullyQualifiedName rule triggered when variable name clashes with package name
+*   java-errorprone
+    *   [#1922](https://github.com/pmd/pmd/issues/1922): \[java] CloseResource possible false positive with Streams
+    *   [#1966](https://github.com/pmd/pmd/issues/1966): \[java] CloseResource false positive if Stream is passed as method parameter
+    *   [#1967](https://github.com/pmd/pmd/issues/1967): \[java] CloseResource false positive with late assignment of variable

 ### API Changes

@@ -20,14 +20,17 @@ import net.sourceforge.pmd.RuleContext;
 import net.sourceforge.pmd.lang.ast.Node;
 import net.sourceforge.pmd.lang.java.ast.ASTAllocationExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTArgumentList;
+import net.sourceforge.pmd.lang.java.ast.ASTAssignmentOperator;
 import net.sourceforge.pmd.lang.java.ast.ASTBlock;
 import net.sourceforge.pmd.lang.java.ast.ASTBlockStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType;
 import net.sourceforge.pmd.lang.java.ast.ASTConstructorDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTFormalParameters;
 import net.sourceforge.pmd.lang.java.ast.ASTIfStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTMethodOrConstructorDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTName;
 import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix;
@@ -40,6 +43,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTTryStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclarator;
 import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
 import net.sourceforge.pmd.lang.java.ast.ASTVariableInitializer;
+import net.sourceforge.pmd.lang.java.ast.MethodLikeNode.MethodLikeKind;
 import net.sourceforge.pmd.lang.java.ast.TypeNode;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
 import net.sourceforge.pmd.lang.java.symboltable.VariableNameDeclaration;
@@ -89,7 +93,7 @@ public class CloseResourceRule extends AbstractJavaRule {
            stringListProperty("allowedResourceTypes")
            .desc("Exact class names that do not need to be closed")
            .defaultValues("java.io.ByteArrayOutputStream", "java.io.ByteArrayInputStream", "java.io.StringWriter",
-                    "java.io.CharArrayWriter")
+                    "java.io.CharArrayWriter", "java.util.stream.Stream")
            .build();


@@ -141,7 +145,7 @@ public class CloseResourceRule extends AbstractJavaRule {
        return super.visit(node, data);
    }

-    private void checkForResources(Node node, Object data) {
+    private void checkForResources(ASTMethodOrConstructorDeclaration node, Object data) {
        List<ASTLocalVariableDeclaration> localVars = node.findDescendantsOfType(ASTLocalVariableDeclaration.class);
        List<ASTVariableDeclarator> vars = new ArrayList<>();
        Map<ASTVariableDeclaratorId, TypeNode> ids = new HashMap<>();
@@ -174,13 +178,13 @@ public class CloseResourceRule extends AbstractJavaRule {
                    }
                }

-                if (!isAllowedResourceType(type)) {
+                if (!isAllowedResourceType(type) && !isMethodParameter(var, node)) {
                    ids.put(var.getVariableId(), type);
                }
            }
        }

-        // if there are connections, ensure each is closed.
+        // if there are closables, ensure each is closed.
        for (Map.Entry<ASTVariableDeclaratorId, TypeNode> entry : ids.entrySet()) {
            ASTVariableDeclaratorId variableId = entry.getKey();
            ensureClosed((ASTLocalVariableDeclaration) variableId.jjtGetParent().jjtGetParent(), variableId,
@@ -188,6 +192,41 @@ public class CloseResourceRule extends AbstractJavaRule {
        }
    }

+    /**
+     * Checks whether the variable is initialized from a method parameter.
+     * @param var the variable that is being initialized
+     * @param methodOrCstor the method or constructor in which the variable is declared
+     * @return <code>true</code> if the variable is initialized from a method parameter. <code>false</code>
+     *         otherwise.
+     */
+    private boolean isMethodParameter(ASTVariableDeclarator var, ASTMethodOrConstructorDeclaration methodOrCstor) {
+        if (!var.hasInitializer()) {
+            return false;
+        }
+
+        boolean result = false;
+        ASTVariableInitializer initializer = var.getInitializer();
+        ASTName name = initializer.getFirstDescendantOfType(ASTName.class);
+        if (name != null) {
+            ASTFormalParameters formalParameters = null;
+            if (methodOrCstor.getKind() == MethodLikeKind.METHOD) {
+                formalParameters = ((ASTMethodDeclaration) methodOrCstor).getFormalParameters();
+            } else if (methodOrCstor.getKind() == MethodLikeKind.CONSTRUCTOR) {
+                formalParameters = ((ASTConstructorDeclaration) methodOrCstor).getFormalParameters();
+            }
+            if (formalParameters != null) {
+                List<ASTVariableDeclaratorId> ids = formalParameters.findDescendantsOfType(ASTVariableDeclaratorId.class);
+                for (ASTVariableDeclaratorId id : ids) {
+                    if (id.hasImageEqualTo(name.getImage()) && isResourceTypeOrSubtype(id)) {
+                        result = true;
+                        break;
+                    }
+                }
+            }
+        }
+        return result;
+    }
+
    private ASTExpression getAllocationFirstArgument(ASTExpression expression) {
        List<ASTAllocationExpression> allocations = expression.findDescendantsOfType(ASTAllocationExpression.class);
        ASTExpression firstArgument = null;
@@ -228,7 +267,7 @@ public class CloseResourceRule extends AbstractJavaRule {
                    return true;
                }
            }
-        } else if (refType.jjtGetChild(0) instanceof ASTReferenceType) {
+        } else if (refType.jjtGetNumChildren() > 0 && refType.jjtGetChild(0) instanceof ASTReferenceType) {
            // no type information (probably missing auxclasspath) - use simple types
            ASTReferenceType ref = (ASTReferenceType) refType.jjtGetChild(0);
            if (ref.jjtGetChild(0) instanceof ASTClassOrInterfaceType) {
@@ -307,10 +346,14 @@ public class CloseResourceRule extends AbstractJavaRule {
                boolean criticalStatements = false;

                for (int i = parentBlockIndex + 1; i < tryBlockIndex; i++) {
-                    // assume variable declarations are not critical
-                    ASTLocalVariableDeclaration varDecl = blocks.get(i)
+                    // assume variable declarations are not critical and assignments are not critical
+                    ASTBlockStatement block = blocks.get(i);
+                    ASTLocalVariableDeclaration varDecl = block
                            .getFirstDescendantOfType(ASTLocalVariableDeclaration.class);
-                    if (varDecl == null) {
+                    ASTStatementExpression statementExpression = block.getFirstDescendantOfType(ASTStatementExpression.class);
+
+                    if (varDecl == null && (statementExpression == null
+                            || statementExpression.getFirstChildOfType(ASTAssignmentOperator.class) == null)) {
                        criticalStatements = true;
                        break;
                    }
@@ -0,0 +1,14 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.java.rule.errorprone.closeresource;
+
+
+/**
+ * This Statement has nothing to do with {@link java.sql.Statement}. So using this,
+ * should not trigger the rule CloseResource, since this class is not autoclosable.
+ */
+public class Statement {
+
+}
@@ -1117,6 +1117,137 @@ public class CloseResourceCase {
            e.printStackTrace();
        }
    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>#1966 [java] CloseResource false positive if Stream is passed as method parameter</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import java.io.*;
+
+public class CloseResourceFP {
+    public void check(InputStream in) {
+        if (in instanceof FileInputStream) {
+            FileInputStream fin = (FileInputStream) in;
+            doCheck(fin);
+        } else if (in instanceof ByteArrayInputStream) {
+            ByteArrayInputStream bin = (ByteArrayInputSream) in;
+            doCheck(bin);
+        } else {
+            BufferedInputStream buf = new BufferedInputStream(in);
+            doCheck(buf);
+        }
+    }
+
+    public void dump(final Writer writer) {
+        final PrintWriter printWriter = writer instanceof PrintWriter ? (PrintWriter) writer : new PrintWriter(writer);
+        printWriter.println(this);
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>#1967 [java] CloseResource false positive with late assignment of variable</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import java.io.*;
+
+public class CloseResourceFP {
+    public void check(File outputFile) {
+        final OutputStream os;
+        if (outputFile == null) {
+            os = System.out;
+        } else if (outputFile.isAbsolute()) {
+            os = Files.newOutputStream(outputFile.toPath());
+        } else {
+            os = Files.newOutputStream(new File(getProject().getBaseDir(), outputFile.toString()).toPath());
+        }
+
+        try (Writer writer = new BufferedWriter(new OutputStreamWriter(os, "UTF-8"))) {
+            renderer.render(cpd.getMatches(), writer);
+        }
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>#1922 [java] CloseResource possible false positive with Streams</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import java.util.*;
+import java.util.function.*;
+import java.util.regex.*;
+import java.util.stream.*;
+
+public class CloseResourceStream {
+     public static <T> Stream<MatchResult<T>> filterResults(List<T> candidates, Function<T, String> matchExtractor, String query, MatchSelector<T> limiter) {
+        if (query.length() < MIN_QUERY_LENGTH) {
+            return Stream.empty();
+        }
+
+        // violation here
+        Stream<MatchResult<T>> base = candidates.stream()
+                                                .map(it -> {
+                                                    String cand = matchExtractor.apply(it);
+                                                    return new MatchResult<>(0, it, cand, query, new TextFlow(makeNormalText(cand)));
+                                                });
+        return limiter.selectBest(base);
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>#1076 [java] CloseResource false positive on non-SQL classes called Statement</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import net.sourceforge.pmd.lang.java.rule.errorprone.closeresource.Statement;
+
+public class CloseResourceStatementFP {
+    public void check() {
+        Statement s = new Statement();
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>False-negative if only byte array is passed in as method parameter</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>6</expected-linenumbers>
+        <code><![CDATA[
+import java.io.*;
+
+public class CloseResourceFN {
+    public Object deserialize(byte[] bytes) {
+        try {
+            ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(bytes));
+            return ois.readObject();
+        }
+        catch (IOException ex) {
+            throw new IllegalArgumentException("Failed to deserialize object", ex);
+        }
+        catch (ClassNotFoundException ex) {
+            throw new IllegalStateException("Failed to deserialize object type", ex);
+        }
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>NullPointerException if type of method parameter is not known</description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+import java.io.*;
+public class CloseResourceNullPointer {
+    public void check(UnknownType param) {
+        InputStream in = param;
+    }
 }
        ]]></code>
    </test-code>