Expose type parameters of JavaClass

[wolfs]

It should be possible to obtain the type parameters for a JavaClass.

When given code like this:

public class Foo implements List<Bar> {
}

I now want to obtain the type parameters of the interface. Same goes for fields, method return types and parameters, etc.

Something like this would be great:

JavaClass genericType = <List<Foo>>
genericType.isAssignableTo(Iterable.class) // true, I guess that works already
genericType.getTypeParameters() == [Foo]

[slq]

This might not be as easy as it seems at the first spot.

Due to the recursive nature of generic types (a generic type can be parameterized by a generic type – consider for example List<List<String>>) the grammar of type signatures is quite complex.

1. Add parameter to JavaType interface and its implementation

public interface JavaType {
    ...
    JavaType getParameter();
    boolean isParametrizedType();
    ...
}

2. Rewrite com.tngtech.archunit.core.importer.ImportedClasses to use JavaType instead of String as cache key in #directlyImported, #additionalClasses.
3. Write implementation of org.objectweb.asm.signature.SignatureVisitor that will instantiate JavaType implementations and add type parameters.

//com.tngtech.archunit.core.importer.JavaClassProcessor
public FieldVisitor visitField(int access, String name, String desc, String signature, Object value) {
        ...
        if (signature != null) {
            XXXSignatureVisitor visitor = new XXXSignatureVisitor();
            SignatureReader reader = new SignatureReader(signature);
            reader.acceptType(visitor);
            // visitor.getParametrizedType();
       }
       ...
}

I will try to proceed with implementation using smaller steps:

1. Implementation for simple case List<String>, Set<Order>, etc.
2. Add support for recursive generic types List<List<List<String>>>, Map<String, List<Order>>, etc.
3. Expose generic type as dependency (same as in Dependencies should consider annotations #136)

Few additional questions arise:

1. How to proceed with type parameters: Set<T>, Set<? extends T>, Set<? super T>?
2. How to proceed with parametrized types like: Set<?>, Set<? extends Order>, Set<? super Order>?

[codecholeric]

Thanks for looking into that 😃 Nobody ever said this is easy 😉
I can tell you my two cents. I would try to stick closely to the Reflection API while removing any old bloat that was just added for backwards compatibility (like getType() vs. getGenericType())

1. You'll probably have to extend JavaType with List<JavaTypeVariable> getParameterTypes(). This also touches your additional questions, the <T> in List<T> is not a JavaType but a JavaTypeVariable (the Reflection API has a TypeVariable here as well). The difficulty arises, if you try to treat it as JavaType, since then the concepts clash. There should be a way to check if a JavaTypeVariable actually represents a JavaType or is free/bounded (the Reflection API also offers a method to check the bounds for example, if it's something like <? extends List<String>, Serializable>).
2. I'm not sure if the key in ImportedClasses needs to be refactored. No matter if you consider generic types or not, there is only one foo.Bar, no matter if it's Bar<T> or just Bar. So as long as the value knows about the type + parameter, it should be fine?
3. Yes, this should be the place to start 👍

The outline of your plan sounds good, would be my approach, too. ClassFileImporterTest should be the place to add tests to (even though it has gotten quite big over time...).

[slq]

1. Actually, I believe we are more interested in getting ParameterizedType (as in List<String>) instead of TypeVariable (List<T>). We just need this information to track dependencies, so there is no need to track type parameters, like T. Is that correct assumption? I will just try to add List<JavaType> typeArguments to JavaType. In such case I think there is no need to introduce additional interface JavaTypeVariable.

BTW. It would be best if JavaClass implemented JavaType the same way as java.lang.reflect is implemented. In such case we could have new interface ParameterizedJavaType which would simply extend JavaType and provide api like JavaType[] getActualTypeArguments(). But this sounds like major architecture change, so I will leave it 😉.

2. I have a problem with ImportedClasses. I have parsed type arguments and attached them to JavaType. When com.tngtech.archunit.core.importer.DomainBuilders.JavaFieldBuilder comes into play type is saved using withType to class field type, but it is later retrieved using getType from cache, where Set<Order> and Set<Product> are considered the same.

[codecholeric]

@slq sorry that I'm answering so late, I got sick for a couple of days...
Regarding type variables

1. I don't know if we really are just interested in dependencies, sometimes we also want to ensure consistency / coding standards and it might make sense there
2. You would at least have to deal with upper bounds then, consider a field List<? extends Bar> field, does the class now have a dependency on Bar? I would guess yes, but the type parameter of List is ?. Same holds for Foo<T extends Bar>.

Your problem with JavaField raises an interesting problem. I have to admit that I don't like the Reflection API in that part, especially the permanent casting to something like ParameterizedType. It always felt pretty patched together, which is probably normal, if you try to be backwards compatible but completely try to overhaul the type system.
But now ArchUnit kind of has the same problem *lol*. Because I don't think JavaField.getType() really matches this here, because we can't reasonably return JavaClass if we're interested in type parameters.

