Merge branch 'pr-2519'

[java] Enable rule UnnecessaryCast (codestyle) #2519

docs/pages/release_notes.md

@@ -43,6 +43,11 @@ The coordinates for the new modules are:
 
 The command line version of PMD continues to use **scala 2.13**.
 
+#### New Rules
+
+*   The new Java Rule {% rule "java/codestyle/UnnecessaryCast" %} (`java-codestyle`)
+    finds casts that are unnecessary while accessing collection elements.
+
 ### Fixed Issues
 
 *   c#

pmd-core/src/main/resources/rulesets/releases/6250.xml

@@ -0,0 +1,13 @@
+<?xml version="1.0"?>
+
+<ruleset name="6250"
+    xmlns="http://pmd.sourceforge.net/ruleset/2.0.0"
+    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xsi:schemaLocation="http://pmd.sourceforge.net/ruleset/2.0.0 https://pmd.sourceforge.io/ruleset_2_0_0.xsd">
+  <description>
+This ruleset contains links to rules that are new in PMD v6.25.0
+  </description>
+
+    <rule ref="category/java/codestyle.xml/UnnecessaryCast" />
+
+</ruleset>

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryCastRule.java

@@ -1,11 +1,12 @@
-/**
+/*
  * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
  */
 
-package net.sourceforge.pmd.lang.java.rule.migrating;
+package net.sourceforge.pmd.lang.java.rule.codestyle;
 
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
 import net.sourceforge.pmd.lang.ast.Node;
@@ -13,10 +14,12 @@ import net.sourceforge.pmd.lang.java.ast.ASTCastExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType;
 import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration;
-import net.sourceforge.pmd.lang.java.ast.ASTName;
+import net.sourceforge.pmd.lang.java.ast.ASTTypeArgument;
 import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
+import net.sourceforge.pmd.lang.java.typeresolution.TypeHelper;
 import net.sourceforge.pmd.lang.symboltable.NameOccurrence;
+import net.sourceforge.pmd.lang.symboltable.ScopedNode;
 
 /**
  * This is a rule, that detects unnecessary casts when using Java 1.5 generics
@@ -34,7 +37,6 @@ import net.sourceforge.pmd.lang.symboltable.NameOccurrence;
  *      "http://sourceforge.net/p/pmd/discussion/188192/thread/276fd6f0">Java 5
  *      rules: Unnecessary casts/Iterators</a>
  */
