ArchUnit icon indicating copy to clipboard operation
ArchUnit copied to clipboard

Unused local variable declarations seem not to be considered dependencies

Open AukeHaanstra opened this issue 1 year ago • 8 comments

Unused local variable declarations or unused objects seem not to be considered dependencies, neither are their corresponding import statements. Is this a limitation of ArchUnit, a bug, or am I missing something? Only in case of an exception, there is a functional dependency. For me this became apparent when trying to modularize an application whose boundaries were set by ArchUnit. I needed to refactor the code that was catching a dependency which it ought not to have depended on, which for me was rather unexpected.

Please see: https://github.com/AukeHaanstra/ArchUnitIssue/blob/main/src/test/java/org/example/DependencyAssertionsTest.java

AukeHaanstra avatar Jul 11 '23 15:07 AukeHaanstra

Thanks for raising the issue and providing the example project; that's very helpful! 💚

Some of the presumably missing violations are due to the fact that local variables are currently not analyzed (#768), and that import "statements" do not even exist at byte code level. Therefore your classes F and H don't have dependencies to A nor Ex:

class F {
    void doF() {
        A a = null; // dependency NOT detected
    }
}

class H {
    public void doH() {
        new B().giveEx(); // dependency NOT detected
        Ex ex = new B().giveEx(); // dependency NOT detected
    }
}

ArchUnit does not detect a dependency if there is no specific reference in the byte code. Similarly to #1012, this in your class D:

log.info("Exception from ex", ex);

is just an invokeinterface of InterfaceMethod org/slf4j/Logger.info:(Ljava/lang/String;Ljava/lang/Throwable;)V, but no specific dependency to Ex.

hankem avatar Jul 11 '23 22:07 hankem

But I'm actually unsure whether

    public void doD() {
        B b = new B();
        try {
            b.doB();
        } catch (Ex ex) { // dependency NOT detected
            log.info("Exception from ex", ex);
        }
    }

shouldn't actually have the dependency to Ex that you expected, since classes.get(D.class).getMethod("doD").getTryCatchBlocks().iterator().next().getCaughtThrowables() indeed contains Ex. 🤔

hankem avatar Jul 11 '23 22:07 hankem

So the situation is that ArchUnit could be able to detect that type of dependency, though at this moment, this has not been implemented yet?

AukeHaanstra avatar Jul 12 '23 10:07 AukeHaanstra

Yes, exactly: The domain object for the TryCatchBlock is already available, but this information was not added to the JavaClassDependencies.

hankem avatar Jul 12 '23 12:07 hankem

I think I might try to implement this then. It seems like a quite small and straightforward issue. Though I don't yet know about possibly complicating factors like nested or anonymous classes and nested try-catch statements. I think the first feature to implement would be for the simplest case as in my tests. Besides that I have some difficulty recognizing the ArchUnit API in the ArchUnit test suite, they seem to be white-box type and you seem to have constructed a DSL there. Is there some information on this for new developers, or is it simply in the code?

AukeHaanstra avatar Jul 12 '23 15:07 AukeHaanstra

As a workaround, you can also use a user-defined condition like this:

static imports
import static com.tngtech.archunit.core.domain.JavaClass.Predicates.resideInAPackage;
import static com.tngtech.archunit.lang.SimpleConditionEvent.satisfied;
import static com.tngtech.archunit.lang.conditions.ArchConditions.dependOnClassesThat;
ArchCondition<JavaClass> dependOnClassesInPackage(String packageIdentifier) {
    return dependOnClassesThat(resideInAPackage(packageIdentifier))
        .or(new ArchCondition<JavaClass>("<overridden>") {
            @Override
            public void check(JavaClass javaClass, ConditionEvents events) {
                for (JavaCodeUnit codeUnit : javaClass.getCodeUnits()) {
                    for (TryCatchBlock tryCatchBlock : codeUnit.getTryCatchBlocks()) {
                        tryCatchBlock.getCaughtThrowables().stream()
                            .filter(resideInAPackage(packageIdentifier))
                            .map(throwable -> satisfied(
                                codeUnit,
                                codeUnit.getDescription() + " catches " + throwable.getName() + " in " + tryCatchBlock.getSourceCodeLocation()
                            ))
                            .forEach(events::add);
                    }
                }
            }
        })
        .as("depend on classes in package '%s'", packageIdentifier);
}

hankem avatar Jul 12 '23 23:07 hankem

Ah, that's a neat workaround, thanks a lot! I updated my example tests accordingly.

AukeHaanstra avatar Jul 13 '23 20:07 AukeHaanstra

I guess it would make sense to add the types found within this custom condition to be just part of JavaClassDependencies, right?

codecholeric avatar Aug 05 '23 16:08 codecholeric