TNG / ArchUnit

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

callMethod() ignores lambdas #981

Open rala72 opened 1 year ago

rala72 commented 1 year ago

I saw that #266 is resolved, but method calls described in callMethod are still not recognized in lambdas.

I wrote a test that should ensure, that we don't use optional get:

@ArchTest
private final ArchRule optional_get = freeze(noClasses()
  .should().callMethod(Optional.class, "get")
  .because("orElseThrow() does the same and is more descriptive (https://stackoverflow.com/a/49159955/2715720)"));

I put somewhere in the code the following lines to test it:

void test() {
  Optional.empty().get();
  Supplier s = Optional.empty()::get;
}

The first line is detected correctly, but the second did not.

codecholeric commented 1 year ago

The problem is that Optional::get is no "lambda" for ArchUnit, but a "method reference". The issue you refer to addresses issues like the following, where the call was not detected from origin() before, but some static method lambda$...:

Runnable origin() {
  return () -> {
    target();
  };
}

void target() {..}

So what you need to check for are calls and method references. Unfortunately, the API doesn't make this as convenient yet, but you can e.g. use the following custom code:

noClasses().should(dependOnMethodOptionalGet())

// ...

ArchCondition<JavaClass> dependOnMethodOptionalGet() {
  return new ArchCondition<JavaClass>("depend on method Optional.get(..)") {
    @Override
    public void check(JavaClass javaClass, ConditionEvents events) {
      Stream.concat(javaClass.getMethodCallsFromSelf().stream(), javaClass.getMethodReferencesFromSelf().stream())
          .filter(access -> access.getTargetOwner().isEquivalentTo(Optional.class) && access.getTarget().getName().equals("get"))
          .forEach(access -> events.add(SimpleConditionEvent.satisfied(javaClass, access.getDescription())));
    }
  };
}
rala72 commented 6 months ago

are there any plans to make this more convenient?

hankem commented 6 months ago
ArchUnit at least includes predefined predicates ``` import static com.tngtech.archunit.core.domain.JavaAccess.Predicates.target; import static com.tngtech.archunit.core.domain.JavaClass.Predicates.equivalentTo; import static com.tngtech.archunit.core.domain.properties.HasName.Predicates.nameMatching; import static com.tngtech.archunit.core.domain.properties.HasOwner.Predicates.With.owner; import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noClasses; ```

that allow you to use

    @ArchTest
    ArchRule optional_get = noClasses().should().accessTargetWhere(
            target(owner(equivalentTo(Optional.class))).and(nameMatching("get"))
    );

(but I understand that one would like to have that in a fluent API).

rala72 commented 6 months ago

thank you for pointing this out
are there any plans to add it to the fluent API (in any way)?

otherwise I would close this issue as not planned

hankem commented 6 months ago

I'd keep it. I think it's a very good suggestion. If one only had more time... 🙈