Merge pull request #217 from rweisleder/gh-216

Ensure correct simple name for local classes
TNG · Aug 31, 2019 · 84e8e7c · 84e8e7c
2 parents 00a1c79 + 525be2a
commit 84e8e7c
Show file tree

Hide file tree

Showing 10 changed files with 217 additions and 43 deletions.
diff --git a/archunit/src/main/java/com/tngtech/archunit/core/domain/Formatters.java b/archunit/src/main/java/com/tngtech/archunit/core/domain/Formatters.java
@@ -19,15 +19,12 @@
 import java.util.List;
 
 import com.google.common.base.Joiner;
-import com.google.common.primitives.Ints;
 import com.tngtech.archunit.PublicAPI;
 import com.tngtech.archunit.core.domain.properties.HasName;
 
 import static com.tngtech.archunit.PublicAPI.Usage.ACCESS;
 
 public final class Formatters {
-    private static final String LOCATION_TEMPLATE = "(%s:%d)";
-
     private Formatters() {
     }
 
@@ -99,27 +96,30 @@ public static String formatThrowsDeclarationTypeNames(List<String> typeNames) {
         return Joiner.on(", ").join(typeNames);
     }
 
+    // Excluding the '$' character might be incorrect, but since '$' is a valid character of a class name
+    // and also the delimiter within the fully qualified name between an inner class and its enclosing class,
+    // there is no clean way to derive the simple name from just a fully qualified class name without
+    // further information
+    // Luckily for imported classes we can read this information from the bytecode
     /**
      * @param name A possibly fully qualified class name
      * @return A best guess of the simple name, i.e. prefixes like 'a.b.c.' cut off, 'Some$' of 'Some$Inner' as well.
      * Returns an empty String, if the name belongs to an anonymous class (e.g. some.Type$1).
      */
     @PublicAPI(usage = ACCESS)
     public static String ensureSimpleName(String name) {
-        int innerClassStart = name.lastIndexOf('$');
-        int classStart = name.lastIndexOf('.');
-        if (innerClassStart < 0 && classStart < 0) {
-            return name;
-        }
+        int lastIndexOfDot = name.lastIndexOf('.');
+        String partAfterDot = lastIndexOfDot >= 0 ? name.substring(lastIndexOfDot + 1) : name;
 
-        String lastPart = innerClassStart >= 0 ? name.substring(innerClassStart + 1) : name.substring(classStart + 1);
-        return isAnonymousRest(lastPart) ? "" : lastPart;
-    }
+        int lastIndexOf$ = partAfterDot.lastIndexOf('$');
+        String simpleNameCandidate = lastIndexOf$ >= 0 ? partAfterDot.substring(lastIndexOf$ + 1) : partAfterDot;
 
-    // NOTE: Anonymous classes (e.g. clazz.getName() == some.Type$1) return an empty clazz.getSimpleName(),
-    //       so we mimic this behavior
-    private static boolean isAnonymousRest(String lastPart) {
-        return Ints.tryParse(lastPart) != null;
+        for (int i = 0; i < simpleNameCandidate.length(); i++) {
+            if (Character.isJavaIdentifierStart(simpleNameCandidate.charAt(i))) {
+                return simpleNameCandidate.substring(i);
+            }
+        }
+        return "";
     }
 
     /**

diff --git a/archunit/src/main/java/com/tngtech/archunit/core/domain/JavaClass.java b/archunit/src/main/java/com/tngtech/archunit/core/domain/JavaClass.java
@@ -250,7 +250,7 @@ public <A extends Annotation> A getAnnotationOfType(Class<A> type) {
     @PublicAPI(usage = ACCESS)
     public JavaAnnotation getAnnotationOfType(String typeName) {
         return tryGetAnnotationOfType(typeName).getOrThrow(new IllegalArgumentException(
-                String.format("Type %s is not annotated with @%s", getSimpleName(), Formatters.ensureSimpleName(typeName))));
+                String.format("Type %s is not annotated with @%s", getSimpleName(), typeName)));
     }
 
     @Override

diff --git a/archunit/src/main/java/com/tngtech/archunit/core/domain/JavaMember.java b/archunit/src/main/java/com/tngtech/archunit/core/domain/JavaMember.java
@@ -85,7 +85,7 @@ public <A extends Annotation> A getAnnotationOfType(Class<A> type) {
     public JavaAnnotation getAnnotationOfType(String typeName) {
         return tryGetAnnotationOfType(typeName).getOrThrow(new IllegalArgumentException(String.format(
                 "Member %s is not annotated with @%s",
-                getFullName(), Formatters.ensureSimpleName(typeName))));
+                getFullName(), typeName)));
     }
 
     @Override

diff --git a/archunit/src/main/java/com/tngtech/archunit/core/domain/JavaType.java b/archunit/src/main/java/com/tngtech/archunit/core/domain/JavaType.java
@@ -59,6 +59,8 @@ public interface JavaType {
 
     boolean isArray();
 
+    JavaType withSimpleName(String simpleName);
+
     @Internal
     final class From {
         private static final LoadingCache<String, JavaType> typeCache = CacheBuilder.newBuilder().build(new CacheLoader<String, JavaType>() {
@@ -221,7 +223,16 @@ public String toString() {
 
         private static class ObjectType extends AbstractType {
             ObjectType(String fullName) {
-                super(fullName, ensureSimpleName(fullName), createPackage(fullName));
+                this(fullName, ensureSimpleName(fullName), createPackage(fullName));
+            }
+
+            private ObjectType(String fullName, String simpleName, String packageName) {
+                super(fullName, simpleName, packageName);
+            }
+
+            @Override
+            public JavaType withSimpleName(String simpleName) {
+                return new ObjectType(getName(), simpleName, getPackageName());
             }
         }
 
@@ -245,11 +256,20 @@ Class<?> classForName(ClassLoader classLoader) {
             public boolean isPrimitive() {
                 return true;
             }
+
+            @Override
+            public JavaType withSimpleName(String simpleName) {
+                throw new UnsupportedOperationException("It should never make sense to override the simple type of a primitive");
+            }
         }
 
         private static class ArrayType extends AbstractType {
             ArrayType(String fullName) {
-                super(fullName, createSimpleName(fullName), createPackageOfComponentType(fullName));
+                this(fullName, createSimpleName(fullName), createPackageOfComponentType(fullName));
+            }
+
+            private ArrayType(String fullName, String simpleName, String packageName) {
+                super(fullName, simpleName, packageName);
             }
 
             private static String createPackageOfComponentType(String fullName) {
@@ -270,6 +290,11 @@ public boolean isArray() {
                 return true;
             }
 
+            @Override
+            public JavaType withSimpleName(String simpleName) {
+                return new ArrayType(getName(), simpleName, getPackageName());
+            }
+
             @Override
             public Optional<JavaType> tryGetComponentType() {
                 String canonicalName = getCanonicalName(getName());

diff --git a/archunit/src/main/java/com/tngtech/archunit/core/importer/DomainBuilders.java b/archunit/src/main/java/com/tngtech/archunit/core/importer/DomainBuilders.java
@@ -355,6 +355,11 @@ JavaClassBuilder withModifiers(Set<JavaModifier> modifiers) {
             return this;
         }
 
+        JavaClassBuilder withSimpleName(String simpleName) {
+            this.javaType = javaType.withSimpleName(simpleName);
+            return this;
+        }
+
         JavaClass build() {
             return DomainObjectCreationContext.createJavaClass(this);
         }

diff --git a/archunit/src/main/java/com/tngtech/archunit/core/importer/JavaClassProcessor.java b/archunit/src/main/java/com/tngtech/archunit/core/importer/JavaClassProcessor.java
@@ -58,6 +58,7 @@
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.base.Strings.nullToEmpty;
 import static com.tngtech.archunit.core.domain.JavaConstructor.CONSTRUCTOR_NAME;
 import static com.tngtech.archunit.core.domain.JavaStaticInitializer.STATIC_INITIALIZER_NAME;
 import static com.tngtech.archunit.core.importer.ClassFileProcessor.ASM_API_VERSION;
@@ -144,20 +145,31 @@ public void visitInnerClass(String name, String outerName, String innerName, int
             return;
         }
 
-        if (name != null && outerName != null) {
-            String innerTypeName = createTypeName(name);
-            correctModifiersForNestedClass(innerTypeName, access);
+        String innerTypeName = createTypeName(name);
+        if (!visitingCurrentClass(innerTypeName)) {
+            return;
+        }
+
+        javaClassBuilder.withSimpleName(nullToEmpty(innerName));
+
+        if (isNamedNestedClass(outerName)) {
+            javaClassBuilder.withModifiers(JavaModifier.getModifiersForClass(access));
             declarationHandler.registerEnclosingClass(innerTypeName, createTypeName(outerName));
         }
     }
 
-    // Modifier handling is somewhat counter intuitive for nested classes, even though we 'visit' the nested class
+    // visitInnerClass is called for named inner classes, even if we are currently importing
+    // this class itself (i.e. visit(..) and visitInnerClass(..) are called with the same class name.
+    // visitInnerClass offers some more properties like correct modifiers.
+    // Modifier handling is somewhat counter intuitive for nested named classes, even though we 'visit' the nested class
     // like any outer class in visit(..) before, the modifiers like 'PUBLIC' or 'PRIVATE'
     // are found in the access flags of visitInnerClass(..)
-    private void correctModifiersForNestedClass(String innerTypeName, int access) {
-        if (innerTypeName.equals(className)) {
-            javaClassBuilder.withModifiers(JavaModifier.getModifiersForClass(access));
-        }
+    private boolean visitingCurrentClass(String innerTypeName) {
+        return innerTypeName.equals(className);
+    }
+
+    private boolean isNamedNestedClass(String outerName) {
+        return outerName != null;
     }
 
     @Override

diff --git a/archunit/src/test/java/com/tngtech/archunit/core/domain/FormattersTest.java b/archunit/src/test/java/com/tngtech/archunit/core/domain/FormattersTest.java
@@ -3,15 +3,26 @@
 import java.util.ArrayList;
 
 import com.tngtech.archunit.testutil.ArchConfigurationRule;
+import com.tngtech.java.junit.dataprovider.DataProvider;
+import com.tngtech.java.junit.dataprovider.DataProviderRunner;
+import com.tngtech.java.junit.dataprovider.UseDataProvider;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.runner.RunWith;
 
 import static com.tngtech.archunit.core.domain.Formatters.formatLocation;
 import static com.tngtech.archunit.core.domain.TestUtils.importClassWithContext;
 import static com.tngtech.archunit.testutil.Assertions.assertThat;
+import static com.tngtech.java.junit.dataprovider.DataProviders.$;
+import static com.tngtech.java.junit.dataprovider.DataProviders.$$;
 
+@RunWith(DataProviderRunner.class)
 public class FormattersTest {
+    @Rule
+    public final ExpectedException thrown = ExpectedException.none();
+
     @Rule
     public final ArchConfigurationRule configuration = new ArchConfigurationRule();
 
@@ -43,6 +54,38 @@ private JavaClass getClassWithoutSource() {
         throw new RuntimeException("Could not create any java class without source");
     }
 
+    @Test
+    public void ensureSimpleName_withNullString() {
+        thrown.expect(NullPointerException.class);
+
+        Formatters.ensureSimpleName(null);
+    }
+
+    @DataProvider
+    public static Object[][] simple_name_test_cases() {
+        return $$(
+                $("", ""),
+                $("Dummy", "Dummy"),
+                $("org.example.Dummy", "Dummy"),
+                $("org.example.Dummy$123", ""),
+                $("org.example.Dummy$NestedClass", "NestedClass"),
+                $("org.example.Dummy$NestedClass123", "NestedClass123"),
+                $("org.example.Dummy$NestedClass$123", ""),
+                $("org.example.Dummy$NestedClass$MoreNestedClass", "MoreNestedClass"),
+                $("org.example.Dummy$123LocalClass", "LocalClass"),
+                $("org.example.Dummy$Inner$123LocalClass", "LocalClass"),
+                $("org.example.Dummy$Inner$123LocalClass123", "LocalClass123"),
+                $("Dummy[]", "Dummy[]"),
+                $("org.example.Dummy[]", "Dummy[]"),
+                $("org.example.Dummy$Inner[][]", "Inner[][]"));
+    }
+
+    @Test
+    @UseDataProvider("simple_name_test_cases")
+    public void ensureSimpleName(String input, String expected) {
+        assertThat(Formatters.ensureSimpleName(input)).isEqualTo(expected);
+    }
+
     private static class SomeClass {
         public SomeClass() {
             new ArrayList<>();