To elaborate, consider

class Foo<T extends Serializable> {...}

class Bar {
    Foo<String> field;
}

So the JavaClass Foo can really only tell about its type parameters by declaration, i.e. Foo could tell you it's parameterized with a type variable with upper bound Serializable. However the field Foo<String> field clearly has a field type that's parameterized with String and not just something with upper bound Serializable. So JavaField.getType() can't really return JavaClass for this.

So suppose we return something like FieldType (honestly JavaType would seem like a good name for that thing 😉), then what would FieldType.getTypeParameter(1) return, if it's a

Map<K, V> map;

If we don't want to introduce breaking changes, we could start to do something like

@Deprecated // use getRawType() instead
JavaClass JavaField.getType()

JavaClass JavaField.getRawType()

FieldType JavaField.getGenericType()

where in time getGenericType() could become getType().

I think this "solves" your concrete problem with the ImportedClasses though, because the JavaClass you retrieve is unique by type name anyway (and has exactly one generic type information). However you need additional information from the concrete field declaration to decide what the FieldType.getGenericParameters() are.

Does this make sense in some way? (I'm just rambling about my thoughts, I have no code in front of me, so maybe I'm missing the point 😉)

[odrotbohm]

Continuing on the discussion from #183, I wonder if there's a less invasive way to achieve 90% of the functionality?

First fundamental assumption: we don't actually need to change the API of JavaClass but could start with the point in the model in which Depenency instances get created and are obtained from the members, method and constructor parameters etc. So instead of creating exactly one Dependency instance for e.g. a field, we could inspect the type a bit more closely (more on that below) and create multiple instances so that e.g. a List<Optional<Foo>> field would result in three dependencies for List, Optional and Foo all pointing to the field as source of the dependency. Am I overseeing anything that would make this hard?

Second assumption: we don't need any advanced generics inspection as we're only interested in the point of the declaration, not the ultimate expansion at runtime. Peter gave this example in the other ticket:

class Example<T extends Base<? super T>> {
	List<? extends T> elements;
}

class Base<T> {}

class Foo extends Base<Foo> {}

class ExampleExtension extends Example<Foo> {}

In this example we don't have to find out that for ExampleExtension the T of elements evaluates to Foo. We only need to find out that ExampleExtension depends on Example and Foo (through its generic type inspection of very basic form.

This bit of sample code collects all types depended on directly traversing generics declarations for non-raw types:

public class Sample {

	class Example<T extends Base<? super T>> {
		List<? extends T> elements;
	}

	class Base<T> {}

	class Foo extends Base<Foo> {}

	class ExampleExtension extends Example<Foo> {}

	public static void main(String[] args) throws Exception {

		// prints [class ….Sample$Example, class ….Sample$Base]		
		System.out.println(getTypeDependencies(Example.class));

		// prints [interface java.util.List, class ….Sample$Base]
		
System.out.println(getTypeDependencies(Example.class.getDeclaredField("elements").getGenericType()));

		// prints [class….Sample$ExampleExtension, class ….Sample$Example]
		System.out.println(getTypeDependencies(ExampleExtension.class));
	}

	public static List<Class<?>> getTypeDependencies(Type type) {

		return getTypeParametersRecursively(type, Collections.singleton(Object.class)) //
				.collect(Collectors.toList());
	}

	private static Stream<Class<?>> getTypeParametersRecursively(Type type, Collection<Type> alreadyFound) {

		Stream<Type> result = Stream.empty();
		Type head = null;

		if (Class.class.isInstance(type)) {

			// For classes, inspect type parameters

			var clazz = (Class<?>) type;
			head = clazz;

			var superclass = Stream.of(clazz.getGenericSuperclass());
			var interfaces = Arrays.stream(clazz.getGenericInterfaces());
			var parameters = Arrays.stream(clazz.getTypeParameters());

			result = Stream.concat(Stream.concat(parameters, interfaces), superclass); //

		} else if (ParameterizedType.class.isInstance(type)) {

			// For parameterized types, inspect arguments

			var parameterized = (ParameterizedType) type;
			var arguments = parameterized.getActualTypeArguments();

			head = parameterized.getRawType();
			result = Arrays.stream(arguments);

		} else if (WildcardType.class.isInstance(type)) {

			// For wildcards, inspect bounds

			var wildcard = (WildcardType) type;

			result = Stream.concat(Arrays.stream(wildcard.getLowerBounds()), //
					Arrays.stream(wildcard.getUpperBounds())); //

		} else if (TypeVariable.class.isInstance(type)) {

			// For type variables, inspect bounds

			var variable = (TypeVariable<?>) type;

			result = Arrays.stream(variable.getBounds());
		}

		// All the nested types we found
		var nested = result.collect(Collectors.toSet());

		// Create new set of already known types
		var newFound = new HashSet<>(alreadyFound);
		newFound.addAll(nested);

		// Only recursively consider the ones that we haven't seen before
		var recursive = nested.stream() //
				.filter(Predicate.not(alreadyFound::contains)) //
				.filter(Predicate.not(Class.class::isInstance)) //
				.distinct() //
				.flatMap(it -> getTypeParametersRecursively(it, newFound));

		// Return the currently resolved type plus recursively resolved ones
		return Class.class.isInstance(head) //
				? Stream.concat(Stream.of(Class.class.cast(head)), recursive) //
				: recursive;
	}
}

