Skip to content

Commit 2a0cba7

Browse files
adangeladangel
committed
[java] Improve LocalVariableCouldBeFinal (#5003)
Merge pull request #5003 from oowekyala:issue1619-localVariableCouldBeFinal-FP
2 parents 3890b56 + 8a8402a commit 2a0cba7

10 files changed

Lines changed: 260 additions & 56 deletions

File tree

‎docs/pages/release_notes.md‎

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

‎pmd-core/src/main/java/net/sourceforge/pmd/util/DataMap.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,11 @@
66

77
import java.util.IdentityHashMap;
88
import java.util.Map;
9+
import java.util.function.BiFunction;
910
import java.util.function.Function;
1011
import java.util.function.Supplier;
1112

13+
import org.checkerframework.checker.nullness.qual.NonNull;
1214
import org.checkerframework.checker.nullness.qual.Nullable;
1315

1416
/**
@@ -88,6 +90,14 @@ public <T> T compute(DataKey<? extends K, T> key, Function<? super @Nullable T,
8890
return (T) getMap().compute(key, (k, v) -> function.apply((T) v));
8991
}
9092

93+
/**
94+
* @see Map#merge(Object, Object, BiFunction)
95+
*/
96+
@SuppressWarnings({ "unchecked", "rawtypes" })
97+
public <T> T merge(DataKey<? extends K, T> key, T value, BiFunction<? super @NonNull T, ? super T, ? extends T> function) {
98+
return (T) getMap().merge(key, value, (BiFunction) function);
99+
}
100+
91101
private Map<DataKey<? extends K, ?>, Object> getMap() {
92102
// the map is lazily created, it's only needed if set() is called
93103
// at least once, but get() might be called many more times, as

‎pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTAssignableExpr.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,9 @@ public interface ASTAssignableExpr extends ASTPrimaryExpression {
3636
* If this expression occurs as the left-hand-side of an {@linkplain ASTAssignmentExpression assignment},
3737
* or as the target of an {@linkplain ASTUnaryExpression increment or decrement expression},
3838
* this method returns {@link AccessType#WRITE}. Otherwise the value is just {@linkplain AccessType#READ read}.
39+
*
40+
* @see JavaAstUtils#isVarAccessReadAndWrite(ASTNamedReferenceExpr)
41+
* @see JavaAstUtils#isVarAccessStrictlyWrite(ASTNamedReferenceExpr)
3942
*/
4043
default @NonNull AccessType getAccessType() {
4144

‎pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/JavaAstUtils.java‎

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,9 @@
7979
import net.sourceforge.pmd.lang.java.ast.QualifiableExpression;
8080
import net.sourceforge.pmd.lang.java.ast.TypeNode;
8181
import net.sourceforge.pmd.lang.java.ast.UnaryOp;
82+
import net.sourceforge.pmd.lang.java.rule.internal.DataflowPass;
83+
import net.sourceforge.pmd.lang.java.rule.internal.DataflowPass.DataflowResult;
84+
import net.sourceforge.pmd.lang.java.rule.internal.DataflowPass.ReachingDefinitionSet;
8285
import net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil;
8386
import net.sourceforge.pmd.lang.java.symbols.JExecutableSymbol;
8487
import net.sourceforge.pmd.lang.java.symbols.JFieldSymbol;
@@ -807,7 +810,6 @@ public static boolean isInStaticCtx(JavaNode node) {
807810
);
808811
}
809812

810-
811813
/**
812814
* Return the target of the return. May be an {@link ASTMethodDeclaration},
813815
* {@link ASTLambdaExpression}, {@link ASTInitializer},
@@ -816,11 +818,39 @@ public static boolean isInStaticCtx(JavaNode node) {
816818
*/
817819
public static @Nullable JavaNode getReturnTarget(ASTReturnStatement stmt) {
818820
return stmt.ancestors().first(
819-
it -> it instanceof ASTMethodDeclaration
820-
|| it instanceof ASTLambdaExpression
821-
|| it instanceof ASTConstructorDeclaration
822-
|| it instanceof ASTInitializer
823-
|| it instanceof ASTCompactConstructorDeclaration
821+
it -> it instanceof ASTMethodDeclaration
822+
|| it instanceof ASTLambdaExpression
823+
|| it instanceof ASTConstructorDeclaration
824+
|| it instanceof ASTInitializer
825+
|| it instanceof ASTCompactConstructorDeclaration
824826
);
825827
}
828+
829+
/**
830+
* Return true if the variable is effectively final. This means
831+
* the variable is never reassigned.
832+
*/
833+
public static boolean isEffectivelyFinal(ASTVariableId var) {
834+
if (var.getInitializer() == null && var.isLocalVariable()) {
835+
// blank variables may be assigned on several paths
836+
DataflowResult dataflow = DataflowPass.getDataflowResult(var.getRoot());
837+
for (ASTNamedReferenceExpr usage : var.getLocalUsages()) {
838+
if (usage.getAccessType() == AccessType.WRITE) {
839+
ReachingDefinitionSet reaching = dataflow.getReachingDefinitions(usage);
840+
if (reaching.isNotFullyKnown() || !reaching.getReaching().isEmpty()) {
841+
// If the reaching def is not empty, then that means
842+
// the assignment is killing another one, ie it is a reassignment.
843+
return false;
844+
}
845+
}
846+
}
847+
return true;
848+
}
849+
for (ASTNamedReferenceExpr usage : var.getLocalUsages()) {
850+
if (usage.getAccessType() == AccessType.WRITE) {
851+
return false;
852+
}
853+
}
854+
return true;
855+
}
826856
}

‎pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LocalVariableCouldBeFinalRule.java‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,10 @@
88

99
import net.sourceforge.pmd.lang.java.ast.ASTForeachStatement;
1010
import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration;
11+
import net.sourceforge.pmd.lang.java.ast.ASTVariableId;
12+
import net.sourceforge.pmd.lang.java.ast.internal.JavaAstUtils;
1113
import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
1214
import net.sourceforge.pmd.properties.PropertyDescriptor;
13-
import net.sourceforge.pmd.reporting.RuleContext;
1415

1516
public class LocalVariableCouldBeFinalRule extends AbstractJavaRulechainRule {
1617

@@ -30,7 +31,16 @@ public Object visit(ASTLocalVariableDeclaration node, Object data) {
3031
if (getProperty(IGNORE_FOR_EACH) && node.getParent() instanceof ASTForeachStatement) {
3132
return data;
3233
}
33-
MethodArgumentCouldBeFinalRule.checkForFinal((RuleContext) data, node.getVarIds());
34+
if (node.getVarIds().all(JavaAstUtils::isEffectivelyFinal)) {
35+
// All variables declared in this ASTLocalVariableDeclaration need to be
36+
// effectively final, otherwise we cannot just add a final modifier.
37+
for (ASTVariableId vid : node.getVarIds()) {
38+
if (!JavaAstUtils.isNeverUsed(vid)) {
39+
// filter out unused variables
40+
asCtx(data).addViolation(vid, vid.getName());
41+
}
42+
}
43+
}
3444
return data;
3545
}
3646

‎pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodArgumentCouldBeFinalRule.java‎

Lines changed: 7 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -4,16 +4,13 @@
44

55
package net.sourceforge.pmd.lang.java.rule.codestyle;
66

7-
import net.sourceforge.pmd.lang.ast.NodeStream;
8-
import net.sourceforge.pmd.lang.java.ast.ASTAssignableExpr.ASTNamedReferenceExpr;
9-
import net.sourceforge.pmd.lang.java.ast.ASTAssignableExpr.AccessType;
107
import net.sourceforge.pmd.lang.java.ast.ASTConstructorDeclaration;
118
import net.sourceforge.pmd.lang.java.ast.ASTExecutableDeclaration;
129
import net.sourceforge.pmd.lang.java.ast.ASTFormalParameter;
1310
import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
1411
import net.sourceforge.pmd.lang.java.ast.ASTVariableId;
12+
import net.sourceforge.pmd.lang.java.ast.internal.JavaAstUtils;
1513
import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
16-
import net.sourceforge.pmd.reporting.RuleContext;
1714

1815
public class MethodArgumentCouldBeFinalRule extends AbstractJavaRulechainRule {
1916

@@ -37,24 +34,12 @@ public Object visit(ASTConstructorDeclaration constructor, Object data) {
3734
}
3835

3936
private void lookForViolation(ASTExecutableDeclaration node, Object data) {
40-
checkForFinal((RuleContext) data, node.getFormalParameters().toStream().map(ASTFormalParameter::getVarId));
41-
}
42-
43-
static void checkForFinal(RuleContext ruleContext, NodeStream<ASTVariableId> variables) {
44-
outer:
45-
for (ASTVariableId var : variables) {
46-
if (var.isFinal()) {
47-
continue;
48-
}
49-
boolean used = false;
50-
for (ASTNamedReferenceExpr usage : var.getLocalUsages()) {
51-
used = true;
52-
if (usage.getAccessType() == AccessType.WRITE) {
53-
continue outer;
54-
}
55-
}
56-
if (used) {
57-
ruleContext.addViolation(var, var.getName());
37+
for (ASTFormalParameter param : node.getFormalParameters()) {
38+
ASTVariableId varId = param.getVarId();
39+
if (!param.isFinal()
40+
&& !JavaAstUtils.isNeverUsed(varId)
41+
&& JavaAstUtils.isEffectivelyFinal(varId)) {
42+
asCtx(data).addViolation(varId, varId.getName());
5843
}
5944
}
6045
}

‎pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/SingularFieldRule.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,9 @@ private boolean hasEnclosingLambda(JavaNode stop, ASTNamedReferenceExpr usage) {
165165

166166
private boolean usagesObserveValueBeforeMethodCall(List<ASTNamedReferenceExpr> usages, DataflowResult dataflow) {
167167
for (ASTNamedReferenceExpr usage : usages) {
168+
if (JavaAstUtils.isVarAccessStrictlyWrite(usage)) {
169+
continue;
170+
}
168171
ReachingDefinitionSet reaching = dataflow.getReachingDefinitions(usage);
169172
if (reaching.containsInitialFieldValue()) {
170173
return true;

‎pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/internal/DataflowPass.java‎

Lines changed: 57 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
package net.sourceforge.pmd.lang.java.rule.internal;
66

7+
import static java.util.Collections.emptySet;
78
import static net.sourceforge.pmd.util.CollectionUtil.asSingle;
89

910
import java.util.ArrayDeque;
@@ -177,12 +178,21 @@ private static DataflowResult process(ASTCompilationUnit node) {
177178
*/
178179
public static final class ReachingDefinitionSet {
179180

181+
static final ReachingDefinitionSet UNKNOWN = new ReachingDefinitionSet();
182+
static final ReachingDefinitionSet EMPTY_KNOWN = new ReachingDefinitionSet(emptySet());
183+
180184
private Set<AssignmentEntry> reaching;
181185
private boolean isNotFullyKnown;
182186
private boolean containsInitialFieldValue;
183187

188+
189+
static {
190+
assert !EMPTY_KNOWN.isNotFullyKnown();
191+
assert UNKNOWN.isNotFullyKnown();
192+
}
193+
184194
private ReachingDefinitionSet() {
185-
this.reaching = Collections.emptySet();
195+
this.reaching = emptySet();
186196
this.containsInitialFieldValue = false;
187197
this.isNotFullyKnown = true;
188198
}
@@ -228,6 +238,10 @@ void absorb(ReachingDefinitionSet reaching) {
228238
public static ReachingDefinitionSet unknown() {
229239
return new ReachingDefinitionSet();
230240
}
241+
242+
public static ReachingDefinitionSet blank() {
243+
return new ReachingDefinitionSet(emptySet());
244+
}
231245
}
232246

233247
/**
@@ -256,7 +270,7 @@ public Set<AssignmentEntry> getUnusedAssignments() {
256270
* May be useful to check for reassignment.
257271
*/
258272
public @NonNull Set<AssignmentEntry> getKillers(AssignmentEntry assignment) {
259-
return killRecord.getOrDefault(assignment, Collections.emptySet());
273+
return killRecord.getOrDefault(assignment, emptySet());
260274
}
261275

262276
// These methods are only valid to be called if the dataflow pass has run.
@@ -278,14 +292,25 @@ public Set<AssignmentEntry> getUnusedAssignments() {
278292
return expr.getUserMap().computeIfAbsent(REACHING_DEFS, () -> reachingFallback(expr));
279293
}
280294

281-
// Fallback, to compute reaching definitions for some fields
295+
// Fallback, to compute reaching definitions for some nodes
282296
// that are not tracked by the tree exploration. Final fields
283297
// indeed have a fully known set of reaching definitions.
284-
// TODO maybe they should actually be tracked?
285298
private @NonNull ReachingDefinitionSet reachingFallback(ASTNamedReferenceExpr expr) {
286299
JVariableSymbol sym = expr.getReferencedSym();
287-
if (sym == null || !sym.isField() || !sym.isFinal()) {
300+
if (sym == null || sym.isField() && !sym.isFinal()) {
288301
return ReachingDefinitionSet.unknown();
302+
} else if (!sym.isField()) {
303+
ASTVariableId node = sym.tryGetNode();
304+
assert node != null
305+
: "Not a field, and symbol is known, so should be a local which has a node";
306+
if (node.isLocalVariable()) {
307+
assert node.getInitializer() == null : "Should be a blank local variable";
308+
return ReachingDefinitionSet.blank();
309+
} else {
310+
// Formal parameter or other kind of def which has
311+
// an implicit initializer.
312+
return ReachingDefinitionSet.unknown();
313+
}
289314
}
290315

291316
ASTVariableId node = sym.tryGetNode();
@@ -965,23 +990,25 @@ public SpanInfo visit(ASTAssignmentExpression node, SpanInfo data) {
965990

966991
}
967992

968-
private SpanInfo processAssignment(ASTExpression lhs, // LHS or unary operand
993+
private SpanInfo processAssignment(ASTExpression lhs0, // LHS or unary operand
969994
ASTExpression rhs, // RHS or unary
970995
boolean useBeforeAssigning,
971996
SpanInfo result) {
972997

973-
if (lhs instanceof ASTNamedReferenceExpr) {
974-
JVariableSymbol lhsVar = ((ASTNamedReferenceExpr) lhs).getReferencedSym();
998+
if (lhs0 instanceof ASTNamedReferenceExpr) {
999+
ASTNamedReferenceExpr lhs = (ASTNamedReferenceExpr) lhs0;
1000+
JVariableSymbol lhsVar = lhs.getReferencedSym();
9751001
if (lhsVar != null
9761002
&& (lhsVar instanceof JLocalVariableSymbol
9771003
|| isRelevantField(lhs))) {
9781004

9791005
if (useBeforeAssigning) {
9801006
// compound assignment, to use BEFORE assigning
981-
result.use(lhsVar, (ASTNamedReferenceExpr) lhs);
1007+
result.use(lhsVar, lhs);
9821008
}
9831009

984-
result.assign(lhsVar, rhs);
1010+
VarLocalInfo oldVar = result.assign(lhsVar, rhs);
1011+
SpanInfo.updateReachingDefs(lhs, lhsVar, oldVar);
9851012
}
9861013
}
9871014
return result;
@@ -1321,11 +1348,12 @@ void declareBlank(ASTVariableId id) {
13211348
assign(id.getSymbol(), id);
13221349
}
13231350

1324-
void assign(JVariableSymbol var, JavaNode rhs) {
1325-
assign(var, rhs, SpecialAssignmentKind.NOT_SPECIAL);
1351+
VarLocalInfo assign(JVariableSymbol var, JavaNode rhs) {
1352+
return assign(var, rhs, SpecialAssignmentKind.NOT_SPECIAL);
13261353
}
13271354

1328-
@Nullable AssignmentEntry assign(JVariableSymbol var, JavaNode rhs, SpecialAssignmentKind kind) {
1355+
@Nullable
1356+
VarLocalInfo assign(JVariableSymbol var, JavaNode rhs, SpecialAssignmentKind kind) {
13291357
ASTVariableId node = var.tryGetNode();
13301358
if (node == null) {
13311359
return null; // we don't care about non-local declarations
@@ -1355,7 +1383,7 @@ void assign(JVariableSymbol var, JavaNode rhs) {
13551383
}
13561384
}
13571385
global.allAssignments.add(entry);
1358-
return entry;
1386+
return previous;
13591387
}
13601388

13611389
void declareSpecialFieldValues(JClassSymbol sym, boolean onlyStatic) {
@@ -1399,20 +1427,25 @@ void use(@Nullable JVariableSymbol var, @Nullable ASTNamedReferenceExpr reaching
13991427
if (info != null) {
14001428
global.usedAssignments.addAll(info.reachingDefs);
14011429
if (reachingDefSink != null) {
1402-
ReachingDefinitionSet reaching = new ReachingDefinitionSet(new LinkedHashSet<>(info.reachingDefs));
1403-
// need to merge into previous to account for cyclic control flow
1404-
reachingDefSink.getUserMap().compute(REACHING_DEFS, current -> {
1405-
if (current != null) {
1406-
current.absorb(reaching);
1407-
return current;
1408-
} else {
1409-
return reaching;
1410-
}
1411-
});
1430+
updateReachingDefs(reachingDefSink, var, info);
14121431
}
14131432
}
14141433
}
14151434

1435+
private static void updateReachingDefs(@NonNull ASTNamedReferenceExpr reachingDefSink, JVariableSymbol var, VarLocalInfo info) {
1436+
ReachingDefinitionSet reaching;
1437+
if (info == null || var.isField() && var.isFinal()) {
1438+
return;
1439+
} else {
1440+
reaching = new ReachingDefinitionSet(new LinkedHashSet<>(info.reachingDefs));
1441+
}
1442+
// need to merge into previous to account for cyclic control flow
1443+
reachingDefSink.getUserMap().merge(REACHING_DEFS, reaching, (current, newer) -> {
1444+
current.absorb(newer);
1445+
return current;
1446+
});
1447+
}
1448+
14161449
void deleteVar(JVariableSymbol var) {
14171450
symtable.remove(var);
14181451
}

0 commit comments

Comments
 (0)