diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 70ecfa16f5..27e288291e 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -36,6 +36,16 @@ See also [[all] Ensure PMD/CPD uses tab width of 1 for tabs consistently #2656]( cases where the variable of the caught exception is reassigned. This practice is surprising and prevents further evolution of the code like multi-catch. +#### Modified Rules + +* The Java rule {% rule "java/errorprone/CloseResource" %} (`java-errorprone`) has a new property + `closeNotInFinally`. With this property set to `true` the rule will also find calls to close a + resource, which are not in a finally-block of a try-statement. If a resource is not closed within a + finally block, it might not be closed at all in case of exceptions. + + As this new detection would yield many new violations, it is disabled by default. It might be + enabled in a later version of PMD. + #### Deprecated Rules * The Java rule {% rule "java/errorprone/DataflowAnomalyAnalysis" %} (`java-errorprone`) @@ -61,6 +71,7 @@ See also [[all] Ensure PMD/CPD uses tab width of 1 for tabs consistently #2656]( * java-errorprone * [#2431](https://github.com/pmd/pmd/issues/2431): \[java] InvalidLogMessageFormatRule throws IndexOutOfBoundsException when only logging exception message * [#2439](https://github.com/pmd/pmd/issues/2439): \[java] AvoidCatchingThrowable can not detect the case: catch (java.lang.Throwable t) + * [#2470](https://github.com/pmd/pmd/issues/2470): \[java] CloseResource false positive when resource included in return value * [#2531](https://github.com/pmd/pmd/issues/2531): \[java] UnnecessaryCaseChange can not detect the case like: foo.equals(bar.toLowerCase()) * [#2647](https://github.com/pmd/pmd/issues/2647): \[java] Deprecate rule DataFlowAnomalyAnalysis * java-performance @@ -113,6 +124,7 @@ See also [[all] Ensure PMD/CPD uses tab width of 1 for tabs consistently #2656]( * [#2656](https://github.com/pmd/pmd/pull/2656): \[all] Ensure PMD/CPD uses tab width of 1 for tabs consistently - [Maikel Steneker](https://github.com/maikelsteneker) * [#2659](https://github.com/pmd/pmd/pull/2659): \[java] StringToString can not detect the case: getStringMethod().toString() - [Mykhailo Palahuta](https://github.com/Drofff) * [#2662](https://github.com/pmd/pmd/pull/2662): \[java] UnnecessaryCaseChange can not detect the case like: foo.equals(bar.toLowerCase()) - [Mykhailo Palahuta](https://github.com/Drofff) +* [#2671](https://github.com/pmd/pmd/pull/2671): \[java] CloseResource false positive when resource included in return value - [Mykhailo Palahuta](https://github.com/Drofff) * [#2674](https://github.com/pmd/pmd/pull/2674): \[java] add lombok.EqualsAndHashCode in AbstractLombokAwareRule - [berkam](https://github.com/berkam) * [#2677](https://github.com/pmd/pmd/pull/2677): \[java] RedundantFieldInitializer can not detect a special case for char initialize: `char foo = '\0';` - [Mykhailo Palahuta](https://github.com/Drofff) * [#2678](https://github.com/pmd/pmd/pull/2678): \[java] AvoidCatchingThrowable can not detect the case: catch (java.lang.Throwable t) - [Mykhailo Palahuta](https://github.com/Drofff) 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 ac3bb0bd53..25bf71385a 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 @@ -33,22 +33,19 @@ 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.ASTNullLiteral; import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression; import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix; import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix; -import net.sourceforge.pmd.lang.java.ast.ASTReferenceType; import net.sourceforge.pmd.lang.java.ast.ASTResourceSpecification; import net.sourceforge.pmd.lang.java.ast.ASTReturnStatement; -import net.sourceforge.pmd.lang.java.ast.ASTStatementExpression; 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.JavaNode; import net.sourceforge.pmd.lang.java.ast.TypeNode; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; -import net.sourceforge.pmd.lang.java.symboltable.VariableNameDeclaration; import net.sourceforge.pmd.lang.java.typeresolution.TypeHelper; -import net.sourceforge.pmd.lang.symboltable.NameOccurrence; import net.sourceforge.pmd.properties.PropertyDescriptor; /** @@ -69,10 +66,9 @@ import net.sourceforge.pmd.properties.PropertyDescriptor; */ public class CloseResourceRule extends AbstractJavaRule { - private Set types = new HashSet<>(); - private Set simpleTypes = new HashSet<>(); + private static final String WRAPPING_TRY_WITH_RES_VAR_MESSAGE = "it is recommended to wrap resource in try-with-resource declaration directly"; + private static final String CLOSE_IN_FINALLY_BLOCK_MESSAGE = "'' is not closed within a finally block, thus might not be closed at all in case of exceptions"; - private Set closeTargets = new HashSet<>(); private static final PropertyDescriptor> CLOSE_TARGETS_DESCRIPTOR = stringListProperty("closeTargets") .desc("Methods which may close this resource") @@ -97,12 +93,24 @@ public class CloseResourceRule extends AbstractJavaRule { "java.util.stream.DoubleStream") .build(); + private static final PropertyDescriptor DETECT_CLOSE_NOT_IN_FINALLY = + booleanProperty("closeNotInFinally") + .desc("Detect if 'close' (or other closeTargets) is called outside of a finally-block").defaultValue(false).build(); + + private final Set types = new HashSet<>(); + private final Set simpleTypes = new HashSet<>(); + private final Set closeTargets = new HashSet<>(); + + // keeps track of already reported violations to avoid duplicated violations for the same variable + private final Set reportedVarNames = new HashSet<>(); + public CloseResourceRule() { definePropertyDescriptor(CLOSE_TARGETS_DESCRIPTOR); definePropertyDescriptor(TYPES_DESCRIPTOR); definePropertyDescriptor(USE_CLOSE_AS_DEFAULT_TARGET); definePropertyDescriptor(ALLOWED_RESOURCE_TYPES); + definePropertyDescriptor(DETECT_CLOSE_NOT_IN_FINALLY); } @Override @@ -114,7 +122,7 @@ public class CloseResourceRule extends AbstractJavaRule { if (getProperty(CLOSE_TARGETS_DESCRIPTOR) != null) { closeTargets.addAll(getProperty(CLOSE_TARGETS_DESCRIPTOR)); } - if (getProperty(USE_CLOSE_AS_DEFAULT_TARGET) && !closeTargets.contains("close")) { + if (getProperty(USE_CLOSE_AS_DEFAULT_TARGET)) { closeTargets.add("close"); } if (getProperty(TYPES_DESCRIPTOR) != null) { @@ -146,135 +154,117 @@ public class CloseResourceRule extends AbstractJavaRule { return super.visit(node, data); } - private void checkForResources(ASTMethodOrConstructorDeclaration node, Object data) { - List localVars = node.findDescendantsOfType(ASTLocalVariableDeclaration.class); - List vars = new ArrayList<>(); - Map ids = new HashMap<>(); - - // find all variable declarators - for (ASTLocalVariableDeclaration localVar : localVars) { - vars.addAll(localVar.findChildrenOfType(ASTVariableDeclarator.class)); + private void checkForResources(ASTMethodOrConstructorDeclaration methodOrConstructor, Object data) { + reportedVarNames.clear(); + Map resVars = getResourceVariables(methodOrConstructor); + for (Map.Entry resVarEntry : resVars.entrySet()) { + ASTVariableDeclarator resVar = resVarEntry.getKey(); + TypeNode resVarType = resVarEntry.getValue(); + if (isWrappingResourceSpecifiedInTry(resVar)) { + reportedVarNames.add(resVar.getVariableId().getName()); + addViolationWithMessage(data, resVar, WRAPPING_TRY_WITH_RES_VAR_MESSAGE); + } else if (shouldVarOfTypeBeClosedInMethod(resVar, resVarType, methodOrConstructor)) { + reportedVarNames.add(resVar.getVariableId().getName()); + addCloseResourceViolation(resVar.getVariableId(), resVarType, data); + } } + } - // find all variable references to Connection objects + private Map getResourceVariables(ASTMethodOrConstructorDeclaration method) { + List vars = method.findDescendantsOfType(ASTVariableDeclarator.class); + Map resVars = new HashMap<>(); for (ASTVariableDeclarator var : vars) { - // get the type of the local var declaration - TypeNode type = ((ASTLocalVariableDeclaration) var.getParent()).getTypeNode(); - - if (type != null && isResourceTypeOrSubtype(type)) { - if (var.hasInitializer()) { - // figure out the runtime type. If the variable is initialized, take the type from there - ASTExpression expression = var.getInitializer().getFirstChildOfType(ASTExpression.class); - TypeNode runtimeType = expression; - if (!isMethodCall(expression) && runtimeType != null && runtimeType.getType() != null) { - type = runtimeType; - } - - // consider cases, when the streams are chained - // assumes, that the underlaying stream is always the first argument in the - // constructor call. - ASTExpression firstArgument = getAllocationFirstArgument(expression); - if (firstArgument != null) { - type = firstArgument; - } - } - - if (!isAllowedResourceType(type) && !isMethodParameter(var, node)) { - ids.put(var.getVariableId(), type); - } + TypeNode varType = getTypeOfVariable(var); + if (isResourceTypeOrSubtype(varType)) { + resVars.put(var, wrappedResourceTypeOrReturn(var, varType)); } } - - // if there are closables, ensure each is closed. - for (Map.Entry entry : ids.entrySet()) { - ASTVariableDeclaratorId variableId = entry.getKey(); - ensureClosed((ASTLocalVariableDeclaration) variableId.getParent().getParent(), variableId, - entry.getValue(), data); - } + return resVars; } - /** - * 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 instanceof ASTMethodDeclaration) { - formalParameters = ((ASTMethodDeclaration) methodOrCstor).getFormalParameters(); - } else if (methodOrCstor instanceof ASTConstructorDeclaration) { - 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 TypeNode getTypeOfVariable(ASTVariableDeclarator var) { + TypeNode runtimeType = getRuntimeTypeOfVariable(var); + return runtimeType != null ? runtimeType : getDeclaredTypeOfVariable(var); } - private ASTExpression getAllocationFirstArgument(ASTExpression expression) { - List allocations = expression.findDescendantsOfType(ASTAllocationExpression.class); - ASTExpression firstArgument = null; + private TypeNode getDeclaredTypeOfVariable(ASTVariableDeclarator var) { + ASTLocalVariableDeclaration localVar = (ASTLocalVariableDeclaration) var.getParent(); + return localVar.getTypeNode(); + } - if (!allocations.isEmpty()) { - ASTArgumentList argumentList = allocations.get(allocations.size() - 1).getFirstDescendantOfType(ASTArgumentList.class); - if (argumentList != null) { - firstArgument = argumentList.getFirstChildOfType(ASTExpression.class); - } - } + private TypeNode getRuntimeTypeOfVariable(ASTVariableDeclarator var) { + ASTExpression initExpr = initializerExpressionOf(var); + return isRuntimeType(initExpr) ? initExpr : null; + } - // the argument must not be a literal, it needs to be a Name referring to a variable - if (firstArgument != null && firstArgument.getFirstDescendantOfType(ASTName.class) != null) { - ASTName name = firstArgument.getFirstDescendantOfType(ASTName.class); + private boolean isRuntimeType(ASTExpression expr) { + return expr != null && isNotMethodCall(expr) && expr.getType() != null; + } - Map> vars = firstArgument.getScope() - .getDeclarations(VariableNameDeclaration.class); - for (VariableNameDeclaration nameDecl : vars.keySet()) { - if (nameDecl.getName().equals(name.getImage()) && isResourceTypeOrSubtype(firstArgument)) { - return firstArgument; - } + private TypeNode wrappedResourceTypeOrReturn(ASTVariableDeclarator var, TypeNode defaultVal) { + TypeNode wrappedResType = getWrappedResourceType(var); + return wrappedResType != null ? wrappedResType : defaultVal; + } + + private TypeNode getWrappedResourceType(ASTVariableDeclarator var) { + ASTExpression initExpr = initializerExpressionOf(var); + if (initExpr != null) { + ASTAllocationExpression resAlloc = getLastResourceAllocation(initExpr); + if (resAlloc != null) { + ASTExpression firstArgRes = getFirstArgumentVariableIfResource(resAlloc); + return firstArgRes != null ? firstArgRes : resAlloc; } } return null; } - private boolean isMethodCall(ASTExpression expression) { - return expression != null - && expression.getNumChildren() > 0 - && expression.getChild(0) instanceof ASTPrimaryExpression - && expression.getChild(0).getFirstChildOfType(ASTPrimarySuffix.class) != null; + private ASTExpression initializerExpressionOf(ASTVariableDeclarator var) { + return var.hasInitializer() + ? var.getInitializer().getFirstChildOfType(ASTExpression.class) + : null; } - private boolean isResourceTypeOrSubtype(TypeNode refType) { - if (refType.getType() != null) { - for (String type : types) { - if (TypeHelper.isA(refType, type)) { - return true; - } + private ASTAllocationExpression getLastResourceAllocation(ASTExpression expr) { + List allocations = expr.findDescendantsOfType(ASTAllocationExpression.class); + int lastAllocIndex = allocations.size() - 1; + for (int allocIndex = lastAllocIndex; allocIndex >= 0; allocIndex--) { + ASTAllocationExpression allocation = allocations.get(allocIndex); + if (isResourceTypeOrSubtype(allocation)) { + return allocation; } - } else if (refType.getNumChildren() > 0 && refType.getChild(0) instanceof ASTReferenceType) { - // no type information (probably missing auxclasspath) - use simple types - ASTReferenceType ref = (ASTReferenceType) refType.getChild(0); - if (ref.getChild(0) instanceof ASTClassOrInterfaceType) { - ASTClassOrInterfaceType clazz = (ASTClassOrInterfaceType) ref.getChild(0); - if (simpleTypes.contains(toSimpleType(clazz.getImage())) && !clazz.isReferenceToClassSameCompilationUnit() - || types.contains(clazz.getImage()) && !clazz.isReferenceToClassSameCompilationUnit()) { + } + return null; + } + + private ASTExpression getFirstArgumentVariableIfResource(ASTAllocationExpression allocation) { + ASTArgumentList argsList = allocation.getFirstDescendantOfType(ASTArgumentList.class); + if (argsList != null) { + ASTExpression firstArg = argsList.getFirstChildOfType(ASTExpression.class); + return firstArg != null && isNotMethodCall(firstArg) && isResourceTypeOrSubtype(firstArg) + ? firstArg + : null; + } + return null; + } + + private boolean isNotMethodCall(ASTExpression expr) { + return !isMethodCall(expr); + } + + private boolean isMethodCall(ASTExpression expression) { + if (expression != null) { + ASTPrimaryExpression primaryExpression = expression.getFirstChildOfType(ASTPrimaryExpression.class); + return primaryExpression != null && primaryExpression.getFirstChildOfType(ASTPrimarySuffix.class) != null; + } + return false; + } + + private boolean isWrappingResourceSpecifiedInTry(ASTVariableDeclarator var) { + String wrappedVarName = getWrappedVariableName(var); + if (wrappedVarName != null) { + List tryContainers = var.getParentsOfType(ASTTryStatement.class); + for (ASTTryStatement tryContainer : tryContainers) { + if (isTryWithResourceSpecifyingVariable(tryContainer, wrappedVarName)) { return true; } } @@ -282,9 +272,19 @@ public class CloseResourceRule extends AbstractJavaRule { return false; } + private boolean shouldVarOfTypeBeClosedInMethod(ASTVariableDeclarator var, TypeNode type, + ASTMethodOrConstructorDeclaration method) { + return isNotAllowedResourceType(type) && isNotWrappingResourceMethodParameter(var, method) + && isResourceVariableUnclosed(var); + } + + private boolean isNotAllowedResourceType(TypeNode varType) { + return !isAllowedResourceType(varType); + } + private boolean isAllowedResourceType(TypeNode refType) { List allowedResourceTypes = getProperty(ALLOWED_RESOURCE_TYPES); - if (refType.getType() != null && allowedResourceTypes != null) { + if (allowedResourceTypes != null) { for (String type : allowedResourceTypes) { // the check here must be a exact type match, since subclasses may override close() // and actually require closing @@ -296,223 +296,213 @@ public class CloseResourceRule extends AbstractJavaRule { return false; } - private boolean hasNullInitializer(ASTLocalVariableDeclaration var) { - ASTVariableInitializer init = var.getFirstDescendantOfType(ASTVariableInitializer.class); - if (init != null) { - try { - List nulls = init - .findChildNodesWithXPath("Expression/PrimaryExpression/PrimaryPrefix/Literal/NullLiteral"); - return !nulls.isEmpty(); - } catch (JaxenException e) { - return false; + private boolean isNotWrappingResourceMethodParameter(ASTVariableDeclarator var, + ASTMethodOrConstructorDeclaration method) { + return !isWrappingResourceMethodParameter(var, method); + } + + /** + * Checks whether the variable is resource and initialized from a method parameter. + * @param var the resource variable that is being initialized + * @param method the method or constructor in which the variable is declared + * @return true if the variable is resource and initialized from a method parameter. false + * otherwise. + */ + private boolean isWrappingResourceMethodParameter(ASTVariableDeclarator var, ASTMethodOrConstructorDeclaration method) { + String wrappedVarName = getWrappedVariableName(var); + if (wrappedVarName != null) { + ASTFormalParameters methodParams = method.getFirstDescendantOfType(ASTFormalParameters.class); + if (methodParams != null) { + List ids = methodParams.findDescendantsOfType(ASTVariableDeclaratorId.class); + for (ASTVariableDeclaratorId id : ids) { + if (id.hasImageEqualTo(wrappedVarName) && isResourceTypeOrSubtype(id)) { + return true; + } + } } } return false; } - private void ensureClosed(ASTLocalVariableDeclaration var, ASTVariableDeclaratorId id, TypeNode type, Object data) { - // What are the chances of a Connection being instantiated in a - // for-loop init block? Anyway, I'm lazy! - String variableToClose = id.getImage(); - Node n = var; - - while (!(n instanceof ASTBlock) && !(n instanceof ASTConstructorDeclaration)) { - n = n.getParent(); - } - - Node top = n; - - List tryblocks = top.findDescendantsOfType(ASTTryStatement.class); - - boolean closed = false; - - ASTBlockStatement parentBlock = id.getFirstParentOfType(ASTBlockStatement.class); - - // look for try blocks below the line the variable was - // introduced and make sure there is a .close call in a finally - // block. - for (ASTTryStatement t : tryblocks) { - - // verifies that there are no critical statements between the - // variable declaration and - // the beginning of the try block. - ASTBlockStatement tryBlock = t.getFirstParentOfType(ASTBlockStatement.class); - // no need to check for critical statements, if - // the variable has been initialized with null - if (!hasNullInitializer(var) && parentBlock.getParent() == tryBlock.getParent()) { - - List blocks = parentBlock.getParent().findChildrenOfType(ASTBlockStatement.class); - int parentBlockIndex = blocks.indexOf(parentBlock); - int tryBlockIndex = blocks.indexOf(tryBlock); - boolean criticalStatements = false; - - for (int i = parentBlockIndex + 1; i < tryBlockIndex; i++) { - // assume variable declarations are not critical and assignments are not critical - ASTBlockStatement block = blocks.get(i); - ASTLocalVariableDeclaration varDecl = block - .getFirstDescendantOfType(ASTLocalVariableDeclaration.class); - ASTStatementExpression statementExpression = block.getFirstDescendantOfType(ASTStatementExpression.class); - - if (varDecl == null && (statementExpression == null - || statementExpression.getFirstChildOfType(ASTAssignmentOperator.class) == null)) { - criticalStatements = true; - break; - } - } - if (criticalStatements) { - break; - } - } - - ASTFinallyStatement finallyClause = t.getFinallyClause(); - if (t.getBeginLine() > id.getBeginLine() && finallyClause != null) { - ASTBlock finallyBody = finallyClause.getBody(); - List names = finallyBody.findDescendantsOfType(ASTName.class); - for (ASTName oName : names) { - String name = oName.getImage(); - if (name != null && name.contains(".")) { - String[] parts = name.split("\\."); - if (parts.length == 2) { - String methodName = parts[1]; - String varName = parts[0]; - if (varName.equals(variableToClose) && closeTargets.contains(methodName) - && nullCheckIfCondition(finallyBody, oName, varName)) { - closed = true; - break; - } - - } - } - } - if (closed) { - break; - } - - List exprs = finallyBody.findDescendantsOfType(ASTStatementExpression.class, true); - for (ASTStatementExpression stmt : exprs) { - ASTPrimaryExpression expr = stmt.getFirstChildOfType(ASTPrimaryExpression.class); - if (expr != null) { - ASTPrimaryPrefix prefix = expr.getFirstChildOfType(ASTPrimaryPrefix.class); - ASTPrimarySuffix suffix = expr.getFirstChildOfType(ASTPrimarySuffix.class); - if (prefix != null && suffix != null) { - if (prefix.getImage() == null) { - ASTName prefixName = prefix.getFirstChildOfType(ASTName.class); - if (prefixName != null && closeTargets.contains(prefixName.getImage())) { - // Found a call to a "close target" that is - // a direct - // method call without a "ClassName." - // prefix. - closed = variableIsPassedToMethod(expr, variableToClose); - if (closed) { - break; - } - } - } else if (suffix.getImage() != null) { - String prefixPlusSuffix = prefix.getImage() + "." + suffix.getImage(); - if (closeTargets.contains(prefixPlusSuffix)) { - // Found a call to a "close target" that is - // a method call - // in the form "ClassName.methodName". - closed = variableIsPassedToMethod(expr, variableToClose); - if (closed) { - break; - } - } - } - // look for primary suffix containing the close - // Targets elements. - // If the .close is executed in another class - // accessed by a method - // this form : - // getProviderInstance().closeConnexion(connexion) - // For this use case, we assume the variable is - // correctly closed - // in the other class since there is no way to - // really check it. - if (!closed) { - List suffixes = expr.findDescendantsOfType(ASTPrimarySuffix.class, true); - for (ASTPrimarySuffix oSuffix : suffixes) { - String suff = oSuffix.getImage(); - if (closeTargets.contains(suff)) { - closed = variableIsPassedToMethod(expr, variableToClose); - if (closed) { - break; - } - } - - } - } - } - } - } - if (closed) { - break; - } - } else if (t.isTryWithResources()) { - // maybe the variable is used as a resource - List names = t.getFirstChildOfType(ASTResourceSpecification.class).findDescendantsOfType(ASTName.class); - for (ASTName potentialUsage : names) { - if (potentialUsage.hasImageEqualTo(variableToClose)) { - closed = true; - break; - } - } - } - } - - if (!closed) { - // See if the variable is returned by the method, which means the - // method is a utility for creating the db resource, which means of - // course it can't be closed by the method, so it isn't an error. - List returns = top.findDescendantsOfType(ASTReturnStatement.class, true); - for (ASTReturnStatement returnStatement : returns) { - ASTName name = returnStatement.getFirstDescendantOfType(ASTName.class); - if (name != null && name.getImage().equals(variableToClose)) { - closed = true; - break; - } - } - } - - // if all is not well, complain - if (!closed) { - ASTLocalVariableDeclaration localVarDecl = id.getFirstParentOfType(ASTLocalVariableDeclaration.class); - Class typeClass = type.getType(); - if (typeClass != null) { - addViolation(data, id, typeClass.getSimpleName()); - } else if (localVarDecl != null && localVarDecl.getTypeNode() != null) { - addViolation(data, id, localVarDecl.getTypeNode().getTypeImage()); - } else { - addViolation(data, id, id.getVariableName()); - } + private String getWrappedVariableName(ASTVariableDeclarator var) { + if (var.hasInitializer()) { + ASTName varName = var.getInitializer().getFirstDescendantOfType(ASTName.class); + return varName != null ? varName.getImage() : null; } + return null; } - private boolean variableIsPassedToMethod(ASTPrimaryExpression expr, String variable) { - List methodParams = expr.findDescendantsOfType(ASTName.class, true); - for (ASTName pName : methodParams) { - String paramName = pName.getImage(); - // also check if we've got the a parameter (i.e if it's an argument - // !) - ASTArgumentList parentParam = pName.getFirstParentOfType(ASTArgumentList.class); - if (paramName.equals(variable) && parentParam != null) { + private boolean isResourceTypeOrSubtype(TypeNode refType) { + return refType.getType() != null + ? isNodeInstanceOfResourceType(refType) + : nodeHasReferenceToResourceType(refType); + } + + private boolean isNodeInstanceOfResourceType(TypeNode refType) { + for (String resType : types) { + if (TypeHelper.isA(refType, resType)) { return true; } } return false; } - private ASTIfStatement findIfStatement(ASTBlock enclosingBlock, Node node) { - ASTIfStatement ifStatement = node.getFirstParentOfType(ASTIfStatement.class); - List allIfStatements = enclosingBlock.findDescendantsOfType(ASTIfStatement.class); - if (ifStatement != null && allIfStatements.contains(ifStatement)) { - return ifStatement; + private boolean nodeHasReferenceToResourceType(TypeNode refType) { + ASTClassOrInterfaceType type = refType.getFirstDescendantOfType(ASTClassOrInterfaceType.class); + return type != null && isResourceTypeName(type.getImage()) && !type.isReferenceToClassSameCompilationUnit(); + } + + private boolean isResourceTypeName(String typeName) { + String simpleTypeName = toSimpleType(typeName); + return types.contains(typeName) || simpleTypes.contains(simpleTypeName); + } + + private boolean isResourceVariableUnclosed(ASTVariableDeclarator var) { + return !isResourceVariableClosed(var); + } + + private boolean isResourceVariableClosed(ASTVariableDeclarator var) { + Node methodOfVar = getMethodOfNode(var); + return hasTryStatementClosingResourceVariable(methodOfVar, var) + || isReturnedByMethod(var.getName(), methodOfVar); + } + + private Node getMethodOfNode(Node node) { + Node parent = node.getParent(); + while (isNotMethod(parent)) { + parent = parent.getParent(); } - return null; + return parent; + } + + private boolean isNotMethod(Node node) { + return !(node instanceof ASTBlock || node instanceof ASTConstructorDeclaration); + } + + private boolean hasTryStatementClosingResourceVariable(Node node, ASTVariableDeclarator var) { + List tryStatements = node.findDescendantsOfType(ASTTryStatement.class, true); + for (ASTTryStatement tryStatement : tryStatements) { + if (tryStatementClosesResourceVariable(tryStatement, var)) { + return true; + } + } + return false; + } + + private boolean tryStatementClosesResourceVariable(ASTTryStatement tryStatement, ASTVariableDeclarator var) { + if (tryStatement.getBeginLine() >= var.getBeginLine() && noneCriticalStatementsBetween(var, tryStatement)) { + if (isTryWithResourceSpecifyingVariable(tryStatement, var.getName())) { + return true; + } + if (hasFinallyClause(tryStatement)) { + ASTBlock finallyBody = tryStatement.getFinallyClause().getBody(); + return blockClosesResourceVariable(finallyBody, var.getName()); + } + } + return false; + } + + private boolean noneCriticalStatementsBetween(ASTVariableDeclarator var, ASTTryStatement tryStatement) { + return !anyCriticalStatementBetween(var, tryStatement); + } + + private boolean anyCriticalStatementBetween(ASTVariableDeclarator var, ASTTryStatement tryStatement) { + ASTBlockStatement varBlockStatement = var.getFirstParentOfType(ASTBlockStatement.class); + ASTBlockStatement tryBlockStatement = tryStatement.getFirstParentOfType(ASTBlockStatement.class); + if (isNotNullInitialized(var) && areStatementsOfSameBlock(varBlockStatement, tryBlockStatement)) { + for (ASTBlockStatement bsBetween : getBlockStatementsBetween(varBlockStatement, tryBlockStatement)) { + if (isCriticalStatement(bsBetween)) { + return true; + } + } + } + return false; + } + + private boolean isNotNullInitialized(ASTVariableDeclarator var) { + return !hasNullInitializer(var); + } + + private boolean hasNullInitializer(ASTVariableDeclarator var) { + if (var.hasInitializer()) { + ASTPrimaryPrefix primaryPrefix = var.getInitializer().getFirstDescendantOfType(ASTPrimaryPrefix.class); + return primaryPrefix != null && primaryPrefix.hasDescendantOfType(ASTNullLiteral.class); + } + return false; + } + + private boolean areStatementsOfSameBlock(ASTBlockStatement bs0, ASTBlockStatement bs1) { + return bs0.getParent() == bs1.getParent(); + } + + private List getBlockStatementsBetween(ASTBlockStatement top, ASTBlockStatement bottom) { + List blockStatements = top.getParent().findChildrenOfType(ASTBlockStatement.class); + int topBSIndex = blockStatements.indexOf(top); + int bottomBSIndex = blockStatements.indexOf(bottom); + return blockStatements.subList(topBSIndex + 1, bottomBSIndex); + } + + private boolean isCriticalStatement(ASTBlockStatement blockStatement) { + boolean isVarDeclaration = blockStatement.hasDescendantOfType(ASTLocalVariableDeclaration.class); + boolean isAssignmentOperator = blockStatement.hasDescendantOfType(ASTAssignmentOperator.class); + return !isVarDeclaration && !isAssignmentOperator; + } + + private boolean isTryWithResourceSpecifyingVariable(ASTTryStatement tryStatement, String varName) { + return tryStatement.isTryWithResources() && isVariableSpecifiedInTryWithResource(varName, tryStatement); + } + + private boolean isVariableSpecifiedInTryWithResource(String varName, ASTTryStatement tryWithResource) { + List specifiedResources = getResourcesSpecifiedInTryWith(tryWithResource); + for (JavaNode res : specifiedResources) { + if (res.hasImageEqualTo(varName)) { + return true; + } + } + return false; + } + + private List getResourcesSpecifiedInTryWith(ASTTryStatement tryWithResource) { + ASTResourceSpecification resSpecification = tryWithResource.getFirstChildOfType(ASTResourceSpecification.class); + List initializedVars = resSpecification + .findDescendantsOfType(ASTVariableDeclaratorId.class); + List specifiedVars = resSpecification.findDescendantsOfType(ASTName.class); + return combineNodeLists(initializedVars, specifiedVars); + } + + private List combineNodeLists(List list0, List list1) { + List nodeList = new ArrayList<>(list0); + nodeList.addAll(list1); + return nodeList; + } + + private boolean hasFinallyClause(ASTTryStatement tryStatement) { + return tryStatement.getFinallyClause() != null; + } + + private boolean blockClosesResourceVariable(ASTBlock block, String variableToClose) { + return hasNotConditionalCloseCallOnVariable(block, variableToClose) + || hasMethodCallClosingResourceVariable(block, variableToClose); + } + + private boolean hasNotConditionalCloseCallOnVariable(ASTBlock block, String variableToClose) { + List operations = block.findDescendantsOfType(ASTName.class); + for (ASTName operation : operations) { + if (isCloseCallOnVariable(operation, variableToClose) + && isNotConditional(block, operation, variableToClose)) { + return true; + } + } + return false; + } + + private boolean isCloseCallOnVariable(ASTName op, String variableToClose) { + String closedVar = getVariableClosedByMethodCall(op); + return variableToClose.equals(closedVar); } /** - * Checks, whether the given node is inside a if condition, and if so, + * Checks, whether the given node is inside an if condition, and if so, * whether this is a null check for the given varName. * * @param enclosingBlock @@ -524,7 +514,7 @@ public class CloseResourceRule extends AbstractJavaRule { * @return true if no if condition is involved or if the if * condition is a null-check. */ - private boolean nullCheckIfCondition(ASTBlock enclosingBlock, Node node, String varName) { + private boolean isNotConditional(ASTBlock enclosingBlock, Node node, String varName) { ASTIfStatement ifStatement = findIfStatement(enclosingBlock, node); if (ifStatement != null) { try { @@ -539,4 +529,141 @@ public class CloseResourceRule extends AbstractJavaRule { } return true; } + + private ASTIfStatement findIfStatement(ASTBlock enclosingBlock, Node node) { + ASTIfStatement ifStatement = node.getFirstParentOfType(ASTIfStatement.class); + List allIfStatements = enclosingBlock.findDescendantsOfType(ASTIfStatement.class); + if (ifStatement != null && allIfStatements.contains(ifStatement)) { + return ifStatement; + } + return null; + } + + private boolean hasMethodCallClosingResourceVariable(ASTBlock block, String variableToClose) { + List expressions = block.findDescendantsOfType(ASTPrimaryExpression.class, true); + for (ASTPrimaryExpression expression : expressions) { + if (isMethodCallClosingResourceVariable(expression, variableToClose)) { + return true; + } + } + return false; + } + + private boolean isMethodCallClosingResourceVariable(ASTPrimaryExpression expression, String variableToClose) { + ASTPrimaryPrefix prefix = expression.getFirstDescendantOfType(ASTPrimaryPrefix.class); + ASTPrimarySuffix suffix = expression.getFirstDescendantOfType(ASTPrimarySuffix.class); + if (prefix != null && suffix != null) { + return (isCloseTargetMethodCall(prefix, suffix) || hasChainedCloseTargetMethodCall(expression)) + && variableIsPassedToMethod(variableToClose, expression); + } + return false; + } + + private boolean isCloseTargetMethodCall(ASTPrimaryPrefix prefix, ASTPrimarySuffix suffix) { + String methodCall = getMethodCallStr(prefix, suffix); + return methodCall != null && closeTargets.contains(methodCall); + } + + private String getMethodCallStr(ASTPrimaryPrefix prefix, ASTPrimarySuffix suffix) { + if (prefix.getImage() == null) { + ASTName name = prefix.getFirstDescendantOfType(ASTName.class); + return name != null ? name.getImage() : null; + } else if (suffix.getImage() != null) { + return prefix.getImage() + "." + suffix.getImage(); + } + return null; + } + + private boolean hasChainedCloseTargetMethodCall(ASTPrimaryExpression expr) { + List methodCalls = expr.findDescendantsOfType(ASTPrimarySuffix.class, true); + for (ASTPrimarySuffix methodCall : methodCalls) { + if (closeTargets.contains(methodCall.getImage())) { + return true; + } + } + return false; + } + + private boolean variableIsPassedToMethod(String varName, ASTPrimaryExpression methodCall) { + List methodCallArgs = methodCall.findDescendantsOfType(ASTName.class, true); + for (ASTName methodCallArg : methodCallArgs) { + if (isMethodCallArgument(methodCallArg) && methodCallArg.hasImageEqualTo(varName)) { + return true; + } + } + return false; + } + + private boolean isMethodCallArgument(ASTName varName) { + return varName.getFirstParentOfType(ASTArgumentList.class) != null; + } + + private boolean isReturnedByMethod(String varName, Node method) { + List returns = method.findDescendantsOfType(ASTReturnStatement.class, true); + for (ASTReturnStatement returnStatement : returns) { + ASTName name = returnStatement.getFirstDescendantOfType(ASTName.class); + if (name != null && name.hasImageEqualTo(varName)) { + return true; + } + } + return false; + } + + private void addCloseResourceViolation(ASTVariableDeclaratorId id, TypeNode type, Object data) { + String resTypeName = getResourceTypeName(id, type); + addViolation(data, id, resTypeName); + } + + private String getResourceTypeName(ASTVariableDeclaratorId varId, TypeNode type) { + Class typeClass = type.getType(); + if (typeClass == null) { + ASTLocalVariableDeclaration localVarDecl = varId.getFirstParentOfType(ASTLocalVariableDeclaration.class); + return localVarDecl != null && localVarDecl.getTypeNode() != null + ? localVarDecl.getTypeNode().getTypeImage() + : varId.getName(); + } + return typeClass.getSimpleName(); + } + + @Override + public Object visit(ASTPrimaryPrefix prefix, Object data) { + if (!getProperty(DETECT_CLOSE_NOT_IN_FINALLY)) { + return super.visit(prefix, data); + } + + ASTName methodCall = prefix.getFirstChildOfType(ASTName.class); + if (methodCall != null && isNodeInstanceOfResourceType(methodCall)) { + String closedVar = getVariableClosedByMethodCall(methodCall); + if (closedVar != null && isNotInFinallyBlock(prefix) && !reportedVarNames.contains(closedVar)) { + String violationMsg = closeInFinallyBlockMessageForVar(closedVar); + addViolationWithMessage(data, prefix, violationMsg); + } + } + return super.visit(prefix, data); + } + + private String getVariableClosedByMethodCall(ASTName methodCall) { + String[] callParts = getMethodCallParts(methodCall); + if (callParts != null) { + String varName = callParts[0]; + String methodName = callParts[1]; + return closeTargets.contains(methodName) ? varName : null; + } + return null; + } + + private String[] getMethodCallParts(ASTName methodCall) { + String methodCallStr = methodCall.getImage(); + return methodCallStr != null && methodCallStr.contains(".") + ? methodCallStr.split("\\.") + : null; + } + + private boolean isNotInFinallyBlock(ASTPrimaryPrefix prefix) { + return prefix.getFirstParentOfType(ASTFinallyStatement.class) == null; + } + + private String closeInFinallyBlockMessageForVar(String var) { + return "''" + var + CLOSE_IN_FINALLY_BLOCK_MESSAGE; + } } 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 72e25980fb..3b3894818f 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 @@ -377,7 +377,13 @@ public class Test { #947 CloseResource rule fails if field is marked with annotation + true 2 + 8,9 + + Ensure that resources like this Connection object are closed after use + Ensure that resources like this Statement object are closed after use + #1375 CloseResource not detected properly - ok + true 1 + 6 + + Ensure that resources like this ResultSet object are closed after use + #1375 CloseResource not detected properly - false negative 1 + 6 + + Ensure that resources like this ResultSet object are closed after use + - - False-negative if only byte array is passed in as method parameter - 1 - 6 - - - NullPointerException if type of method parameter is not known 1 @@ -1292,6 +1283,108 @@ public class CloseResourceNullPointer { public void check(UnknownType param) { InputStream in = param; } +} + ]]> + + + + #2470 false-positive at lambda returned from method + 0 + bar() throws IOException { + InputStream inputStream = new FileInputStream("/test.txt"); + return () -> { + try { + return inputStream.read(); + } finally { + inputStream.close(); + } + }; + } +} + ]]> + + + + wrapped ByteArrayInputStream false-negative test + true + 1 + 7 + + 'bos' is not closed within a finally block, thus might not be closed at all in case of exceptions + + + + + + lambda doesn't close resource false-negative test + 1 + 4 + { + try { + int d = is.read(); + } catch (IOException e) { + e.printStackTrace(); + } + }; + } +} + ]]> + + + + don't wrap try-with-resource variable test + 1 + 5 + + it is recommended to wrap resource in try-with-resource declaration directly + + + + + + false positive with close on not closeable + 0 +