TNG / ArchUnit

A Java architecture test library, to specify and assert architecture rules in plain Java
http://archunit.org
Apache License 2.0
3.17k stars 287 forks source link

Using MethodReferences is still breaking soundness #1258

Closed ben-Draeger closed 5 months ago

ben-Draeger commented 5 months ago

I have a question.

Java 8 was released in 2014[1] and introduced Closures along with Lambda expressions and method references into the language[2]. Since then, a number of issues have been opened by different people noting that ArchUnit does not support method references (see for instance #713, #215, #649). All of these issues are closed for a while now (since 2022 to be exact), so I would have expected ArchUnit to support method references by now.

However, when I test the newest version 1.2.1, I find that ArchUnit is still (in 2024 - 10 years after that feature was introduced!) unable to deal with method references.

Concretely,

given the following class whose sole method is for some reason prohibited:

public class Target {

    public static Integer methodInQuestion(Integer arg) {
        System.out.println("Target.methodInQuestion() called!");
        return 0;
    }

}

and this being enforced by the following ArchUnit Rule:

    @ArchTest
    private final static ArchRule checkForIllegitimateMethodCall =
            noClasses()
            .should()
            .callMethodWhere(target(nameMatching("methodInQuestion"))
                    .and(target(owner(assignableTo(base.Target.class)))))
            .because("please do not call Target.methodInQuestion().");

then we see that this works fine on a program like

    public static void main(String[] args)
    {
        System.out.println("start");
        var result = Target.methodInQuestion(1);
        System.out.println("end");
    }

but fails to detect any problem in the following program

    public static void main(String[] args)
    {
        System.out.println("start");
        var result = List.of(1).stream().map(Target::methodInQuestion);
        result.toList();
        System.out.println("end");
    }

although the two are clearly equivalent and both programs clearly call Target.methodInQuestion() as apparent by their outputs:

start
Target.methodInQuestion() called!
end

Could you help me understand what is going on?

References [1] https://en.wikipedia.org/wiki/Java_version_history [2] https://www.oracle.com/java/technologies/javase/8-whats-new.html

hankem commented 5 months ago

Method calls and method references (and also lambda expressions) are very different in the byte code (invokevirtual/invokestatic vs. invokedynamic)¹, and correspondingly in ArchUnit's domain model (JavaMethodCall vs. JavaMethodReference; see also #1221).

These different domain objects need to be addressed accordingly. There is a fluent API for both kinds of rules:


¹ Example:

The following java code: ```java class Target { static void method(int i) { System.out.println("method called!"); } void methodCall() { Target.method(1); } void methodCall() { Target.method(1); } void methodReference() { stream1().forEach(Target::method); } void lambda() { stream1().forEach(i -> Target.method(i)); } static Stream stream1() { return Stream.of(1); } } ```
may be compiled to the following byte code: ``` class Target { Target(); descriptor: ()V flags: Code: stack=1, locals=1, args_size=1 0: aload_0 1: invokespecial #1 // Method java/lang/Object."":()V 4: return LineNumberTable: line 4: 0 static void method(int); descriptor: (I)V flags: ACC_STATIC Code: stack=2, locals=1, args_size=1 0: getstatic #2 // Field java/lang/System.out:Ljava/io/PrintStream; 3: ldc #3 // String method called! 5: invokevirtual #4 // Method java/io/PrintStream.println:(Ljava/lang/String;)V 8: return LineNumberTable: line 6: 0 line 7: 8 void methodCall(); descriptor: ()V flags: Code: stack=1, locals=1, args_size=1 0: iconst_1 1: invokestatic #5 // Method method:(I)V 4: return LineNumberTable: line 10: 0 line 11: 4 void methodReference(); descriptor: ()V flags: Code: stack=2, locals=1, args_size=1 0: invokestatic #6 // Method stream:()Ljava/util/stream/Stream; 3: invokedynamic #7, 0 // InvokeDynamic #0:accept:()Ljava/util/function/Consumer; 8: invokeinterface #8, 2 // InterfaceMethod java/util/stream/Stream.forEach:(Ljava/util/function/Consumer;)V 13: return LineNumberTable: line 14: 0 line 15: 13 void lambda(); descriptor: ()V flags: Code: stack=2, locals=1, args_size=1 0: invokestatic #6 // Method stream:()Ljava/util/stream/Stream; 3: invokedynamic #9, 0 // InvokeDynamic #1:accept:()Ljava/util/function/Consumer; 8: invokeinterface #8, 2 // InterfaceMethod java/util/stream/Stream.forEach:(Ljava/util/function/Consumer;)V 13: return LineNumberTable: line 18: 0 line 19: 13 static java.util.stream.Stream stream(); descriptor: ()Ljava/util/stream/Stream; flags: ACC_STATIC Code: stack=1, locals=0, args_size=0 0: iconst_1 1: invokestatic #10 // Method java/lang/Integer.valueOf:(I)Ljava/lang/Integer; 4: invokestatic #11 // InterfaceMethod java/util/stream/Stream.of:(Ljava/lang/Object;)Ljava/util/stream/Stream; 7: areturn LineNumberTable: line 22: 0 Signature: #27 // ()Ljava/util/stream/Stream; private static void lambda$lambda$0(java.lang.Integer); descriptor: (Ljava/lang/Integer;)V flags: ACC_PRIVATE, ACC_STATIC, ACC_SYNTHETIC Code: stack=1, locals=1, args_size=1 0: aload_0 1: invokevirtual #12 // Method java/lang/Integer.intValue:()I 4: invokestatic #5 // Method method:(I)V 7: return LineNumberTable: line 18: 0 } BootstrapMethods: 0: #41 invokestatic java/lang/invoke/LambdaMetafactory.metafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite; Method arguments: #42 (Ljava/lang/Object;)V #43 invokestatic Target.method:(I)V #44 (Ljava/lang/Integer;)V 1: #41 invokestatic java/lang/invoke/LambdaMetafactory.metafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite; Method arguments: #42 (Ljava/lang/Object;)V #48 invokestatic Target.lambda$lambda$0:(Ljava/lang/Integer;)V #44 (Ljava/lang/Integer;)V ```
ben-Draeger commented 5 months ago

Thanks for the clarification, accessTargetWhere() indeed works in both cases.

I will adapt our rules and that should fix the problem in our case.

However, please make sure that this is mentioned in the Documentation to enable users to choose the API that is appropriate for their usecase.

In [1], both callMethodWhere() and accessTargetWhere() do not provide any JavaDoc. Also in [2], callMethodWhere() is used 2 times without mentioning this restriction and accessTargetWhere() is not mentioned at all.

References [1] https://javadoc.io/doc/com.tngtech.archunit/archunit/latest/com/tngtech/archunit/lang/conditions/ArchConditions.html#callMethodWhere(com.tngtech.archunit.base.DescribedPredicate) [2] https://www.archunit.org/userguide/html/000_Index.html

hankem commented 5 months ago

You're clearly right that the documentation can be improved. 🙈 We also welcome pull requests to this open source project.

ben-Draeger commented 5 months ago

A note for the people that come after us:

when using accessTargetWhere(), it is better to do it this way:

@ArchTest
ArchRule noMethodAccess = noClasses().should().accessTargetWhere(
        target(owner(assignableTo(Target.class))).and(nameMatching("method"))
);

Otherwise, you might run into rather subtle issues.

hankem commented 5 months ago

Aaah... 🤯 I'm sorry for that!