Corrected class name validation to no longer fail for Kotlin classes …

…on class path containing special characters (#1884) * Added test cases to demonstrate incorrect name validation. * Improved class name verification to match JVM specification. Also added logic to allow existing cases where a field descriptor or dotted class name is provided instead of a slashed (binary) class name. * Updated CHANGELOG.md. * Implemented code review feedback. * Documented private methods in class name validity checker with links to JVM spec. * Cleaned up array type checking code. * Added test to ensure that empty class names are rejected. * Fixing incorrect exception documentation in ClassName#isValidArrayFieldDescriptor.
spotbugs · Dec 22, 2021 · 1345756 · 1345756
1 parent 569a383
commit 1345756
Show file tree

Hide file tree

Showing 3 changed files with 156 additions and 2 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 
 ### Fixed
 - Remove duplicated logging frameworks from the Eclipse plugin distribution ([#1868](https://github.com/spotbugs/spotbugs/issues/1868))
+- Corrected class name validation to no longer fail for Kotlin classes on class path containing special characters. ([#1883](https://github.com/spotbugs/spotbugs/issues/1883))
 
 ## 4.5.2 - 2021-12-13
 ### Security

diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/util/ClassName.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/util/ClassName.java
@@ -175,9 +175,65 @@ public static String extractSimpleName(@DottedClassName String className) {
      * @return true if it's a valid class name, false otherwise
      */
     public static boolean isValidClassName(String className) {
-        // FIXME: should use a regex
+        return !className.isEmpty() &&
+                (isValidBinaryClassName(className) ||
+                        isValidDottedClassName(className) ||
+                        isValidArrayFieldDescriptor(className) ||
+                        isValidClassFieldDescriptor(className));
+    }
+
+    /**
+     * @see <a href="https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.2.1">
+     *     JVMS (Java SE 8 Edition) 4.2.1. Binary Class and Interface Names
+     *     </a>
+     */
+    private static boolean isValidBinaryClassName(String className) {
+        return className.indexOf('.') == -1 &&
+                className.indexOf('[') == -1 &&
+                className.indexOf(';') == -1;
+    }
+
+    private static boolean isValidDottedClassName(String className) {
+        return className.indexOf('/') == -1 &&
+                className.indexOf('[') == -1 &&
+                className.indexOf(';') == -1;
+    }
+
+    /**
+     * Determines whether a class name is a valid array field descriptor as per
+     * <a href="https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.3.2">
+     * JVMS (Java SE 8 Edition) 4.3.2</a>
+     *
+     * @param className a class name to test for validity - must be non-{@code null} and non-empty.
+     * @return {@code true} if {@code className} is a valid array field descriptor as
+     *          per JVMS 4.3.2, otherwise {@code false}
+     * @throws IndexOutOfBoundsException if {@code className} is empty.
+     * @throws NullPointerException if {@code className} is {@code null}.
+     */
+    private static boolean isValidArrayFieldDescriptor(String className) {
+        String tail = className.substring(1);
+        return className.startsWith("[") &&
+                (isValidArrayFieldDescriptor(tail) ||
+                        isValidClassFieldDescriptor(tail) ||
+                        isValidBaseTypeFieldDescriptor(tail));
+
+    }
+
+    private static boolean isValidClassFieldDescriptor(String className) {
+        return className.startsWith("L") &&
+                className.endsWith(";") &&
+                isValidBinaryClassName(className.substring(1, className.length() - 1));
+    }
 
-        return className.indexOf('(') < 0;
+    private static boolean isValidBaseTypeFieldDescriptor(String className) {
+        return "B".equals(className) ||
+                "C".equals(className) ||
+                "D".equals(className) ||
+                "F".equals(className) ||
+                "I".equals(className) ||
+                "J".equals(className) ||
+                "S".equals(className) ||
+                "Z".equals(className);
     }
 
     /**

diff --git a/spotbugs/src/test/java/edu/umd/cs/findbugs/util/ClassNameTest.java b/spotbugs/src/test/java/edu/umd/cs/findbugs/util/ClassNameTest.java
@@ -95,4 +95,101 @@ public void testMatchedPrefix() {
                     ClassName.matchedPrefixes(searchString, "com.test.TestClass"));
         }
     }
+
+    @Test
+    public void testSimpleBinaryClassNameIsValidClassName() {
+        assertTrue(ClassName.isValidClassName("com/bla/Parent"));
+    }
+
+    @Test
+    public void testSimpleDottedClassNameIsValidClassName() {
+        assertTrue(ClassName.isValidClassName("com.bla.Parent"));
+    }
+
+    @Test
+    public void testInnerClassBinaryClassNameIsValidClassName() {
+        assertTrue(ClassName.isValidClassName("com/bla/Parent$Child"));
+    }
+
+    @Test
+    public void testInnerClassDottedClassNameIsValidClassName() {
+        assertTrue(ClassName.isValidClassName("com.bla.Parent$Child"));
+    }
+
+    @Test
+    public void testJavaStyleAnonymousInnerClassBinaryClassNameIsValidClassName() {
+        assertTrue(ClassName.isValidClassName("com/bla/Parent$Child$1"));
+    }
+
+    @Test
+    public void testJavaStyleAnonymousInnerClassDottedClassNameIsValidClassName() {
+        assertTrue(ClassName.isValidClassName("com.bla.Parent$Child$1"));
+    }
+
+    @Test
+    public void testKotlinStyleAnonymousInnerClassBinaryClassNameIsValidClassName() {
+        assertTrue(ClassName.isValidClassName("com/bla/Parent$function$variable$1"));
+    }
+
+    @Test
+    public void testKotlinStyleAnonymousInnerClassDottedClassNameIsValidClassName() {
+        assertTrue(ClassName.isValidClassName("com.bla.Parent$function$variable$1"));
+    }
+
+    @Test
+    public void testBinaryClassNameContainingAllowedSpecialCharactersIsValidClassName() {
+        assertTrue(ClassName.isValidClassName("com/bla/Parent$function!@#%^&*,?{}]()$variable$1"));
+    }
+
+    @Test
+    public void testDottedClassNameContainingAllowedSpecialCharactersIsValidClassName() {
+        assertTrue(ClassName.isValidClassName("com.bla.Parent$function!@#%^&*,?{}]()$variable$1"));
+    }
+
+    @Test
+    public void testFieldDescriptorClassNameIsValidClassName() {
+        assertTrue(ClassName.isValidClassName("Lcom/bla/Parent;"));
+    }
+
+    @Test
+    public void testFieldDescriptorOneDimensionalArrayClassNameIsValidClassName() {
+        assertTrue(ClassName.isValidClassName("[Lcom/bla/Parent;"));
+    }
+
+    @Test
+    public void testFieldDescriptorOneDimensionalPrimitiveArrayClassNameIsValidClassName() {
+        assertTrue(ClassName.isValidClassName("[I"));
+    }
+
+    @Test
+    public void testFieldDescriptorTwoDimensionalArrayClassNameIsValidClassName() {
+        assertTrue(ClassName.isValidClassName("[[Lcom/bla/Parent;"));
+    }
+
+    @Test
+    public void testFieldDescriptorClassNameContainingAllowedSpecialCharactersIsValidClassName() {
+        assertTrue(ClassName.isValidClassName("[[Lcom/bla/Parent$function!@#%^&*,?{}]()$variable$1;"));
+    }
+
+    @Test
+    public void testFieldDescriptorClassNameContainingDotsIsInvalidClassName() {
+        assertFalse(ClassName.isValidClassName("Lcom.bla.Parent;"));
+    }
+
+    @Test
+    public void testClassNameContainingBothSlashesAndDotsIsInvalidClassName() {
+        assertFalse(ClassName.isValidClassName("com.bla/Parent"));
+    }
+
+    @Test
+    public void testBinaryClassNameContainingDisallowedSpecialCharactersIsInvalidClassName() {
+        assertFalse(ClassName.isValidClassName("com/bla/Parent$function$variable$1;"));
+        assertFalse(ClassName.isValidClassName("com/bla/Parent$function;$variable$1"));
+        assertFalse(ClassName.isValidClassName("com/bla/Parent$function[$variable$1"));
+    }
+
+    @Test
+    public void testEmptyStringIsInvalidClassName() {
+        assertFalse(ClassName.isValidClassName(("")));
+    }
 }