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

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

Open
AukeHaanstra opened this issue Jul 11, 2023 · 8 comments

Comments

@AukeHaanstra
Copy link

AukeHaanstra commented Jul 11, 2023

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 AukeHaanstra changed the title Local variable declarations seem not to be considered dependencies Unused local variable declarations seem not to be considered dependencies Jul 11, 2023
@hankem
Copy link
Member

hankem commented Jul 11, 2023

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

hankem commented Jul 11, 2023

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

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

hankem commented Jul 12, 2023

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

@AukeHaanstra
Copy link
Author

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

hankem commented Jul 12, 2023

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);
}

@AukeHaanstra
Copy link
Author

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

@codecholeric
Copy link
Collaborator

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

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

3 participants