Fix pmd#1619 - LocalVariableCouldBeFinal FP with loops

oowekyala · May 11, 2024 · 02cb570 · 02cb570
1 parent e5f55c0
commit 02cb570
Show file tree

Hide file tree

Showing 4 changed files with 37 additions and 24 deletions.
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
@@ -802,4 +802,13 @@ public static boolean isInStaticCtx(JavaNode node) {
                        || it instanceof ASTExplicitConstructorInvocation
                    );
     }
+
+    public static boolean isEffectivelyFinal(ASTVariableId var) {
+        for (ASTNamedReferenceExpr usage : var.getLocalUsages()) {
+            if (usage.getAccessType() == AccessType.WRITE) {
+                return false;
+            }
+        }
+        return true;
+    }
 }
diff --git a/...main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LocalVariableCouldBeFinalRule.java b/...main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LocalVariableCouldBeFinalRule.java
@@ -8,9 +8,10 @@
 
 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 Object visit(ASTLocalVariableDeclaration node, Object data) {
         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 (!vid.getLocalUsages().isEmpty()) {
+                    // filter out unused variables
+                    asCtx(data).addViolation(vid, vid.getName());
+                }
+            }
+        }
         return data;
     }
 

diff --git a/...ain/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodArgumentCouldBeFinalRule.java b/...ain/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,10 @@ public Object visit(ASTConstructorDeclaration constructor, Object data) {
     }
 
     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() && !varId.getLocalUsages().isEmpty() && JavaAstUtils.isEffectivelyFinal(varId)) {
+                asCtx(data).addViolation(varId, varId.getName());
             }
         }
     }

diff --git a/.../resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/LocalVariableCouldBeFinal.xml b/.../resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/LocalVariableCouldBeFinal.xml
@@ -308,6 +308,17 @@ 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>