Review: Improve simple name handling -> We should pull whatever infor…

…mation is available from the bytecode. That way there are no ambiguities for classes containing a '$' within their simple name.
TNG · Aug 29, 2019 · 525be2a · 525be2a
1 parent 1d101c8
commit 525be2a
Show file tree

Hide file tree

Showing 6 changed files with 157 additions and 26 deletions.
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/importer/ClassFileImporterTest.java b/archunit/src/test/java/com/tngtech/archunit/core/importer/ClassFileImporterTest.java
@@ -21,7 +21,6 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
-import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -35,6 +34,7 @@
 import com.google.common.collect.Sets;
 import com.tngtech.archunit.ArchConfiguration;
 import com.tngtech.archunit.base.DescribedPredicate;
+import com.tngtech.archunit.base.ForwardingCollection;
 import com.tngtech.archunit.base.Optional;
 import com.tngtech.archunit.core.domain.AccessTarget.ConstructorCallTarget;
 import com.tngtech.archunit.core.domain.AccessTarget.FieldAccessTarget;
@@ -168,6 +168,7 @@
 import com.tngtech.archunit.core.importer.testexamples.simpleimport.ClassToImportTwo;
 import com.tngtech.archunit.core.importer.testexamples.simpleimport.EnumToImport;
 import com.tngtech.archunit.core.importer.testexamples.simpleimport.InterfaceToImport;
+import com.tngtech.archunit.core.importer.testexamples.simplenames.SimpleNameExamples;
 import com.tngtech.archunit.core.importer.testexamples.specialtargets.ClassCallingSpecialTarget;
 import com.tngtech.archunit.testutil.LogTestRule;
 import com.tngtech.archunit.testutil.OutsideOfClassPathRule;
@@ -266,6 +267,7 @@ public void imports_simple_class_details() throws Exception {
         assertThat(javaClass.isEnum()).as("is enum").isFalse();
         assertThat(javaClass.getEnclosingClass()).as("enclosing class").isAbsent();
         assertThat(javaClass.isInnerClass()).as("is inner class").isFalse();
+        assertThat(javaClass.isAnonymous()).as("is anonymous class").isFalse();
 
         assertThat(classes.get(ClassToImportTwo.class).getModifiers()).containsOnly(JavaModifier.PUBLIC, JavaModifier.FINAL);
     }
@@ -285,6 +287,52 @@ public void imports_simple_enum() throws Exception {
         assertThat(javaClass.isEnum()).as("is enum").isTrue();
     }
 
