diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 14ac6e3c1c..946fc1f8d3 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -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 diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/util/DataMap.java b/pmd-core/src/main/java/net/sourceforge/pmd/util/DataMap.java index 796bdba7bf..8629416288 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/util/DataMap.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/util/DataMap.java @@ -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 { return (T) getMap().compute(key, (k, v) -> function.apply((T) v)); } + /** + * @see Map#merge(Object, Object, BiFunction) + */ + @SuppressWarnings({ "unchecked", "rawtypes" }) + public T merge(DataKey key, T value, BiFunction function) { + return (T) getMap().merge(key, value, (BiFunction) function); + } + private Map, 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 diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTAssignableExpr.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTAssignableExpr.java index 72ea5e9282..d504ee9244 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTAssignableExpr.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTAssignableExpr.java @@ -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() { diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/JavaAstUtils.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/JavaAstUtils.java index 31149fc70c..6cc5b24cf8 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/JavaAstUtils.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/JavaAstUtils.java @@ -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; + } } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LocalVariableCouldBeFinalRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LocalVariableCouldBeFinalRule.java index 47bc0b8846..ec60baffac 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LocalVariableCouldBeFinalRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LocalVariableCouldBeFinalRule.java @@ -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; } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodArgumentCouldBeFinalRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodArgumentCouldBeFinalRule.java index efbb5102a4..23b180e761 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodArgumentCouldBeFinalRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodArgumentCouldBeFinalRule.java @@ -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 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()); } } } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/SingularFieldRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/SingularFieldRule.java index 48fea5699a..a9ef81f209 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/SingularFieldRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/SingularFieldRule.java @@ -165,6 +165,9 @@ public class SingularFieldRule extends AbstractJavaRulechainRule { private boolean usagesObserveValueBeforeMethodCall(List usages, DataflowResult dataflow) { for (ASTNamedReferenceExpr usage : usages) { + if (JavaAstUtils.isVarAccessStrictlyWrite(usage)) { + continue; + } ReachingDefinitionSet reaching = dataflow.getReachingDefinitions(usage); if (reaching.containsInitialFieldValue()) { return true; diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/internal/DataflowPass.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/internal/DataflowPass.java index 6624bc5323..d188c63e43 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/internal/DataflowPass.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/internal/DataflowPass.java @@ -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 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 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); } diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/internal/DataflowPassTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/internal/DataflowPassTest.java index 3fb0c5506d..b33e3cfeea 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/internal/DataflowPassTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/internal/DataflowPassTest.java @@ -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 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 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()); + } } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/LocalVariableCouldBeFinal.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/LocalVariableCouldBeFinal.xml index 69686f20af..54b7d3201a 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/LocalVariableCouldBeFinal.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/LocalVariableCouldBeFinal.xml @@ -308,6 +308,84 @@ public class ClassWithLambda { System.out.println(a); }; } +} + ]]> + + + #1619 FP with loop variable + 0 + + + + #3122 should consider blank variables + 2 + 12,24 + + + + #3122 should consider blank variables + 1 + 5 +