diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 5725e4e1a5..fe9fbfe609 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -37,6 +37,8 @@ Note that XPath 1.0 support, the default XPath version, is deprecated since PMD * apex * [#2210](https://github.com/pmd/pmd/issues/2210): \[apex] ApexCRUDViolation: Support WITH SECURITY_ENFORCED * [#2358](https://github.com/pmd/pmd/issues/2358): \[apex] Invalid Apex in Cognitive Complexity tests +* java + * [#2378](https://github.com/pmd/pmd/issues/2378): \[java] AbstractJUnitRule has bad performance on large code bases * java-design * [#2390](https://github.com/pmd/pmd/issues/2390): \[java] AbstractClassWithoutAnyMethod: missing violation for nested classes diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTCompilationUnit.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTCompilationUnit.java index 5fbd67f90d..d8f2f532a1 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTCompilationUnit.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTCompilationUnit.java @@ -60,6 +60,11 @@ public class ASTCompilationUnit extends AbstractJavaTypeNode implements RootNode return null; } + @Override + public ASTCompilationUnit getRoot() { + return this; + } + /** * Returns the package name of this compilation unit. If this is in * the default package, returns the empty string. diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/AbstractJavaNode.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/AbstractJavaNode.java index 413c5d53b0..92027f5095 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/AbstractJavaNode.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/AbstractJavaNode.java @@ -15,6 +15,7 @@ public abstract class AbstractJavaNode extends AbstractJjtreeNode impl protected JavaParser parser; private Scope scope; private Comment comment; + private ASTCompilationUnit root; @InternalApi @Deprecated @@ -64,6 +65,14 @@ public abstract class AbstractJavaNode extends AbstractJjtreeNode impl return data; } + @Override + public ASTCompilationUnit getRoot() { + if (root == null) { + root = getParent().getRoot(); + } + return root; + } + @Override public Scope getScope() { if (scope == null) { diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/JavaNode.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/JavaNode.java index b8310ea936..443c956810 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/JavaNode.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/JavaNode.java @@ -41,6 +41,7 @@ public interface JavaNode extends ScopedNode { @Deprecated Object childrenAccept(JavaParserVisitor visitor, Object data); + ASTCompilationUnit getRoot(); @Override JavaNode getChild(int index); diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/DuplicateImportsRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/DuplicateImportsRule.java index 655cd8c600..ee26f49b20 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/DuplicateImportsRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/DuplicateImportsRule.java @@ -66,7 +66,7 @@ public class DuplicateImportsRule extends AbstractJavaRule { return true; } } else { - Class importClass = node.getClassTypeResolver().loadClass(thisImportOnDemand.getName()); + Class importClass = node.getClassTypeResolver().loadClassOrNull(thisImportOnDemand.getName()); if (importClass != null) { for (Method m : importClass.getMethods()) { if (Modifier.isStatic(m.getModifiers()) && m.getName().equals(singleTypeName)) { diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/ClassTypeResolver.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/ClassTypeResolver.java index fb57427a60..98a1858c85 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/ClassTypeResolver.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/ClassTypeResolver.java @@ -99,6 +99,7 @@ import net.sourceforge.pmd.lang.java.ast.JavaParserVisitorAdapter; import net.sourceforge.pmd.lang.java.ast.TypeNode; import net.sourceforge.pmd.lang.java.symboltable.ClassScope; import net.sourceforge.pmd.lang.java.symboltable.VariableNameDeclaration; +import net.sourceforge.pmd.lang.java.typeresolution.internal.NullableClassLoader; import net.sourceforge.pmd.lang.java.typeresolution.typedefinition.JavaTypeDefinition; import net.sourceforge.pmd.lang.symboltable.NameOccurrence; import net.sourceforge.pmd.lang.symboltable.Scope; @@ -112,7 +113,7 @@ import net.sourceforge.pmd.lang.symboltable.Scope; @Deprecated @InternalApi -public class ClassTypeResolver extends JavaParserVisitorAdapter { +public class ClassTypeResolver extends JavaParserVisitorAdapter implements NullableClassLoader { private static final Logger LOG = Logger.getLogger(ClassTypeResolver.class.getName()); @@ -1487,10 +1488,15 @@ public class ClassTypeResolver extends JavaParserVisitorAdapter { return pmdClassLoader.loadClassOrNull(fullyQualifiedClassName) != null; } - public Class loadClass(String fullyQualifiedClassName) { + @Override + public Class loadClassOrNull(String fullyQualifiedClassName) { return pmdClassLoader.loadClassOrNull(fullyQualifiedClassName); } + public Class loadClass(String fullyQualifiedClassName) { + return loadClassOrNull(fullyQualifiedClassName); + } + private Class processOnDemand(String qualifiedName) { for (String entry : importedOnDemand) { String fullClassName = entry + "." + qualifiedName; @@ -1533,12 +1539,12 @@ public class ClassTypeResolver extends JavaParserVisitorAdapter { String strPackage = anImportDeclaration.getPackageName(); if (anImportDeclaration.isStatic()) { if (anImportDeclaration.isImportOnDemand()) { - importOnDemandStaticClasses.add(JavaTypeDefinition.forClass(loadClass(strPackage))); + importOnDemandStaticClasses.add(JavaTypeDefinition.forClass(loadClassOrNull(strPackage))); } else { // not import on-demand String strName = anImportDeclaration.getImportedName(); String fieldName = strName.substring(strName.lastIndexOf('.') + 1); - Class staticClassWithField = loadClass(strPackage); + Class staticClassWithField = loadClassOrNull(strPackage); if (staticClassWithField != null) { JavaTypeDefinition typeDef = getFieldType(JavaTypeDefinition.forClass(staticClassWithField), fieldName, currentAcu.getType()); diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/PMDASMClassLoader.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/PMDASMClassLoader.java index 553a8ed672..40cf772f62 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/PMDASMClassLoader.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/PMDASMClassLoader.java @@ -15,6 +15,7 @@ import java.util.concurrent.ConcurrentMap; import org.objectweb.asm.ClassReader; import net.sourceforge.pmd.annotation.InternalApi; +import net.sourceforge.pmd.lang.java.typeresolution.internal.NullableClassLoader; import net.sourceforge.pmd.lang.java.typeresolution.visitors.PMDASMVisitor; /* @@ -36,7 +37,7 @@ import net.sourceforge.pmd.lang.java.typeresolution.visitors.PMDASMVisitor; */ @InternalApi @Deprecated -public final class PMDASMClassLoader extends ClassLoader { +public final class PMDASMClassLoader extends ClassLoader implements NullableClassLoader { private static PMDASMClassLoader cachedPMDASMClassLoader; private static ClassLoader cachedClassLoader; @@ -81,6 +82,7 @@ public final class PMDASMClassLoader extends ClassLoader { * Not throwing CNFEs to represent failure makes a huge performance * difference. Typeres as a whole is 2x faster. */ + @Override public Class loadClassOrNull(String name) { if (dontBother.containsKey(name)) { return null; diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java index aa76317c80..9114a2fe60 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java @@ -4,7 +4,12 @@ package net.sourceforge.pmd.lang.java.typeresolution; -import org.apache.commons.lang3.ClassUtils; +import java.lang.reflect.Array; +import java.util.HashMap; +import java.util.Map; + +import org.apache.commons.lang3.StringUtils; +import org.apache.commons.lang3.Validate; import net.sourceforge.pmd.lang.java.ast.ASTAnnotationTypeDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration; @@ -13,9 +18,27 @@ import net.sourceforge.pmd.lang.java.ast.ASTEnumDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTImplementsList; import net.sourceforge.pmd.lang.java.ast.TypeNode; import net.sourceforge.pmd.lang.java.symboltable.TypedNameDeclaration; +import net.sourceforge.pmd.lang.java.typeresolution.internal.NullableClassLoader; +import net.sourceforge.pmd.lang.java.typeresolution.internal.NullableClassLoader.ClassLoaderWrapper; public final class TypeHelper { + /** Maps names of primitives to their corresponding primitive {@code Class}es. */ + private static final Map> PRIMITIVES_BY_NAME = new HashMap<>(); + + + static { + PRIMITIVES_BY_NAME.put("boolean", Boolean.TYPE); + PRIMITIVES_BY_NAME.put("byte", Byte.TYPE); + PRIMITIVES_BY_NAME.put("char", Character.TYPE); + PRIMITIVES_BY_NAME.put("short", Short.TYPE); + PRIMITIVES_BY_NAME.put("int", Integer.TYPE); + PRIMITIVES_BY_NAME.put("long", Long.TYPE); + PRIMITIVES_BY_NAME.put("double", Double.TYPE); + PRIMITIVES_BY_NAME.put("float", Float.TYPE); + PRIMITIVES_BY_NAME.put("void", Void.TYPE); + } + private TypeHelper() { // utility class } @@ -34,6 +57,10 @@ public final class TypeHelper { * @return true if type node n is of type clazzName or a subtype of clazzName */ public static boolean isA(final TypeNode n, final String clazzName) { + if (n.getType() != null && n.getType().isAnnotation()) { + return isAnnotationSubtype(n.getType(), clazzName); + } + final Class clazz = loadClassWithNodeClassloader(n, clazzName); if (clazz != null || n.getType() != null) { @@ -43,6 +70,20 @@ public final class TypeHelper { return fallbackIsA(n, clazzName); } + /** + * Returns true if the class n is a subtype of clazzName, given n + * is an annotationt type. + */ + private static boolean isAnnotationSubtype(Class n, String clazzName) { + assert n != null && n.isAnnotation() : "Not an annotation type"; + // then, the supertype may only be Object, j.l.Annotation, or the class name + // this avoids classloading altogether + // this is used e.g. by the typeIs function in XPath + return "java.lang.annotation.Annotation".equals(clazzName) + || "java.lang.Object".equals(clazzName) + || clazzName.equals(n.getName()); + } + private static boolean fallbackIsA(TypeNode n, String clazzName) { if (clazzName.equals(n.getImage()) || clazzName.endsWith("." + n.getImage())) { return true; @@ -73,9 +114,11 @@ public final class TypeHelper { return "java.lang.Enum".equals(clazzName) // supertypes of Enum || "java.lang.Comparable".equals(clazzName) - || "java.io.Serializable".equals(clazzName); + || "java.io.Serializable".equals(clazzName) + || "java.lang.Object".equals(clazzName); } else if (n instanceof ASTAnnotationTypeDeclaration) { - return "java.lang.annotation.Annotation".equals(clazzName); + return "java.lang.annotation.Annotation".equals(clazzName) + || "java.lang.Object".equals(clazzName); } return false; @@ -90,6 +133,11 @@ public final class TypeHelper { * @return true if type node n is exactly of type clazzName. */ public static boolean isExactlyA(final TypeNode n, final String clazzName) { + if (n.getType() != null && n.getType().getName().equals(clazzName)) { + // fast path avoiding classloading + return true; + } + final Class clazz = loadClassWithNodeClassloader(n, clazzName); if (clazz != null) { @@ -101,34 +149,87 @@ public final class TypeHelper { private static Class loadClassWithNodeClassloader(final TypeNode n, final String clazzName) { if (n.getType() != null) { - return loadClass(n.getType().getClassLoader(), clazzName); + return loadClass(n.getRoot().getClassTypeResolver(), clazzName); } return null; } - private static Class loadClass(final ClassLoader nullableClassLoader, final String clazzName) { - try { - ClassLoader classLoader = nullableClassLoader; - if (classLoader == null) { - // Using the system classloader then - classLoader = ClassLoader.getSystemClassLoader(); + + /** + * Load a class. Supports loading array types like 'java.lang.String[]' and + * converting a canonical name to a binary name (eg 'java.util.Map.Entry' -> + * 'java.util.Map$Entry'). + */ + // test only + static Class loadClass(NullableClassLoader ctr, String className) { + return loadClassMaybeArray(ctr, StringUtils.deleteWhitespace(className)); + } + + private static Class loadClassFromCanonicalName(NullableClassLoader ctr, String className) { + Class clazz = PRIMITIVES_BY_NAME.get(className); + if (clazz == null) { + clazz = ctr.loadClassOrNull(className); + } + if (clazz != null) { + return clazz; + } + // allow path separators (.) as inner class name separators + final int lastDotIndex = className.lastIndexOf('.'); + + if (lastDotIndex >= 0) { + String asInner = className.substring(0, lastDotIndex) + + '$' + className.substring(lastDotIndex + 1); + return loadClassFromCanonicalName(ctr, asInner); + } + return null; + } + + + private static Class loadClassMaybeArray(NullableClassLoader classLoader, + String className) { + Validate.notNull(className, "className must not be null."); + if (className.endsWith("[]")) { + int dimension = 0; + int i = className.length(); + while (i >= 2 && className.startsWith("[]", i - 2)) { + dimension++; + i -= 2; } - // If the requested type is in the classpath, using the same classloader should work - return ClassUtils.getClass(classLoader, clazzName); - } catch (ClassNotFoundException ignored) { - // The requested type is not on the auxclasspath. This might happen, if the type node - // is probed for a specific type (e.g. is is a JUnit5 Test Annotation class). - // Failing to resolve clazzName does not necessarily indicate an incomplete auxclasspath. - } catch (final LinkageError expected) { - // We found the class but it's invalid / incomplete. This may be an incomplete auxclasspath - // if it was a NoClassDefFoundError. TODO : Report it? + checkJavaIdent(className, i); + String elementName = className.substring(0, i); + + Class elementType = loadClassFromCanonicalName(classLoader, elementName); + if (elementType == null) { + return null; + } + + return Array.newInstance(elementType, (int[]) Array.newInstance(int.class, dimension)).getClass(); + } else { + checkJavaIdent(className, className.length()); + return loadClassFromCanonicalName(classLoader, className); + } + } + + private static IllegalArgumentException invalidClassName(String className) { + return new IllegalArgumentException("Not a valid class name \"" + className + "\""); + } + + private static void checkJavaIdent(String className, int endOffsetExclusive) { + if (endOffsetExclusive <= 0 || !Character.isJavaIdentifierStart(className.charAt(0))) { + throw invalidClassName(className); } - return null; + for (int i = 1; i < endOffsetExclusive; i++) { + char c = className.charAt(i); + if (!(Character.isJavaIdentifierPart(c) || c == '.')) { + throw invalidClassName(className); + } + } } + /** @see #isA(TypeNode, String) */ public static boolean isA(TypeNode n, Class clazz) { return subclasses(n, clazz); @@ -142,7 +243,7 @@ public final class TypeHelper { Class type = vnd.getType(); for (final Class clazz : clazzes) { if (type != null && type.equals(clazz) || type == null - && (clazz.getSimpleName().equals(vnd.getTypeImage()) || clazz.getName().equals(vnd.getTypeImage()))) { + && (clazz.getSimpleName().equals(vnd.getTypeImage()) || clazz.getName().equals(vnd.getTypeImage()))) { return true; } } @@ -192,7 +293,7 @@ public final class TypeHelper { public static boolean isA(TypedNameDeclaration vnd, String className) { Class type = vnd.getType(); if (type != null) { - Class clazz = loadClass(type.getClassLoader(), className); + Class clazz = loadClass(ClassLoaderWrapper.wrapNullable(type.getClassLoader()), className); if (clazz != null) { return clazz.isAssignableFrom(type); } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/internal/NullableClassLoader.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/internal/NullableClassLoader.java new file mode 100644 index 0000000000..b7ba7d8b89 --- /dev/null +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/internal/NullableClassLoader.java @@ -0,0 +1,49 @@ +/* + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.typeresolution.internal; + +import net.sourceforge.pmd.lang.java.typeresolution.PMDASMClassLoader; + +/** + * A classloader that doesn't throw a {@link ClassNotFoundException} + * to report unresolved classes. This is a big performance improvement + * for {@link PMDASMClassLoader}, which caches negative cases. + * + *

See https://github.com/pmd/pmd/pull/2236 + */ +public interface NullableClassLoader { + + /** + * Load a class from its binary name. Returns null if not found. + */ + Class loadClassOrNull(String binaryName); + + + class ClassLoaderWrapper implements NullableClassLoader { + + private final ClassLoader classLoader; + + private ClassLoaderWrapper(ClassLoader classLoader) { + assert classLoader != null : "Null classloader"; + this.classLoader = classLoader; + } + + @Override + public Class loadClassOrNull(String binaryName) { + try { + return classLoader.loadClass(binaryName); + } catch (ClassNotFoundException e) { + return null; + } + } + + public static ClassLoaderWrapper wrapNullable(ClassLoader classLoader) { + if (classLoader == null) { + classLoader = ClassLoader.getSystemClassLoader(); + } + return new ClassLoaderWrapper(classLoader); + } + } +} diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelperTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelperTest.java index c158e669dc..e0a744135d 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelperTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelperTest.java @@ -4,19 +4,30 @@ package net.sourceforge.pmd.lang.java.typeresolution; +import static net.sourceforge.pmd.lang.java.typeresolution.internal.NullableClassLoader.ClassLoaderWrapper.wrapNullable; + import java.io.Serializable; import java.lang.annotation.Annotation; +import java.util.Map; import org.junit.Assert; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import net.sourceforge.pmd.lang.java.ast.ASTAnnotationTypeDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTEnumDeclaration; +import net.sourceforge.pmd.lang.java.ast.TypeNode; import net.sourceforge.pmd.lang.java.symboltable.BaseNonParserTest; +import net.sourceforge.pmd.lang.java.typeresolution.internal.NullableClassLoader; public class TypeHelperTest extends BaseNonParserTest { + private static final NullableClassLoader LOADER = wrapNullable(TypeHelperTest.class.getClassLoader()); + + @Rule + public final ExpectedException expect = ExpectedException.none(); @Test public void testIsAFallback() { @@ -46,11 +57,10 @@ public class TypeHelperTest extends BaseNonParserTest { Assert.assertNull(klass.getType()); Assert.assertTrue(TypeHelper.isA(klass, "org.FooBar")); - Assert.assertTrue(TypeHelper.isA(klass, "java.lang.Iterable")); - Assert.assertTrue(TypeHelper.isA(klass, Iterable.class)); - Assert.assertTrue(TypeHelper.isA(klass, Enum.class)); - Assert.assertTrue(TypeHelper.isA(klass, Serializable.class)); - Assert.assertTrue(TypeHelper.isA(klass, Comparable.class)); + assertIsA(klass, Iterable.class); + assertIsA(klass, Enum.class); + assertIsA(klass, Serializable.class); + assertIsA(klass, Object.class); } @Test @@ -64,8 +74,58 @@ public class TypeHelperTest extends BaseNonParserTest { Assert.assertNull(klass.getType()); Assert.assertTrue(TypeHelper.isA(klass, "org.FooBar")); - Assert.assertTrue(TypeHelper.isA(klass, Annotation.class)); + assertIsA(klass, Annotation.class); + assertIsA(klass, Object.class); + } + + private void assertIsA(TypeNode node, Class type) { + Assert.assertTrue("TypeHelper::isA with class arg: " + type.getCanonicalName(), + TypeHelper.isA(node, type)); + Assert.assertTrue("TypeHelper::isA with string arg: " + type.getCanonicalName(), + TypeHelper.isA(node, type.getCanonicalName())); + } + + private void assertIsExactlyA(TypeNode node, Class type) { + Assert.assertTrue(TypeHelper.isExactlyA(node, type.getCanonicalName())); + assertIsA(node, type); } + @Test + public void testNestedClass() { + Class c = TypeHelper.loadClass(LOADER, "java.util.Map.Entry"); + Assert.assertEquals(Map.Entry.class, c); + } + + + @Test + public void testPrimitiveArray() { + Class c = TypeHelper.loadClass(LOADER, "int[ ]"); + Assert.assertEquals(int[].class, c); + } + + @Test + public void testNestedClassArray() { + Class c = TypeHelper.loadClass(LOADER, "java.util.Map.Entry[ ]"); + Assert.assertEquals(Map.Entry[].class, c); + } + + @Test + public void testInvalidName() { + expect.expect(IllegalArgumentException.class); + TypeHelper.loadClass(LOADER, "java.util.Map ]"); + } + + @Test + public void testInvalidName2() { + expect.expect(IllegalArgumentException.class); + TypeHelper.loadClass(LOADER, "[]"); + } + + @Test + public void testNullName() { + expect.expect(NullPointerException.class); + TypeHelper.loadClass(LOADER, null); + } + }