Usability: Java confuses the static and non-static versions of DescribedPredicate.and() when using the API the wrong way.

[ben-Draeger]

I spottet a strange phaenomenon when using accessTargetWhere():

when using it like this:

    @ArchTest
    private final static ArchRule doNotUseOptionalGet = noClasses()
            .should()
            .accessTargetWhere(owner(assignableTo(Optional.class)).and(nameMatching("get")))
            .because("We want to use Optional.orElseThrow() instead of Optional.get().");

then IntelliJ IDEA gives me the following rather cryptic warning

Static member 'com.tngtech.archunit.base.DescribedPredicate<com.tngtech.archunit.core.domain.properties.HasOwner<com.tngtech.archunit.core.domain.JavaClass>>.and(com.tngtech.archunit.base.DescribedPredicate<? super com.tngtech.archunit.core.domain.JavaAccess<?>>...)' accessed via instance reference

However, when ignoring this warning, the rule above flags all method calls to method named "get" as violations instead of only those to Optional.get().

After some trial-and-error I found out that rewriting the rule like this:

    @ArchTest
    private final static ArchRule doNotUseOptionalGet = noClasses()
            .should()
            .accessTargetWhere(target(owner(assignableTo(Optional.class))).and(nameMatching("get")))
            .because("We want to use Optional.orElseThrow() instead of Optional.get().");

solves both the warning and the problem.

The key to understanding this issue is that in the first version, the method call to and() in above rule calls the static method and() in line 160 of DescribedPredicate.java

    @SafeVarargs
    @PublicAPI(
        usage = Usage.ACCESS
    )
    public static <T> DescribedPredicate<T> and(DescribedPredicate<? super T>... predicates) {
        return and((Iterable)ImmutableList.copyOf(predicates));
    }

As one can see, this method builds and AndPredicate, but passed only the predicates (right-hand side) and not this (left-hand side) into it. The left-hand side of the and() is hence lost and the rule becomes
'no classes should access target where name matching 'get', because We want to use Optional.orElseThrow() instead of Optional.get().'

In the second version of the rule, the call to and() calls the non-static method and() in line 59 of DescribedPredicate.java:

    public DescribedPredicate<T> and(DescribedPredicate<? super T> other) {
        return new AndPredicate(this, other);
    }

which builds the AndPredicate correctly.

Now my questions are:

• why are there a static and a non-static method and() in DescribedPredicate?
• what is the static version that builds the AndPredicate in such a weird way good for?
  • should it not be really important to have it, then I would suggest to remove it in order to cause a compiler error when using the API the wrong way (like in the first example) instead of causing rather subtle issues like the one I encountered, the debugging of which causes headaces.

NOTE: a similar problem seems to exist for the method or() in the same class as well.

[hankem]

I've already seen this unfortunate confusion in #1152 (comment). Sorry for the inconvenience!

public static <T> DescribedPredicate<T> and(DescribedPredicate<? super T>... predicates) is a factory method to AND multiple predicates together, whereas public DescribedPredicate<T> and(DescribedPredicate<? super T> other) is an instance method to produce a predicate for this AND other.

Moving the static method somewhere else would be a breaking change, also for the legit usages, but the method could log a warning when used with a single argument; maybe that would help already?

[ben-Draeger]

Regarding the Problem at hand:
I can see that the non-static and() method must be in the class DescribedPredicate in order to allow your DSL to do conjunction of Predicates in an infix notation.

I can also see that Factory methods like the static and() are useful. However, do they really need to be in the same class so that the Java Typesystem can confuse one for the other? Or could they be moved to a different class (maybe one whose name ends in "Factory" to make its purpose clear?)

Regarding Breaking Changes:
The current situation is obviously broken. What good does it do to maintain backwards-compatibility in such a situation? IMHO, the priority should be to fix this (and similar issues) in order to ensure that ArchUnit rules have a well-defined semantics and behave as expected. When the Java Typesystem flags an error on an old rule after the fix, then there is a good chance that this rule was broken (= did not do what the user intended) anyway.

Regarding the proposed warning:
The warning can be logged only at runtime while this type of problem is best flagged (and hence prevented) directly while writing the rule. I'm afraid only the compiler / typesystem are able to do that.

[hankem]

The current situation is obviously broken.

It's not that obvious to me: A usage like DescribedPredicate.and(predicate1, predicate2) works as intended, right?

The warning can be logged only at runtime

There is no doubt that compile-time safety is more desirable. What if we changed the signature like this?

- public static <T> DescribedPredicate<T> and(DescribedPredicate<? super T>... predicates) 
+ public static <T> DescribedPredicate<T> and(DescribedPredicate<? super T> predicate1, DescribedPredicate<? super T> predicate2, DescribedPredicate<? super T>... morePredicates) 

I think that this would prevent the confusion without breaking legit use cases that spell out individual predicates as method arguments. It would only break use cases of the factory method that pass an array of DescribedPredicates to the varargs parameter. For those, we could probably offer something like

  public static <T> DescribedPredicate<T> and(Iterable<DescribedPredicate<? super T>> predicates)