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

More easily detect overridden methods & calls to super #982

Open
timtebeek opened this issue Oct 16, 2022 · 9 comments · May be fixed by #1040
Open

More easily detect overridden methods & calls to super #982

timtebeek opened this issue Oct 16, 2022 · 9 comments · May be fixed by #1040

Comments

@timtebeek
Copy link
Contributor

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.

@codecholeric
Copy link
Collaborator

Hey, I agree that this would be nice to have in the core, since it already came up a couple of times. Regarding your current code it's hard for me to say where the problem is, because if I just copy your code and add some dummy code I can't reproduce the problem 🤔 If you want me to look into your concrete example, then maybe you can give me a full example to reproduce the problem? One thing I'm not completely sure about is why in hasMatchingNameAndParameters you test descriptor and rawParameterTypes 🤔 A method descriptor should already contain all the parameter types + the return type, no? What I could imagine especially difficult is when generics and bridge methods come in 🤔

In any case, regarding the API, would you picture something like boolean JavaMethod.isOverridden()? I.e. would that be good enough for your use case?

@timtebeek
Copy link
Contributor Author

Hmm I won't have access to a laptop to provide you with the compete example until January, as I'm on holiday right now. I did have the same use case in this example. Can't mentally reproduce the exact edge cases for the duplicated checks, but know I went through quite a few iterations before I landed on this implementation, with feedback from all the projects where I applied this.

Indeed a boolean method to indicate when a method overrides (any) super class or interface method would be helpful!

@java-coding-prodigy
Copy link

Could I take on this issue?

@codecholeric
Copy link
Collaborator

@java-coding-prodigy sure, if you're open I'd be happy 🙂

@java-coding-prodigy
Copy link

java-coding-prodigy commented Dec 5, 2022

@java-coding-prodigy sure, if you're open I'd be happy 🙂

Yes, I am ready to contribute.
Since this is my first time working with this API I might need some guidance along the way, hope that's alright.

@codecholeric
Copy link
Collaborator

Sure, you can always ask any questions and I'll try to support you as well as possible 🙂

@java-coding-prodigy
Copy link

So what exactly am I supposed to implement here?
Just a boolean isOverriden? or something else?

@codecholeric
Copy link
Collaborator

I think that would be a good start, yes 🙂 If we later on see that there is something more we need, we can still extend it. But from what I saw so far isOverridden() would be everything necessary (correct me if I'm wrong @timtebeek ?)

@timtebeek
Copy link
Contributor Author

That would be very helpful indeed! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants