Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using MethodReferences is still breaking soundness #1258

Closed
ben-Draeger opened this issue Mar 5, 2024 · 5 comments
Closed

Using MethodReferences is still breaking soundness #1258

ben-Draeger opened this issue Mar 5, 2024 · 5 comments

Comments

@ben-Draeger
Copy link

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
Copy link
Member

hankem commented Mar 5, 2024

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:

  • A rule using callMethodWhere

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

    explicitly forbids method calls (including those from lambda expressions).

  • If you additionally want to forbid method references, you can use accessTargetWhere:

    @ArchTest
    ArchRule noMethodAccess = noClasses().should().accessTargetWhere(
            // WRONG: owner(assignableTo(Target.class)).and(nameMatching("method"))  // see #1260
            target(owner(assignableTo(Target.class))).and(nameMatching("method"))  // Thanks to ben-Draeger for the fix! #issuecomment-1984051341
    );

¹ Example:

The following java code:
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<Integer> 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."<init>":()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<java.lang.Integer> 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<Ljava/lang/Integer;>;

  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
Copy link
Author

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
Copy link
Member

hankem commented Mar 6, 2024

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

@ben-Draeger
Copy link
Author

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
Copy link
Member

hankem commented Mar 8, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants