Description
On a recent project we're heavily using the visitor pattern, and would like to ensure that people who override visitor methods call the super implementation. Seemed to me like a reasonable thing to check with ArchUnit, for fast feedback. Yet when implementing this I found there's some challenges with the current APIs, which might benefit from explicitly named constructs in the DSL.
Here's a reproduction sample of the domain:
class Tree {
UUID id;
List<Tree> nodes;
}
class TreeVisitor {
public Tree visit(Tree tree) {
// Also visit child nodes
for (Tree child : tree.nodes) {
visit(child);
}
return tree;
}
}
class GoodVisitor extends TreeVisitor {
@Override
public Tree visit(Tree tree) {
return super.visit(tree);
}
}
class IndifferentVisitor extends TreeVisitor {
// Does not override, should be OK
}
class BadVisitor extends TreeVisitor {
@Override
public Tree visit(Tree tree) {
return tree; // Lacks call to super.visit(tree), and thus traversal of child nodes
}
}
I'm able for the simplest case to test this using the below constructs:
import java.util.List;
import java.util.Set;
import java.util.UUID;
import java.util.function.Predicate;
import com.tngtech.archunit.base.DescribedPredicate;
import com.tngtech.archunit.core.domain.*;
import com.tngtech.archunit.core.domain.AccessTarget.CodeUnitCallTarget;
import com.tngtech.archunit.core.importer.ClassFileImporter;
import com.tngtech.archunit.junit.AnalyzeClasses;
import com.tngtech.archunit.lang.ArchCondition;
import com.tngtech.archunit.lang.ArchRule;
import com.tngtech.archunit.lang.ConditionEvents;
import com.tngtech.archunit.lang.SimpleConditionEvent;
import org.junit.jupiter.api.Test;
import static com.tngtech.archunit.core.domain.properties.HasName.Utils.namesOf;
import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.methods;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;
@AnalyzeClasses
class ArchitectureTest {
// @ArchTest
public static final ArchRule visitor_methods_should_call_super = methods()
.that().areDeclaredInClassesThat().areAssignableTo(TreeVisitor.class)
.and().haveNameStartingWith("visit")
.and(new OverridesSuperClassMethod())
.should(new CallSuperClassMethod())
.as("Overridden visit methods should call super class method")
.because("visitor nagivation requires calls to super class methods");
@Test
void verify_visitor() {
JavaClasses javaClasses = new ClassFileImporter().importPackagesOf(TreeVisitor.class);
AssertionError error = assertThrows(AssertionError.class,
() -> visitor_methods_should_call_super.check(javaClasses));
assertThat(error.getMessage()).startsWith(
"""
Architecture Violation [Priority: MEDIUM] - Rule 'Overridden visit methods should call super class method, because visitor nagivation requires calls to super class methods' was violated (1 times):
Method <com.github.timtebeek.BadVisitor.visit(com.github.timtebeek.Tree)> does not call super class method at (ArchitectureTest.java:""");
}
}
class OverridesSuperClassMethod extends DescribedPredicate<JavaMethod> {
OverridesSuperClassMethod() {
super("overrides super class method");
}
@Override
public boolean test(JavaMethod inputMethod) {
return inputMethod.getOwner().getAllRawSuperclasses().stream()
.flatMap(c -> c.getMethods().stream())
.anyMatch(hasMatchingNameAndParameters(inputMethod));
}
static Predicate<JavaMethod> hasMatchingNameAndParameters(JavaMethod inputMethod) {
return superMethod -> superMethod.getName().equals(inputMethod.getName())
&& superMethod.getRawParameterTypes().size() == inputMethod.getRawParameterTypes().size()
&& (superMethod.getDescriptor().equals(inputMethod.getDescriptor())
|| namesOf(superMethod.getRawParameterTypes())
.containsAll(namesOf(inputMethod.getRawParameterTypes())));
}
}
class CallSuperClassMethod extends ArchCondition<JavaMethod> {
CallSuperClassMethod() {
super("call super class method");
}
@Override
public void check(JavaMethod inputMethod, ConditionEvents events) {
String ownerFullName = inputMethod.getOwner().getFullName();
List<JavaClass> inputParameters = inputMethod.getRawParameterTypes();
Set<JavaMethodCall> callsFromSelf = inputMethod.getMethodCallsFromSelf();
boolean satisfied = callsFromSelf.stream()
.map(JavaCall::getTarget)
.filter(target -> !ownerFullName.equals(target.getOwner().getFullName()))
.filter(target -> target.getName().equals(inputMethod.getName()))
.map(CodeUnitCallTarget::getRawParameterTypes)
.anyMatch(targetTypes -> targetTypes.size() == inputParameters.size());
String message = String.format("%s does not call super class method at %s",
inputMethod.getDescription(), inputMethod.getSourceCodeLocation());
events.add(new SimpleConditionEvent(inputMethod, satisfied, message));
}
}
Yet the OverridesSuperClassMethod
& CallSuperClassMethod
conditions are awkward and fragile to implement.
Worse still, the conditions as they are right now break down when we add subclasses into the mix:
class JavaTree extends Tree {
}
class GoodJavaVisitor extends TreeVisitor {
@Override
public JavaTree visit(Tree tree) {
return (JavaTree) super.visit(tree);
}
}
class BadJavaVisitor extends TreeVisitor {
@Override
public JavaTree visit(Tree tree) {
return (JavaTree) tree;
}
}
Architecture Violation [Priority: MEDIUM] - Rule 'Overridden visit methods should call super class method, because visitor nagivation requires calls to super class methods' was violated (4 times):
Method <com.github.timtebeek.BadJavaVisitor.visit(com.github.timtebeek.Tree)> does not call super class method at (ArchitectureTest.java:1)
Method <com.github.timtebeek.BadJavaVisitor.visit(com.github.timtebeek.Tree)> does not call super class method at (ArchitectureTest.java:136)
Method <com.github.timtebeek.BadVisitor.visit(com.github.timtebeek.Tree)> does not call super class method at (ArchitectureTest.java:119)
Method <com.github.timtebeek.GoodJavaVisitor.visit(com.github.timtebeek.Tree)> does not call super class method at (ArchitectureTest.java:1)
Not immediately clear to me why this would add a violation for GoodJavaVisitor
or two violations for BadJavaVisitor
.
Would it be possible to implement detection of overridden methods & calls to super methods properly once in ArchUnit, and make that available through the API? That way more people can add similar checks more easily.