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

[java] Improve LocalVariableCouldBeFinal (#5003)

Browse Source 
Merge pull request #5003 from oowekyala:issue1619-localVariableCouldBeFinal-FP
This commit is contained in:
Andreas Dangel
2024-05-31 10:39:37 +02:00
parent 3890b561a1 8a8402a0bd
commit 2a0cba7b91
10 changed files with 260 additions and 56 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
docs/pages/release_notes.md
View File

@ -49,6 +49,8 @@ For the changes, see [PMD Designer Changelog (7.2.0)](https://github.com/pmd/pmd
* [#4975](https://github.com/pmd/pmd/issues/4975): \[java] UnusedPrivateMethod false positive when using @MethodSource on a @Nested test
* [#4985](https://github.com/pmd/pmd/issues/4985): \[java] UnusedPrivateMethod false-positive / method reference in combination with custom object
* java-codestyle
* [#1619](https://github.com/pmd/pmd/issues/1619): \[java] LocalVariableCouldBeFinal on 'size' variable in for loop
* [#3122](https://github.com/pmd/pmd/issues/3122): \[java] LocalVariableCouldBeFinal should consider blank local variables
* [#4903](https://github.com/pmd/pmd/issues/4903): \[java] UnnecessaryBoxing, but explicit conversion is necessary
* [#4924](https://github.com/pmd/pmd/issues/4924): \[java] UnnecessaryBoxing false positive in PMD 7.0.0 in lambda
* [#4930](https://github.com/pmd/pmd/issues/4930): \[java] EmptyControlStatement should not allow empty try with concise resources

10
pmd-core/src/main/java/net/sourceforge/pmd/util/DataMap.java
View File

@ -6,9 +6,11 @@ package net.sourceforge.pmd.util;
import java.util.IdentityHashMap;
import java.util.Map;
import java.util.function.BiFunction;
import java.util.function.Function;
import java.util.function.Supplier;
import org.checkerframework.checker.nullness.qual.NonNull;
import org.checkerframework.checker.nullness.qual.Nullable;
/**
@ -88,6 +90,14 @@ public final class DataMap<K> {
return (T) getMap().compute(key, (k, v) -> function.apply((T) v));
}
/**
* @see Map#merge(Object, Object, BiFunction)
*/
@SuppressWarnings({ "unchecked", "rawtypes" })
public <T> T merge(DataKey<? extends K, T> key, T value, BiFunction<? super @NonNull T, ? super T, ? extends T> function) {
return (T) getMap().merge(key, value, (BiFunction) function);
}
private Map<DataKey<? extends K, ?>, Object> getMap() {
// the map is lazily created, it's only needed if set() is called
// at least once, but get() might be called many more times, as

3
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTAssignableExpr.java
View File

@ -36,6 +36,9 @@ public interface ASTAssignableExpr extends ASTPrimaryExpression {
* If this expression occurs as the left-hand-side of an {@linkplain ASTAssignmentExpression assignment},
* or as the target of an {@linkplain ASTUnaryExpression increment or decrement expression},
* this method returns {@link AccessType#WRITE}. Otherwise the value is just {@linkplain AccessType#READ read}.
*
* @see JavaAstUtils#isVarAccessReadAndWrite(ASTNamedReferenceExpr)
* @see JavaAstUtils#isVarAccessStrictlyWrite(ASTNamedReferenceExpr)
*/
default @NonNull AccessType getAccessType() {

42
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/JavaAstUtils.java
View File

@ -79,6 +79,9 @@ import net.sourceforge.pmd.lang.java.ast.ModifierOwner.Visibility;
import net.sourceforge.pmd.lang.java.ast.QualifiableExpression;
import net.sourceforge.pmd.lang.java.ast.TypeNode;
import net.sourceforge.pmd.lang.java.ast.UnaryOp;
import net.sourceforge.pmd.lang.java.rule.internal.DataflowPass;
import net.sourceforge.pmd.lang.java.rule.internal.DataflowPass.DataflowResult;
import net.sourceforge.pmd.lang.java.rule.internal.DataflowPass.ReachingDefinitionSet;
import net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil;
import net.sourceforge.pmd.lang.java.symbols.JExecutableSymbol;
import net.sourceforge.pmd.lang.java.symbols.JFieldSymbol;
@ -807,7 +810,6 @@ public final class JavaAstUtils {
);
}
/**
* Return the target of the return. May be an {@link ASTMethodDeclaration},
* {@link ASTLambdaExpression}, {@link ASTInitializer},
@ -816,11 +818,39 @@ public final class JavaAstUtils {
*/
public static @Nullable JavaNode getReturnTarget(ASTReturnStatement stmt) {
return stmt.ancestors().first(
it -> it instanceof ASTMethodDeclaration
|| it instanceof ASTLambdaExpression
|| it instanceof ASTConstructorDeclaration
|| it instanceof ASTInitializer
|| it instanceof ASTCompactConstructorDeclaration
it -> it instanceof ASTMethodDeclaration
|| it instanceof ASTLambdaExpression
|| it instanceof ASTConstructorDeclaration
|| it instanceof ASTInitializer
|| it instanceof ASTCompactConstructorDeclaration
);
}
/**
* Return true if the variable is effectively final. This means
* the variable is never reassigned.
*/
public static boolean isEffectivelyFinal(ASTVariableId var) {
if (var.getInitializer() == null && var.isLocalVariable()) {
// blank variables may be assigned on several paths
DataflowResult dataflow = DataflowPass.getDataflowResult(var.getRoot());
for (ASTNamedReferenceExpr usage : var.getLocalUsages()) {
if (usage.getAccessType() == AccessType.WRITE) {
ReachingDefinitionSet reaching = dataflow.getReachingDefinitions(usage);
if (reaching.isNotFullyKnown() || !reaching.getReaching().isEmpty()) {
// If the reaching def is not empty, then that means
// the assignment is killing another one, ie it is a reassignment.
return false;
}
}
}
return true;
}
for (ASTNamedReferenceExpr usage : var.getLocalUsages()) {
if (usage.getAccessType() == AccessType.WRITE) {
return false;
}
}
return true;
}
}

14
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LocalVariableCouldBeFinalRule.java
View File

@ -8,9 +8,10 @@ import static net.sourceforge.pmd.properties.PropertyFactory.booleanProperty;
import net.sourceforge.pmd.lang.java.ast.ASTForeachStatement;
import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTVariableId;
import net.sourceforge.pmd.lang.java.ast.internal.JavaAstUtils;
import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
import net.sourceforge.pmd.properties.PropertyDescriptor;
import net.sourceforge.pmd.reporting.RuleContext;
public class LocalVariableCouldBeFinalRule extends AbstractJavaRulechainRule {
@ -30,7 +31,16 @@ public class LocalVariableCouldBeFinalRule extends AbstractJavaRulechainRule {
if (getProperty(IGNORE_FOR_EACH) && node.getParent() instanceof ASTForeachStatement) {
return data;
}
MethodArgumentCouldBeFinalRule.checkForFinal((RuleContext) data, node.getVarIds());
if (node.getVarIds().all(JavaAstUtils::isEffectivelyFinal)) {
// All variables declared in this ASTLocalVariableDeclaration need to be
// effectively final, otherwise we cannot just add a final modifier.
for (ASTVariableId vid : node.getVarIds()) {
if (!JavaAstUtils.isNeverUsed(vid)) {
// filter out unused variables
asCtx(data).addViolation(vid, vid.getName());
}
}
}
return data;
}

29
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodArgumentCouldBeFinalRule.java
View File

@ -4,16 +4,13 @@
package net.sourceforge.pmd.lang.java.rule.codestyle;
import net.sourceforge.pmd.lang.ast.NodeStream;
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.ASTConstructorDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTExecutableDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTFormalParameter;
import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTVariableId;
import net.sourceforge.pmd.lang.java.ast.internal.JavaAstUtils;
import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
import net.sourceforge.pmd.reporting.RuleContext;
public class MethodArgumentCouldBeFinalRule extends AbstractJavaRulechainRule {
@ -37,24 +34,12 @@ public class MethodArgumentCouldBeFinalRule extends AbstractJavaRulechainRule {
}
private void lookForViolation(ASTExecutableDeclaration node, Object data) {
checkForFinal((RuleContext) data, node.getFormalParameters().toStream().map(ASTFormalParameter::getVarId));
}
static void checkForFinal(RuleContext ruleContext, NodeStream<ASTVariableId> variables) {
outer:
for (ASTVariableId var : variables) {
if (var.isFinal()) {
continue;
}
boolean used = false;
for (ASTNamedReferenceExpr usage : var.getLocalUsages()) {
used = true;
if (usage.getAccessType() == AccessType.WRITE) {
continue outer;
}
}
if (used) {
ruleContext.addViolation(var, var.getName());
for (ASTFormalParameter param : node.getFormalParameters()) {
ASTVariableId varId = param.getVarId();
if (!param.isFinal()
&& !JavaAstUtils.isNeverUsed(varId)
&& JavaAstUtils.isEffectivelyFinal(varId)) {
asCtx(data).addViolation(varId, varId.getName());
}
}
}

3
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/SingularFieldRule.java
View File

@ -165,6 +165,9 @@ public class SingularFieldRule extends AbstractJavaRulechainRule {
private boolean usagesObserveValueBeforeMethodCall(List<ASTNamedReferenceExpr> usages, DataflowResult dataflow) {
for (ASTNamedReferenceExpr usage : usages) {
if (JavaAstUtils.isVarAccessStrictlyWrite(usage)) {
continue;
}
ReachingDefinitionSet reaching = dataflow.getReachingDefinitions(usage);
if (reaching.containsInitialFieldValue()) {
return true;

81
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/internal/DataflowPass.java
View File

@ -4,6 +4,7 @@
package net.sourceforge.pmd.lang.java.rule.internal;
import static java.util.Collections.emptySet;
import static net.sourceforge.pmd.util.CollectionUtil.asSingle;
import java.util.ArrayDeque;
@ -177,12 +178,21 @@ public final class DataflowPass {
*/
public static final class ReachingDefinitionSet {
static final ReachingDefinitionSet UNKNOWN = new ReachingDefinitionSet();
static final ReachingDefinitionSet EMPTY_KNOWN = new ReachingDefinitionSet(emptySet());
private Set<AssignmentEntry> reaching;
private boolean isNotFullyKnown;
private boolean containsInitialFieldValue;
static {
assert !EMPTY_KNOWN.isNotFullyKnown();
assert UNKNOWN.isNotFullyKnown();
}
private ReachingDefinitionSet() {
this.reaching = Collections.emptySet();
this.reaching = emptySet();
this.containsInitialFieldValue = false;
this.isNotFullyKnown = true;
}
@ -228,6 +238,10 @@ public final class DataflowPass {
public static ReachingDefinitionSet unknown() {
return new ReachingDefinitionSet();
}
public static ReachingDefinitionSet blank() {
return new ReachingDefinitionSet(emptySet());
}
}
/**
@ -256,7 +270,7 @@ public final class DataflowPass {
* May be useful to check for reassignment.
*/
public @NonNull Set<AssignmentEntry> getKillers(AssignmentEntry assignment) {
return killRecord.getOrDefault(assignment, Collections.emptySet());
return killRecord.getOrDefault(assignment, emptySet());
}
// These methods are only valid to be called if the dataflow pass has run.
@ -278,14 +292,25 @@ public final class DataflowPass {
return expr.getUserMap().computeIfAbsent(REACHING_DEFS, () -> reachingFallback(expr));
}
// Fallback, to compute reaching definitions for some fields
// Fallback, to compute reaching definitions for some nodes
// that are not tracked by the tree exploration. Final fields
// indeed have a fully known set of reaching definitions.
// TODO maybe they should actually be tracked?
private @NonNull ReachingDefinitionSet reachingFallback(ASTNamedReferenceExpr expr) {
JVariableSymbol sym = expr.getReferencedSym();
if (sym == null || !sym.isField() || !sym.isFinal()) {
if (sym == null || sym.isField() && !sym.isFinal()) {
return ReachingDefinitionSet.unknown();
} else if (!sym.isField()) {
ASTVariableId node = sym.tryGetNode();
assert node != null
: "Not a field, and symbol is known, so should be a local which has a node";
if (node.isLocalVariable()) {
assert node.getInitializer() == null : "Should be a blank local variable";
return ReachingDefinitionSet.blank();
} else {
// Formal parameter or other kind of def which has
// an implicit initializer.
return ReachingDefinitionSet.unknown();
}
}
ASTVariableId node = sym.tryGetNode();
@ -965,23 +990,25 @@ public final class DataflowPass {
}
private SpanInfo processAssignment(ASTExpression lhs, // LHS or unary operand
private SpanInfo processAssignment(ASTExpression lhs0, // LHS or unary operand
ASTExpression rhs, // RHS or unary
boolean useBeforeAssigning,
SpanInfo result) {
if (lhs instanceof ASTNamedReferenceExpr) {
JVariableSymbol lhsVar = ((ASTNamedReferenceExpr) lhs).getReferencedSym();
if (lhs0 instanceof ASTNamedReferenceExpr) {
ASTNamedReferenceExpr lhs = (ASTNamedReferenceExpr) lhs0;
JVariableSymbol lhsVar = lhs.getReferencedSym();
if (lhsVar != null
&& (lhsVar instanceof JLocalVariableSymbol
|| isRelevantField(lhs))) {
if (useBeforeAssigning) {
// compound assignment, to use BEFORE assigning
result.use(lhsVar, (ASTNamedReferenceExpr) lhs);
result.use(lhsVar, lhs);
}
result.assign(lhsVar, rhs);
VarLocalInfo oldVar = result.assign(lhsVar, rhs);
SpanInfo.updateReachingDefs(lhs, lhsVar, oldVar);
}
}
return result;
@ -1321,11 +1348,12 @@ public final class DataflowPass {
assign(id.getSymbol(), id);
}
void assign(JVariableSymbol var, JavaNode rhs) {
assign(var, rhs, SpecialAssignmentKind.NOT_SPECIAL);
VarLocalInfo assign(JVariableSymbol var, JavaNode rhs) {
return assign(var, rhs, SpecialAssignmentKind.NOT_SPECIAL);
}
@Nullable AssignmentEntry assign(JVariableSymbol var, JavaNode rhs, SpecialAssignmentKind kind) {
@Nullable
VarLocalInfo assign(JVariableSymbol var, JavaNode rhs, SpecialAssignmentKind kind) {
ASTVariableId node = var.tryGetNode();
if (node == null) {
return null; // we don't care about non-local declarations
@ -1355,7 +1383,7 @@ public final class DataflowPass {
}
}
global.allAssignments.add(entry);
return entry;
return previous;
}
void declareSpecialFieldValues(JClassSymbol sym, boolean onlyStatic) {
@ -1399,20 +1427,25 @@ public final class DataflowPass {
if (info != null) {
global.usedAssignments.addAll(info.reachingDefs);
if (reachingDefSink != null) {
ReachingDefinitionSet reaching = new ReachingDefinitionSet(new LinkedHashSet<>(info.reachingDefs));
// need to merge into previous to account for cyclic control flow
reachingDefSink.getUserMap().compute(REACHING_DEFS, current -> {
if (current != null) {
current.absorb(reaching);
return current;
} else {
return reaching;
}
});
updateReachingDefs(reachingDefSink, var, info);
}
}
}
private static void updateReachingDefs(@NonNull ASTNamedReferenceExpr reachingDefSink, JVariableSymbol var, VarLocalInfo info) {
ReachingDefinitionSet reaching;
if (info == null || var.isField() && var.isFinal()) {
return;
} else {
reaching = new ReachingDefinitionSet(new LinkedHashSet<>(info.reachingDefs));
}
// need to merge into previous to account for cyclic control flow
reachingDefSink.getUserMap().merge(REACHING_DEFS, reaching, (current, newer) -> {
current.absorb(newer);
return current;
});
}
void deleteVar(JVariableSymbol var) {
symtable.remove(var);
}

54
pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/internal/DataflowPassTest.java
View File

@ -5,13 +5,22 @@
package net.sourceforge.pmd.lang.java.rule.internal;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.is;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import java.util.List;
import org.hamcrest.Matchers;
import org.junit.jupiter.api.Test;
import net.sourceforge.pmd.lang.java.BaseParserTest;
import net.sourceforge.pmd.lang.java.ast.ASTCompilationUnit;
import net.sourceforge.pmd.lang.java.ast.ASTVariableId;
import net.sourceforge.pmd.lang.java.ast.internal.JavaAstUtils;
import net.sourceforge.pmd.lang.java.rule.internal.DataflowPass.AssignmentEntry;
import net.sourceforge.pmd.lang.java.rule.internal.DataflowPass.DataflowResult;
import net.sourceforge.pmd.lang.java.rule.internal.DataflowPass.ReachingDefinitionSet;
/**
* @author Clément Fournier
@ -28,8 +37,49 @@ class DataflowPassTest extends BaseParserTest {
);
DataflowResult dataflow = DataflowPass.getDataflowResult(ast);
assertThat(dataflow.getUnusedAssignments(), Matchers.hasSize(0));
assertThat(dataflow.getUnusedAssignments(), hasSize(0));
}
@Test
void testBlankLocals() {
ASTCompilationUnit ast = java.parse(
"class A { static {"
+ " int a;"
+ " a = 0;"
+ " } }");
DataflowResult df = DataflowPass.getDataflowResult(ast);
List<ASTVariableId> list = ast.descendants(ASTVariableId.class).toList();
ASTVariableId a = list.get(0);
ReachingDefinitionSet reachingAEqZero = df.getReachingDefinitions(a.getLocalUsages().get(0));
assertThat(reachingAEqZero.isNotFullyKnown(), is(false));
assertThat(reachingAEqZero.getReaching(), hasSize(0));
}
@Test
void testBlankFinalField() {
ASTCompilationUnit ast = java.parse(
"class A { final int field; int nonFinal; A() { field = 2; } {"
+ " use(field);"
+ " use(nonFinal);"
+ " } }");
DataflowResult df = DataflowPass.getDataflowResult(ast);
List<ASTVariableId> list = ast.descendants(ASTVariableId.class).toList();
ASTVariableId field = list.get(0);
ReachingDefinitionSet finalUse = df.getReachingDefinitions(field.getLocalUsages().get(0));
assertThat(finalUse.isNotFullyKnown(), is(false));
assertThat(finalUse.getReaching(), hasSize(1));
AssignmentEntry assignment = finalUse.getReaching().iterator().next();
assertFalse(assignment.isBlankDeclaration());
assertFalse(assignment.isFieldDefaultValue());
assertTrue(JavaAstUtils.isLiteralInt(assignment.rhs, 2));
ASTVariableId nonFinal = list.get(1);
ReachingDefinitionSet nonFinalUse = df.getReachingDefinitions(nonFinal.getLocalUsages().get(0));
assertThat(nonFinalUse.isNotFullyKnown(), is(true));
assertThat(nonFinalUse.getReaching(), hasSize(1));
assignment = nonFinalUse.getReaching().iterator().next();
assertTrue(assignment.isBlankDeclaration());
assertTrue(assignment.isFieldDefaultValue());
}
}

78
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/LocalVariableCouldBeFinal.xml
View File

@ -308,6 +308,84 @@ public class ClassWithLambda {
System.out.println(a);
};
}
}
]]></code>
</test-code>
<test-code>
<description>#1619 FP with loop variable</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class ClassWithLambda {
void method() {
for (int i = 0, size = loaders.size(); i < size; ++i);
}
}
]]></code>
</test-code>
<test-code>
<description>#3122 should consider blank variables</description>
<expected-problems>2</expected-problems>
<expected-linenumbers>12,24</expected-linenumbers>
<code><![CDATA[
public class ClassWithLambda {
void method() {
int a = 2; // nowarn
a = 4;
System.out.println(a);
int b; // nowarn
b = 1 + 2;
b = 4;
System.out.println(b);
int c; // warn
c = 1 + 2;
System.out.println(c);
int x;
if (getClass().isAnonymousClass()) {
x = 2;
} else {
x = 4;
}
// x is unused
int y; // warn
if (getClass().isAnonymousClass()) {
y = 2;
} else {
y = 4;
}
System.out.println(y);
}
}
]]></code>
</test-code>
<test-code>
<description>#3122 should consider blank variables</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>5</expected-linenumbers>
<code><![CDATA[
public class ClassWithLambda {
void expandKey(byte key[]) {
int octet;
byte ek[] = new byte[128];
octet = key[0];
if ((octet & 0x80) != 0) {
ek[104] |= 4; ek[122] |= 32;
}
octet = key[1];
if ((octet & 0x80) != 0) {
ek[ 1] |= 8; ek[ 8] |= 32; ek[ 17] |= 1;
ek[ 24] |= 16; ek[ 35] |= 4; ek[ 50] |= 1;
ek[ 57] |= 16; ek[ 67] |= 8; ek[ 83] |= 1;
ek[ 88] |= 1; ek[ 98] |= 4; ek[105] |= 32;
ek[114] |= 32; ek[123] |= 2;
}
}
}
]]></code>
</test-code>