Merge pull request #3855 from adangel:issue-3850

[java] Fix ImmutableField with conditionally assignment in ctors #3855
pmd · Mar 27, 2022 · 55c089d · 55c089d
2 parents 6c3fcc6 + b33e944
commit 55c089d
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 19 deletions.
diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md
@@ -123,6 +123,7 @@ The CLI itself remains compatible, if you run PMD via command-line, no action is
     *   [#2504](https://github.com/pmd/pmd/issues/2504): \[doc] Improve "Edit me on github" button
     *   [#3812](https://github.com/pmd/pmd/issues/3812): \[doc] Documentation website table of contents broken on pages with many subheadings
 *   java-design
+    *   [#3850](https://github.com/pmd/pmd/issues/3850): \[java] ImmutableField - false negative when field assigned in constructor conditionally
     *   [#3851](https://github.com/pmd/pmd/issues/3851): \[java] ClassWithOnlyPrivateConstructorsShouldBeFinal - false negative when a compilation unit contains two class declarations
 *   xml
     *   [#2766](https://github.com/pmd/pmd/issues/2766): \[xml] XMLNS prefix is not pre-declared in xpath query

diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/ImmutableFieldRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/ImmutableFieldRule.java
@@ -16,9 +16,7 @@
 import net.sourceforge.pmd.lang.java.ast.ASTConstructorDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTDoStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTForStatement;
-import net.sourceforge.pmd.lang.java.ast.ASTIfStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTLambdaExpression;
-import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTTryStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTVariableInitializer;
 import net.sourceforge.pmd.lang.java.ast.ASTWhileStatement;
@@ -60,11 +58,15 @@ public Object visit(ASTClassOrInterfaceDeclaration node, Object data) {
                 continue;
             }
 
-            FieldImmutabilityType type = initializedInConstructor(field, entry.getValue(), new HashSet<>(constructors));
+            List<NameOccurrence> usages = entry.getValue();
+            FieldImmutabilityType type = initializedInConstructor(field, usages, new HashSet<>(constructors));
             if (type == FieldImmutabilityType.MUTABLE) {
                 continue;
             }
-            if (type == FieldImmutabilityType.IMMUTABLE || type == FieldImmutabilityType.CHECKDECL && initializedWhenDeclared(field)) {
+            if (initializedWhenDeclared(field) && usages.isEmpty()) {
+                addViolation(data, field.getNode(), field.getImage());
+            }
+            if (type == FieldImmutabilityType.IMMUTABLE || type == FieldImmutabilityType.CHECKDECL && !initializedWhenDeclared(field)) {
                 addViolation(data, field.getNode(), field.getImage());
             }
         }
@@ -90,14 +92,7 @@ private FieldImmutabilityType initializedInConstructor(VariableNameDeclaration f
                         methodInitCount++;
                         continue;
                     }
-                    // Check for assigns in if-statements, which can depend on
-                    // constructor
-                    // args or other runtime knowledge and can be a valid reason
-                    // to instantiate
-                    // in one constructor only
-                    if (node.getFirstParentOfType(ASTIfStatement.class) != null) {
-                        methodInitCount++;
-                    }
+
                     if (inAnonymousInnerClass(node)) {
                         methodInitCount++;
                     } else if (node.getFirstParentOfType(ASTLambdaExpression.class) != null) {
@@ -106,15 +101,15 @@ private FieldImmutabilityType initializedInConstructor(VariableNameDeclaration f
                         consSet.add(constructor);
                     }
                 } else {
-                    if (node.getFirstParentOfType(ASTMethodDeclaration.class) != null) {
-                        methodInitCount++;
-                    } else if (node.getFirstParentOfType(ASTLambdaExpression.class) != null) {
+                    if (node.getFirstParentOfType(ASTLambdaExpression.class) != null) {
                         lambdaUsage++;
+                    } else {
+                        methodInitCount++;
                     }
                 }
             }
         }
-        if (usages.isEmpty() || methodInitCount == 0 && lambdaUsage == 0 && consSet.isEmpty()) {
+        if (usages.isEmpty() || methodInitCount == 0 && lambdaUsage == 0 && allConstructors.equals(consSet)) {
             result = FieldImmutabilityType.CHECKDECL;
         } else {
             allConstructors.removeAll(consSet);

diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/ImmutableField.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/ImmutableField.xml
@@ -294,7 +294,7 @@ public final class Foo {
         <expected-problems>0</expected-problems>
         <code><![CDATA[
 public class MyClass {
-    private int positive = 1;
+    private int positive = 1; // cannot be final, is modified in ctor
 
     public MyClass(int value) {
         // if negative keep default
@@ -308,10 +308,11 @@ public class MyClass {
 
     <test-code>
         <description>Bug 1740480, assignment in single constructor based on constructor argument check</description>
-        <expected-problems>0</expected-problems>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>2</expected-linenumbers>
         <code><![CDATA[
 public class MyClass {
-    private int positive;
+    private int positive; // can be final
 
     public MyClass(int value) {
         if (value > 0) {
@@ -548,6 +549,26 @@ public class TestFinal {
     while (random.nextBoolean())
       e = e.add(BigInteger.ONE);
   }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>[java] ImmutableField - false negative when field assigned in constructor conditionally #3850</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>3</expected-linenumbers>
+        <code><![CDATA[
+public class ExampleImmutableField {
+
+    private String str; // false negative here, could be final
+
+    public ExampleImmutableField(String strLocal, boolean flag) { 
+        if (flag){
+            this.str = strLocal;
+        } else {
+            this.str = strLocal+"123";
+        }
+    }
 }
         ]]></code>
     </test-code>