diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index db03d020e9..21189ba198 100644 --- a/docs/pages/release_notes.md +++ b/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# diff --git a/pmd-core/src/main/resources/rulesets/releases/6250.xml b/pmd-core/src/main/resources/rulesets/releases/6250.xml new file mode 100644 index 0000000000..db652d271d --- /dev/null +++ b/pmd-core/src/main/resources/rulesets/releases/6250.xml @@ -0,0 +1,13 @@ + + + + +This ruleset contains links to rules that are new in PMD v6.25.0 + + + + + diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/migrating/UnnecessaryCastRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryCastRule.java similarity index 54% rename from pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/migrating/UnnecessaryCastRule.java rename to pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryCastRule.java index dab321faab..c6d97db928 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/migrating/UnnecessaryCastRule.java +++ b/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 */ -// TODO This is not referenced by any RuleSet? public class UnnecessaryCastRule extends AbstractJavaRule { private static Set 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) { - ASTClassOrInterfaceType cit = node.getFirstDescendantOfType(ASTClassOrInterfaceType.class); - if (cit == null || !implClassNames.contains(cit.getImage())) { - return data; + private void process(Node node, Object data) { + ASTClassOrInterfaceType collectionType = node.getFirstDescendantOfType(ASTClassOrInterfaceType.class); + if (collectionType == null || !implClassNames.contains(collectionType.getImage())) { + return; } - cit = cit.getFirstDescendantOfType(ASTClassOrInterfaceType.class); + ASTClassOrInterfaceType cit = getCollectionItemType(collectionType); if (cit == null) { - return data; + return; } ASTVariableDeclaratorId decl = node.getFirstDescendantOfType(ASTVariableDeclaratorId.class); List usages = decl.getUsages(); for (NameOccurrence no : usages) { - ASTName name = (ASTName) no.getLocation(); - Node n = name.getParent().getParent().getParent(); - if (n instanceof ASTCastExpression) { - addViolation(data, n); + ASTCastExpression castExpression = findCastExpression(no.getLocation()); + if (castExpression != null) { + ASTClassOrInterfaceType castTarget = castExpression.getFirstDescendantOfType(ASTClassOrInterfaceType.class); + 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 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; } } diff --git a/pmd-java/src/main/resources/category/java/codestyle.xml b/pmd-java/src/main/resources/category/java/codestyle.xml index e8995ca3a9..41d06456bd 100644 --- a/pmd-java/src/main/resources/category/java/codestyle.xml +++ b/pmd-java/src/main/resources/category/java/codestyle.xml @@ -1855,6 +1855,30 @@ public class Foo { + + +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. + + 3 + + stringList = Arrays.asList("a", "b"); + String element = (String) stringList.get(0); // this cast is unnecessary + String element2 = stringList.get(0); + } +} +]]> + + --> + diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryCastTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryCastTest.java new file mode 100644 index 0000000000..992a7e91ac --- /dev/null +++ b/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 +} diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryCast.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryCast.xml new file mode 100644 index 0000000000..e3403cb86d --- /dev/null +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryCast.xml @@ -0,0 +1,157 @@ + + + + + Basic Violations + 5 + 12,15,18,23,28 + map = new HashMap<>(); + + public void localVars() { + List stringList = Arrays.asList("a"); + String element = (String) stringList.get(0); + + List doubleList = new ArrayList<>(); + Double number = (Double) doubleList.get(0); + + Map 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); + } +} + ]]> + + + + Without casts there should be no violation + 0 + map = new HashMap<>(); + + public void localVars() { + List stringList = Arrays.asList("a"); + String element = stringList.get(0); + + List 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); + } +} + ]]> + + + + Unnecessary casts with iterator + 2 + 10,17 + stringList = Arrays.asList("a"); + Iterator stringIt = stringList.iterator(); + while (stringIt.hasNext()) { + String element = (String) stringIt.next(); + String element2 = stringIt.next(); + } + + List doubleList = new ArrayList<>(); + Iterator doubleIt = doubleList.iterator(); + while (doubleIt.hasNext()) { + Double number = (Double) doubleIt.next(); + Double number2 = doubleIt.next(); + } + } +} + ]]> + + + + Avoid cast false-positives + 0 + numbers = Arrays.asList(1, 2, 3); + Integer myInt = (Integer) numbers.get(0); + + List data = new ArrayList<>(); + String item = (String) data.get(0); + + Map map = new HashMap<>(); + String dataFromMap = (String) map.get("foo"); + } +} + ]]> + + + + Avoid clone false-positive + 0 + strings = new ArrayList<>(); + List copy = (List) strings.clone(); + } +} + ]]> + + + + Necessary Map Cast (nested generics) false-positive + 0 + , Map> resourceCaches = new ConcurrentHashMap<>(4); + + @SuppressWarnings("unchecked") + public Map getResourceCache(Class valueType) { + return (Map) this.resourceCaches.computeIfAbsent(valueType, key -> new ConcurrentHashMap<>()); + } +} + ]]> + +