Merge pull request #4535 from oowekyala:issue4435-unusedAssignment

[java] Fix #4435 - FP with UnusedAssignment #4535

docs/pages/release_notes.md

@@ -18,10 +18,13 @@ This is a {{ site.pmd.release_type }} release.
 * cli
   * [#4791](https://github.com/pmd/pmd/issues/4791): \[cli] Could not find or load main class
 * java-bestpractices
+  * [#4435](https://github.com/pmd/pmd/issues/4435): \[java] \[7.0-rc1] UnusedAssignment for used field
   * [#4569](https://github.com/pmd/pmd/issues/4569): \[java] ForLoopCanBeForeach reports on loop `for (int i = 0; i < list.size(); i += 2)`
+  * [#4618](https://github.com/pmd/pmd/issues/4618): \[java] UnusedAssignment false positive with conditional assignments of fields
 * java-codestyle
   * [#4881](https://github.com/pmd/pmd/issues/4881): \[java] ClassNamingConventions: interfaces are identified as abstract classes (regression in 7.0.0)
 * java-design
+  * [#3694](https://github.com/pmd/pmd/issues/3694): \[java] SingularField ignores static variables
   * [#4873](https://github.com/pmd/pmd/issues/4873): \[java] AvoidCatchingGenericException: Can no longer suppress on the exception itself
 * java-performance
   * [#3845](https://github.com/pmd/pmd/issues/3845): \[java] InsufficientStringBufferDeclaration should consider literal expression

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTExecutableDeclaration.java

@@ -113,4 +113,26 @@ default boolean isVarargs() {
         return lastFormal instanceof ASTFormalParameter && ((ASTFormalParameter) lastFormal).isVarargs();
     }
 
+
+    /**
+     * Returns true if this is a static method.
+     * If this is a constructor, return false.
+     *
+     * @since 7.1.0
+     */
+    default boolean isStatic() {
+        return hasModifiers(JModifier.STATIC);
+    }
+
+
+    /**
+     * Returns true if this is a final method.
+     * If this is a constructor, return false.
+     *
+     * @since 7.1.0
+     */
+    default boolean isFinal() {
+        return hasModifiers(JModifier.FINAL);
+    }
+
 }

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTMethodDeclaration.java

@@ -124,15 +124,6 @@ public boolean isVoid() {
         return getResultTypeNode().isVoid();
     }
 
-    /** Returns true if this method is static. */
-    public boolean isStatic() {
-        return hasModifiers(JModifier.STATIC);
-    }
-
-    public boolean isFinal() {
-        return hasModifiers(JModifier.FINAL);
-    }
-
     /**
      * Returns the default clause, if this is an annotation method declaration
      * that features one. Otherwise returns null.

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTVariableId.java

@@ -94,10 +94,24 @@ public ASTModifierList getModifiers() {
         return getModifierOwnerParent().getModifiers();
     }
 
+    /**
+     * Return true if the declared variable is static.
+     * There may not be an explicit final modifier, e.g. for enum constants.
+     */
     public boolean isFinal() {
         return hasModifiers(JModifier.FINAL);
     }
 
+    /**
+     * Return true if the declared variable is static.
+     * There may not be an explicit static modifier, e.g. for enum constants.
+     *
+     * @since 7.1.0
+     */
+    public boolean isStatic() {
+        return hasModifiers(JModifier.STATIC);
+    }
+
     @Override
     public Visibility getVisibility() {
         return isPatternBinding() ? Visibility.V_LOCAL

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

@@ -39,13 +39,15 @@
 import net.sourceforge.pmd.lang.java.ast.ASTClassType;
 import net.sourceforge.pmd.lang.java.ast.ASTConstructorCall;
 import net.sourceforge.pmd.lang.java.ast.ASTExecutableDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTExplicitConstructorInvocation;
 import net.sourceforge.pmd.lang.java.ast.ASTExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTExpressionStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTFieldAccess;
 import net.sourceforge.pmd.lang.java.ast.ASTForStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTFormalParameter;
 import net.sourceforge.pmd.lang.java.ast.ASTFormalParameters;
 import net.sourceforge.pmd.lang.java.ast.ASTInfixExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTInitializer;
 import net.sourceforge.pmd.lang.java.ast.ASTLabeledStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTList;
 import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration;
@@ -74,6 +76,7 @@
 import net.sourceforge.pmd.lang.java.ast.TypeNode;
 import net.sourceforge.pmd.lang.java.ast.UnaryOp;
 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;
 import net.sourceforge.pmd.lang.java.symbols.JVariableSymbol;
 import net.sourceforge.pmd.lang.java.symbols.internal.ast.AstLocalVarSym;
@@ -599,16 +602,28 @@ public static boolean isRefToFieldOfThisClass(ASTExpression usage) {
         return false;
     }
 
-    public static boolean isCallOnThisInstance(ASTMethodCall call) {
+    /**
+     * Return whether the method call is a call whose receiver is the
+     * {@code this} object. This is the case also if the method is called
+     * with a {@code super} qualifier, or if it is not syntactically
+     * qualified but refers to a non-static method of the enclosing class.
+     */
+    public static OptionalBool isCallOnThisInstance(ASTMethodCall call) {
         // syntactic approach.
         if (call.getQualifier() != null) {
-            return isUnqualifiedThisOrSuper(call.getQualifier());
+            return OptionalBool.definitely(isUnqualifiedThisOrSuper(call.getQualifier()));
         }
 
         // unqualified call
         JMethodSig mtype = call.getMethodType();
-        return !mtype.getSymbol().isUnresolved()
-            && mtype.getSymbol().getEnclosingClass().equals(call.getEnclosingType().getSymbol());
+        JExecutableSymbol methodSym = mtype.getSymbol();
+        if (methodSym.isUnresolved()) {
+            return OptionalBool.UNKNOWN;
+        }
+        return OptionalBool.definitely(
+            !methodSym.isStatic()
+                && methodSym.getEnclosingClass().equals(call.getEnclosingType().getSymbol())
+        );
     }
 
     public static ASTClassType getThisOrSuperQualifier(ASTExpression expr) {
@@ -779,4 +794,12 @@ && isReferenceToVar(((ASTThrowStatement) stmt).getExpr(),
         }
         return false;
     }
+
+    public static boolean isInStaticCtx(JavaNode node) {
+        return node.ancestors()
+                   .any(it -> it instanceof ASTExecutableDeclaration && ((ASTExecutableDeclaration) it).isStatic()
+                       || it instanceof ASTInitializer && ((ASTInitializer) it).isStatic()
+                       || it instanceof ASTExplicitConstructorInvocation
+                   );
+    }
 }

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

@@ -7,6 +7,7 @@
 import static net.sourceforge.pmd.util.CollectionUtil.setOf;
 
 import java.util.Set;
+import java.util.function.Function;
 
 import net.sourceforge.pmd.lang.ast.NodeStream;
 import net.sourceforge.pmd.lang.java.ast.ASTAssignableExpr.ASTNamedReferenceExpr;
@@ -40,6 +41,10 @@ public class ImmutableFieldRule extends AbstractJavaRulechainRule {
         setOf(
             "lombok.Setter"
         );
+    private static final Function<Object, JavaNode> INTERESTING_ANCESTOR =
+        NodeStream.asInstanceOf(ASTLambdaExpression.class,
+                                ASTTypeDeclaration.class,
+                                ASTConstructorDeclaration.class);
 
     public ImmutableFieldRule() {
         super(ASTFieldDeclaration.class);
@@ -64,7 +69,7 @@ public Object visit(ASTFieldDeclaration field, Object data) {
                     if (usage.getAccessType() == AccessType.WRITE) {
                         hasWrite = true;
 
-                        JavaNode enclosing = usage.ancestors().map(NodeStream.asInstanceOf(ASTLambdaExpression.class, ASTTypeDeclaration.class, ASTConstructorDeclaration.class)).first();
+                        JavaNode enclosing = usage.ancestors().map(INTERESTING_ANCESTOR).first();
                         if (!(enclosing instanceof ASTConstructorDeclaration)
                             || enclosing.getEnclosingType() != enclosingType) {
                             continue outer; // written-to outside ctor
@@ -103,7 +108,16 @@ private boolean defaultValueDoesNotReachEndOfCtor(DataflowResult dataflow, ASTVa
     }
 
     private boolean isReassignedOnSomeCodePath(DataflowResult dataflow, AssignmentEntry anAssignment) {
+        // Ie, return whether there exists an assignment that overwrites the given assignment,
+        // and simultaneously is not unbound (=happens for sure). Unbound assignments are introduced by the
+        // dataflow pass to help analysis, but are overly conservative. They represent the
+        // "final value" of a field when a ctor ends, and also the value a field may have
+        // been set to when an instance method is called (we don't know whether the method
+        // actually sets anything, but we assume it does). The first case can be ignored
+        // because we already check for that in the logic of this rule. The second case
+        // can be ignored because we already made sure that the field is only written to
+        // in ctors, but not in any instance method.
         Set<AssignmentEntry> killers = dataflow.getKillers(anAssignment);
-        return CollectionUtil.any(killers, killer -> !killer.isFieldAssignmentAtEndOfCtor());
+        return CollectionUtil.any(killers, killer -> !killer.isUnbound());
     }
 }

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

@@ -55,6 +55,7 @@
 import net.sourceforge.pmd.lang.java.types.TypeTestUtil;
 import net.sourceforge.pmd.properties.PropertyDescriptor;
 import net.sourceforge.pmd.properties.PropertyFactory;
+import net.sourceforge.pmd.util.OptionalBool;
 import net.sourceforge.pmd.reporting.RuleContext;
 
 /**
@@ -228,7 +229,7 @@ private int foreignDegreeImpl(ASTExpression expr) {
     private int methodCallDegree(ASTMethodCall call) {
         if (call.getOverloadSelectionInfo().isFailed() // be conservative
             || call.getMethodType().isStatic() // static methods are taken to be construction methods.
-            || isCallOnThisInstance(call)
+            || isCallOnThisInstance(call) != OptionalBool.NO
             || call.getQualifier() == null // either static or call on this. Prevents NPE when unresolved
             || isFactoryMethod(call)
             || isBuilderPattern(call.getQualifier())

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

@@ -4,8 +4,6 @@
 
 package net.sourceforge.pmd.lang.java.rule.design;
 
-import static net.sourceforge.pmd.lang.java.ast.JModifier.FINAL;
-import static net.sourceforge.pmd.lang.java.ast.JModifier.STATIC;
 import static net.sourceforge.pmd.util.CollectionUtil.setOf;
 
 import java.util.ArrayList;
@@ -19,9 +17,11 @@
 import net.sourceforge.pmd.lang.java.ast.ASTAssignableExpr.ASTNamedReferenceExpr;
 import net.sourceforge.pmd.lang.java.ast.ASTBodyDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTInitializer;
 import net.sourceforge.pmd.lang.java.ast.ASTLambdaExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTTypeDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTVariableId;
+import net.sourceforge.pmd.lang.java.ast.JModifier;
 import net.sourceforge.pmd.lang.java.ast.JavaNode;
 import net.sourceforge.pmd.lang.java.ast.ModifierOwner;
 import net.sourceforge.pmd.lang.java.ast.ModifierOwner.Visibility;
@@ -95,21 +95,27 @@ public Object visitJavaNode(JavaNode node, Object data) {
         return null;
     }
 
-    public static boolean mayBeSingular(ModifierOwner varId) {
+    private static boolean mayBeSingular(ModifierOwner varId) {
         return varId.getEffectiveVisibility().isAtMost(Visibility.V_PRIVATE)
-            && !varId.getModifiers().hasAny(STATIC, FINAL);
+            // We ignore final variables for a reason:
+            // - if they're static they are there to share a value and that is not a mistake
+            // - if they're not static then if this rule matches, then so does FinalFieldCouldBeStatic,
+            // and that rule has the better fix.
+            && !varId.hasModifiers(JModifier.FINAL);
     }
 
     private boolean isSingularField(ASTTypeDeclaration fieldOwner, ASTVariableId varId, DataflowResult dataflow) {
         if (JavaAstUtils.isNeverUsed(varId)) {
             return false; // don't report unused field
         }
 
+        boolean isStaticField = varId.isStatic();
         //Check usages for validity & group them by scope
         //They're valid if they don't escape the scope of their method, eg by being in a nested class or lambda
         Map<ASTBodyDeclaration, List<ASTNamedReferenceExpr>> usagesByScope = new HashMap<>();
         for (ASTNamedReferenceExpr usage : varId.getLocalUsages()) {
-            if (usage.getEnclosingType() != fieldOwner || !JavaAstUtils.isThisFieldAccess(usage)) {
+            if (usage.getEnclosingType() != fieldOwner
+                || !isStaticField && !JavaAstUtils.isThisFieldAccess(usage)) {
                 return false; // give up
             }
             ASTBodyDeclaration enclosing = getEnclosingBodyDecl(fieldOwner, usage);
@@ -121,18 +127,24 @@ private boolean isSingularField(ASTTypeDeclaration fieldOwner, ASTVariableId var
 
         // the field is singular if it is used as a local var in every method.
         for (ASTBodyDeclaration method : usagesByScope.keySet()) {
-            if (method != null && !usagesDontObserveValueBeforeMethodCall(usagesByScope.get(method), dataflow)) {
+            if (method != null && usagesObserveValueBeforeMethodCall(usagesByScope.get(method), dataflow)) {
                 return false;
             }
         }
 
         return true;
     }
 
-    private @Nullable ASTBodyDeclaration getEnclosingBodyDecl(JavaNode stop, ASTNamedReferenceExpr usage) {
-        return usage.ancestors()
-                    .takeWhile(it -> it != stop)
-                    .first(ASTBodyDeclaration.class);
+    private @Nullable ASTBodyDeclaration getEnclosingBodyDecl(ASTTypeDeclaration enclosingType, ASTNamedReferenceExpr usage) {
+        ASTBodyDeclaration decl = usage.ancestors()
+                                       .takeWhile(it -> it != enclosingType)
+                                       .first(ASTBodyDeclaration.class);
+        if (decl instanceof ASTFieldDeclaration
+            || decl instanceof ASTInitializer) {
+            // then the usage is logically part of the ctors.
+            return enclosingType;
+        }
+        return decl;
     }
 
     private boolean hasEnclosingLambda(JavaNode stop, ASTNamedReferenceExpr usage) {
@@ -141,13 +153,13 @@ private boolean hasEnclosingLambda(JavaNode stop, ASTNamedReferenceExpr usage) {
                     .any(it -> it instanceof ASTLambdaExpression);
     }
 
-    private boolean usagesDontObserveValueBeforeMethodCall(List<ASTNamedReferenceExpr> usages, DataflowResult dataflow) {
+    private boolean usagesObserveValueBeforeMethodCall(List<ASTNamedReferenceExpr> usages, DataflowResult dataflow) {
         for (ASTNamedReferenceExpr usage : usages) {
             ReachingDefinitionSet reaching = dataflow.getReachingDefinitions(usage);
             if (reaching.containsInitialFieldValue()) {
-                return false;
+                return true;
             }
         }
-        return true;
+        return false;
     }
 }