Open ben-Draeger opened 5 months ago
I've already seen this unfortunate confusion in https://github.com/TNG/ArchUnit/issues/1152#issuecomment-1674776006. 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?
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.
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 DescribedPredicate
s to the varargs parameter. For those, we could probably offer something like
public static <T> DescribedPredicate<T> and(Iterable<DescribedPredicate<? super T>> predicates)
I spottet a strange phaenomenon when using accessTargetWhere():
when using it like this:
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:
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
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:
which builds the AndPredicate correctly.
Now my questions are:
NOTE: a similar problem seems to exist for the method or() in the same class as well.