phoedos/pmd
1
1
Fork 1
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge branch 'pr-2811' into master

Browse Source 
[java] CloseResource - Fix #2764: False-negative when re-assigning variable #2811
This commit is contained in:
Andreas Dangel
2020-10-22 09:39:05 +02:00
parent 40a8ac6f5c 76e833376c
commit 873564e69f
3 changed files with 133 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
docs/pages/release_notes.md
View File

@ -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)

82
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java
View File

@ -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<List<String>> 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<ASTStatementExpression> 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<ASTIfStatement> parents1 = statement1.getParentsOfType(ASTIfStatement.class);
List<ASTIfStatement> parents2 = statement2.getParentsOfType(ASTIfStatement.class);
parents1.retainAll(parents2);
return !parents1.isEmpty();
}
private boolean isClosingVariableStatement(ASTStatementExpression statement, ASTVariableDeclarator variable) {
List<ASTPrimaryExpression> expressions = statement.findDescendantsOfType(ASTPrimaryExpression.class);
for (ASTPrimaryExpression expression : expressions) {
if (isMethodCallClosingResourceVariable(expression, variable.getName())) {
return true;
}
}
List<ASTName> 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());
}
}

47
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml
View File

@ -1426,6 +1426,53 @@ public class CloseResourceWithVar {
int c = inputStream.read();
return c;
}
}
]]></code>
</test-code>
<test-code>
<description>#2764 false-negative when re-assigning connection</description>
<rule-property name="types">java.sql.Connection,java.sql.Statement,java.sql.ResultSet</rule-property>
<expected-problems>1</expected-problems>
<expected-linenumbers>7</expected-linenumbers>
<expected-messages>
<message>'c' is reassigned, but the original instance is not closed</message>
</expected-messages>
<code><![CDATA[
import java.sql.Connection;
public class Foo {
void bar() {
Connection c = pool.getConnection();
try {
c = pool.getConnection();
} catch (Exception e) {
} finally {
c.close();
}
}
}
]]></code>
</test-code>
<test-code>
<description>#2764 false-negative when re-assigning connection - no problem when closed before</description>
<rule-property name="types">java.sql.Connection,java.sql.Statement,java.sql.ResultSet</rule-property>
<expected-problems>0</expected-problems>
<code><![CDATA[
import java.sql.Connection;
public class Foo {
void bar() {
Connection c = pool.getConnection();
try {
c.close();
c = pool.getConnection();
} catch (Exception e) {
} finally {
c.close();
}
}
}
]]></code>
</test-code>