Merge branch 'pr-1637'

docs/pages/release_notes.md

@@ -17,6 +17,7 @@ This is a {{ site.pmd.release_type }} release.
 ### Fixed Issues
 
 *   java-bestpractices
+    *   [#808](https://github.com/pmd/pmd/issues/808): \[java] AccessorMethodGeneration false positives with compile time constants
     *   [#1555](https://github.com/pmd/pmd/issues/1555): \[java] UnusedImports false positive for method parameter type in @see Javadoc
 *   java-codestyle
     *   [#1543](https://github.com/pmd/pmd/issues/1543): \[java] LinguisticNaming should ignore overriden methods
@@ -37,6 +38,7 @@ This is a {{ site.pmd.release_type }} release.
 *   [#1628](https://github.com/pmd/pmd/pull/1628): \[java] LinguisticNaming should ignore overriden methods - [Shubham](https://github.com/Shubham-2k17)
 *   [#1634](https://github.com/pmd/pmd/pull/1634): \[java] BeanMembersShouldSerializeRule does not recognize lombok accessors - [Shubham](https://github.com/Shubham-2k17)
 *   [#1635](https://github.com/pmd/pmd/pull/1635): \[java] UnsynchronizedStaticFormatter reports commons lang FastDateFormat - [Shubham](https://github.com/Shubham-2k17)
+*   [#1637](https://github.com/pmd/pmd/pull/1637): \[java] Compile time constants initialized by literals avoided by AccessorMethodGenerationRule - [Shubham](https://github.com/Shubham-2k17)
 *   [#1640](https://github.com/pmd/pmd/pull/1640): \[java] Update instead of override classHasLombokAnnotation flag - [Phokham Nonava](https://github.com/fluxroot)
 
 {% endtocmaker %}

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/AccessorMethodGenerationRule.java

@@ -4,12 +4,21 @@
 
 package net.sourceforge.pmd.lang.java.rule.bestpractices;
 
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 
+import net.sourceforge.pmd.RuleContext;
 import net.sourceforge.pmd.lang.java.ast.ASTCompilationUnit;
+import net.sourceforge.pmd.lang.java.ast.ASTExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTLiteral;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTName;
+import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix;
+import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclarator;
+import net.sourceforge.pmd.lang.java.ast.ASTVariableInitializer;
 import net.sourceforge.pmd.lang.java.ast.AbstractJavaAccessNode;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
 import net.sourceforge.pmd.lang.java.symboltable.AbstractJavaScope;
@@ -22,6 +31,8 @@ import net.sourceforge.pmd.lang.symboltable.NameOccurrence;
 
 public class AccessorMethodGenerationRule extends AbstractJavaRule {
 
+    private List<String> cache = new ArrayList<>();
+
     @Override
     public Object visit(final ASTCompilationUnit node, final Object data) {
         final SourceFileScope file = node.getScope().getEnclosingScope(SourceFileScope.class);
@@ -57,6 +68,30 @@ public class AccessorMethodGenerationRule extends AbstractJavaRule {
             return;
         }
 
+        if (node.isFinal()) {
+            for (final ASTVariableDeclarator varDecl: node.findChildrenOfType(ASTVariableDeclarator.class)) {
+                if (varDecl.hasInitializer()) {
+                    ASTVariableInitializer varInit = varDecl.getInitializer();
+                    List<ASTExpression> initExpression = varInit.findDescendantsOfType(ASTExpression.class);
+                    boolean isConstantExpression = true;
+                    constantCheck:
+                    for (ASTExpression exp: initExpression) {
+                        List<ASTPrimaryExpression> primaryExpressions = exp.findDescendantsOfType(ASTPrimaryExpression.class);
+                        for (ASTPrimaryExpression expression: primaryExpressions) {
+                            if (!isCompileTimeConstant(expression)) {
+                                isConstantExpression = false;
+                                break constantCheck;
+                            }
+                        }
+                    }
+                    if (isConstantExpression) {
+                        cache.add(varDecl.getName());
+                        return;
+                    }
+                }
+            }
+        }
+
         for (final NameOccurrence no : occurrences) {
             ClassScope usedAtScope = no.getLocation().getScope().getEnclosingScope(ClassScope.class);
 
@@ -66,4 +101,39 @@ public class AccessorMethodGenerationRule extends AbstractJavaRule {
             }
         }
     }
+
+    public boolean isCompileTimeConstant(ASTPrimaryExpression expressions) {
+        // function call detected
+        List<ASTPrimarySuffix> suffix = expressions.findDescendantsOfType(ASTPrimarySuffix.class);
+        if (!suffix.isEmpty()) {
+            return false;
+        }
+
+        // single node expression
+        List<ASTName> nameNodes = expressions.findDescendantsOfType(ASTName.class);
+        List<ASTLiteral> literalNodes = expressions.findDescendantsOfType(ASTLiteral.class);
+        if (nameNodes.size() + literalNodes.size() < 2) {
+            for (ASTName node: nameNodes) {
+                // TODO : use the symbol table to get the declaration of the referenced var and check it
+                if (!cache.contains(node.getImage())) {
+                    return false;
+                }
+            }
+            return true;
+        }
+
+        // multiple node expression
+        List<ASTPrimaryExpression> subExpressions = expressions.findDescendantsOfType(ASTPrimaryExpression.class);
+        for (ASTPrimaryExpression exp: subExpressions) {
+            if (!isCompileTimeConstant(exp)) {
+                return false;
+            }
+        }
+        return true;
+    }
+
+    @Override
+    public void end(RuleContext ctx) {
+        cache.clear();
+    }
 }

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/AccessorMethodGeneration.xml

@@ -190,6 +190,46 @@ public class Foo implements Parcelable {
             return value == 1;
         }
     };
+}
+     ]]></code>
+     <source-type>java 10</source-type>
+    </test-code>
+    <test-code>
+        <description>#808 - [java] AccessorMethodGeneration false positives with compile time constants</description>
+        <expected-problems>4</expected-problems>
+        <expected-linenumbers>25,26,27,28</expected-linenumbers>
+        <code><![CDATA[
+@SuppressWarnings("unused")
+class Outer {
+  private static final String CONST_STRING = "value";
+  private static final String CALCULATED_STRING = "value" + 0;
+  private static final int LITERAL = 0;
+  private static final int CALCULATED = LITERAL * 2;
+  private static final int CAST = (int) 0L;
+  private static final long NON_CONSTANT = java.util.concurrent.TimeUnit.SECONDS.toMillis(10);
+  private static final String NON_CONSTANT_STR = NON_CONSTANT + "foo";
+  private static final int LATE_INIT;
+  private static String STATIC_STRING = "value";
+  static {
+  	LATE_INIT = 0;
+  }
+
+  class Inner {
+    @Override
+    public String toString() {
+      return "" // separate lines so the rule violations show up better
+           + CONST_STRING
+           + CALCULATED_STRING
+           + LITERAL
+           + CALCULATED
+           + CAST
+           + NON_CONSTANT     // valid violation
+           + NON_CONSTANT_STR // valid violation
+           + LATE_INIT        // valid violation
+           + STATIC_STRING    // valid violation
+      ;
+    }
+  }
 }
      ]]></code>
      <source-type>java 10</source-type>