TNG / ArchUnit

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

Unused local variable declarations seem not to be considered dependencies #1137

Open AukeHaanstra opened 1 year ago

AukeHaanstra commented 1 year ago

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

hankem commented 1 year ago

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 commented 1 year ago

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. 🤔

AukeHaanstra commented 1 year ago

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?

hankem commented 1 year ago

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

AukeHaanstra commented 1 year ago

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?

hankem commented 1 year ago

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

static imports ```java 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);
}
AukeHaanstra commented 1 year ago

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

codecholeric commented 1 year ago

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