With that in place, we could create additional Dependency instances from types, fields, method and constructor parameters and – in case of the former – return types. Wouldn't that be sufficient?

In case you think this might be a way you'd like to investigate I'd happily spent some time on a PR. For now it looks like changes in Dependency should be sufficient.

[odrotbohm]

Another quick glance, it looks like the main challenge is to turn the discovered types back into JavaClass instances from within Dependency. Any pointers appreciated.

[codecholeric]

Thanks a lot for listing the cases of generic dependencies so nicely @odrotbohm 😃
In my opinion the approach via reflection is valid to mitigate this problem, until we can really solve it. However I would not pull this into ArchUnit itself, because the guide line was always to be independent of the classpath, and in my opinion it should stay that way. I.e. if you only want to import some random folder on your hard drive, or some Jar file, it should just work without putting these classes on the classpath (I've used that often in the past, and in particular with legacy projects, it often makes life a lot easier).
So what I would propose for the meantime would be to offer a workaround via reflection through some custom copy and paste code (yes, I know it's ugly)...
To turn your code into a (somewhat shady) ArchCondition, you could for example use

class RuleSample {
    public static void main(String[] args) {
        noClasses().should(haveDependenciesWhere(dependencyTarget(Sample.Foo.class)))
                .check(new ClassFileImporter().importPackagesOf(RuleSample.class));
    }

    private static ArchCondition<JavaClass> haveDependenciesWhere(DescribedPredicate<? super Dependency> predicate) {
        return new ArchCondition<>("not have dependencies where " + predicate.getDescription()) {
            private Map<String, JavaClass> allClasses;

            @Override
            public void init(Iterable<JavaClass> allObjectsToTest) {
                allClasses = StreamSupport.stream(allObjectsToTest.spliterator(), false)
                        .collect(toMap(JavaClass::getName, identity()));
            }

            @Override
            public void check(JavaClass javaClass, ConditionEvents events) {
                Stream<Dependency> additionalDependencies = Sample.getTypeDependencies(javaClass.reflect()).stream()
                        .filter(c -> allClasses.containsKey(c.getName()))
                        .map(c -> allClasses.get(c.getName()))
                        .map(targetClass -> newDependency(javaClass, targetClass,
                                "Class<%s> depends on %s by type parameter"));
                Stream.concat(javaClass.getDirectDependenciesFromSelf().stream(), additionalDependencies)
                        .forEach(dependency -> {
                            boolean satisfied = predicate.apply(dependency);
                            events.add(new SimpleConditionEvent(dependency, satisfied, dependency.getDescription()));
                        });
            }
        };
    }

    private static Dependency newDependency(JavaClass originClass, JavaClass targetClass, String descriptionTemplate) {
        try {
            String description = String.format(descriptionTemplate, originClass.getName(), targetClass.getName());
            Constructor<Dependency> constructor = Dependency.class.getDeclaredConstructor(
                    JavaClass.class, JavaClass.class, int.class, String.class);
            constructor.setAccessible(true);
            return constructor.newInstance(originClass, targetClass, 0, description);
        } catch (Exception e) {
            throw new RuntimeException(e);
        }
    }
}

This code would have the advantage, that any existing infrastructure for Dependency, i.e. predicates and functions, could be reused on the additional dependencies. E.g.

noClasses().should(haveDependenciesWhere(
    dependencyTarget(JavaClass.Predicates.resideInAnyPackage("..pkg.."))));

Seems like for ExampleExtension extends Example<Foo> it does not report the dependency on Foo though? Do you disregard all instances of Class<?> within nested on purpose?

However, to get back to the clean solution, I did the renaming from type to rawType in most places, so I think support for generics could be introduced without breaking too much. Even for method parameters there is the type JavaClassList that could be extended (even though it is then a very bad name and does not really make sense ☹️). I don't think adding the info would break too much anymore, it would just take some careful pondering, on which level type variables and co want to be modeled, i.e. what is the right amount of information and abstraction for ArchUnit.