phoedos/pmd
1
1
Fork 1
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #3619 from adangel:java-rule-improvements-2

Browse Source 
[java] Rule improvements part 2 #3619

* pr-3619:
  [java] MethodReturnsInternalArray - add test case (#3630)
  [doc] Update release notes (#3618)
  [java] ExceptionAsFlowControl - use rulechain
  [java] CouplingBetweenObjects - consider nested classes in interfaces
  [java] UnnecessaryReturn - use rulechain
  [java] UnnecessaryFullyQualifiedName - remove unnecessary super
  [java] UseCollectionIsEmpty - use rulechain
  [java] UnusedFormalParameter - fix false negative with anonymous
classes
  [java] MethodReturnsInternalArray - use rulechain
This commit is contained in:
Andreas Dangel
2021-11-26 08:46:28 +01:00
parent 2d76292ae8 48714f8161
commit af3c3fb9cb
14 changed files with 205 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
docs/pages/release_notes.md
View File

@ -23,6 +23,8 @@ This is a {{ site.pmd.release_type }} release.
* java-bestpractices
* [#3613](https://github.com/pmd/pmd/issues/3613): \[java] ArrayIsStoredDirectly doesn't consider nested classes
* [#3614](https://github.com/pmd/pmd/issues/3614): \[java] JUnitTestsShouldIncludeAssert doesn't consider nested classes
* [#3618](https://github.com/pmd/pmd/issues/3618): \[java] UnusedFormalParameter doesn't consider anonymous classes
* [#3630](https://github.com/pmd/pmd/issues/3630): \[java] MethodReturnsInternalArray doesn't consider anonymous classes
* java-errorprone
* [#3624](https://github.com/pmd/pmd/issues/3624): \[java] TestClassWithoutTestCases reports wrong classes in a file
* java-performance

9
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/MethodReturnsInternalArrayRule.java
View File

@ -12,7 +12,6 @@ import net.sourceforge.pmd.lang.ast.Node;
import net.sourceforge.pmd.lang.java.ast.ASTAllocationExpression;
import net.sourceforge.pmd.lang.java.ast.ASTAnyTypeDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTArrayInitializer;
import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTExpression;
import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
@ -32,12 +31,8 @@ import net.sourceforge.pmd.lang.java.ast.ASTVariableInitializer;
*/
public class MethodReturnsInternalArrayRule extends AbstractSunSecureRule {
@Override
public Object visit(ASTClassOrInterfaceDeclaration node, Object data) {
if (node.isInterface()) {
return data;
}
return super.visit(node, data);
public MethodReturnsInternalArrayRule() {
addRuleChainVisit(ASTMethodDeclaration.class);
}
@Override

10
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnusedFormalParameterRule.java
View File

@ -12,6 +12,7 @@ import java.util.List;
import java.util.Map;
import net.sourceforge.pmd.lang.ast.Node;
import net.sourceforge.pmd.lang.java.ast.ASTAllocationExpression;
import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTConstructorDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTFormalParameter;
@ -42,18 +43,18 @@ public class UnusedFormalParameterRule extends AbstractJavaRule {
@Override
public Object visit(ASTConstructorDeclaration node, Object data) {
check(node, data);
return data;
return super.visit(node, data);
}
@Override
public Object visit(ASTMethodDeclaration node, Object data) {
if (!node.isPrivate() && !getProperty(CHECKALL_DESCRIPTOR)) {
return data;
return super.visit(node, data);
}
if (!node.isNative() && !node.isAbstract() && !isSerializationMethod(node) && !hasOverrideAnnotation(node)) {
check(node, data);
}
return data;
return super.visit(node, data);
}
private boolean isSerializationMethod(ASTMethodDeclaration node) {
@ -86,7 +87,8 @@ public class UnusedFormalParameterRule extends AbstractJavaRule {
private void check(Node node, Object data) {
Node parent = node.getParent().getParent().getParent();
if (parent instanceof ASTClassOrInterfaceDeclaration
&& !((ASTClassOrInterfaceDeclaration) parent).isInterface()) {
&& !((ASTClassOrInterfaceDeclaration) parent).isInterface()
|| parent instanceof ASTAllocationExpression) {
Map<VariableNameDeclaration, List<NameOccurrence>> vars = ((JavaNode) node).getScope()
.getDeclarations(VariableNameDeclaration.class);
for (Map.Entry<VariableNameDeclaration, List<NameOccurrence>> entry : vars.entrySet()) {

6
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UseCollectionIsEmptyRule.java
View File

@ -22,6 +22,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix;
import net.sourceforge.pmd.lang.java.ast.ASTResultType;
import net.sourceforge.pmd.lang.java.ast.ASTType;
import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclarator;
import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
import net.sourceforge.pmd.lang.java.rule.AbstractInefficientZeroCheck;
import net.sourceforge.pmd.lang.java.symboltable.ClassScope;
import net.sourceforge.pmd.lang.java.symboltable.JavaNameOccurrence;
@ -38,6 +39,11 @@ import net.sourceforge.pmd.util.CollectionUtil;
*/
public class UseCollectionIsEmptyRule extends AbstractInefficientZeroCheck {
public UseCollectionIsEmptyRule() {
addRuleChainVisit(ASTVariableDeclaratorId.class); // visited in AbstractInefficientZeroCheck
addRuleChainVisit(ASTPrimarySuffix.class);
}
@Override
public boolean appliesToClassName(String name) {
return CollectionUtil.isCollectionType(name, true);

8
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryFullyQualifiedNameRule.java
View File

@ -40,10 +40,10 @@ public class UnnecessaryFullyQualifiedNameRule extends AbstractJavaRule {
private String currentPackage;
public UnnecessaryFullyQualifiedNameRule() {
super.addRuleChainVisit(ASTPackageDeclaration.class);
super.addRuleChainVisit(ASTImportDeclaration.class);
super.addRuleChainVisit(ASTClassOrInterfaceType.class);
super.addRuleChainVisit(ASTName.class);
addRuleChainVisit(ASTPackageDeclaration.class);
addRuleChainVisit(ASTImportDeclaration.class);
addRuleChainVisit(ASTClassOrInterfaceType.class);
addRuleChainVisit(ASTName.class);
}
@Override

22
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryReturnRule.java
View File

@ -13,22 +13,26 @@ import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
public class UnnecessaryReturnRule extends AbstractJavaRule {
@Override
public Object visit(ASTMethodDeclaration node, Object data) {
if (node.getResultType().isVoid()) {
super.visit(node, data);
}
return data;
public UnnecessaryReturnRule() {
addRuleChainVisit(ASTReturnStatement.class);
}
@Override
public Object visit(ASTReturnStatement node, Object data) {
if (node.getParent() instanceof ASTStatement && node.getNthParent(2) instanceof ASTBlockStatement
&& node.getNthParent(3) instanceof ASTBlock && node.getNthParent(4) instanceof ASTMethodDeclaration) {
if (node.getNumChildren() == 0 && isDirectMethodStatement(node)) {
addViolation(data, node);
}
return data;
}
/**
* Checks whether the given return statement is nested in some other statement (e.g. if condition)
* or whether it is a top-level statement in the method, a "direct method statement".
*/
private boolean isDirectMethodStatement(ASTReturnStatement node) {
return node.getParent() instanceof ASTStatement
&& node.getNthParent(2) instanceof ASTBlockStatement
&& node.getNthParent(3) instanceof ASTBlock
&& node.getNthParent(4) instanceof ASTMethodDeclaration;
}
}

21
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/CouplingBetweenObjectsRule.java
View File

@ -7,6 +7,7 @@ package net.sourceforge.pmd.lang.java.rule.design;
import static net.sourceforge.pmd.properties.constraints.NumericConstraints.positive;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import net.sourceforge.pmd.lang.ast.Node;
@ -54,7 +55,7 @@ public class CouplingBetweenObjectsRule extends AbstractJavaRule {
typesFoundSoFar = new HashSet<>();
couplingCount = 0;
Object returnObj = cu.childrenAccept(this, data);
Object returnObj = super.visit(cu, data);
if (couplingCount > getProperty(THRESHOLD_DESCRIPTOR)) {
addViolation(data, cu,
@ -64,14 +65,6 @@ public class CouplingBetweenObjectsRule extends AbstractJavaRule {
return returnObj;
}
@Override
public Object visit(ASTClassOrInterfaceDeclaration node, Object data) {
if (node.isInterface()) {
return data;
}
return super.visit(node, data);
}
@Override
public Object visit(ASTResultType node, Object data) {
for (int x = 0; x < node.getNumChildren(); x++) {
@ -138,10 +131,18 @@ public class CouplingBetweenObjectsRule extends AbstractJavaRule {
* The variable type.
*/
private void checkVariableType(Node nameNode, String variableType) {
List<ASTClassOrInterfaceDeclaration> parentTypes = nameNode.getParentsOfType(ASTClassOrInterfaceDeclaration.class);
// TODO - move this into the symbol table somehow?
if (nameNode.getParentsOfType(ASTClassOrInterfaceDeclaration.class).isEmpty()) {
if (parentTypes.isEmpty()) {
return;
}
// skip interfaces
if (parentTypes.get(0).isInterface()) {
return;
}
// if the field is of any type other than the class type
// increment the count
ClassScope clzScope = ((JavaNode) nameNode).getScope().getEnclosingScope(ClassScope.class);

4
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/ExceptionAsFlowControlRule.java
View File

@ -23,6 +23,10 @@ import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
*/
public class ExceptionAsFlowControlRule extends AbstractJavaRule {
public ExceptionAsFlowControlRule() {
addRuleChainVisit(ASTThrowStatement.class);
}
@Override
public Object visit(ASTThrowStatement node, Object data) {
ASTTryStatement parent = node.getFirstParentOfType(ASTTryStatement.class);

38
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/MethodReturnsInternalArray.xml
View File

@ -242,7 +242,7 @@ public class MethodReturnsInternalArrayCase {
</test-code>
<test-code>
<description> #1738 MethodReturnsInternalArray in inner classes</description>
<description>#1738 MethodReturnsInternalArray in inner classes</description>
<expected-problems>1</expected-problems>
<code><![CDATA[
public class Outer {
@ -440,6 +440,42 @@ public class MyClass {
return fooBarNonFinal;
}
};
}
]]></code>
</test-code>
<test-code>
<description>nested class in interface</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>4</expected-linenumbers>
<code><![CDATA[
public interface Outer {
class Foo {
String [] arr;
String [] getArr() {return arr;}
}
}
]]></code>
</test-code>
<test-code>
<description>[java] MethodReturnsInternalArray doesn't consider anonymous classes #3630</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>7</expected-linenumbers>
<code><![CDATA[
public class Outer {
private static final String[] names = new String[] {"a", "b"};
public static Provider getProvider() {
return new Provider() {
@Override
public String[] getNames() {
return names;
}
};
}
public interface Provider {
String[] getNames();
}
}
]]></code>
</test-code>

27
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UnusedFormalParameter.xml
View File

@ -75,6 +75,7 @@ class Foo {
<description>flag public methods if checkall property is set</description>
<rule-property name="checkAll">true</rule-property>
<expected-problems>1</expected-problems>
<expected-linenumbers>2</expected-linenumbers>
<code-ref id="public-methods"/>
</test-code>
@ -302,6 +303,32 @@ class Imp1 {
int ignoredArg2,
int unused,
String ignored) {}
}
]]></code>
</test-code>
<test-code>
<description>anonymous classes defined in methods should be considered</description>
<rule-property name="checkAll">false</rule-property>
<expected-problems>2</expected-problems>
<expected-linenumbers>4,12</expected-linenumbers>
<code><![CDATA[
class Impl {
public void m1() {
class Local {
private void m2(int arg1) { }
private void m3() { m2(0); }
}
Local l = new Local();
l.m3();
}
public void m4() {
Object o = new Object() {
private void m5(int arg1) { }
public boolean equals(Object other) { m5(0); return false; }
};
o.equals(o);
}
}
]]></code>
</test-code>

35
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UseCollectionIsEmpty.xml
View File

@ -451,6 +451,41 @@ public class Foo {
if (theList.size() == 0) throw new IllegalArgumentException("empty list");
if (theList.isEmpty()) throw new IllegalArgumentException("empty list");
}
}
]]></code>
</test-code>
<test-code>
<description>With nested classes</description>
<expected-problems>3</expected-problems>
<expected-linenumbers>7,15,21</expected-linenumbers>
<code><![CDATA[
import java.util.List;
public class Foo {
void myMethod() {
Object anonymous = new Object() {
public boolean check(List lst) {
if (lst.size() == 0) {
return true;
}
return false;
}
};
class Local {
public boolean check2(List lst) {
return lst.size() == 0;
}
}
}
class Inner {
public static boolean bar(List lst) {
if(lst.size() == 0){
return true;
}
return false;
}
}
}
]]></code>
</test-code>

36
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryReturn.xml
View File

@ -72,6 +72,42 @@ public class Foo {
return;
}
}
}
]]></code>
</test-code>
<test-code>
<description>nested classes</description>
<expected-problems>4</expected-problems>
<expected-linenumbers>6,12,19,26</expected-linenumbers>
<code><![CDATA[
public class Foo {
void myMethod() {
Object anonymous = new Object() {
public void bar1() {
int y = 1;
return;
}
};
class Local {
public void bar2() {
int y = 2;
return;
}
}
}
class Inner {
void bar3() {
int y = 3;
return;
}
}
}
interface MyInterface {
default void myDefaultMethod() {
int y = 4;
return;
}
}
]]></code>
</test-code>

22
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/CouplingBetweenObjects.xml
View File

@ -8,6 +8,7 @@
<description>lots of coupling</description>
<rule-property name="threshold">2</rule-property>
<expected-problems>1</expected-problems>
<expected-linenumbers>1</expected-linenumbers>
<code><![CDATA[
import java.util.*;
public class Foo {
@ -34,7 +35,26 @@ public class Foo {
<expected-problems>0</expected-problems>
<code><![CDATA[
public interface Foo {
public static final int FOO = 2;
List foo();
ArrayList foo();
Vector foo();
}
]]></code>
</test-code>
<test-code>
<description>lots of coupling in inner class in interfaces</description>
<rule-property name="threshold">2</rule-property>
<expected-problems>1</expected-problems>
<expected-linenumbers>1</expected-linenumbers>
<code><![CDATA[
import java.util.*;
public interface Foo {
class Inner {
public List foo() {return null;}
public ArrayList foo() {return null;}
public Vector foo() {return null;}
}
}
]]></code>
</test-code>

1
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/ExceptionAsFlowControl.xml
View File

@ -7,6 +7,7 @@
<test-code>
<description>failure case</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>9</expected-linenumbers>
<code><![CDATA[
public class Foo {
void bar() {