diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 7b7c985251..4c3c30bf6f 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -22,9 +22,9 @@ This is a {{ site.pmd.release_type }} release. the Java VM, which is bad, if the VM runs an application server which many independent applications. ### Fixed Issues - -* java-errorprone - * [#2157](https://github.com/pmd/pmd/issues/2157): \[java] Improve DoNotCallSystemExit: permit call in main(), flag System.halt +* java-errorprone + * [#2157](https://github.com/pmd/pmd/issues/2157): \[java] Improve DoNotCallSystemExit: permit call in main(), flag System.halt + * [#2764](https://github.com/pmd/pmd/issues/2764): \[java] CloseResourceRule does not recognize multiple assignment done to resource ### API Changes @@ -33,6 +33,7 @@ This is a {{ site.pmd.release_type }} release. * [#2803](https://github.com/pmd/pmd/pull/2803): \[java] Improve DoNotCallSystemExit (Fixes #2157) - [Vitaly Polonetsky](https://github.com/mvitaly) * [#2809](https://github.com/pmd/pmd/pull/2809): \[java] Move test config from file to test class - [Stefan Birkner](https://github.com/stefanbirkner) * [#2810](https://github.com/pmd/pmd/pull/2810): \[core] Move method "renderTempFile" to XMLRendererTest - [Stefan Birkner](https://github.com/stefanbirkner) +* [#2811](https://github.com/pmd/pmd/pull/2811): \[java] CloseResource - Fix #2764: False-negative when re-assigning variable - [Andi Pabst](https://github.com/andipabst) * [#2813](https://github.com/pmd/pmd/pull/2813): \[core] Use JUnit's TemporaryFolder rule - [Stefan Birkner](https://github.com/stefanbirkner) * [#2829](https://github.com/pmd/pmd/pull/2829): \[doc] Small correction in pmd\_report\_formats.md - [Gustavo Krieger](https://github.com/gustavopcassol) 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 54e31d0b5d..5af54e056e 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 @@ -39,6 +39,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix; import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix; 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; @@ -46,6 +47,7 @@ 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.types.TypeTestUtil; +import net.sourceforge.pmd.lang.symboltable.NameDeclaration; import net.sourceforge.pmd.properties.PropertyDescriptor; /** @@ -67,6 +69,7 @@ import net.sourceforge.pmd.properties.PropertyDescriptor; public class CloseResourceRule extends AbstractJavaRule { 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 REASSIGN_BEFORE_CLOSED_MESSAGE = "'' is reassigned, but the original instance is not closed"; 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 static final PropertyDescriptor> CLOSE_TARGETS_DESCRIPTOR = @@ -166,6 +169,12 @@ public class CloseResourceRule extends AbstractJavaRule { } else if (shouldVarOfTypeBeClosedInMethod(resVar, resVarType, methodOrConstructor)) { reportedVarNames.add(resVar.getVariableId().getName()); addCloseResourceViolation(resVar.getVariableId(), resVarType, data); + } else { + ASTStatementExpression reassigningStatement = getFirstReassigningStatementBeforeBeingClosed(resVar, methodOrConstructor); + if (reassigningStatement != null) { + reportedVarNames.add(resVar.getVariableId().getName()); + addViolationWithMessage(data, reassigningStatement, reassignBeforeClosedMessageForVar(resVar.getName())); + } } } } @@ -666,4 +675,77 @@ public class CloseResourceRule extends AbstractJavaRule { private String closeInFinallyBlockMessageForVar(String var) { return "''" + var + CLOSE_IN_FINALLY_BLOCK_MESSAGE; } + + private String reassignBeforeClosedMessageForVar(String var) { + return "''" + var + REASSIGN_BEFORE_CLOSED_MESSAGE; + } + + private ASTStatementExpression getFirstReassigningStatementBeforeBeingClosed(ASTVariableDeclarator variable, ASTMethodOrConstructorDeclaration methodOrConstructor) { + List statements = methodOrConstructor.findDescendantsOfType(ASTStatementExpression.class); + boolean variableClosed = false; + boolean isInitialized = !hasNullInitializer(variable); + ASTExpression initializingExpression = initializerExpressionOf(variable); + for (ASTStatementExpression statement : statements) { + if (isClosingVariableStatement(statement, variable)) { + variableClosed = true; + } + + if (isAssignmentForVariable(statement, variable)) { + if (isInitialized && !variableClosed) { + if (initializingExpression != null && !inSameIfBlock(statement, initializingExpression)) { + return statement; + } + } + + if (variableClosed) { + variableClosed = false; + } + if (!isInitialized) { + isInitialized = true; + initializingExpression = statement.getFirstDescendantOfType(ASTExpression.class); + } + } + } + return null; + } + + private boolean inSameIfBlock(ASTStatementExpression statement1, ASTExpression statement2) { + List parents1 = statement1.getParentsOfType(ASTIfStatement.class); + List parents2 = statement2.getParentsOfType(ASTIfStatement.class); + parents1.retainAll(parents2); + return !parents1.isEmpty(); + } + + private boolean isClosingVariableStatement(ASTStatementExpression statement, ASTVariableDeclarator variable) { + List expressions = statement.findDescendantsOfType(ASTPrimaryExpression.class); + for (ASTPrimaryExpression expression : expressions) { + if (isMethodCallClosingResourceVariable(expression, variable.getName())) { + return true; + } + } + List names = statement.findDescendantsOfType(ASTName.class); + for (ASTName name : names) { + if (isCloseCallOnVariable(name, variable.getName())) { + return true; + } + } + return false; + } + + private boolean isAssignmentForVariable(ASTStatementExpression statement, ASTVariableDeclarator variable) { + if (statement == null || variable == null) { + return false; + } + ASTName name = statement.getFirstDescendantOfType(ASTName.class); + if (name == null) { + return false; + } + NameDeclaration statementVariable = name.getNameDeclaration(); + if (statementVariable == null) { + return false; + } + + return statement.hasDescendantOfType(ASTAssignmentOperator.class) + && statementVariable.equals(variable.getVariableId().getNameDeclaration()); + } } 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 b0f15f986e..eb28592406 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 @@ -1426,6 +1426,53 @@ public class CloseResourceWithVar { int c = inputStream.read(); return c; } +} + ]]> + + + + #2764 false-negative when re-assigning connection + java.sql.Connection,java.sql.Statement,java.sql.ResultSet + 1 + 7 + + 'c' is reassigned, but the original instance is not closed + + + + + + #2764 false-negative when re-assigning connection - no problem when closed before + java.sql.Connection,java.sql.Statement,java.sql.ResultSet + 0 +