Add generic type info to `JavaClass` #398

codecholeric · 2020-07-14T19:38:31Z

This will solve a part of #115 and add support for generic types to JavaClass. It will still not be possible to query superclass or interface type parameters, but it will add support for arbitrarily nested type signatures of JavaClass itself, like

class Foo<T extends Serializable, U extends List<? super Map<? extends Bar, T[][]>>, V extends T> {}

Issue: #115

archunit/src/test/java/com/tngtech/archunit/testutil/Assertions.java

hankem

My comments so far are only from reading the code itself.

I haven't really used it yet; in particular, I'd still like to

measure the time to import a larger project, and
of course play with new API in my own rules.

archunit/src/test/java/com/tngtech/archunit/testutil/assertion/JavaTypesAssertion.java

archunit/src/main/java/com/tngtech/archunit/core/domain/JavaTypeVariable.java

archunit/src/main/java/com/tngtech/archunit/core/importer/DomainBuilders.java

archunit/src/test/java/com/tngtech/archunit/core/domain/JavaTypeTest.java

archunit/src/test/java/com/tngtech/archunit/testutil/assertion/JavaTypeAssertion.java

archunit/src/test/java/com/tngtech/archunit/core/domain/JavaTypeVariableTest.java

archunit/src/test/java/com/tngtech/archunit/core/domain/JavaWildcardTypeTest.java

...it/src/test/java/com/tngtech/archunit/core/importer/ClassFileImporterGenericClassesTest.java

.../src/test/java/com/tngtech/archunit/testutil/assertion/JavaTypeVariableOfClassAssertion.java

archunit/src/main/java/com/tngtech/archunit/core/domain/JavaType.java

hankem · 2020-08-31T05:48:39Z

I assume that extending JavaClass.getDirectDependenciesFromSelf() to include classes from generic type (bounds) is not in this PR's scope?

codecholeric · 2020-08-31T19:36:35Z

I assume that extending JavaClass.getDirectDependenciesFromSelf() to include classes from generic type (bounds) is not in this PR's scope?

Yeah, I think this PR is already quite big and provides a benefit by itself, so I would add it do the JavaClass.dependencies in a separate PR... But yes, I'm gonna do that soon to provide a real value to users of the fluent API (i.e. without custom predicates)

codecholeric · 2020-08-31T19:38:20Z

@hankem thanks a lot for your intensive review 😃 I know that this is not the smallest / easiest PR to review 😂 But yes, if you want to play around a little and measure performance, I'd really appreciate this! I can wait for this a couple more days 😉

hankem · 2020-09-04T06:59:05Z

The increase in runtime can be detected, but seems acceptable. (The import has obviously more work to do.)
Importing 3833 classes took:

`master`	`add-generic-type-info`
5.25 ± 0.18 s	5.73 ± 0.23 s

(based on only 5 repetitions of the import so far)

codecholeric · 2020-09-05T13:00:32Z

Thanks for testing! 😃 I expected something in that area, won't come for free 😉 Guess I'll profile it to make sure there is no unnecessary waste in there though, since it is a 10% increase after all 😞 But if this is unavoidable it would also be possible to make generic support configurable very easily, if it should be a real problem for some people.

codecholeric · 2020-09-05T14:37:17Z

Hmm, I found the place where the performance drain happens and it's actually not related to the additional generic support per se, but that the order of traversing the classes is now relevant for the import process 🤔 I'll look if this can be solved differently. Because without that I see almost no performance loss when I import the whole classpath of ArchUnit (~40000 classes)

codecholeric · 2020-09-05T15:05:21Z

@hankem I tried to remove the source of the performance loss, can you check again if that is reflected in your performance test as well?

We need to free the name `JavaType` if we want to add support for generics to be consistent with the Reflection API. I.e. if the Reflection API provides a super type `Type` for `Class` and `TypeVariable`, we should probably mimic this as `JavaType`, `JavaClass` and `JavaTypeVariable`. Otherwise we break the consistency with the Reflection API there, making the API confusing. Note that I do not consider `JavaClassDescriptor` the best name in the universe, but merely could not come up with anything better. It is pretty much all the information we can parse out of the info parsed by ASM, name parts, component type, etc. Signed-off-by: Peter Gafert <[email protected]>

