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

Merge branch 'issue-1546'

Browse Source 
Closes #133
PR has been rebased for 5.4.x/5.5.x/master
This commit is contained in:
Andreas Dangel
2016-11-26 18:27:45 +01:00
parent 9e26497bd1 9460758e69
commit 70cd1e816d
4 changed files with 245 additions and 176 deletions
Download Patch File Download Diff File Expand all files Collapse all files

228
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/imports/UnnecessaryFullyQualifiedNameRule.java
View File

@ -3,6 +3,7 @@
*/
package net.sourceforge.pmd.lang.java.rule.imports;
import java.lang.reflect.Method;
import java.util.ArrayList;
import java.util.List;
@ -19,129 +20,161 @@ public class UnnecessaryFullyQualifiedNameRule extends AbstractJavaRule {
private List<ASTImportDeclaration> imports = new ArrayList<>();
private List<ASTImportDeclaration> matches = new ArrayList<>();
private List<PotentialViolation> violations = new ArrayList<>();
private List<PotentialViolation> enumViolations = new ArrayList<>();
public UnnecessaryFullyQualifiedNameRule() {
super.addRuleChainVisit(ASTCompilationUnit.class);
super.addRuleChainVisit(ASTImportDeclaration.class);
super.addRuleChainVisit(ASTClassOrInterfaceType.class);
super.addRuleChainVisit(ASTName.class);
super.addRuleChainVisit(ASTCompilationUnit.class);
super.addRuleChainVisit(ASTImportDeclaration.class);
super.addRuleChainVisit(ASTClassOrInterfaceType.class);
super.addRuleChainVisit(ASTName.class);
}
@Override
public Object visit(ASTCompilationUnit node, Object data) {
imports.clear();
violations.clear();
enumViolations.clear();
imports.clear();
violations.clear();
super.visit(node, data);
super.visit(node, data);
filterPotentialViolations();
reportViolations(data);
return data;
reportViolations(data);
return data;
}
@Override
public Object visit(ASTImportDeclaration node, Object data) {
imports.add(node);
return data;
imports.add(node);
return data;
}
@Override
public Object visit(ASTClassOrInterfaceType node, Object data) {
checkImports(node);
return data;
checkImports(node);
return data;
}
@Override
public Object visit(ASTName node, Object data) {
if (!(node.jjtGetParent() instanceof ASTImportDeclaration)
&& !(node.jjtGetParent() instanceof ASTPackageDeclaration)) {
checkImports(node);
}
return data;
if (!(node.jjtGetParent() instanceof ASTImportDeclaration)
&& !(node.jjtGetParent() instanceof ASTPackageDeclaration)) {
checkImports(node);
}
return data;
}
private void checkImports(JavaNode node) {
String name = node.getImage();
matches.clear();
String name = node.getImage();
matches.clear();
// Find all "matching" import declarations
for (ASTImportDeclaration importDeclaration : imports) {
if (importDeclaration.isImportOnDemand()) {
// On demand import exactly matches the package of the type
if (name.startsWith(importDeclaration.getImportedName())) {
if (name.lastIndexOf('.') == importDeclaration.getImportedName().length()) {
matches.add(importDeclaration);
continue;
}
}
} else {
// Exact match of imported class
if (name.equals(importDeclaration.getImportedName())) {
matches.add(importDeclaration);
continue;
}
// Match of static method call on imported class
if (name.startsWith(importDeclaration.getImportedName())) {
if (name.lastIndexOf('.') == importDeclaration.getImportedName().length()) {
matches.add(importDeclaration);
continue;
}
}
}
}
// Find all "matching" import declarations
for (ASTImportDeclaration importDeclaration : imports) {
if (importDeclaration.isImportOnDemand()) {
// On demand import exactly matches the package of the type
if (name.startsWith(importDeclaration.getImportedName())) {
if (name.lastIndexOf('.') == importDeclaration.getImportedName().length()) {
matches.add(importDeclaration);
continue;
}
}
} else {
// Exact match of imported class
if (name.equals(importDeclaration.getImportedName())) {
matches.add(importDeclaration);
continue;
}
// Match of static method call on imported class
if (name.startsWith(importDeclaration.getImportedName())) {
if (name.lastIndexOf('.') == importDeclaration.getImportedName().length()) {
matches.add(importDeclaration);
continue;
}
}
}
}
// If there is no direct match, consider if we match the tail end of a
// direct static import, but also a static method on a class import?
// For example:
//
// import java.util.Arrays;
// import static java.util.Arrays.asList;
// static {
// List list1 = Arrays.asList("foo"); // Array class name not needed!
// List list2 = asList("foo"); // Preferred, used static import
// }
if (matches.isEmpty() && name.indexOf('.') >= 0) {
for (ASTImportDeclaration importDeclaration : imports) {
if (importDeclaration.isStatic()) {
String[] importParts = importDeclaration.getImportedName().split("\\.");
String[] nameParts = name.split("\\.");
if (importDeclaration.isImportOnDemand()) {
// Name class part matches class part of static import?
if (nameParts[nameParts.length - 2].equals(importParts[importParts.length - 1])) {
matches.add(importDeclaration);
}
} else {
// Last 2 parts match?
if (nameParts[nameParts.length - 1].equals(importParts[importParts.length - 1])
&& nameParts[nameParts.length - 2].equals(importParts[importParts.length - 2])) {
matches.add(importDeclaration);
}
}
}
}
}
// If there is no direct match, consider if we match the tail end of a
// direct static import, but also a static method on a class import?
// For example:
//
// import java.util.Arrays;
// import static java.util.Arrays.asList;
// static {
// List list1 = Arrays.asList("foo"); // Array class name not needed!
// List list2 = asList("foo"); // Preferred, used static import
// }
if (matches.isEmpty() && name.indexOf('.') >= 0) {
for (ASTImportDeclaration importDeclaration : imports) {
if (importDeclaration.isStatic()) {
String[] importParts = importDeclaration.getImportedName().split("\\.");
String[] nameParts = name.split("\\.");
if (importDeclaration.isImportOnDemand()) {
// Name class part matches class part of static import?
if (nameParts[nameParts.length - 2].equals(importParts[importParts.length - 1])) {
matches.add(importDeclaration);
}
} else {
// Last 2 parts match?
if (nameParts[nameParts.length - 1].equals(importParts[importParts.length - 1])
&& nameParts[nameParts.length - 2].equals(importParts[importParts.length - 2])) {
matches.add(importDeclaration);
}
}
}
}
}
if (!matches.isEmpty()) {
ASTImportDeclaration firstMatch = matches.get(0);
String importStr = firstMatch.getImportedName() + (matches.get(0).isImportOnDemand() ? ".*" : "");
String type = firstMatch.isStatic() ? "static " : "";
if (!matches.isEmpty()) {
ASTImportDeclaration firstMatch = matches.get(0);
PotentialViolation v = new PotentialViolation(node, importStr, type);
violations.add(v);
if (isEnum(firstMatch.getType())) {
enumViolations.add(v);
}
}
// Could this done to avoid a conflict?
if (!isAvoidingConflict(name, firstMatch)) {
String importStr = firstMatch.getImportedName() + (firstMatch.isImportOnDemand() ? ".*" : "");
String type = firstMatch.isStatic() ? "static " : "";
matches.clear();
PotentialViolation v = new PotentialViolation(node, importStr, type);
violations.add(v);
}
}
matches.clear();
}
private boolean isAvoidingConflict(final String name, final ASTImportDeclaration firstMatch) {
if (firstMatch.isImportOnDemand() && firstMatch.isStatic() && name.indexOf('.') != -1) {
final String methodCalled = name.substring(name.indexOf('.') + 1);
// Is there any other static import conflictive?
for (final ASTImportDeclaration importDeclaration : imports) {
if (importDeclaration != firstMatch && importDeclaration.isStatic()) {
if (importDeclaration.getImportedName().startsWith(firstMatch.getImportedName())
&& importDeclaration.getImportedName().lastIndexOf('.') == firstMatch.getImportedName().length()) {
// A conflict against the same class is not an excuse, ie:
// import java.util.Arrays;
// import static java.util.Arrays.asList;
continue;
}
if (importDeclaration.isImportOnDemand()) {
// We need type resolution to make sure there is a conflicting method
if (importDeclaration.getType() != null) {
for (final Method m : importDeclaration.getType().getMethods()) {
if (m.getName().equals(methodCalled)) {
return true;
}
}
}
} else if (importDeclaration.getImportedName().endsWith(methodCalled)) {
return true;
}
}
}
}
return false;
}
private static class PotentialViolation {
private JavaNode node;
private String importStr;
private String importType;
private String importStr;
private String importType;
public PotentialViolation(JavaNode node, String importStr, String importType) {
this.node = node;
@ -154,22 +187,9 @@ public class UnnecessaryFullyQualifiedNameRule extends AbstractJavaRule {
}
}
private void filterPotentialViolations() {
if (enumViolations.size() > 1) {
violations.removeAll(enumViolations);
}
}
private void reportViolations(Object data) {
for (PotentialViolation v : violations) {
v.addViolation(this, data);
}
}
private boolean isEnum(Class<?> type) {
if (type != null && Enum.class.isAssignableFrom(type)) {
return true;
}
return false;
}
}

16
pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/imports/ImportsRulesTest.java
View File

@ -40,4 +40,20 @@ public class ImportsRulesTest extends SimpleAggregatorTst {
public enum ENUM2 {
C, D;
}
// Do not delete these two classes - it is needed for a test case
// see: /pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/imports/xml/UnnecessaryFullyQualifiedName.xml
// #1546 part 1 UnnecessaryFullyQualifiedName doesn't take into consideration conflict resolution
// #1546 part 2 UnnecessaryFullyQualifiedName doesn't take into consideration conflict resolution
public static class PhonyMockito {
public static <T> T mock(Class<T> clazz) {
return null;
}
}
public static class PhonyPowerMockito {
public static <T> T mock(Class<T> clazz) {
return null;
}
}
}

32
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/imports/xml/UnnecessaryFullyQualifiedName.xml
View File

@ -323,14 +323,44 @@ public class InitialPageComponent extends BaseExecutionCourseComponent {
<description>#1436 UnnecessaryFullyQualifiedName false positive on clashing static imports with enums</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import net.sourceforge.pmd.lang.java.rule.imports.ImportsRulesTest.ENUM2;
import static net.sourceforge.pmd.lang.java.rule.imports.ImportsRulesTest.ENUM1.*;
import static net.sourceforge.pmd.lang.java.rule.imports.ImportsRulesTest.ENUM2.*;
public class UnnecessaryFullyQualifiedName {
public static void main(String[] args) {
System.out.println(ENUM1.values());
System.out.println(A);
System.out.println(D);
System.out.println(ENUM2.values());
}
}
]]></code>
</test-code>
<test-code>
<description>#1546 part 1 UnnecessaryFullyQualifiedName doesn't take into consideration conflict resolution</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import net.sourceforge.pmd.lang.java.rule.imports.ImportsRulesTest.PhonyMockito;
import static net.sourceforge.pmd.lang.java.rule.imports.ImportsRulesTest.PhonyMockito.*;
import static net.sourceforge.pmd.lang.java.rule.imports.ImportsRulesTest.PhonyPowerMockito.*;
public class Foo {
private Bar bar = Mockito.mock(Bar.class); // doing simply mock(Bar.class) is ambiguous (compile error)
}
]]></code>
</test-code>
<test-code>
<description>#1546 part 2 UnnecessaryFullyQualifiedName doesn't take into consideration conflict resolution</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import net.sourceforge.pmd.lang.java.rule.imports.ImportsRulesTest.PhonyMockito;
import static net.sourceforge.pmd.lang.java.rule.imports.ImportsRulesTest.PhonyMockito.*;
import static net.sourceforge.pmd.lang.java.rule.imports.ImportsRulesTest.PhonyPowerMockito.mock;
public class Foo {
private Bar bar = Mockito.mock(Bar.class); // doing simply mock(Bar.class) would use a differen method than intended
}
]]></code>
</test-code>