-// TODO This is not referenced by any RuleSet?
 public class UnnecessaryCastRule extends AbstractJavaRule {
 
     private static Set<String> implClassNames = new HashSet<>();
@@ -62,36 +64,67 @@ public class UnnecessaryCastRule extends AbstractJavaRule {
         implClassNames.add("java.util.TreeSet");
         implClassNames.add("java.util.TreeMap");
         implClassNames.add("java.util.Vector");
+        implClassNames.add("Iterator");
+        implClassNames.add("java.util.Iterator");
     }
 
     @Override
     public Object visit(ASTLocalVariableDeclaration node, Object data) {
-        return process(node, data);
+        process(node, data);
+        return super.visit(node, data);
     }
 
     @Override
     public Object visit(ASTFieldDeclaration node, Object data) {
-        return process(node, data);
+        process(node, data);
+        return super.visit(node, data);
     }
 
-    private Object process(Node node, Object data) {
+    private void process(Node node, Object data) {
-        ASTClassOrInterfaceType cit = node.getFirstDescendantOfType(ASTClassOrInterfaceType.class);
+        ASTClassOrInterfaceType collectionType = node.getFirstDescendantOfType(ASTClassOrInterfaceType.class);
-        if (cit == null || !implClassNames.contains(cit.getImage())) {
+        if (collectionType == null || !implClassNames.contains(collectionType.getImage())) {
-            return data;
+            return;
         }
-        cit = cit.getFirstDescendantOfType(ASTClassOrInterfaceType.class);
+        ASTClassOrInterfaceType cit = getCollectionItemType(collectionType);
         if (cit == null) {
-            return data;
+            return;
         }
         ASTVariableDeclaratorId decl = node.getFirstDescendantOfType(ASTVariableDeclaratorId.class);
         List<NameOccurrence> usages = decl.getUsages();
         for (NameOccurrence no : usages) {
-            ASTName name = (ASTName) no.getLocation();
+            ASTCastExpression castExpression = findCastExpression(no.getLocation());
-            Node n = name.getParent().getParent().getParent();
+            if (castExpression != null) {
-            if (n instanceof ASTCastExpression) {
+                ASTClassOrInterfaceType castTarget = castExpression.getFirstDescendantOfType(ASTClassOrInterfaceType.class);
-                addViolation(data, n);
+                if (castTarget != null
+                        && cit.getImage().equals(castTarget.getImage())
+                        && !castTarget.hasDescendantOfType(ASTTypeArgument.class)) {
+                    addViolation(data, castExpression);
+                }
             }
         }
+    }
+
+    private ASTClassOrInterfaceType getCollectionItemType(ASTClassOrInterfaceType collectionType) {
+        if (TypeHelper.isA(collectionType, Map.class)) {
+            List<ASTClassOrInterfaceType> types = collectionType.findDescendantsOfType(ASTClassOrInterfaceType.class);
+            if (types.size() >= 2) {
+                return types.get(1); // the value type of the map
+            }
+        } else {
+            return collectionType.getFirstDescendantOfType(ASTClassOrInterfaceType.class);
+        }
+        return null;
+    }
+
+    private ASTCastExpression findCastExpression(ScopedNode usage) {
+        Node n = usage.getNthParent(2);
+        if (n instanceof ASTCastExpression) {
+            return (ASTCastExpression) n;
+        }
+        n = n.getParent();
+        if (n instanceof ASTCastExpression) {
+            return (ASTCastExpression) n;
+        }
         return null;
     }
 }

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

@@ -1855,6 +1855,30 @@ public class Foo {
         </example>
     </rule>
 
+    <rule name="UnnecessaryCast"
+          language="java"
+          minimumLanguageVersion="1.5"
+          since="6.24.0"
+          message="Avoid unnecessary casts"
+          class="net.sourceforge.pmd.lang.java.rule.codestyle.UnnecessaryCastRule"
+          externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_codestyle.html#unnecessarycast">
+        <description>
+This rule detects when a cast is unnecessary while accessing collection elements. This rule is mostly useful
+for old java code before generics where introduced with java 1.5.
+        </description>
+        <priority>3</priority>
+        <example>
+<![CDATA[
+public class UnnecessaryCastSample {
+    public void method() {
+        List<String> stringList = Arrays.asList("a", "b");
+        String element = (String) stringList.get(0); // this cast is unnecessary
+        String element2 = stringList.get(0);
+    }
+}
+]]>
+        </example>
+    </rule>
 
     <rule name="UnnecessaryConstructor"
           language="java"

pmd-java/src/main/resources/rulesets/java/quickstart.xml

@@ -105,6 +105,7 @@
     <!-- <rule ref="category/java/codestyle.xml/SuspiciousConstantFieldName" /> -->
     <!-- <rule ref="category/java/codestyle.xml/TooManyStaticImports" /> -->
     <rule ref="category/java/codestyle.xml/UnnecessaryAnnotationValueElement"/>
+    <!-- <rule ref="category/java/codestyle.xml/UnnecessaryCast" /> -->
     <rule ref="category/java/codestyle.xml/UnnecessaryConstructor"/>
     <rule ref="category/java/codestyle.xml/UnnecessaryFullyQualifiedName"/>
     <rule ref="category/java/codestyle.xml/UnnecessaryLocalBeforeReturn"/>

pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryCastTest.java

@@ -0,0 +1,12 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+
+package net.sourceforge.pmd.lang.java.rule.codestyle;
+
+import net.sourceforge.pmd.testframework.PmdRuleTst;
+
+public class UnnecessaryCastTest extends PmdRuleTst {
+    // no additional unit tests
+}

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryCast.xml

@@ -0,0 +1,157 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<test-data
+    xmlns="http://pmd.sourceforge.net/rule-tests"
+    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd">
+
+    <test-code>
+        <description>Basic Violations</description>
+        <expected-problems>5</expected-problems>
+        <expected-linenumbers>12,15,18,23,28</expected-linenumbers>
+        <code><![CDATA[
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.uilt.HashMap;
+
+public class UnnecessaryCastSample {
+    private Map<Integer, String> map = new HashMap<>();
+
+    public void localVars() {
+        List<String> stringList = Arrays.asList("a");
+        String element = (String) stringList.get(0);
+
+        List<Double> doubleList = new ArrayList<>();
+        Double number = (Double) doubleList.get(0);
+
+        Map<String, String> stringMap = new HashMap<>();
+        String mapData = (String) stringMap.get("a");
+    }
+
+    public void fields() {
+        map.put(1, "test");
+        String val = (String) map.get(1);
+    }
+
+    public void fields2() {
+        this.map.put(1, "test");
+        String val = (String) this.map.get(1);
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Without casts there should be no violation</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.uilt.HashMap;
+
+public class UnnecessaryCastSample {
+    private Map<Integer, String> map = new HashMap<>();
+
+    public void localVars() {
+        List<String> stringList = Arrays.asList("a");
+        String element = stringList.get(0);
+
+        List<Double> doubleList = new ArrayList<>();
+        Double number = doubleList.get(0);
+    }
+
+    public void fields() {
+        map.put(1, "test");
+        String val = map.get(1);
+    }
+
+    public void fields2() {
+        this.map.put(1, "test");
+        String val = this.map.get(1);
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Unnecessary casts with iterator</description>
+        <expected-problems>2</expected-problems>
+        <expected-linenumbers>10,17</expected-linenumbers>
+        <code><![CDATA[
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+public class UnnecessaryCastSample {
+    public void localVars() {
+        List<String> stringList = Arrays.asList("a");
+        Iterator<String> stringIt = stringList.iterator();
+        while (stringIt.hasNext()) {
+            String element = (String) stringIt.next();
+            String element2 = stringIt.next();
+        }
+
+        List<Double> doubleList = new ArrayList<>();
+        Iterator<Double> doubleIt = doubleList.iterator();
+        while (doubleIt.hasNext()) {
+            Double number = (Double) doubleIt.next();
+            Double number2 = doubleIt.next();
+        }
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Avoid cast false-positives</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class UnnecessaryCastSample {
+    public void localVars() {
+        List<Number> numbers = Arrays.asList(1, 2, 3);
+        Integer myInt = (Integer) numbers.get(0);
+
+        List<Object> data = new ArrayList<>();
+        String item = (String) data.get(0);
+
+        Map<String, ?> map = new HashMap<>();
+        String dataFromMap = (String) map.get("foo");
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Avoid clone false-positive</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class UnnecessaryCastSample {
+    public void localVars() {
+        List<String> strings = new ArrayList<>();
+        List<String> copy = (List<String>) strings.clone();
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Necessary Map Cast (nested generics) false-positive</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+public class MapCasts {
+    private final Map<Class<?>, Map<String, ?>> resourceCaches = new ConcurrentHashMap<>(4);
+
+    @SuppressWarnings("unchecked")
+    public <T> Map<String, T> getResourceCache(Class<T> valueType) {
+        return (Map<String, T>) this.resourceCaches.computeIfAbsent(valueType, key -> new ConcurrentHashMap<>());
+    }
+}
+        ]]></code>
+    </test-code>
+</test-data>