From b22a94c4d49e1c4af5ba7bd3e4f53568557e0101 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sun, 29 Jul 2018 10:04:10 +0200 Subject: [PATCH 1/2] [java] UnnecessaryFullyQualifiedName false positive: static method on shadowed implicitly imported class Fixes #1255 --- docs/pages/release_notes.md | 2 + .../UnnecessaryFullyQualifiedNameRule.java | 63 +++++++++++++++++-- .../xml/UnnecessaryFullyQualifiedName.xml | 14 +++++ 3 files changed, 75 insertions(+), 4 deletions(-) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index fa5dea7182..42c1545499 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -28,6 +28,8 @@ This is a minor release. ### Fixed Issues +* java-codestyle + * [#1255](https://github.com/pmd/pmd/issues/1255): \[java] UnnecessaryFullyQualifiedName false positive: static method on shadowed implicitly imported class * java-errorprone * [#1078](https://github.com/pmd/pmd/issues/1078): \[java] MissingSerialVersionUID rule does not seem to catch inherited classes * plsql diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryFullyQualifiedNameRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryFullyQualifiedNameRule.java index 37c2b06d9d..5b6d35e4e1 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryFullyQualifiedNameRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryFullyQualifiedNameRule.java @@ -17,7 +17,11 @@ import net.sourceforge.pmd.lang.java.ast.ASTCompilationUnit; import net.sourceforge.pmd.lang.java.ast.ASTImportDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTName; import net.sourceforge.pmd.lang.java.ast.ASTPackageDeclaration; +import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression; +import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix; +import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix; import net.sourceforge.pmd.lang.java.ast.AbstractJavaTypeNode; +import net.sourceforge.pmd.lang.java.ast.JavaNode; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; import net.sourceforge.pmd.lang.java.symboltable.SourceFileScope; @@ -79,6 +83,16 @@ public class UnnecessaryFullyQualifiedNameRule extends AbstractJavaRule { && name.lastIndexOf('.') == decl.getImportedName().length(); } + private boolean couldBeMethodCall(JavaNode node) { + if (node.getNthParent(2) instanceof ASTPrimaryExpression && node.getNthParent(1) instanceof ASTPrimaryPrefix) { + int nextSibling = node.jjtGetParent().jjtGetChildIndex() + 1; + if (node.getNthParent(2).jjtGetNumChildren() > nextSibling) { + return node.getNthParent(2).jjtGetChild(nextSibling) instanceof ASTPrimarySuffix; + } + } + return false; + } + private void checkImports(AbstractJavaTypeNode node, Object data) { String name = node.getImage(); List matches = new ArrayList<>(); @@ -127,7 +141,7 @@ public class UnnecessaryFullyQualifiedNameRule extends AbstractJavaRule { matches.add(importDeclaration); } } else { - // Last 2 parts match? + // Last 2 parts match? Class + Method name if (nameParts[nameParts.length - 1].equals(importParts[importParts.length - 1]) && nameParts[nameParts.length - 2].equals(importParts[importParts.length - 2])) { matches.add(importDeclaration); @@ -137,6 +151,10 @@ public class UnnecessaryFullyQualifiedNameRule extends AbstractJavaRule { // last part matches? if (nameParts[nameParts.length - 1].equals(importParts[importParts.length - 1])) { matches.add(importDeclaration); + } else if (couldBeMethodCall(node) + && nameParts.length > 1 && nameParts[nameParts.length - 2].equals(importParts[importParts.length - 1])) { + // maybe the Name is part of a method call, then the second two last part needs to match + matches.add(importDeclaration); } } } @@ -147,7 +165,7 @@ public class UnnecessaryFullyQualifiedNameRule extends AbstractJavaRule { } if (!matches.isEmpty()) { - ASTImportDeclaration firstMatch = matches.get(0); + ASTImportDeclaration firstMatch = findFirstMatch(matches); // Could this done to avoid a conflict? if (!isAvoidingConflict(node, name, firstMatch)) { @@ -159,6 +177,29 @@ public class UnnecessaryFullyQualifiedNameRule extends AbstractJavaRule { } } + private ASTImportDeclaration findFirstMatch(List imports) { + // first search only static imports + ASTImportDeclaration result = null; + for (ASTImportDeclaration importDeclaration : imports) { + if (importDeclaration.isStatic()) { + result = importDeclaration; + break; + } + } + + // then search all non-static, if needed + if (result == null) { + for (ASTImportDeclaration importDeclaration : imports) { + if (!importDeclaration.isStatic()) { + result = importDeclaration; + break; + } + } + } + + return result; + } + private boolean isJavaLangImplicit(AbstractJavaTypeNode node) { String name = node.getImage(); boolean isJavaLang = name != null && name.startsWith("java.lang."); @@ -231,15 +272,29 @@ public class UnnecessaryFullyQualifiedNameRule extends AbstractJavaRule { } // There could be a conflict between an import of a class with the same name as the FQN + String importName = firstMatch.getImportedName(); + String importUnqualified = importName.substring(importName.lastIndexOf('.') + 1); if (!firstMatch.isImportOnDemand() && !firstMatch.isStatic()) { - String importName = firstMatch.getImportedName(); - String importUnqualified = importName.substring(importName.lastIndexOf('.') + 1); // the package is different, but the unqualified name is same if (!firstMatch.getImportedName().equals(name) && importUnqualified.equals(unqualifiedName)) { return true; } } + // There could be a conflict between an import of a class with the same name as the FQN, which + // could be a method call: + // import x.y.Thread; + // valid qualification (node): java.util.Thread.currentThread() + if (couldBeMethodCall(node)) { + String[] nameParts = name.split("\\."); + String fqnName = name.substring(0, name.lastIndexOf('.')); + // seems to be a static method call on a different FQN + if (!fqnName.equals(importName) && !firstMatch.isStatic() && !firstMatch.isImportOnDemand() + && nameParts.length > 1 && nameParts[nameParts.length - 2].equals(importUnqualified)) { + return true; + } + } + // Is it a conflict with a class in the same file? final Set qualifiedTypes = node.getScope().getEnclosingScope(SourceFileScope.class) .getQualifiedTypeNames().keySet(); diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFullyQualifiedName.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFullyQualifiedName.xml index 4077e9beeb..7df5ae73fe 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFullyQualifiedName.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFullyQualifiedName.xml @@ -458,4 +458,18 @@ public class JavaLang { } ]]> + + + #1255 [java] UnnecessaryFullyQualifiedName false positive: static method on shadowed implicitly imported class + 0 + + From 23f57b5819d22a335d776fa825af08a03c514f26 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Mon, 30 Jul 2018 21:40:25 +0200 Subject: [PATCH 2/2] Fix NPE for array types --- .../UnnecessaryFullyQualifiedNameRule.java | 5 +++-- .../xml/UnnecessaryFullyQualifiedName.xml | 16 ++++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryFullyQualifiedNameRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryFullyQualifiedNameRule.java index 5b6d35e4e1..ad12b080e8 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryFullyQualifiedNameRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryFullyQualifiedNameRule.java @@ -204,9 +204,10 @@ public class UnnecessaryFullyQualifiedNameRule extends AbstractJavaRule { String name = node.getImage(); boolean isJavaLang = name != null && name.startsWith("java.lang."); - if (isJavaLang && node.getType() != null) { + if (isJavaLang && node.getType() != null && node.getType().getPackage() != null) { // valid would be ProcessBuilder.Redirect.PIPE but not java.lang.ProcessBuilder.Redirect.PIPE - String packageName = node.getType().getPackage().getName(); + String packageName = node.getType().getPackage() // package might be null, if type is an array type... + .getName(); return "java.lang".equals(packageName); } else if (isJavaLang) { // only java.lang.* is implicitly imported, but not e.g. java.lang.reflection.* diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFullyQualifiedName.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFullyQualifiedName.xml index 7df5ae73fe..3bc1edd25f 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFullyQualifiedName.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryFullyQualifiedName.xml @@ -469,6 +469,22 @@ public class ThreadStuff { public Thread stuff() { return new Thread(java.lang.Thread.currentThread()); } +} + ]]> + + + + Nullpointer in isJavaLangImplicit for java.lang.String[] arrays + 1 + 6 +