fix inconsistent behavior with respect to primitive array types

codecholeric · codecholeric · commit db0088f27339 · 2020-04-28T21:34:29.000+02:00
When we tried to resolve the component type of primitive array types (or other types missing from the import), we used `org.objectweb.asm.Type.getType(descriptor)` in some places, which unfortunately uses the canonical name as type name, instead of the class name. Then when the imported classes were checked for the type, e.g. only the type `[B` was present, but not the canonical name `byte[]` and consequently a new type `byte[]` was created that behaved inconsistently and could not return its component type.
The correct handling of this case was actually already handled within `JavaType`, thus I have encapsulated all type parsing into this class and its facade `JavaTypeImporter` and written an architecture test to ensure that in the future only `JavaType` and `JavaTypeImporter` will be used to parse types from type names.
On the way I noticed that `AnnotationTypeConversion` was actually misleading, it looked like a generic class to handle many different cases when in fact for each caller only one specific case was relevant. Thus I have inlined most of the cases where they are actually relevant (e.g. some places only cared to handle a type `Class[]` but did not care about `org.objectweb.asm.Type`, while the concrete conversion only cared about that case and not e.g. `Class`, etc.). Usually only one specific case was relevant and most of the time there was only one caller. The only thing left is the generic `convert(Object value)` method, since it is called from different places and encapsulates that we need to convert `org.objectweb.asm.Type` transparently, since we do not want to deal with this type anymore outside of `JavaType(Importer)`.

Signed-off-by: Peter Gafert &lt;peter.gafert@tngtech.com&gt;
diff --git a/archunit-integration-test/src/test/java/com/tngtech/archunit/ArchUnitArchitectureTest.java b/archunit-integration-test/src/test/java/com/tngtech/archunit/ArchUnitArchitectureTest.java
@@ -13,8 +13,6 @@
 import com.tngtech.archunit.core.domain.JavaClass;
 import com.tngtech.archunit.core.domain.properties.HasOwner;
 import com.tngtech.archunit.core.domain.properties.HasOwner.Predicates.With;
-import com.tngtech.archunit.core.importer.ClassFileImporter;
-import com.tngtech.archunit.core.importer.DomainBuilders;
 import com.tngtech.archunit.core.importer.ImportOption;
 import com.tngtech.archunit.core.importer.Location;
 import com.tngtech.archunit.junit.AnalyzeClasses;
