hiifong/pmd
1
0 Fork 0
forked from phoedos/pmd
Code Pull Requests Activity

Merge pull request #3177 from oowekyala:java-update-LooseCoupling

Browse Source 
[java] Update rule LooseCoupling #3177
This commit is contained in:
Andreas Dangel committed 2021-04-02 10:37:02 +02:00
2 parents c6b8147ec1 86c24760ab
commit 43feee9416
6 files changed
+229 -25
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

.ci/files/all-java.xml
+1 -1
View File
@@ -36,7 +36,7 @@
<rule ref="category/java/bestpractices.xml/JUnitTestsShouldIncludeAssert"/>
<rule ref="category/java/bestpractices.xml/JUnitUseExpected"/>
<rule ref="category/java/bestpractices.xml/LiteralsFirstInComparisons"/>
<!-- <rule ref="category/java/bestpractices.xml/LooseCoupling"/> -->
<rule ref="category/java/bestpractices.xml/LooseCoupling"/>
<rule ref="category/java/bestpractices.xml/MethodReturnsInternalArray"/>
<rule ref="category/java/bestpractices.xml/MissingOverride"/>
<!-- <rule ref="category/java/bestpractices.xml/OneDeclarationPerLine"/> -->
docs/pages/7_0_0_release_notes.md
+3
View File
@@ -83,6 +83,7 @@ The default version is always ES6.
not necessary are allowed, if they separate expressions of different precedence.
The other property `ignoreBalancing` (default: true) is similar, in that it allows parentheses that help
reading and understanding the expressions.
* The rule {% rule "java/bestpractices/LooseCoupling" %} has a new property to allow some types to be coupled to (`allowedTypes`).
#### Removed Rules
@@ -133,10 +134,12 @@ The following previously deprecated rules have been finally removed:
* [#1998](https://github.com/pmd/pmd/issues/1998): \[java] AccessorClassGeneration false-negative: subclass calls private constructor
* [#2130](https://github.com/pmd/pmd/issues/2130): \[java] UnusedLocalVariable: false-negative with array
* [#2147](https://github.com/pmd/pmd/issues/2147): \[java] JUnitTestsShouldIncludeAssert - false positives with lambdas and static methods
* [#2464](https://github.com/pmd/pmd/issues/2464): \[java] LooseCoupling must ignore class literals: ArrayList.class
* [#2542](https://github.com/pmd/pmd/issues/2542): \[java] UseCollectionIsEmpty can not detect the case `foo.bar().size()`
* [#2796](https://github.com/pmd/pmd/issue/2796): \[java] UnusedAssignment false positive with call chains
* [#2797](https://github.com/pmd/pmd/issues/2797): \[java] MissingOverride long-standing issues
* [#2806](https://github.com/pmd/pmd/issues/2806): \[java] SwitchStmtsShouldHaveDefault false-positive with Java 14 switch non-fallthrough branches
* [#2822](https://github.com/pmd/pmd/issues/2822): \[java] LooseCoupling rule: Extend to cover user defined implementations and interfaces
* [#2883](https://github.com/pmd/pmd/issues/2883): \[java] JUnitAssertionsShouldIncludeMessage false positive with method call
* [#2890](https://github.com/pmd/pmd/issues/2890): \[java] UnusedPrivateMethod false positive with generics
* java-codestyle
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LooseCouplingRule.java
+68 -20
View File
@@ -5,38 +5,86 @@
package net.sourceforge.pmd.lang.java.rule.bestpractices;
import java.util.Collection;
import java.util.List;
import java.util.Map;
import net.sourceforge.pmd.lang.ast.Node;
import net.sourceforge.pmd.lang.ast.NodeStream;
import net.sourceforge.pmd.lang.java.ast.ASTArrayAllocation;
import net.sourceforge.pmd.lang.java.ast.ASTArrayType;
import net.sourceforge.pmd.lang.java.ast.ASTBlock;
import net.sourceforge.pmd.lang.java.ast.ASTCastExpression;
import net.sourceforge.pmd.lang.java.ast.ASTClassLiteral;
import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType;
import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTFormalParameter;
import net.sourceforge.pmd.lang.java.ast.ASTConstructorCall;
import net.sourceforge.pmd.lang.java.ast.ASTExtendsList;
import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTResultType;
import net.sourceforge.pmd.lang.java.ast.ASTSuperExpression;
import net.sourceforge.pmd.lang.java.ast.ASTThisExpression;
import net.sourceforge.pmd.lang.java.ast.ASTTypeExpression;
import net.sourceforge.pmd.lang.java.ast.JavaNode;
import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
import net.sourceforge.pmd.lang.java.types.TypeTestUtil;
import net.sourceforge.pmd.properties.PropertyDescriptor;
import net.sourceforge.pmd.properties.PropertyFactory;
public class LooseCouplingRule extends AbstractJavaRule {
public class LooseCouplingRule extends AbstractJavaRulechainRule {
private static final PropertyDescriptor<List<String>> ALLOWED_TYPES =
PropertyFactory.stringListProperty("allowedTypes")
.desc("Exceptions to the rule")
.defaultValues("java.util.Properties")
.build();
public LooseCouplingRule() {
super(ASTClassOrInterfaceType.class);
definePropertyDescriptor(ALLOWED_TYPES);
}
@Override
public Object visit(ASTClassOrInterfaceType node, Object data) {
if (methodHasOverride(node)) {
return data;
if (isConcreteCollectionType(node)
&& !isInOverriddenMethodSignature(node)
&& !isInAllowedSyntacticCtx(node)
&& !isAllowedType(node)) {
addViolation(data, node, node.getSimpleName());
}
Node parent = node.getNthParent(3);
boolean isType = (TypeTestUtil.isA(Collection.class, node) || TypeTestUtil.isA(Map.class, node))
&& !(node.getType() != null && node.getType().isInterface());
if (isType && (parent instanceof ASTFieldDeclaration || parent instanceof ASTFormalParameter
|| parent instanceof ASTResultType)) {
addViolation(data, node, node.getImage());
}
return data;
return null;
}
private boolean methodHasOverride(JavaNode node) {
ASTMethodDeclaration method = node.ancestors(ASTMethodDeclaration.class).first();
return method != null && method.isAnnotationPresent(Override.class);
private boolean isInAllowedSyntacticCtx(ASTClassOrInterfaceType node) {
JavaNode parent = node.getParent();
return parent instanceof ASTConstructorCall // new ArrayList<>()
|| parent instanceof ASTTypeExpression // instanceof, method reference
|| parent instanceof ASTCastExpression // if we allow instanceof, we should allow cast
|| parent instanceof ASTClassLiteral // ArrayList.class
|| parent instanceof ASTClassOrInterfaceType // AbstractMap.SimpleEntry
|| parent instanceof ASTExtendsList // extends AbstractMap<...>
|| parent instanceof ASTThisExpression // Enclosing.this
|| parent instanceof ASTSuperExpression // Enclosing.super
|| parent instanceof ASTArrayType && parent.getParent() instanceof ASTArrayAllocation;
}
private boolean isAllowedType(ASTClassOrInterfaceType node) {
for (String allowed : getProperty(ALLOWED_TYPES)) {
if (TypeTestUtil.isA(allowed, node)) {
return true;
}
}
return false;
}
private boolean isConcreteCollectionType(ASTClassOrInterfaceType node) {
return (TypeTestUtil.isA(Collection.class, node) || TypeTestUtil.isA(Map.class, node))
&& !node.getTypeMirror().isInterface();
}
private static boolean isInOverriddenMethodSignature(JavaNode node) {
JavaNode ancestor = node.ancestors().map(NodeStream.asInstanceOf(ASTMethodDeclaration.class, ASTBlock.class)).first();
if (ancestor instanceof ASTMethodDeclaration) {
// then it's in a signature and not the body
return ((ASTMethodDeclaration) ancestor).isOverridden();
}
return false;
}
}
pmd-java/src/main/resources/category/java/bestpractices.xml
+6 -3
View File
@@ -939,9 +939,12 @@ class Foo {
class="net.sourceforge.pmd.lang.java.rule.bestpractices.LooseCouplingRule"
externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_bestpractices.html#loosecoupling">
<description>
The use of implementation types (i.e., HashSet) as object references limits your ability to use alternate
implementations in the future as requirements change. Whenever available, referencing objects
by their interface types (i.e, Set) provides much more flexibility.
Excessive coupling to implementation types (e.g., `HashSet`) limits your ability to use alternate
implementations in the future as requirements change. Whenever available, declare variables
and parameters using a more general type (e.g, `Set`).
This rule reports uses of concrete collection types. User-defined types that should be treated
the same as interfaces can be configured with the property `allowedTypes`.
</description>
<priority>3</priority>
<example>
pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LooseCouplingTest.java
-1
View File
@@ -6,7 +6,6 @@ package net.sourceforge.pmd.lang.java.rule.bestpractices;
import net.sourceforge.pmd.testframework.PmdRuleTst;
@org.junit.Ignore("Rule has not been updated yet")
public class LooseCouplingTest extends PmdRuleTst {
// no additional unit tests
}
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LooseCoupling.xml
+151
View File
@@ -135,6 +135,157 @@ import java.util.LinkedHashMap;
public class Test {
@Override
public LinkedHashMap findGetters() {}
}
]]></code>
</test-code>
<test-code>
<description>FP with method reference</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import java.util.HashMap;
public class Test {
private static final ThreadLocal TREE_CACHE =
ThreadLocal.withInitial(HashMap::new);
}
]]></code>
</test-code>
<test-code>
<description>FP with instanceof and cast</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import java.util.HashMap;
public class Test {
boolean m(Map m) {
if (m instanceof HashMap) {
return ((HashMap) m).isEmpty();
}
return false;
}
}
]]></code>
</test-code>
<test-code>
<description>FP with static method call</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import java.util.HashMap;
public class MyMap implements Map {
static MyMap create() { return null; }
}
class Foo {
final Map map =
MyMap.create();
}
]]></code>
</test-code>
<test-code>
<description>FP with array creation</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import java.util.HashMap;
public class MyMap implements Map {
static MyMap create() { return null; }
}
class Foo {
final Map[] map = new MyMap[5]; // ok
}
]]></code>
</test-code>
<test-code>
<description>FP with j.util.Properties</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import java.util.Properties;
class Foo {
final Properties map = new Properties();
}
]]></code>
</test-code>
<test-code>
<description>Exception list, no exception</description>
<expected-problems>1</expected-problems>
<code><![CDATA[
package mine;
import java.util.List;
class O implements List {
final O map = new O();
}
]]></code>
</test-code>
<test-code>
<description>Exception list</description>
<rule-property name="allowedTypes">mine.O</rule-property>
<expected-problems>0</expected-problems>
<code><![CDATA[
package mine;
import java.util.List;
class O implements List {
final O map = new O();
}
]]></code>
</test-code>
<test-code>
<description>#2464 class literals</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import java.util.ArrayList;
class Foo {
final Object o = ArrayList.class;
}
]]></code>
</test-code>
<test-code>
<description>Inner class selection</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import java.util.AbstractMap;
class Foo {
final Object o =
new AbstractMap.SimpleEntry<>("", "");
}
]]></code>
</test-code>
<test-code>
<description>Extends clause</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import java.util.AbstractMap;
class Foo extends AbstractMap<String, String> {
}
]]></code>
</test-code>
<test-code>
<description>This/super qualifier</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import java.util.ArrayList;
class Foo extends ArrayList {
{
Foo.super.clear();
}
class Inner {
{
Foo.this.clear();
Foo.super.clear();
}
}
}
]]></code>
</test-code>