diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index ee819f2f0c..f16921b9c3 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -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 diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java index a9cad8b0ac..0f4907d54b 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java @@ -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 localVars = node.findDescendantsOfType(ASTLocalVariableDeclaration.class); List vars = new ArrayList<>(); Map 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 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 true if the variable is initialized from a method parameter. false + * 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 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 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; } diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/closeresource/Statement.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/closeresource/Statement.java new file mode 100644 index 0000000000..74998af9ad --- /dev/null +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/closeresource/Statement.java @@ -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 { + +} diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml index 008b5521b0..dbbc83e9d9 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml @@ -1117,6 +1117,137 @@ public class CloseResourceCase { e.printStackTrace(); } } +} + ]]> + + + + #1966 [java] CloseResource false positive if Stream is passed as method parameter + 0 + + + + + #1967 [java] CloseResource false positive with late assignment of variable + 0 + + + + + #1922 [java] CloseResource possible false positive with Streams + 0 + Stream> filterResults(List candidates, Function matchExtractor, String query, MatchSelector limiter) { + if (query.length() < MIN_QUERY_LENGTH) { + return Stream.empty(); + } + + // violation here + Stream> 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); + } +} + ]]> + + + + #1076 [java] CloseResource false positive on non-SQL classes called Statement + 0 + + + + + False-negative if only byte array is passed in as method parameter + 1 + 6 + + + + + NullPointerException if type of method parameter is not known + 1 +