From f0d8759b7974569bd554a43827462ea98d89e4ad Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sat, 30 Mar 2013 15:04:47 +0100 Subject: [PATCH] pmd: fix #938 False positive on LooseCoupling for overriding methods --- pmd/etc/changelog.txt | 1 + .../java/rule/coupling/LooseCouplingRule.java | 23 ++++++++++++++++++- .../typeresolution/ClassTypeResolver.java | 8 +++++++ .../typeresolution/rules/LooseCoupling.java | 22 ++++++++++++++++++ .../typeresolution/ClassTypeResolverTest.java | 6 +++++ .../testdata/DefaultJavaLangImport.java | 8 +++++++ .../java/rule/coupling/xml/LooseCoupling.xml | 11 +++++++++ .../rule/typeresolution/xml/LooseCoupling.xml | 13 +++++++++++ 8 files changed, 91 insertions(+), 1 deletion(-) create mode 100644 pmd/src/test/java/net/sourceforge/pmd/typeresolution/testdata/DefaultJavaLangImport.java diff --git a/pmd/etc/changelog.txt b/pmd/etc/changelog.txt index b9d8ee7f3f..ba19b46ce6 100644 --- a/pmd/etc/changelog.txt +++ b/pmd/etc/changelog.txt @@ -15,6 +15,7 @@ New Java rule: ????? ??, 2013 - 5.0.3: +Fixed bug 938: False positive on LooseCoupling for overriding methods Fixed bug 940: False positive on UnsynchronizedStaticDateFormatter Fixed bug 942: CheckResultSet False Positive and Negative Fixed bug 943: PreserveStackTrace false positive if a StringBuffer exists diff --git a/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/coupling/LooseCouplingRule.java b/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/coupling/LooseCouplingRule.java index a11dc2dde7..573e6ae90b 100644 --- a/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/coupling/LooseCouplingRule.java +++ b/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/coupling/LooseCouplingRule.java @@ -4,9 +4,13 @@ package net.sourceforge.pmd.lang.java.rule.coupling; import net.sourceforge.pmd.lang.ast.Node; +import net.sourceforge.pmd.lang.java.ast.ASTAnnotation; +import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceBodyDeclaration; 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.ASTMarkerAnnotation; +import net.sourceforge.pmd.lang.java.ast.ASTName; import net.sourceforge.pmd.lang.java.ast.ASTResultType; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; import net.sourceforge.pmd.util.CollectionUtil; @@ -22,11 +26,28 @@ public class LooseCouplingRule extends AbstractJavaRule { // }); public Object visit(ASTClassOrInterfaceType node, Object data) { - Node parent = node.jjtGetParent().jjtGetParent().jjtGetParent(); + if (methodHasOverride(node)) { + return data; + } + Node parent = node.getNthParent(3); String typeName = node.getImage(); if (CollectionUtil.isCollectionType(typeName, false) && (parent instanceof ASTFieldDeclaration || parent instanceof ASTFormalParameter || parent instanceof ASTResultType)) { addViolation(data, node, typeName); } return data; } + + private boolean methodHasOverride(Node node) { + ASTClassOrInterfaceBodyDeclaration method = node.getFirstParentOfType(ASTClassOrInterfaceBodyDeclaration.class); + if (method.jjtGetNumChildren() > 0 && method.jjtGetChild(0) instanceof ASTAnnotation) { + ASTMarkerAnnotation marker = method.getFirstDescendantOfType(ASTMarkerAnnotation.class); + if (marker != null && marker.getFirstChildOfType(ASTName.class) != null) { + ASTName name = marker.getFirstChildOfType(ASTName.class); + if ("Override".equals(name.getImage())) { + return true; + } + } + } + return false; + } } diff --git a/pmd/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/ClassTypeResolver.java b/pmd/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/ClassTypeResolver.java index 5d3021c904..dde4eb1dbd 100644 --- a/pmd/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/ClassTypeResolver.java +++ b/pmd/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/ClassTypeResolver.java @@ -663,6 +663,14 @@ public class ClassTypeResolver extends JavaParserVisitorAdapter { } } } + if (myType == null && qualifiedName != null && !qualifiedName.contains(".")) { + // try again with java.lang.... + try { + myType = pmdClassLoader.loadClass("java.lang." + qualifiedName); + } catch (Exception e) { + // ignored + } + } if (myType != null) { node.setType(myType); } diff --git a/pmd/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/rules/LooseCoupling.java b/pmd/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/rules/LooseCoupling.java index 9f548c6a24..ac71341846 100644 --- a/pmd/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/rules/LooseCoupling.java +++ b/pmd/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/rules/LooseCoupling.java @@ -4,9 +4,13 @@ package net.sourceforge.pmd.lang.java.typeresolution.rules; import net.sourceforge.pmd.lang.ast.Node; +import net.sourceforge.pmd.lang.java.ast.ASTAnnotation; +import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceBodyDeclaration; 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.ASTMarkerAnnotation; +import net.sourceforge.pmd.lang.java.ast.ASTName; import net.sourceforge.pmd.lang.java.ast.ASTResultType; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; import net.sourceforge.pmd.util.CollectionUtil; @@ -18,6 +22,9 @@ public class LooseCoupling extends AbstractJavaRule { @Override public Object visit(ASTClassOrInterfaceType node, Object data) { + if (methodHasOverride(node)) { + return data; + } Node parent = node.getNthParent(3); Class clazzType = node.getType(); boolean isType = CollectionUtil.isCollectionType(clazzType, false); @@ -27,4 +34,19 @@ public class LooseCoupling extends AbstractJavaRule { } return data; } + + private boolean methodHasOverride(Node node) { + ASTClassOrInterfaceBodyDeclaration method = node.getFirstParentOfType(ASTClassOrInterfaceBodyDeclaration.class); + if (method.jjtGetNumChildren() > 0 && method.jjtGetChild(0) instanceof ASTAnnotation) { + ASTMarkerAnnotation marker = method.getFirstDescendantOfType(ASTMarkerAnnotation.class); + if (marker != null && marker.getFirstChildOfType(ASTName.class) != null) { + ASTName name = marker.getFirstChildOfType(ASTName.class); + System.out.println(name.getType()); + if (name.getType() == Override.class) { + return true; + } + } + } + return false; + } } diff --git a/pmd/src/test/java/net/sourceforge/pmd/typeresolution/ClassTypeResolverTest.java b/pmd/src/test/java/net/sourceforge/pmd/typeresolution/ClassTypeResolverTest.java index ced6e5b4db..867abdf85f 100644 --- a/pmd/src/test/java/net/sourceforge/pmd/typeresolution/ClassTypeResolverTest.java +++ b/pmd/src/test/java/net/sourceforge/pmd/typeresolution/ClassTypeResolverTest.java @@ -20,6 +20,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTFormalParameter; import net.sourceforge.pmd.lang.java.ast.ASTImportDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTLiteral; +import net.sourceforge.pmd.lang.java.ast.ASTName; import net.sourceforge.pmd.lang.java.ast.ASTNullLiteral; import net.sourceforge.pmd.lang.java.ast.ASTReferenceType; import net.sourceforge.pmd.lang.java.ast.ASTStatementExpression; @@ -31,6 +32,7 @@ import net.sourceforge.pmd.lang.java.ast.TypeNode; import net.sourceforge.pmd.lang.java.typeresolution.ClassTypeResolver; import net.sourceforge.pmd.typeresolution.testdata.AnonymousInnerClass; import net.sourceforge.pmd.typeresolution.testdata.ArrayListFound; +import net.sourceforge.pmd.typeresolution.testdata.DefaultJavaLangImport; import net.sourceforge.pmd.typeresolution.testdata.ExtraTopLevelClass; import net.sourceforge.pmd.typeresolution.testdata.InnerClass; import net.sourceforge.pmd.typeresolution.testdata.Literals; @@ -65,6 +67,10 @@ public class ClassTypeResolverTest { assertEquals(ArrayList.class, acu.getFirstDescendantOfType(ASTVariableDeclaratorId.class).getType()); assertEquals(ArrayList.class, acu.getFirstDescendantOfType(ASTVariableDeclarator.class).getType()); assertEquals(ArrayList.class, acu.getFirstDescendantOfType(ASTFieldDeclaration.class).getType()); + + acu = parseAndTypeResolveForClass(DefaultJavaLangImport.class); + assertEquals(String.class, acu.getFirstDescendantOfType(ASTClassOrInterfaceType.class).getType()); + assertEquals(Override.class, acu.findDescendantsOfType(ASTName.class).get(1).getType()); } @Test diff --git a/pmd/src/test/java/net/sourceforge/pmd/typeresolution/testdata/DefaultJavaLangImport.java b/pmd/src/test/java/net/sourceforge/pmd/typeresolution/testdata/DefaultJavaLangImport.java new file mode 100644 index 0000000000..c4db4c590a --- /dev/null +++ b/pmd/src/test/java/net/sourceforge/pmd/typeresolution/testdata/DefaultJavaLangImport.java @@ -0,0 +1,8 @@ +package net.sourceforge.pmd.typeresolution.testdata; + +public class DefaultJavaLangImport { + @Override + public String toString() { + return "foo"; + } +} diff --git a/pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/coupling/xml/LooseCoupling.xml b/pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/coupling/xml/LooseCoupling.xml index e8d00fde9a..a829860faa 100644 --- a/pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/coupling/xml/LooseCoupling.xml +++ b/pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/coupling/xml/LooseCoupling.xml @@ -124,4 +124,15 @@ public class Foo { } ]]> + + + #938 False positive on LooseCoupling for overriding methods + 0 + + diff --git a/pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/typeresolution/xml/LooseCoupling.xml b/pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/typeresolution/xml/LooseCoupling.xml index 819ff5c042..3bbec71205 100644 --- a/pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/typeresolution/xml/LooseCoupling.xml +++ b/pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/typeresolution/xml/LooseCoupling.xml @@ -145,4 +145,17 @@ public class Foo { } ]]> + + + #938 False positive on LooseCoupling for overriding methods + 0 + +