ArchUnit type Predicate Problem When Class Used in Array Declaration

[Bill]

(description below is excerpted from the README on this repo: https://github.com/Bill/archunitarraytest)

ArchUnit's type predicate doesn't seem to match a class used in an array declaration, when that type predicate is used inside an or predicate, to define an exception to some global condition.

Class Bar.java has a method with this signature:

  static Foo[] baz()

Our rule over in ArchUnitArrayTest.java says Bar can depend on classes in its own package or java.lang or:

.or(type(Foo.class))

We expect the test to pass. But it fails with:

Exception in thread "main" java.lang.AssertionError: Architecture Violation [Priority: MEDIUM] - Rule 'classes that have simple name 'Bar' should only depend on classes that reside in a package 'core.sub' or reside in a package 'java.lang' or type core.Foo' was violated (1 times):
Method <core.sub.Bar.baz()> has return type <[Lcore.Foo;> in (Bar.java:0)
	at com.tngtech.archunit.lang.ArchRule$Assertions.assertNoViolation(ArchRule.java:91)
	at com.tngtech.archunit.lang.ArchRule$Assertions.check(ArchRule.java:81)
	at com.tngtech.archunit.lang.ArchRule$Factory$SimpleArchRule.check(ArchRule.java:195)
	at com.tngtech.archunit.lang.syntax.ObjectsShouldInternal.check(ObjectsShouldInternal.java:81)
	at core.ArchUnitArrayTest.main(ArchUnitArrayTest.java:22)

if you go over to Bar.java and eliminate the Foo[] reference by changing the baz() signature to:

static Foo baz()

…then ArchUnit will throw no exception.

Array declarations, seem to work in other situations. See, for instance, the definition of baz2():

static Quux[] baz2()

That signature generates no exception. ArchUnit recognizes that the array type is derived from Quux and that Quux resides in the core.sub package.

A workaround is to add a clause like this to the rule:

.or(type(Foo[].class))

But it seems wrong to have to explicitly mention array types.

[codecholeric]

From my point of view this behavior seems as expected. The JLS considers Foo a different type than Foo[], and the type predicate matches exactly the Java type. So I wouldn't consider your last example a workaround but the natural behavior of the type predicate.
There is no predefined predicate to match types or array component types, but you could easily write it yourself (I've just noticed that JavaClass still doesn't offer a way to get the component type of an array ☹️ Going to open a new issue for it -> #187)

DescribedPredicate<JavaClass> typeOrArrayComponentType(final Class<?> type) {
    return new DescribedPredicate<JavaClass>("type or array component type " + type.getName()) {
        @Override
        public boolean apply(JavaClass javaClass) {
            String typeOrComponentTypeName = javaClass.getName()
                    .replace("[L", "")
                    .replace("[", "")
                    .replace(";", "");
            return typeOrComponentTypeName.equals(type.getName());
        }
    };
}

This could be written in a nicer way, if the component type could already be queried from JavaClass, but for now I think it should still suffice.

[Bill]

Perhaps my Java thinking is weak, but I view Foo[] similarly to Collection<Foo>. I don't write separate predicates for e.g. type(Collection<Foo>) or type(AtomicReference<Foo>).

What I'm trying to express with a type(Foo) predicate, is a dependence on Foo. That works for any reference to the Foo class, or its members, and it works for situations where generics are parameterized on the Foo class right?

That's why I find it surprising that type(Foo) doesn't imply type(Foo[]) also.

[codecholeric]

That this works differently with Collection<Foo> is because Collection resides within java.util. A while ago I changed the behavior with respect to arrays, because if you consider the Java Reflection API then Foo[] would actually not have any package. By popular demand(TM) I've changed the behavior with 0.10.0 to make JavaClass.getPackageName() return the package of the component type instead of empty.
I think it's important to have a type predicate that matches exactly. I get that for your use case you would want a different behavior, but in general the type(Foo.class) predicate should match the type exactly.
If you take type parameters again, consider you have your own type Bar<T> and you want to forbid the use of Bar<?>, but allow the use of Foo. Then how should Bar<Foo> be treated? In my opinion that just depends on your intentions.
So I just think those are two different predicates, the one that matches the type exactly and the one that matches any sort of dependency on that type like Foo[], Collection<Foo> and Bar<Foo>. And even there it is not clear if you want to treat all parameterized types the same or just want to allow collection types, etc.

[Bill]

@codecholeric said:

By popular demand(TM) I've changed the behavior with 0.10.0 to make JavaClass.getPackageName() return the package of the component type instead of empty.

Ah so that's why static Quux[] baz2() generates no error—because Quux is in core.sub.

So an array of Foo resides in the same package as Foo as far as ArchUnit is concerned.

Back to my Collection analogy. I guess because of type erasure, Collection<Foo> results in dependency on the Collection class only. No reference to the Foo class remains after compilation. So I can see why that is different from Foo[].

While I think I understand all that, I still struggle to understand why I'd want to allow a dependency on Foo but not allow the construction of array of Foo. An array type is a sibling of a reference type. They are both types derived from some object type (in this case, Foo). That ArchUnit treats array types as second-class citizens is surprising.

Thinking of it another way. In my mind, the purpose of ArchUnit is to enforce things like modularity, "levelization", no circular dependencies. Treating array types as second class seems to put ArchUnit into a different realm.

But from your explanation @codecholeric, I have come to understand that ArchUnit may actually be a tool for expressing what types (in detail) that a class may reference. If that's what ArchUnit is actually doing, then wouldn't we want to let it talk about primitive types as well (not just object types)? That would give rise to predicates like: typeInt(), typeDouble(), etc. Oh and I suppose typeArrayInt(), typeArrayDouble() and friends too. Or maybe typeArray(typeInt()), typeArray(typeDouble()). But why stop there. Once we are talking explicitly about primitives and arrays, why not treat reference types regularly also, renaming the type() predicate to typeReference().

Maybe what I just described already exists right underneath the surface API. I guess it just wasn't apparent from my first explorations into the tool. It does seem though, that there are two levels here. The level of "types" (TypeUnit? anybody?) and the level of "dependencies" (between classes and between packages): ArchUnit.

In closing: I really appreciate the discussion, the workaround (typeOrArrayComponentType()), the ticket to fix retrieval of component type of an array (#187), and also ArchUnit. It's a great tool!

[codecholeric]

Maybe we also talked a little bit about different things 😉 I completely agree with your case, if you want to allow a dependency to Foo, it makes sense to also allow a dependency to Foo[] in pretty much all cases I can think of.
What I was talking about was the generic predicate type(..), because that predicate can just be used in many different cases, and the implication that is correct for dependency might not be correct there anymore.
So if anything, then I think the dependOnClassesThat(predicate) behavior would have to be changed to also match if predicate.apply(javaClass.getComponentType()) == true.
I'm just wondering if that is the correct behavior for all predicates supplied to dependOnClassesThat(..) and not just for type(..)?

[hankem]

You can already talk about primitive types or array types to some extent, e.g.:

    @ArchTest
    private ArchRule i_methods_should_return_int = methods()
            .that().haveNameMatching("i.*")
            .should().haveRawReturnType(int.class);

    @ArchTest
    private ArchRule ai_methods_should_return_int_array = methods()
            .that().haveNameMatching("ai.*")
            .should().haveRawReturnType(int[].class);

    static class Example {
        int i_ok() { return 0; }
        long i_fail() { return 0; }
        int[] ai_ok() { return new int[]{}; }
        long[] ai_fail() { return new long[]{}; }
    }

[odrotbohm]

I am running into a similar problem with disallowed dependencies not being properly discovered:

public class Foo {

	private List<Bar> bars;
	private Bar bar;
}

// Foo should not depend on Bar

ArchRuleDefinition.noClasses().that() //
	.haveSimpleName(Foo.class.getSimpleName()) //
	.should().dependOnClassesThat().haveSimpleName(Bar.class.getSimpleName()) //
	.check(classes);

This check reports bar as violating the rule, but not bars, which – IMO – is wrong. With the List declaration (or essentially any kind of generics), Foo still creates a dependency to Bar in the sense of Bar being required on the classpath to be able to use Foo.

Assume you had all disallowed references hidden in some generics context (Optional etc.), the architecture rule would completely dismiss an offending candidate.

Just one note about generics reification as it's quite often misunderstood – and actually is here. By no means generics information is discarded after compilation. Of course that information is around as how would the compiler of client code verify the proper usage of the generic code. Reification talks about generics information of objects, not types. The component type of a List<String> object is not discoverable at runtime, it very well is from a MyList extends List<String> as the additional type can be used to reflectively inspect the type hierarchy.

[hankem]

The need to access generic type parameters has already been described with #115, but not yet implemented. 🤷

[odrotbohm]

That's a helpful pointer, Manfred, thanks. 👍

[codecholeric]

Sorry @odrotbohm, yes, that's still open ☹️ It turned out to be massively more complicated than thought, and it would pretty much break most of the core API I think. You could probably cover it by using reflection, if it is very important to you (i.e. enrich the info using Java...reflect() and checking generic type parameters). But that is also pretty complicated in a generic way, e.g.

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

On the upside, if your only dependency is the type parameter, that pretty much means, that you never do anything with those dependencies in your class. So it is hopefully easy to resolve by relaxing the type bound (the only time I can imagine that not to be correct is, if A -> B -> C where A needs type Foo, B only passes it on as a generic collection or Optional and C again accesses type Foo, but then I would wonder if there is some other strange stuff going on.

[codecholeric]

Nevertheless I do of course plan to have this this issue solved at some point in the future, if I do it myself I just need to allocate some sufficient time slot to focus in my schedule 😉

[liechtir]

Hi, I'm writing a rule that checks for all constructor calls to be wrapped into a factory. So no class shall call the constructor of Foo except the factory which is annotated with @Factory(Foo.class)

With ArchUnit, I can get all constructors like ArchRuleDefinition.constructors() and check the JavaConstructorCall's to match certain criteria.
However, it does not contain calls to new Foo[]. What is the way to also get all instantiations of all array types so I can check them, too?

[TAndronicus]

I wrote a custom DescribedPredicate to compose it with others, maybe it will come in handy:

    private static DescribedPredicate<JavaClass> arrayOf(DescribedPredicate<? super JavaClass> predicate) {
        return new DescribedPredicate<JavaClass>("is an array of type that") {
            @Override
            public boolean test(JavaClass javaClass) {
                return javaClass.isArray() && predicate.test(javaClass.getBaseComponentType());
            }
        };
    }