Merge pull request #3199 from oowekyala:update-PrematureDeclaration

[java] Update rule PrematureDeclaration #3199
This commit is contained in:
Andreas Dangel
2021-04-16 09:31:48 +02:00
13 changed files with 897 additions and 373 deletions

View File

@ -98,7 +98,7 @@
<rule ref="category/java/codestyle.xml/NoPackage"/>
<!-- <rule ref="category/java/codestyle.xml/OnlyOneReturn"/> -->
<rule ref="category/java/codestyle.xml/PackageCase"/>
<!-- <rule ref="category/java/codestyle.xml/PrematureDeclaration"/> -->
<rule ref="category/java/codestyle.xml/PrematureDeclaration"/>
<rule ref="category/java/codestyle.xml/RemoteInterfaceNamingConvention"/>
<rule ref="category/java/codestyle.xml/RemoteSessionInterfaceNamingConvention"/>
<rule ref="category/java/codestyle.xml/ShortClassName"/>

View File

@ -139,6 +139,8 @@ The following previously deprecated rules have been finally removed:
* [#2883](https://github.com/pmd/pmd/issues/2883): \[java] JUnitAssertionsShouldIncludeMessage false positive with method call
* [#2890](https://github.com/pmd/pmd/issues/2890): \[java] UnusedPrivateMethod false positive with generics
* java-codestyle
* [#1208](https://github.com/pmd/pmd/issues/1208): \[java] PrematureDeclaration rule false-positive on variable declared to measure time
* [#1429](https://github.com/pmd/pmd/issues/1429): \[java] PrematureDeclaration as result of method call (false positive)
* [#1673](https://github.com/pmd/pmd/issues/1673): \[java] UselessParentheses false positive with conditional operator
* [#1790](https://github.com/pmd/pmd/issues/1790): \[java] UnnecessaryFullyQualifiedName false positive with enum constant
* [#1918](https://github.com/pmd/pmd/issues/1918): \[java] UselessParentheses false positive with boolean operators
@ -146,6 +148,7 @@ The following previously deprecated rules have been finally removed:
* [#2528](https://github.com/pmd/pmd/issues/2528): \[java] MethodNamingConventions - JUnit 5 method naming not support ParameterizedTest
* [#2739](https://github.com/pmd/pmd/issues/2739): \[java] UselessParentheses false positive for string concatenation
* [#3195](https://github.com/pmd/pmd/pull/3195): \[java] Improve rule UnnecessaryReturn to detect more cases
* [#3221](https://github.com/pmd/pmd/issues/3221): \[java] PrematureDeclaration false positive for unused variables
* java-errorprone
* [#1005](https://github.com/pmd/pmd/issues/1005): \[java] CloneMethodMustImplementCloneable triggers for interfaces
* [#2532](https://github.com/pmd/pmd/issues/2532): \[java] AvoidDecimalLiteralsInBigDecimalConstructor can not detect the case new BigDecimal(Expression)

View File

@ -32,8 +32,8 @@ import net.sourceforge.pmd.lang.java.ast.BinaryOp;
import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
import net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil;
import net.sourceforge.pmd.lang.java.symbols.JVariableSymbol;
import net.sourceforge.pmd.lang.java.types.InvocationMatcher;
import net.sourceforge.pmd.lang.java.types.TypeTestUtil;
import net.sourceforge.pmd.lang.java.types.TypeTestUtil.InvocationMatcher;
/**
* @author Clément Fournier

View File

@ -4,31 +4,28 @@
package net.sourceforge.pmd.lang.java.rule.bestpractices;
import static net.sourceforge.pmd.util.CollectionUtil.listOf;
import java.util.List;
import net.sourceforge.pmd.lang.java.ast.ASTMethodCall;
import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
import net.sourceforge.pmd.lang.java.rule.internal.TestFrameworksUtil;
import net.sourceforge.pmd.lang.java.types.TypeTestUtil.InvocationMatcher;
import net.sourceforge.pmd.lang.java.types.InvocationMatcher;
import net.sourceforge.pmd.lang.java.types.InvocationMatcher.CompoundInvocationMatcher;
public class JUnitAssertionsShouldIncludeMessageRule extends AbstractJavaRulechainRule {
private final List<InvocationMatcher> checks =
listOf(
InvocationMatcher.parse("_#assertEquals(_,_)"),
InvocationMatcher.parse("_#assertTrue(_)"),
InvocationMatcher.parse("_#assertFalse(_)"),
InvocationMatcher.parse("_#assertSame(_,_)"),
InvocationMatcher.parse("_#assertNotSame(_,_)"),
InvocationMatcher.parse("_#assertNull(_)"),
InvocationMatcher.parse("_#assertNotNull(_)"),
InvocationMatcher.parse("_#assertArrayEquals(_,_)"),
InvocationMatcher.parse("_#assertThat(_,_)"),
InvocationMatcher.parse("_#fail()"),
InvocationMatcher.parse("_#assertEquals(float,float,float)"),
InvocationMatcher.parse("_#assertEquals(double,double,double)")
private final CompoundInvocationMatcher checks =
InvocationMatcher.parseAll(
"_#assertEquals(_,_)",
"_#assertTrue(_)",
"_#assertFalse(_)",
"_#assertSame(_,_)",
"_#assertNotSame(_,_)",
"_#assertNull(_)",
"_#assertNotNull(_)",
"_#assertArrayEquals(_,_)",
"_#assertThat(_,_)",
"_#fail()",
"_#assertEquals(float,float,float)",
"_#assertEquals(double,double,double)"
);
public JUnitAssertionsShouldIncludeMessageRule() {
@ -38,11 +35,8 @@ public class JUnitAssertionsShouldIncludeMessageRule extends AbstractJavaRulecha
@Override
public Object visit(ASTMethodCall node, Object data) {
if (TestFrameworksUtil.isCallOnAssertionContainer(node)) {
for (InvocationMatcher check : checks) {
if (check.matchesCall(node)) {
addViolation(data, node);
break;
}
if (checks.anyMatch(node)) {
addViolation(data, node);
}
}
return null;

View File

@ -4,20 +4,28 @@
package net.sourceforge.pmd.lang.java.rule.codestyle;
import static java.util.Collections.emptySet;
import static net.sourceforge.pmd.lang.ast.NodeStream.asInstanceOf;
import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;
import net.sourceforge.pmd.lang.ast.Node;
import net.sourceforge.pmd.lang.java.ast.ASTBlockStatement;
import net.sourceforge.pmd.lang.ast.NodeStream;
import net.sourceforge.pmd.lang.java.ast.ASTAssignableExpr.ASTNamedReferenceExpr;
import net.sourceforge.pmd.lang.java.ast.ASTExpression;
import net.sourceforge.pmd.lang.java.ast.ASTForInit;
import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTName;
import net.sourceforge.pmd.lang.java.ast.ASTResource;
import net.sourceforge.pmd.lang.java.ast.ASTReturnStatement;
import net.sourceforge.pmd.lang.java.ast.ASTStatement;
import net.sourceforge.pmd.lang.java.ast.ASTThrowStatement;
import net.sourceforge.pmd.lang.java.ast.ASTVariableAccess;
import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
import net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil;
import net.sourceforge.pmd.lang.java.symbols.JVariableSymbol;
import net.sourceforge.pmd.lang.java.types.InvocationMatcher;
import net.sourceforge.pmd.lang.java.types.InvocationMatcher.CompoundInvocationMatcher;
/**
* Checks for variables in methods that are defined before they are really
@ -27,90 +35,98 @@ import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
*
* @author Brian Remedios
*/
public class PrematureDeclarationRule extends AbstractJavaRule {
public class PrematureDeclarationRule extends AbstractJavaRulechainRule {
private static final CompoundInvocationMatcher TIME_METHODS =
InvocationMatcher.parseAll(
"java.lang.System#nanoTime()",
"java.lang.System#currentTimeMillis()"
);
public PrematureDeclarationRule() {
super(ASTLocalVariableDeclaration.class);
}
@Override
public Object visit(ASTLocalVariableDeclaration node, Object data) {
// is it part of a for-loop declaration?
if (node.getParent() instanceof ASTForInit) {
// yes, those don't count
return super.visit(node, data);
if (node.getParent() instanceof ASTForInit
|| node.getParent() instanceof ASTResource) {
// those don't count
return null;
}
for (ASTVariableDeclaratorId id : node) {
for (ASTBlockStatement block : statementsAfter(node)) {
if (hasReferencesIn(block, id.getVariableName())) {
ASTExpression initializer = id.getInitializer();
if (JavaRuleUtil.isNeverUsed(id) // avoid the duplicate with unused variables
|| cannotBeMoved(initializer)
|| JavaRuleUtil.hasSideEffect(initializer, emptySet())) {
continue;
}
Set<JVariableSymbol> refsInInitializer = getReferencedVars(initializer);
// If there's no initializer, or the initializer doesn't depend on anything (eg, a literal),
// then we don't care about side-effects
boolean hasStatefulInitializer = !refsInInitializer.isEmpty() || JavaRuleUtil.hasSideEffect(initializer, emptySet());
for (ASTStatement stmt : statementsAfter(node)) {
if (hasReferencesIn(stmt, id)
|| hasStatefulInitializer && JavaRuleUtil.hasSideEffect(stmt, refsInInitializer)) {
break;
}
if (hasExit(block)) {
addViolation(data, node);
if (hasExit(stmt)) {
addViolation(data, node, id.getName());
break;
}
}
}
return super.visit(node, data);
return null;
}
/**
* Returns the set of local variables referenced inside the expression.
*/
private static Set<JVariableSymbol> getReferencedVars(ASTExpression term) {
return term == null ? emptySet()
: term.descendantsOrSelf()
.filterIs(ASTNamedReferenceExpr.class)
.filter(it -> it.getReferencedSym() != null)
.collect(Collectors.mapping(ASTNamedReferenceExpr::getReferencedSym, Collectors.toSet()));
}
/**
* Time methods cannot be moved ever, even when there are no side-effects.
* The side effect they depend on is the program being executed. Are they
* the only methods like that?
*/
private boolean cannotBeMoved(ASTExpression initializer) {
return TIME_METHODS.anyMatch(initializer);
}
/**
* Returns whether the block contains a return call or throws an exception.
* Exclude blocks that have these things as part of an inner class.
*/
private boolean hasExit(ASTBlockStatement block) {
return block.descendants().map(asInstanceOf(ASTThrowStatement.class, ASTReturnStatement.class)).nonEmpty();
private static boolean hasExit(ASTStatement block) {
return block.descendants()
.map(asInstanceOf(ASTThrowStatement.class, ASTReturnStatement.class))
.nonEmpty();
}
/**
* Returns whether the variable is mentioned within the statement or not.
*/
private static boolean hasReferencesIn(ASTBlockStatement block, String varName) {
// allow for closures on the var
for (ASTName name : block.findDescendantsOfType(ASTName.class, true)) {
if (isReference(varName, name.getImage())) {
return true;
}
}
return false;
private static boolean hasReferencesIn(ASTStatement stmt, ASTVariableDeclaratorId var) {
return stmt.descendants(ASTVariableAccess.class)
.crossFindBoundaries()
.filterMatching(ASTNamedReferenceExpr::getReferencedSym, var.getSymbol())
.nonEmpty();
}
/**
* Return whether the shortName is part of the compound name by itself or as
* a method call receiver.
*/
private static boolean isReference(String shortName, String compoundName) {
int dotPos = compoundName.indexOf('.');
return dotPos < 0 ? shortName.equals(compoundName) : shortName.equals(compoundName.substring(0, dotPos));
}
/**
* Returns all the block statements following the given local var declaration.
*/
private static List<ASTBlockStatement> statementsAfter(ASTLocalVariableDeclaration node) {
Node blockOrSwitch = node.getParent().getParent();
int count = blockOrSwitch.getNumChildren();
int start = node.getParent().getIndexInParent() + 1;
List<ASTBlockStatement> nextBlocks = new ArrayList<>(count - start);
for (int i = start; i < count; i++) {
Node maybeBlock = blockOrSwitch.getChild(i);
if (maybeBlock instanceof ASTBlockStatement) {
nextBlocks.add((ASTBlockStatement) maybeBlock);
}
}
return nextBlocks;
/** Returns all the statements following the given local var declaration. */
private static NodeStream<ASTStatement> statementsAfter(ASTLocalVariableDeclaration node) {
return node.asStream().followingSiblings().filterIs(ASTStatement.class);
}
}

View File

@ -28,6 +28,8 @@ import net.sourceforge.pmd.lang.ast.NodeStream;
import net.sourceforge.pmd.lang.ast.impl.javacc.JavaccToken;
import net.sourceforge.pmd.lang.java.ast.ASTAnyTypeDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTArgumentList;
import net.sourceforge.pmd.lang.java.ast.ASTArrayAccess;
import net.sourceforge.pmd.lang.java.ast.ASTAssignableExpr;
import net.sourceforge.pmd.lang.java.ast.ASTAssignableExpr.ASTNamedReferenceExpr;
import net.sourceforge.pmd.lang.java.ast.ASTAssignableExpr.AccessType;
import net.sourceforge.pmd.lang.java.ast.ASTAssignmentExpression;
@ -55,6 +57,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTNumericLiteral;
import net.sourceforge.pmd.lang.java.ast.ASTStatement;
import net.sourceforge.pmd.lang.java.ast.ASTSuperExpression;
import net.sourceforge.pmd.lang.java.ast.ASTThisExpression;
import net.sourceforge.pmd.lang.java.ast.ASTThrowStatement;
import net.sourceforge.pmd.lang.java.ast.ASTUnaryExpression;
import net.sourceforge.pmd.lang.java.ast.ASTVariableAccess;
import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
@ -68,6 +71,8 @@ import net.sourceforge.pmd.lang.java.ast.TypeNode;
import net.sourceforge.pmd.lang.java.ast.UnaryOp;
import net.sourceforge.pmd.lang.java.symbols.JFieldSymbol;
import net.sourceforge.pmd.lang.java.symbols.JVariableSymbol;
import net.sourceforge.pmd.lang.java.types.InvocationMatcher;
import net.sourceforge.pmd.lang.java.types.InvocationMatcher.CompoundInvocationMatcher;
import net.sourceforge.pmd.lang.java.types.JPrimitiveType.PrimitiveTypeKind;
import net.sourceforge.pmd.lang.java.types.JTypeMirror;
import net.sourceforge.pmd.lang.java.types.TypeTestUtil;
@ -78,6 +83,22 @@ import net.sourceforge.pmd.util.CollectionUtil;
*/
public final class JavaRuleUtil {
// this is a hacky way to do it, but let's see where this goes
private static final CompoundInvocationMatcher KNOWN_PURE_METHODS = InvocationMatcher.parseAll(
"_#toString()",
"_#hashCode()",
"_#equals(java.lang.Object)",
"java.lang.String#_(_*)",
// actually not all of them, probs only stream of some type
// arg which doesn't implement Closeable...
"java.util.stream.Stream#_(_*)",
"java.util.Collection#size()",
"java.util.List#get(int)",
"java.util.Map#get(_)",
"java.lang.Iterable#iterator()",
"java.lang.Comparable#compareTo(_)"
);
private JavaRuleUtil() {
// utility class
}
@ -648,6 +669,17 @@ public final class JavaRuleUtil {
return e instanceof ASTThisExpression && ((ASTThisExpression) e).getQualifier() == null;
}
/**
* Returns true if the expression is a {@link ASTNamedReferenceExpr}
* that references any of the symbol in the set.
*/
public static boolean isReferenceToVar(@Nullable ASTExpression expression, @NonNull Set<? extends JVariableSymbol> symbols) {
if (expression instanceof ASTNamedReferenceExpr) {
return symbols.contains(((ASTNamedReferenceExpr) expression).getReferencedSym());
}
return false;
}
/**
* Returns true if both expressions refer to the same variable.
* A "variable" here can also means a field path, eg, {@code this.field.a}.
@ -688,6 +720,17 @@ public final class JavaRuleUtil {
return false;
}
/**
* Returns true if the expression is a reference to a local variable.
*/
public static boolean isReferenceToLocal(ASTExpression expr) {
if (expr instanceof ASTVariableAccess) {
JVariableSymbol sym = ((ASTVariableAccess) expr).getReferencedSym();
return sym != null && !sym.isField();
}
return false;
}
/**
* Returns true if the expression has the form `field`, or `this.field`,
* where `field` is a field declared in the enclosing class.
@ -750,4 +793,59 @@ public final class JavaRuleUtil {
Node parent = it.getParent();
return parent == null || it.getIndexInParent() == parent.getNumChildren() - 1;
}
/**
* Whether the node or one of its descendants is an expression with
* side effects. Conservatively, any method call is a potential side-effect,
* as well as assignments to fields or array elements. We could relax
* this assumption with (much) more data-flow logic, including a memory model.
*
* <p>By default assignments to locals are not counted as side-effects,
* unless the lhs is in the given set of symbols.
*
* @param node A node
* @param localVarsToTrack Local variables to track
*/
public static boolean hasSideEffect(@Nullable JavaNode node, Set<? extends JVariableSymbol> localVarsToTrack) {
return node != null && node.descendantsOrSelf()
.filterIs(ASTExpression.class)
.any(e -> hasSideEffectNonRecursive(e, localVarsToTrack));
}
/**
* Returns true if the expression has side effects we don't track.
* Does not recurse into sub-expressions.
*/
private static boolean hasSideEffectNonRecursive(ASTExpression e, Set<? extends JVariableSymbol> localVarsToTrack) {
if (e instanceof ASTAssignmentExpression) {
ASTAssignableExpr lhs = ((ASTAssignmentExpression) e).getLeftOperand();
return isNonLocalLhs(lhs) || isReferenceToVar(lhs, localVarsToTrack);
} else if (e instanceof ASTUnaryExpression) {
ASTUnaryExpression unary = (ASTUnaryExpression) e;
ASTExpression lhs = unary.getOperand();
return !unary.getOperator().isPure()
&& (isNonLocalLhs(lhs) || isReferenceToVar(lhs, localVarsToTrack));
}
if (e.ancestors(ASTThrowStatement.class).nonEmpty()) {
// then this side effect can never be observed in containing code,
// because control flow jumps out of the method
return false;
}
return e instanceof ASTMethodCall && !isPure((ASTMethodCall) e)
|| e instanceof ASTConstructorCall;
}
private static boolean isNonLocalLhs(ASTExpression lhs) {
return lhs instanceof ASTArrayAccess || !isReferenceToLocal(lhs);
}
/**
* Whether the invocation has no side-effects. Very conservative.
*/
private static boolean isPure(ASTMethodCall call) {
return isGetterCall(call) || KNOWN_PURE_METHODS.anyMatch(call);
}
}

View File

@ -5,7 +5,7 @@
package net.sourceforge.pmd.lang.java.rule.xpath.internal;
import net.sourceforge.pmd.lang.java.ast.InvocationNode;
import net.sourceforge.pmd.lang.java.types.TypeTestUtil.InvocationMatcher;
import net.sourceforge.pmd.lang.java.types.InvocationMatcher;
import net.sf.saxon.trans.XPathException;

View File

@ -1204,18 +1204,28 @@ public class SomeClass {
<rule name="PrematureDeclaration"
language="java"
since="5.0"
message="Avoid declaring a variable if it is unreferenced before a possible exit point."
message="Declaration of ''{0}'' can be moved closer to its usages"
class="net.sourceforge.pmd.lang.java.rule.codestyle.PrematureDeclarationRule"
externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_codestyle.html#prematuredeclaration">
<description>
Checks for variables that are defined before they might be used. A reference is deemed to be premature if it is created right before a block of code that doesn't use it that also has the ability to return or throw an exception.
Checks for variables that are defined before they might be used. A declaration is
deemed to be premature if there are some statements that may return or throw an
exception between the time the variable is declared and the time it is first read.
Some variables cannot be declared close to their first usage because of side-effects
occurring before they're first used. We try to avoid reporting those by considering
most method and constructor invocations to be impure. See the second example.
Note that this rule is meant to improve code readability but is not an optimization.
A smart JIT will not care whether the variable is declared prematurely or not, as it
can reorder code.
</description>
<priority>3</priority>
<example>
<![CDATA[
public int getLength(String[] strings) {
int length = 0; // declared prematurely
int length = 0; // could be moved closer to the loop
if (strings == null || strings.length == 0) return 0;
@ -1225,6 +1235,25 @@ public int getLength(String[] strings) {
return length;
}
]]>
</example>
<example>
<![CDATA[
public int getLength(String[] strings) {
int startTime = System.nanoTime(); // cannot be moved because initializer is impure
if (strings == null || strings.length == 0) {
// some error logic
throw new SomeException(...);
}
for (String str : strings) {
length += str.length();
}
return System.nanoTime() - startTime;
}
]]>
</example>
</rule>

View File

@ -6,7 +6,6 @@ package net.sourceforge.pmd.lang.java.rule.codestyle;
import net.sourceforge.pmd.testframework.PmdRuleTst;
@org.junit.Ignore("Rule has not been updated yet")
public class PrematureDeclarationTest extends PmdRuleTst {
// no additional unit tests
}

View File

@ -4,7 +4,7 @@
package net.sourceforge.pmd.lang.java.types;
import static net.sourceforge.pmd.lang.java.types.TypeTestUtil.InvocationMatcher.parse;
import static net.sourceforge.pmd.lang.java.types.InvocationMatcher.parse;
import static org.hamcrest.Matchers.equalTo;
import org.hamcrest.MatcherAssert;