Merge branch 'master' into pmd/7.0.x

pmd-java/src/main/resources/category/java/bestpractices.xml

@@ -1195,11 +1195,14 @@ public class Foo {
     <rule name="SwitchStmtsShouldHaveDefault"
           language="java"
           since="1.0"
-          message="Switch statements should have a default label"
+          message="Switch statements should be exhaustive, add a default case (or missing enum branches)"
           class="net.sourceforge.pmd.lang.rule.XPathRule"
           externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_bestpractices.html#switchstmtsshouldhavedefault">
         <description>
-All switch statements should include a default option to catch any unspecified values.
+            Switch statements should be exhaustive, to make their control flow
+            easier to follow. This can be achieved by adding a `default` case, or,
+            if the switch is on an enum type, by ensuring there is one switch branch
+            for each enum constant.
         </description>
         <priority>3</priority>
         <properties>
@@ -1211,14 +1214,14 @@ All switch statements should include a default option to catch any unspecified v
         </properties>
         <example>
 <![CDATA[
-public void bar() {
+class Foo {{
     int x = 2;
     switch (x) {
       case 1: int j = 6;
       case 2: int j = 8;
-          // missing default: here
+      // missing default: here
     }
-}
+}}
 ]]>
         </example>
     </rule>

pmd-java/src/main/resources/category/java/errorprone.xml

@@ -1091,11 +1091,22 @@ public class Bar {
           class="net.sourceforge.pmd.lang.rule.XPathRule"
           externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_errorprone.html#compareobjectswithequals">
         <description>
-Use equals() to compare object references; avoid comparing them with ==.
+Use `equals()` to compare object references; avoid comparing them with `==`.
+
+Since comparing objects with named constants is useful in some cases (eg, when
+defining constants for sentinel values), the rule ignores comparisons against
+fields with all-caps name (eg `this == SENTINEL`), which is a common naming
+convention for constant fields.
+
+You may allow some types to be compared by reference by listing the exceptions
+in the `typesThatCompareByReference` property.
         </description>
         <priority>3</priority>
         <properties>
             <property name="version" value="2.0"/>
+            <property name="typesThatCompareByReference" type="List[String]" delimiter="," description="List of canonical type names for which reference comparison is allowed.">
+                <value>java.lang.Enum,java.lang.Class</value>
+            </property>
             <property name="xpath">
                 <value>
 <![CDATA[
@@ -1104,10 +1115,13 @@ Use equals() to compare object references; avoid comparing them with ==.
     [count(*
         [not(self::NullLiteral)]
         [pmd-java:typeIs('java.lang.Object')]
-        [not(pmd-java:typeIs('java.lang.Enum'))]
-        [not(pmd-java:typeIs('java.lang.Class'))]) = 2
+        [not(some $t in $typesThatCompareByReference satisfies pmd-java:typeIs($t))]
+      ) = 2
     ]
     [not(ancestor::MethodDeclaration[1][@Name = "equals"])]
+    (: Is not a field access with an all-caps identifier :)
+    [not(FieldAccess[upper-case(@Name)=@Name]
+     or VariableAccess[upper-case(@Name)=@Name])]
 ]]>
         </value>
             </property>
@@ -2794,9 +2808,9 @@ public class Singleton {
           class="net.sourceforge.pmd.lang.java.rule.errorprone.SingletonClassReturningNewInstanceRule"
           externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_errorprone.html#singletonclassreturningnewinstance">
         <description>
-Some classes contain overloaded getInstance. The problem with overloaded getInstance methods
-is that the instance created using the overloaded method is not cached and so,
-for each call and new objects will be created for every invocation.
+            A singleton class should only ever have one instance. Failure to check
+            whether an instance has already been created may result in multiple
+            instances being created.
         </description>
         <priority>2</priority>
         <example>
@@ -2805,7 +2819,7 @@ class Singleton {
     private static Singleton instance = null;
     public static Singleton getInstance() {
         synchronized(Singleton.class) {
-            return new Singleton();
+            return new Singleton(); // this should be assigned to the field
         }
     }
 }

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

@@ -391,4 +391,43 @@ public class Foo {
 }
         ]]></code>
     </test-code>
+    <test-code>
+        <description>#3236 [java] LiteralsFirstInComparisons should consider constant fields (cont'd)</description>
+        <expected-problems>5</expected-problems>
+        <expected-linenumbers>6,8,17,24,26</expected-linenumbers>
+        <code><![CDATA[
+            class DT1 {
+                public static final String Q = "q";
+                public static final String T = "t";
+
+                public static int convert(String type) {
+                    if (type.equals(Q)) { // 6
+                        return 1;
+                    } else if (type.equals(T)) { // 8
+                        return 2;
+                    } else {
+                        return 3;
+                    }
+                }
+                public static int convert2(String type) {
+                    if (Q.equals(type)) { // 15
+                        return 1;
+                    } else if (type.equals(T)) { // 17
+                        return 2;
+                    } else {
+                        return 3;
+                    }
+                }
+                public static int convert3(String type) {
+                    if (type.equals("q")) { // 24
+                        return 1;
+                    } else if (type.equals("t")) { // 26
+                        return 2;
+                    } else {
+                        return 3;
+                    }
+                }
+            }
+        ]]></code>
+    </test-code>
 </test-data>

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CompareObjectsWithEquals.xml

@@ -114,7 +114,7 @@ package net.sourceforge.pmd.lang.java.rule.errorprone.compareobjectswithequals;
 
 public class CompareObjectsWithEqualsSample {
     void array(int[] a, String[] b) {
-        if (a[1] == b[1]) {} // int == String - this comparison doesn't make sense
+        if (a[1] == b[1]) {} // int == String - this comparison doesn't make sense (and doesn't compile...)
     }
     void array2(int[] c, int[] d) {
         if (c[1] == d[1]) {}
@@ -410,4 +410,93 @@ public class C0 {
         ]]></code>
     </test-code>
 
+    <test-code>
+        <description>[java] CompareObjectsWithEqualsRule: False positive with Enums #2716</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class EnumTest {
+    enum Type {
+        A, B;
+    }
+    private final Type type = Type.A;
+    public String isTypeA(Type param) {
+        return param == type ? "Yes" : "No";
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>static constant #3205</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+            class MyClass {
+                static final MyClass MISSING = new MyClass();
+
+                public static void isMissing(MyClass obj) {
+                    return obj == MISSING; // no violation expected...
+                }
+            }
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>static constant in other class #3205</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+            class MyClass {
+                static class Ts {
+                    static final MyClass MISSING = new MyClass();
+                }
+
+                public static void isMissing(MyClass obj) {
+                    return obj == Ts.MISSING; // no violation expected...
+                }
+            }
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description>constant field on some object #3205</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+            class MyClass {
+                static class Ts {
+                    final MyClass MISSING = new MyClass();
+                }
+
+                public static void isMissing(MyClass obj, Ts ts) {
+                    return obj == ts.MISSING; // no violation expected...
+                }
+            }
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description>constant field on some object, more complicated expr #3205</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+            class MyClass {
+                static class Ts {
+                    final MyClass MISSING = new MyClass();
+                    Ts id() { return this; }
+                }
+
+                public static void isMissing(MyClass obj, Ts ts) {
+                    return obj == (ts.id()).id().MISSING; // no violation expected...
+                }
+            }
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description>Property typesThatCompareByReference #3110</description>
+        <rule-property name="typesThatCompareByReference">java.lang.String</rule-property>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+            class MyClass {
+                public static void isMissing(String obj, Object ts) {
+                    return obj == ts;
+                }
+            }
+        ]]></code>
+    </test-code>
+
 </test-data>