Similar to the Reflection API we will need a common super type of `JavaClass` and `TypeVariable` to describe generic bounds. Signed-off-by: Peter Gafert <[email protected]>

Instead of `JavaClass(es)Assertion` we will need `JavaType(s)Assertion` in the future to be able to handle generic test cases. We can easily allow assertions on `JavaType` by simply pre-asserting `instanceof JavaClass` for the relevant cases specific to `JavaClass`. We could introduce a special sub-assertion `JavaClassAssertion` extending `JavaTypeAssertion`, where an assertion of the correct sub type is not necessary anymore, but to me the complexity seems to be fairly low to simply assert the correct sub type where necessary, so I kept it simple and only introduced `JavaType(s)Assertion`. Signed-off-by: Peter Gafert <[email protected]>

hankem · 2020-09-06T13:38:23Z

Impressive! With your performance-tuned SourceCodeLocation, the class file import seems to be even faster than before:

`master` (`ae16a10`)	`add-generic-type-info` (`f49af67`)
5.39 ± 0.21 s	4.97 ± 0.21 s

I was wondering whether SourceCodeLocation.description needs to be pre-computed at all (I think it's probably only needed for violations, so most of the descriptions might actually be dispensable), but a first attempt with a description-supplier did not give any further benefit.

codecholeric · 2020-09-06T15:01:38Z

Honestly I'm not sure if it's really that change, or the change to ImportedClasses 😉 I just saw that pop up on the profiler and it was about 2-3% speed increase on my machine 😉
But hey, one way or the other, add more features and get faster speed, I take it 😂

Introduce `JavaTypeVariable` to model type parameters like `MyClass<T>`. For now the model is quite simple, it only supports type bounds that are not generic themselves. In fact the current implementation will behave wrongly, as soon as there are types like `MyClass<T extends SomeGeneric<AnotherType>>`. However the current state already shows correct behavior for simple cases like `MyClass<T extends SomeClass & SomeInterface, U extends YetAnotherClass & More & Interfaces>`. Signed-off-by: Peter Gafert <[email protected]>

We now support importing the first level of generic bounds of type parameters where type parameters are assigned to concrete classes. E.g. `SomeClass<T extends Foo<String> & Bar<Integer>>`. For now we still do not support this in a generic way, i.e. `SomeClass<T extends Foo<Bar<Integer>>` is not supported correctly (bounds nested more than one level will be ignored, in this case `Integer`). Signed-off-by: Peter Gafert <[email protected]>

We now support wildcards at the first level of parameterized type bounds. E.g. wildcards in bounds like `T extends List<? extends Serializable>`. To support quick localizations of errors in assertions I have added a `DescriptionContext` which can trace the steps (i.e. upper bound of second parameter of third type parameter) through the assertion and print the necessary details if a recursive assertion fails. Signed-off-by: Peter Gafert <[email protected]>

We now support signatures like `Foo<S, T extends S>`, where one type parameter is bound by another type parameter. What makes this difficult is the fact that `extends S` in this case is a reference to the original type parameter `S` (same holds for the Java Reflection API). I.e. in a case like `Foo<S extends String, T extends S>` it must be possible to retrieve `typeParameters[1].upperBounds[0].upperBounds[0]` to be `java.lang.String`. So to build the type parameter `T`, we need the type parameter `S` already built so we can reference it. To support this, I have split the creation of type parameters into `build()`, which will skip the bounds (since we need the built type parameters for this), and `complete` where the bounds are finished and set into the type parameters (thus unfortunately removing immutability, but I do not think you can have this in such a chicken and egg scenario with Java capabilities; in any way the mutability is package private, from an API point of view this object will be immutable like all ArchUnit domain objects). Unfortunately this is a fairly recursive thing, so each builder in the type parameter bounds tree needs all constructed type parameters, because it might have bounds or assigned type parameters somewhere down the line that are type parameters themselves. Within the import process we now need one more level of indirection, because we must decide if this is a new type to build (like a parameterized type) or if it simply references another type parameter and thus needs to return an already constructed one. Therefore there is now `JavaTypeCreationProcess` to encapsulate these two cases (technically it would have been possible to hack `TypeVariableBuilder` to return an existing type variable on build, but I have decided against it, since it would be a violation of the builder pattern and thus surprising). Signed-off-by: Peter Gafert <[email protected]>

So far we support signatures like `Foo<S, T extends S>`, where one type parameter is bound by another type parameter. However it is possible, that type parameters of inner classes refer to type parameters of outer classes, e.g. `class Foo<T> { class Inner<U extends T> {} }`. This means that we not only need all constructed type parameters of the class under construction to finish the type arguments, but also all type parameters of all enclosing classes. Unfortunately enclosing class creation has been too late in the construction process for this, thus I have moved it to an earlier place, namely when type hierarchies are assembled. Also to ensure that the construction of the type parameters occurs in the correct order (after all we need the type parameters of the enclosing class constructed before we can process the type parameters of the inner class) I have added sorting by type name to the processing of imported classes. I realize that this is somewhat implicit to ensure the correct order this way (i.e. rely on the naming, that inner class names start with the outer classes name and thus sorting the class names lexicographically will result in the outer classes being processed first), but I still opted for this solution, because I think the complexity to integrate it into the current code base is low and the solution should not impact performance in a noticeable way. In the central place I added a comment and made the internal API clear by replacing the return type with `Sorted...` (at least to point out that the order is of relevance). Should this be broken in the future, tests will likely notice the problem, even though the symptom might unfortunately just be flaky tests. Signed-off-by: Peter Gafert <[email protected]>

We can now also import upper and lower bounds of wildcards that are type variables, i.e. within `Foo<T, U extends List<? extends T>, V extends List<? super T>` we now support detecting `? extends T` and `? super T`. Signed-off-by: Peter Gafert <[email protected]>

Reduced the number of visitor objects needed and generalize the design. We will need to apply more recursion to be able to follow the tree of arbitrarily nested types, e.g. `Foo<Bar<Baz<? extends Quux<T>, ?>, ?>, T>`. It will be possible to recursively reuse the type argument processors in the next step. Signed-off-by: Peter Gafert <[email protected]>

We now traverse the tree of type parameters recursively, thus supporting an arbitrarily nested tree of type parameters. I have thought about reusing the same visitor(s) to traverse the tree to reduce the number of created objects, but decided it is not worth the complexity at the moment. If we create new visitors for each level we have an unmodified clean state once we come back from a nested level without the need to clean up / implement `visitEnd()`. Also in general such nested type signatures are a real corner case, most type signatures have at max one level of nexted bounds, so together with only a small subset of used types even being generic I decided to go for simplicity and safety instead of premature optimization (before we would introduce extra complexity here, we should measure this on some real life code bases if this really provides any measurable performance boost). Signed-off-by: Peter Gafert <[email protected]>

So far types like `Foo<T extends List<SomeType[]>>` were not correctly handled. We now support any arbitrarily nested array types as well, as long as they are concrete array types and not generic array types. Note that by the JLS direct bounds cannot be array types (i.e. `Foo<T extends SomeType[]>` is not a valid signature). Signed-off-by: Peter Gafert <[email protected]>

We can now handle generic array types like `T[][]` in `Foo<T, A extends List<T[][]>>`. Note that a generic array type cannot be a toplevel bound of a generic type, but can only appear as part of the nested type signature. Signed-off-by: Peter Gafert <[email protected]>

Once again surprising, but for sufficiently many classes it has a noticeable performance impact to use `String.format(..)` instead of `StringBuilder` (which constant string concatenation will be compiled to in the byte code for all non-ancient JDK versions I know). Since we create the formatted source code location for every class in the import this scales linear with the number of classes. Signed-off-by: Peter Gafert <[email protected]>

To keep the required adjustments of JLS links in Javadoc low, we decided to always link to the latest LTS JLS version where possible, instead of the latest available JLS version. If we need a concept only present in a newer JLS it is okay to simply link to the current JLS. When a new JLS is then released we will update all the links in one shot, including the ones that temporarily had to link to a newer version. Signed-off-by: Peter Gafert <[email protected]>

This is the next step of #398. It will make the imported type parameters of `JavaClass` widely useful by adding type parameter dependencies (e.g. `Bar` for `class Foo<T extends Bar>`) to the `JavaClass.directDependencies{From/To}Self`. This way type parameter dependencies will now cause violations in all dependency based `ArchRules` like `LayeredArchitecture` or any `classes()...dependOn...()` fluent API methods.

This will solve a part of #115 and add support for generic types to `JavaClass`. It will still not be possible to query superclass or interface type parameters, but it will add support for arbitrarily nested type signatures of `JavaClass` itself, like ``` class Foo<T extends Serializable, U extends List<? super Map<? extends Bar, T[][]>>, V extends T> {} ``` Issue: #115

This is the next step of #398. It will make the imported type parameters of `JavaClass` widely useful by adding type parameter dependencies (e.g. `Bar` for `class Foo<T extends Bar>`) to the `JavaClass.directDependencies{From/To}Self`. This way type parameter dependencies will now cause violations in all dependency based `ArchRules` like `LayeredArchitecture` or any `classes()...dependOn...()` fluent API methods.

codecholeric requested a review from hankem July 14, 2020 19:38

hankem reviewed Jul 18, 2020

View reviewed changes

archunit/src/test/java/com/tngtech/archunit/testutil/Assertions.java Outdated Show resolved Hide resolved

hankem mentioned this pull request Jul 28, 2020

JavaFields and composition hierarchy #405

Closed

hankem approved these changes Aug 30, 2020

View reviewed changes

codecholeric force-pushed the add-generic-type-info branch 2 times, most recently from fffddcb to c5647cc Compare August 31, 2020 22:01

codecholeric force-pushed the add-generic-type-info branch from c5647cc to d51ed9b Compare September 5, 2020 15:04

codecholeric force-pushed the add-generic-type-info branch 2 times, most recently from 4657067 to d0863d5 Compare September 5, 2020 15:17

codecholeric added 3 commits September 5, 2020 23:43

introduce JavaType as super type of JavaClass

79c103e

Similar to the Reflection API we will need a common super type of `JavaClass` and `TypeVariable` to describe generic bounds. Signed-off-by: Peter Gafert <[email protected]>

codecholeric force-pushed the add-generic-type-info branch from d0863d5 to f49af67 Compare September 5, 2020 21:43

codecholeric added 8 commits September 11, 2020 11:57

codecholeric added 4 commits September 11, 2020 15:11

codecholeric force-pushed the add-generic-type-info branch from f49af67 to bbfd4d5 Compare September 11, 2020 13:13

codecholeric merged commit 17a0747 into master Sep 12, 2020

codecholeric deleted the add-generic-type-info branch September 12, 2020 14:45

codecholeric mentioned this pull request Dec 3, 2020

Add JavaClass type parameters to dependencies #484

Merged

BenjaminKlatt mentioned this pull request Jan 27, 2021

Expose type parameters of JavaClass #115

Closed

Add generic type info to JavaClass #398

Add generic type info to JavaClass #398

Uh oh!

Conversation

codecholeric commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

hankem left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hankem commented Aug 31, 2020

Uh oh!

codecholeric commented Aug 31, 2020

Uh oh!

codecholeric commented Aug 31, 2020

Uh oh!

hankem commented Sep 4, 2020

Uh oh!

codecholeric commented Sep 5, 2020

Uh oh!

codecholeric commented Sep 5, 2020

Uh oh!

codecholeric commented Sep 5, 2020

Uh oh!

hankem commented Sep 6, 2020

Uh oh!

codecholeric commented Sep 6, 2020

Uh oh!

Uh oh!

Add generic type info to `JavaClass` #398

Add generic type info to `JavaClass` #398

codecholeric commented Jul 14, 2020 •

edited

Loading