+    @Test
+    public void imports_simple_inner_class() throws Exception {
+        ImportedClasses classes = classesIn("testexamples/innerclassimport");
+        JavaClass innerClass = classes.get(ClassWithInnerClass.Inner.class);
+
+        assertThat(innerClass).matches(ClassWithInnerClass.Inner.class);
+        assertThat(innerClass.isInnerClass()).as("is inner class").isTrue();
+        assertThat(innerClass.isAnonymous()).as("is anonymous class").isFalse();
+    }
+
+    @Test
+    public void imports_simple_anonymous_class() throws Exception {
+        ImportedClasses classes = classesIn("testexamples/innerclassimport");
+        JavaClass anonymousClass = classes.get(ClassWithInnerClass.class.getName() + "$1");
+
+        assertThat(anonymousClass).matches(Class.forName(anonymousClass.getName()));
+        assertThat(anonymousClass.isInnerClass()).as("is inner class").isTrue();
+        assertThat(anonymousClass.isAnonymous()).as("class is anonymous").isTrue();
+    }
+
+    @Test
+    public void imports_simple_local_class() throws Exception {
+        ImportedClasses classes = classesIn("testexamples/innerclassimport");
+        JavaClass localClass = classes.get(ClassWithInnerClass.class.getName() + "$1LocalCaller");
+
+        assertThat(localClass).matches(Class.forName(localClass.getName()));
+        assertThat(localClass.isInnerClass()).as("is inner class").isTrue();
+        assertThat(localClass.isAnonymous()).as("class is anonymous").isFalse();
+    }
+
+    @Test
+    public void imports_simple_class_names_of_generated_types_correctly() throws Exception {
+        ImportedClasses classes = classesIn("testexamples/simplenames");
+
+        assertSameSimpleNameOfArchUnitAndReflection(classes, SimpleNameExamples.class);
+        assertSameSimpleNameOfArchUnitAndReflection(classes, SimpleNameExamples.Crazy$InnerClass$$LikeAByteCodeGenerator_might_create.class);
+        assertSameSimpleNameOfArchUnitAndReflection(classes, SimpleNameExamples.class.getName() + "$1");
+        assertSameSimpleNameOfArchUnitAndReflection(classes, SimpleNameExamples.class.getName() + "$1Crazy$LocalClass");
+        assertSameSimpleNameOfArchUnitAndReflection(classes,
+                SimpleNameExamples.Crazy$InnerClass$$LikeAByteCodeGenerator_might_create.NestedInnerClass$Also$$_crazy.class);
+        assertSameSimpleNameOfArchUnitAndReflection(classes,
+                SimpleNameExamples.Crazy$InnerClass$$LikeAByteCodeGenerator_might_create.class.getName() + "$1");
+        assertSameSimpleNameOfArchUnitAndReflection(classes,
+                SimpleNameExamples.Crazy$InnerClass$$LikeAByteCodeGenerator_might_create.class.getName() + "$1Crazy$$NestedLocalClass");
+    }
+
     @Test
     public void imports_interfaces() throws Exception {
         JavaClass simpleInterface = classesIn("testexamples/simpleimport").get(InterfaceToImport.class);
@@ -994,26 +1042,21 @@ public void imports_enclosing_classes() throws Exception {
         JavaClass classWithInnerClass = classes.get(ClassWithInnerClass.class);
         JavaClass innerClass = classes.get(ClassWithInnerClass.Inner.class);
         JavaClass anonymousClass = classes.get(ClassWithInnerClass.class.getName() + "$1");
+        JavaClass localClass = classes.get(ClassWithInnerClass.class.getName() + "$1LocalCaller");
         JavaMethod calledTarget = getOnlyElement(classes.get(CalledClass.class).getMethods());
 
-        assertThat(classWithInnerClass.isAnonymous()).as("class is anonymous").isFalse();
-        assertThat(innerClass.isAnonymous()).as("class is anonymous").isFalse();
-
-        assertThat(innerClass.isInnerClass()).isTrue();
         assertThat(innerClass.getEnclosingClass()).contains(classWithInnerClass);
-        assertThat(innerClass).matches(ClassWithInnerClass.Inner.class);
         assertThat(anonymousClass.getEnclosingClass()).contains(classWithInnerClass);
-        assertThat(anonymousClass.getName()).isEqualTo(ClassWithInnerClass.class.getName() + "$1");
-        assertThat(anonymousClass.isAnonymous()).as("class is anonymous").isTrue();
-        assertThat(anonymousClass.getSimpleName()).as("simple name").isEmpty();
-        assertThat(anonymousClass.getPackageName()).as("package name").isEqualTo(ClassWithInnerClass.class.getPackage().getName());
+        assertThat(localClass.getEnclosingClass()).contains(classWithInnerClass);
 
         JavaMethodCall call = getOnlyElement(innerClass.getCodeUnitWithParameterTypes("call").getMethodCallsFromSelf());
+        assertThatCall(call).isFrom("call").isTo(calledTarget).inLineNumber(31);
 
-        assertThatCall(call).isFrom("call").isTo(calledTarget).inLineNumber(20);
         call = getOnlyElement(anonymousClass.getCodeUnitWithParameterTypes("call").getMethodCallsFromSelf());
+        assertThatCall(call).isFrom("call").isTo(calledTarget).inLineNumber(11);
 
-        assertThatCall(call).isFrom("call").isTo(calledTarget).inLineNumber(10);
+        call = getOnlyElement(localClass.getCodeUnitWithParameterTypes("call").getMethodCallsFromSelf());
+        assertThatCall(call).isFrom("call").isTo(calledTarget).inLineNumber(21);
     }
 
     @Test
@@ -1961,6 +2004,14 @@ public void is_resilient_against_broken_ClassFileSources() throws MalformedURLEx
         assertThat(classes).isEmpty();
     }
 