@@ -59,9 +57,7 @@ public class ArchUnitArchitectureTest {
             .whereLayer("Base").mayOnlyBeAccessedByLayers("Root", "Core", "Lang", "Library", "JUnit");
 
     @ArchTest
-    public static final ArchRule domain_does_not_access_importer =
-            noClasses().that().resideInAPackage("..core.domain..")
-                    .should().accessClassesThat(belong_to_the_import_context());
+    public static final ArchRules importer_rules = ArchRules.in(ImporterRules.class);
 
     @ArchTest
     public static final ArchRule types_are_only_resolved_via_reflection_in_allowed_places =
@@ -70,18 +66,7 @@ public class ArchUnitArchitectureTest {
                     .as("no classes should illegally resolve classes via reflection");
 
     @ArchTest
-    public static final ArchRules public_API_rules =
-            ArchRules.in(PublicAPIRules.class);
-
-    private static DescribedPredicate<JavaClass> belong_to_the_import_context() {
-        return new DescribedPredicate<JavaClass>("belong to the import context") {
-            @Override
-            public boolean apply(JavaClass input) {
-                return input.getPackageName().startsWith(ClassFileImporter.class.getPackage().getName())
-                        && !input.getName().contains(DomainBuilders.class.getSimpleName());
-            }
-        };
-    }
+    public static final ArchRules public_API_rules = ArchRules.in(PublicAPIRules.class);
 
     private static DescribedPredicate<JavaCall<?>> typeIsIllegallyResolvedViaReflection() {
         DescribedPredicate<JavaCall<?>> explicitlyAllowedUsage =
diff --git a/archunit-integration-test/src/test/java/com/tngtech/archunit/ImporterRules.java b/archunit-integration-test/src/test/java/com/tngtech/archunit/ImporterRules.java
@@ -0,0 +1,43 @@
+package com.tngtech.archunit;
+
+import com.tngtech.archunit.base.DescribedPredicate;
+import com.tngtech.archunit.core.domain.JavaClass;
+import com.tngtech.archunit.core.domain.JavaType;
+import com.tngtech.archunit.core.importer.ClassFileImporter;
+import com.tngtech.archunit.core.importer.DomainBuilders;
+import com.tngtech.archunit.junit.ArchTest;
+import com.tngtech.archunit.lang.ArchRule;
+
+import static com.tngtech.archunit.ArchUnitArchitectureTest.THIRDPARTY_PACKAGE_IDENTIFIER;
+import static com.tngtech.archunit.base.DescribedPredicate.not;
+import static com.tngtech.archunit.core.domain.JavaClass.Predicates.belongToAnyOf;
+import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noClasses;
+
+public class ImporterRules {
+
+    @ArchTest
+    public static final ArchRule domain_does_not_access_importer =
+            noClasses().that().resideInAPackage("..core.domain..")
+                    .should().accessClassesThat(belong_to_the_import_context());
+
+    @ArchTest
+    public static final ArchRule ASM_type_is_only_accessed_within_JavaType_or_JavaTypeImporter =
+            noClasses()
+                    .that().resideOutsideOfPackage(THIRDPARTY_PACKAGE_IDENTIFIER)
+                    .and(not(belongToAnyOf(JavaType.class)))
+                    .and().doNotHaveFullyQualifiedName("com.tngtech.archunit.core.importer.JavaTypeImporter")
+                    .should().dependOnClassesThat().haveNameMatching(".*\\.asm\\..*Type")
+                    .as("org.objectweb.asm.Type should only be accessed within JavaType(Importer)")
+                    .because("org.objectweb.asm.Type handles array types inconsistently (uses the canonical name instead of the class name), "
+                            + "so the correct behavior is implemented only within JavaType");
+
+    private static DescribedPredicate<JavaClass> belong_to_the_import_context() {
+        return new DescribedPredicate<JavaClass>("belong to the import context") {
+            @Override
+            public boolean apply(JavaClass input) {
+                return input.getPackageName().startsWith(ClassFileImporter.class.getPackage().getName())
+                        && !input.getName().contains(DomainBuilders.class.getSimpleName());
+            }
+        };
+    }
+}
diff --git a/archunit/src/main/java/com/tngtech/archunit/core/importer/AccessRecord.java b/archunit/src/main/java/com/tngtech/archunit/core/importer/AccessRecord.java
@@ -36,6 +36,7 @@
 import com.tngtech.archunit.core.domain.JavaField;
 import com.tngtech.archunit.core.domain.JavaFieldAccess.AccessType;
 import com.tngtech.archunit.core.domain.JavaMethod;
+import com.tngtech.archunit.core.domain.JavaType;
 import com.tngtech.archunit.core.domain.properties.HasDescriptor;
 import com.tngtech.archunit.core.domain.properties.HasName;
 import com.tngtech.archunit.core.domain.properties.HasOwner;
@@ -44,7 +45,6 @@
 import com.tngtech.archunit.core.importer.DomainBuilders.MethodCallTargetBuilder;
 import com.tngtech.archunit.core.importer.RawAccessRecord.CodeUnit;
 import com.tngtech.archunit.core.importer.RawAccessRecord.TargetInfo;
-import org.objectweb.asm.Type;
 
 import static com.google.common.collect.Iterables.getOnlyElement;
 import static com.tngtech.archunit.core.domain.DomainObjectCreationContext.createJavaClassList;
@@ -167,7 +167,7 @@ public JavaCodeUnit getCaller() {
             public MethodCallTarget getTarget() {
                 Supplier<Set<JavaMethod>> methodsSupplier = new MethodTargetSupplier(targetOwner.getAllMethods(), record.target);
                 JavaClassList parameters = getArgumentTypesFrom(record.target.desc, classes);
-                JavaClass returnType = classes.getOrResolve(Type.getReturnType(record.target.desc).getClassName());
+                JavaClass returnType = classes.getOrResolve(JavaTypeImporter.importAsmMethodReturnType(record.target.desc).getName());
                 return new MethodCallTargetBuilder()
                         .withOwner(targetOwner)
                         .withName(record.target.name)
@@ -224,7 +224,7 @@ public JavaCodeUnit getCaller() {
             @Override
             public FieldAccessTarget getTarget() {
                 Supplier<Optional<JavaField>> fieldSupplier = new FieldTargetSupplier(targetOwner.getAllFields(), record.target);
-                JavaClass fieldType = classes.getOrResolve(Type.getType(record.target.desc).getClassName());
+                JavaClass fieldType = classes.getOrResolve(JavaTypeImporter.importAsmType(record.target.desc).getName());
                 return new FieldAccessTargetBuilder()
                         .withOwner(targetOwner)
                         .withName(record.target.name)
@@ -290,8 +290,8 @@ private static <T> Optional<T> uniqueTargetIn(Collection<T> collection) {
 
         private static JavaClassList getArgumentTypesFrom(String descriptor, ImportedClasses classes) {
             List<JavaClass> paramTypes = new ArrayList<>();
-            for (Type type : Type.getArgumentTypes(descriptor)) {
-                paramTypes.add(classes.getOrResolve(type.getClassName()));
+            for (JavaType type : JavaTypeImporter.importAsmMethodArgumentTypes(descriptor)) {
+                paramTypes.add(classes.getOrResolve(type.getName()));
             }
             return createJavaClassList(paramTypes);
         }
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
@@ -21,11 +21,9 @@
 import java.util.Collection;
 import java.util.HashSet;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
 
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.primitives.Booleans;
 import com.google.common.primitives.Bytes;
@@ -36,7 +34,6 @@
 import com.google.common.primitives.Longs;
 import com.google.common.primitives.Shorts;
 import com.tngtech.archunit.Internal;
-import com.tngtech.archunit.base.Function;
 import com.tngtech.archunit.base.HasDescription;
 import com.tngtech.archunit.base.Optional;
 import com.tngtech.archunit.core.MayResolveTypesViaReflection;
@@ -207,7 +204,7 @@ public FieldVisitor visitField(int access, String name, String desc, String sign
 
         DomainBuilders.JavaFieldBuilder fieldBuilder = new DomainBuilders.JavaFieldBuilder()
                 .withName(name)
-                .withType(JavaTypeImporter.importAsmType(Type.getType(desc)))
+                .withType(JavaTypeImporter.importAsmType(desc))
                 .withModifiers(JavaModifier.getModifiersForField(access))
                 .withDescriptor(desc);
         declarationHandler.onDeclaredField(fieldBuilder);
@@ -221,29 +218,21 @@ public MethodVisitor visitMethod(int access, String name, String desc, String si
         }
 
         LOG.trace("Analyzing method {}.{}:{}", className, name, desc);
-        accessHandler.setContext(new CodeUnit(name, namesOf(Type.getArgumentTypes(desc)), className));
+        List<JavaType> parameters = JavaTypeImporter.importAsmMethodArgumentTypes(desc);
+        accessHandler.setContext(new CodeUnit(name, namesOf(parameters), className));
 
         DomainBuilders.JavaCodeUnitBuilder<?, ?> codeUnitBuilder = addCodeUnitBuilder(name);
-        Type methodType = Type.getMethodType(desc);
         codeUnitBuilder
                 .withName(name)
                 .withModifiers(JavaModifier.getModifiersForMethod(access))
-                .withParameters(typesFrom(methodType.getArgumentTypes()))
-                .withReturnType(JavaTypeImporter.importAsmType(methodType.getReturnType()))
+                .withParameters(parameters)
+                .withReturnType(JavaTypeImporter.importAsmMethodReturnType(desc))
                 .withDescriptor(desc)
                 .withThrowsClause(typesFrom(exceptions));
 
         return new MethodProcessor(className, accessHandler, codeUnitBuilder);
     }
 
-    private List<JavaType> typesFrom(Type[] asmTypes) {
-        List<JavaType> result = new ArrayList<>();
-        for (Type asmType : asmTypes) {
-            result.add(JavaTypeImporter.importAsmType(asmType));
-        }
-        return result;
-    }
-
     private List<JavaType> typesFrom(String[] throwsDeclarations) {
         List<JavaType> result = new ArrayList<>();
         if (throwsDeclarations != null) {
@@ -289,10 +278,10 @@ public void visitEnd() {
         LOG.trace("Done analyzing {}", className);
     }
 
-    private static List<String> namesOf(Type[] types) {
+    private static List<String> namesOf(Iterable<JavaType> types) {
         ImmutableList.Builder<String> result = ImmutableList.builder();
-        for (Type type : types) {
-            result.add(JavaTypeImporter.importAsmType(type).getName());
+        for (JavaType type : types) {
+            result.add(type.getName());
         }
         return result.build();
     }
@@ -487,7 +476,7 @@ public void visitEnd() {
     }
 
     private static DomainBuilders.JavaAnnotationBuilder annotationBuilderFor(String desc) {
-        return new DomainBuilders.JavaAnnotationBuilder().withType(JavaTypeImporter.importAsmType(Type.getType(desc)));
+        return new DomainBuilders.JavaAnnotationBuilder().withType(JavaTypeImporter.importAsmType(desc));
     }
 
     private static class AnnotationProcessor extends AnnotationVisitor {
@@ -575,7 +564,11 @@ private AnnotationArrayProcessor(AnnotationArrayContext annotationArrayContext)
 
         @Override
         public void visit(String name, Object value) {
-            setDerivedComponentType(value);
+            if (value instanceof Type) {
+                setDerivedComponentType(JavaClass.class);
+            } else {
+                setDerivedComponentType(value.getClass());
+            }
             values.add(AnnotationTypeConversion.convert(value));
         }
 
@@ -596,16 +589,11 @@ public void visitEnum(String name, final String desc, final String value) {
             values.add(javaEnumBuilder(desc, value));
         }
 
-        private void setDerivedComponentType(Object value) {
-            setDerivedComponentType(value.getClass());
-        }
-
         // NOTE: If the declared annotation is not imported itself, we can still use this heuristic,
         //       to determine additional information about the respective array.
         //       (It the annotation is imported itself, we could easily determine this from the respective
         //       JavaClass methods)
         private void setDerivedComponentType(Class<?> type) {
-            type = AnnotationTypeConversion.convert(type);
             checkState(derivedComponentType == null || derivedComponentType.equals(type),
                     "Found mixed component types while importing array, this is most likely a bug");
 
@@ -668,22 +656,21 @@ private Optional<Class<?>> determineComponentType(ClassesByTypeName importedClas
                         .tryGetMethod(annotationArrayContext.getDeclaringAnnotationMemberName());
 
                 return method.isPresent() ?
-                        determineComponentTypeFromReturnValue(method) :
+                        determineComponentTypeFromReturnValue(method.get()) :
                         Optional.<Class<?>>absent();
             }
 
-            private Optional<Class<?>> determineComponentTypeFromReturnValue(Optional<JavaMethod> method) {
-                String name = method.get().getRawReturnType().getName();
-                Optional<Class<?>> result = AnnotationTypeConversion.tryConvert(name);
-                if (result.isPresent()) {
-                    return Optional.<Class<?>>of(result.get().getComponentType());
+            private Optional<Class<?>> determineComponentTypeFromReturnValue(JavaMethod method) {
+                if (method.getRawReturnType().isEquivalentTo(Class[].class)) {
+                    return Optional.<Class<?>>of(JavaClass.class);
                 }
-                return resolveComponentTypeFrom(name);
+
+                return resolveComponentTypeFrom(method.getRawReturnType().getName());
             }
 
             @MayResolveTypesViaReflection(reason = "Resolving primitives does not really use reflection")
-            private Optional<Class<?>> resolveComponentTypeFrom(String name) {
-                JavaType type = JavaType.From.name(name);
+            private Optional<Class<?>> resolveComponentTypeFrom(String arrayTypeName) {
+                JavaType type = JavaType.From.name(arrayTypeName);
                 JavaType componentType = getComponentType(type);
 
                 if (componentType.isPrimitive()) {
@@ -725,50 +712,25 @@ private static ValueBuilder javaEnumBuilder(final String desc, final String valu
             public <T extends HasDescription> Optional<Object> build(T owner, ClassesByTypeName importedClasses) {
                 return Optional.<Object>of(
                         new DomainBuilders.JavaEnumConstantBuilder()
-                                .withDeclaringClass(importedClasses.get(Type.getType(desc).getClassName()))
+                                .withDeclaringClass(importedClasses.get(JavaTypeImporter.importAsmType(desc).getName()))
                                 .withName(value)
                                 .build());
             }
         };
     }
 
     private static class AnnotationTypeConversion {
-        private static final Map<String, Class<?>> externalTypeToInternalType = ImmutableMap.of(
-                Type.class.getName(), JavaClass.class,
-                Class.class.getName(), JavaClass.class,
-                Class[].class.getName(), JavaClass[].class
-        );
-        private static final Map<Class<?>, Function<Object, ValueBuilder>> importedValueToInternalValue =
-                ImmutableMap.<Class<?>, Function<Object, ValueBuilder>>of(
-                        Type.class, new Function<Object, ValueBuilder>() {
-                            @Override
-                            public ValueBuilder apply(final Object input) {
-                                return new ValueBuilder() {
-                                    @Override
-                                    public <T extends HasDescription> Optional<Object> build(T owner, ClassesByTypeName importedClasses) {
-                                        return Optional.<Object>of(importedClasses.get(((Type) input).getClassName()));
-                                    }
-                                };
-                            }
-                        }
-                );
-
-        static Class<?> convert(Class<?> type) {
-            return externalTypeToInternalType.containsKey(type.getName()) ?
-                    externalTypeToInternalType.get(type.getName()) :
-                    type;
-        }
-
-        static Optional<Class<?>> tryConvert(String typeName) {
-            return externalTypeToInternalType.containsKey(typeName) ?
-                    Optional.<Class<?>>of(externalTypeToInternalType.get(typeName)) :
-                    Optional.<Class<?>>absent();
-        }
-
-        static ValueBuilder convert(Object value) {
-            return importedValueToInternalValue.containsKey(value.getClass()) ?
-                    importedValueToInternalValue.get(value.getClass()).apply(value) :
-                    ValueBuilder.ofFinished(value);
+        static ValueBuilder convert(Object input) {
+            final Object value = JavaTypeImporter.importAsmTypeIfPossible(input);
+            if (value instanceof JavaType) {
+                return new ValueBuilder() {
+                    @Override
+                    public <T extends HasDescription> Optional<Object> build(T owner, ClassesByTypeName importedClasses) {
+                        return Optional.<Object>of(importedClasses.get(((JavaType) value).getName()));
+                    }
+                };
+            }
+            return ValueBuilder.ofFinished(value);
         }
     }
 }
diff --git a/archunit/src/main/java/com/tngtech/archunit/core/importer/JavaTypeImporter.java b/archunit/src/main/java/com/tngtech/archunit/core/importer/JavaTypeImporter.java
@@ -15,6 +15,9 @@
  */
 package com.tngtech.archunit.core.importer;
 
+import java.util.List;
+
+import com.google.common.collect.ImmutableList;
 import com.tngtech.archunit.core.domain.JavaType;
 import org.objectweb.asm.Type;
 
@@ -30,4 +33,24 @@ static JavaType createFromAsmObjectTypeName(String objectTypeName) {
     static JavaType importAsmType(Type type) {
         return JavaType.From.name(type.getClassName());
     }
+
+    static Object importAsmTypeIfPossible(Object value) {
+        return value instanceof Type ? importAsmType((Type) value) : value;
+    }
+
+    static JavaType importAsmType(String typeDescriptor) {
+        return importAsmType(Type.getType(typeDescriptor));
+    }
+
+    static List<JavaType> importAsmMethodArgumentTypes(String methodDescriptor) {
+        ImmutableList.Builder<JavaType> result = ImmutableList.builder();
+        for (Type type : Type.getArgumentTypes(methodDescriptor)) {
+            result.add(importAsmType(type));
+        }
+        return result.build();
+    }
+
+    static JavaType importAsmMethodReturnType(String methodDescriptor) {
+        return importAsmType(Type.getReturnType(methodDescriptor));
+    }
 }
diff --git a/archunit/src/test/java/com/tngtech/archunit/testutil/Assertions.java b/archunit/src/test/java/com/tngtech/archunit/testutil/Assertions.java
@@ -326,8 +326,13 @@ public void matches(Class<?> clazz) {
                     .isEqualTo(getExpectedPackageName(clazz));
             assertThat(actual.getModifiers()).as("Modifiers of " + actual)
                     .isEqualTo(JavaModifier.getModifiersForClass(clazz.getModifiers()));
+            assertThat(actual.isArray()).as(actual + " is array").isEqualTo(clazz.isArray());
             assertThat(propertiesOf(actual.getAnnotations())).as("Annotations of " + actual)
                     .isEqualTo(propertiesOf(clazz.getAnnotations()));
+
+            if (clazz.isArray()) {
+                new JavaClassAssertion(actual.getComponentType()).matches(clazz.getComponentType());
+            }
         }
 
         private String ensureArrayName(String name) {