3
src/site/markdown/overview/changelog.md
View File

@ -37,6 +37,7 @@
* [#129](https://github.com/pmd/pmd/pull/129): \[plsql] Added correct parse of IS [NOT] NULL and multiline DML
* [#130](https://github.com/pmd/pmd/pull/130); \[core] Reduce thread contention
* [#131](https://github.com/pmd/pmd/pull/131): \[core] Make RuleSetFactory immutable
* [#133](https://github.com/pmd/pmd/pull/133): \[java] UnnecessaryFullyQualifiedName can detect conflicts
* [#135](https://github.com/pmd/pmd/pull/135): \[apex] New ruleset for Apex security
**Bugfixes:**
@ -45,6 +46,8 @@
* [#1542](https://sourceforge.net/p/pmd/bugs/1542/): \[java] CPD throws an NPE when parsing enums with -ignore-identifiers
* apex-apexunit
* [#1543](https://sourceforge.net/p/pmd/bugs/1543/): \[apex] ApexUnitTestClassShouldHaveAsserts assumes APEX is case sensitive
* java-imports
* [#1546](https://sourceforge.net/p/pmd/bugs/1546/): \[java] UnnecessaryFullyQualifiedNameRule doesn't take into consideration conflict resolution
* XML
* [#1518](https://sourceforge.net/p/pmd/bugs/1518/): \[xml] Error while processing xml file with ".webapp" in the file or directory name
* psql