Method reference is not considered as a dependency

[dpfg]

Lets say there are two packages:

• org.company.domain
• org.company.integration

where one of classes in integration package has the following code:

Runnable a = DomainClass::methodName; // or DomainClass::new

and the following rule

@ArchTest
  public ArchRule integration_does_not_access_domain =
      noClasses()
          .that()
          .resideInAnyPackage("org.company.integration..")
          .should()
          .accessClassesThat()
          .resideInAnyPackage("org.company.domain..");

Check passes successfully, while when used with regular construction of DomainClass inside IntegrationClass check fails.

Seems like the root cause is the same as in #131. Dependency has been moved to the constant pool at compile time. In this case it gets replaced by invoke dynamic that refers to the constant pool. And this is a huge difference as now we are talking about legitimate not-dead code that violates constraints but not catched.

ASM exposes API to access constant pools so theoretically can be used to detect all references to classes.

Does it make sense to mention this limitations/corner cases in the documentation? Seems like problem doesn't get solution in near future.

[codecholeric]

Yeah, you're right, it's a shortcoming at the moment. ArchUnit uses the visitor-API though, but I still think this could be enhanced. I've looked into it, and I think even the case from #131 might be solvable with the current ASM version. For your case, I think the information could be retrieved by checking

org.objectweb.asm.MethodVisitor.visitInvokeDynamicInsn

While for #131, it might be possible to use

org.objectweb.asm.MethodVisitor.visitLdcInsn

At least with a quick check the first one had a reference to the target class of the method handle within the arguments, the second one was called specifically with the type of the target class. That would actually be cool to add those features somehow 😄

The info could easily be retrieved by extending com.tngtech.archunit.core.importer.JavaClassProcessor.

I guess a JavaCodeUnit could have JavaConstantAccess or something similar (like JavaFieldAccess), then JavaConstantAccess could be integrated into a JavaClass's Dependencies.

Similar a JavaCodeUnit could also have a JavaDynamicInvocation or something like that, to solve your actual problem.

This should be feasible, even though it's not trivial. But at least it should be possible to integrate this into the current ArchUnit code base without having to change much existing code.

I'll tag this and keep it here for now, maybe someone wants to jump in and work on this. Otherwise I'll try to get around to it as quickly as possible (which might unfortunately be a while ☹️)

[dpfg]

@codecholeric I will take a look at this closer a little bit later and if work it out will create a PR.

[codecholeric]

@dpfg Okay, cool 😃 Let me know, if I can support you in any way!

[ghost]

We have the same problem regarding lambda statements.

We are trying to check whether methods in some particular inner classes are ever actually accessed at all. We use an ArchRuleDefinition targeting these classes together with an ArchCondition collecting all "JavaMethod"s and checking getCallsOfSelf() on each. If this list is empty, the method has never been accessed and the test fails.

(for sake of completeness: we also tried getCallsToSelf(), getAccessesToSelf() and getAccessesFromSelf(), all with the same result)

For "normal" method calls this works fine, i.e. the test is successful if the method is used and fails if it doesn't. However, in cases when there is a lamda statement involved (e.g. exampleClass::exampleMethod), the test produces false positives, since it fails even though the method has been called in a lambda statement.

[KorSin]

I guess a JavaCodeUnit could have JavaConstantAccess or something similar (like JavaFieldAccess), then JavaConstantAccess could be integrated into a JavaClass's Dependencies.

Similar a JavaCodeUnit could also have a JavaDynamicInvocation or something like that, to solve your actual problem.

@codecholeric After reading through the code, I think I understand your idea of introducing an equivalent of JavaFieldAcess.
I tried some things and realised that another possibility would be, during the visitInvokeDynamicInsn call, to extract the called method reference and add it to the CodeUnit's accesHandler as done in here. With that, the above test would fail correctly.

There are some things I am not sure about:

• Maybe it is neccessary to have a JavaConstantAccess (as you suggested) class to test something specific like "class A accesses a method reference of class B" ?
• This approach does not work if the method reference is contained in a Lambda. But looking at the constant pool, I cannot find a way to retrieve this information. Maybe this has to be handled differently, for which we have getFieldAccesses() ignores accesses from lambdas #266.

[codecholeric]

Yes, but if you simply invoke the same accessHandler, then the reference will be interpreted as JavaMethodCall with an MethodCallTarget, right? For me this feels semantically wrong, because it is not a "method call", but a reference to a method. And also it might be beneficial to be able to distinguish the two. Otherwise later I can never write a rule that only targets calls or only targets method references.

I'm not super sure about how to extend the model though, I think we should sync about it 😉 Then we can also discuss the part about the lambda.

[stepan-prokipchyn-vitech]

Hey guys! Thank you for all the hard work you put into the framework.

I'm trying to create a rule that forbids throwing exceptions without a message. It is pretty simple to detect such cases:

throw new MyCustomExpcetion();

However there are also cases similar to this:

Optional
   .ofNullable(null)
   .orElseThrow(MyCustomExpcetion::new);

I wonder if this issue makes it impossible to detect passing constructor without parameter as a reference?

Just to be clear, the following piece of code should not violate the rule, because it actually calls another constructor overload:

Optional
   .ofNullable(null)
   .orElseThrow(() -> new MyCustomExpcetion("some details"));

BTW I'm eager to contribute to the solution, but unfortunately I don't quite understand the suggestion above

[codecholeric]

Hey, sorry, you just need to wait for the next release and it should work! (it's missing #649) I'll try to release within the next 1-2 weeks!!

[codecholeric]

This issue should be solved by #649