+    private void assertSameSimpleNameOfArchUnitAndReflection(ImportedClasses classes, String className) throws ClassNotFoundException {
+        assertSameSimpleNameOfArchUnitAndReflection(classes, Class.forName(className));
+    }
+
+    private void assertSameSimpleNameOfArchUnitAndReflection(ImportedClasses classes, Class<?> clazz) {
+        assertThat(classes.get(clazz.getName()).getSimpleName()).isEqualTo(clazz.getSimpleName());
+    }
+
     private Set<Dependency> withoutJavaLangTargets(Set<Dependency> dependencies) {
         Set<Dependency> result = new HashSet<>();
         for (Dependency dependency : dependencies) {
@@ -2149,7 +2200,7 @@ private ImportedClasses classesIn(String path) throws Exception {
         return new ImportedClasses(path);
     }
 
-    private class ImportedClasses implements Iterable<JavaClass> {
+    private class ImportedClasses extends ForwardingCollection<JavaClass> {
         private final ClassFileImporter importer = new ClassFileImporter();
         private final JavaClasses classes;
 
@@ -2166,8 +2217,8 @@ private JavaClass get(String className) {
         }
 
         @Override
-        public Iterator<JavaClass> iterator() {
-            return classes.iterator();
+        protected Collection<JavaClass> delegate() {
+            return classes;
         }
 
         Set<JavaCodeUnit> getCodeUnits() {

diff --git a/...com/tngtech/archunit/core/importer/testexamples/innerclassimport/ClassWithInnerClass.java b/...com/tngtech/archunit/core/importer/testexamples/innerclassimport/ClassWithInnerClass.java
@@ -1,5 +1,6 @@
 package com.tngtech.archunit.core.importer.testexamples.innerclassimport;
 
+@SuppressWarnings("unused")
 public class ClassWithInnerClass {
     void callInsideOfAnonymous() {
         final CalledClass calledClass = null;
@@ -12,7 +13,17 @@ public void call() {
         };
     }
 
-    public class Inner implements CanBeCalled {
+    void callInsideOfLocalClass() {
+        final CalledClass calledClass = null;
+
+        class LocalCaller {
+            void call() {
+                calledClass.doIt();
+            }
+        }
+    }
+
+    public static class Inner implements CanBeCalled {
         private CalledClass calledClass;
 
         @Override

diff --git a/.../java/com/tngtech/archunit/core/importer/testexamples/simplenames/SimpleNameExamples.java b/.../java/com/tngtech/archunit/core/importer/testexamples/simplenames/SimpleNameExamples.java
@@ -0,0 +1,27 @@
+package com.tngtech.archunit.core.importer.testexamples.simplenames;
+
+import java.io.Serializable;
+
+@SuppressWarnings("unused")
+public class SimpleNameExamples {
+    void simpleNames() {
+        class Crazy$LocalClass {
+        }
+
+        Serializable anonymousClass = new Serializable() {
+        };
+    }
+
+    public static class Crazy$InnerClass$$LikeAByteCodeGenerator_might_create {
+        void simpleNames() {
+            class Crazy$$NestedLocalClass {
+            }
+
+            Serializable nestedAnonymousClass = new Serializable() {
+            };
+        }
+
+        public class NestedInnerClass$Also$$_crazy {
+        }
+    }
+}