From 77b183704f8130fee59d1116bb72ad58c1dd9050 Mon Sep 17 00:00:00 2001 From: andi Date: Thu, 1 Oct 2020 12:59:44 +0200 Subject: [PATCH 1/2] fix #2764: false-negative when re-assigning variable --- .../rule/errorprone/CloseResourceRule.java | 82 +++++++++++++++++++ .../rule/errorprone/xml/CloseResource.xml | 47 +++++++++++ 2 files changed, 129 insertions(+) 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 + From 76e833376ceee29c663329834a96993131373e80 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 22 Oct 2020 09:38:49 +0200 Subject: [PATCH 2/2] [doc] Update release notes, refs #2811, fixes #2764 --- docs/pages/release_notes.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index b8f8783555..5f459cebf2 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -15,10 +15,13 @@ This is a {{ site.pmd.release_type }} release. ### New and noteworthy ### Fixed Issues +* java-errorprone + * [#2764](https://github.com/pmd/pmd/issues/2764): \[java] CloseResourceRule does not recognize multiple assignment done to resource ### API Changes ### External Contributions +* [#2811](https://github.com/pmd/pmd/pull/2811): \[java] CloseResource - Fix #2764: False-negative when re-assigning variable - [Andi Pabst](https://github.com/andipabst) {% endtocmaker %}