💡 Make visitors generated by Refastor templates return early where feasible

[Philzen]

What problem are you trying to solve?

Having a simple method migration recipe such as this:

        @BeforeTemplate void before(double actual, double expected, double delta, String msg) {
            Assert.assertEquals(actual, expected, delta, msg);
        }

        @AfterTemplate
        void after(double actual, double expected, double delta, String msg) {
            Assertions.assertEquals(expected, actual, delta, msg);
        }

will generate a visitMethodDeclaration implementation such as:

        public J visitMethodInvocation(J.MethodInvocation elem, ExecutionContext ctx) {
            // …
            return super.visitMethodInvocation(elem, ctx);
        }

Describe the solution you'd like

@MBoegers and i were wondering over another review whether it may be feasible to simply return elem; instead, leading to increased performance as the visitation of the method contents would be skipped.

Not sure if i'm overlooking something here (maybe anonymous classes within the method body?) but i believe it's worth contemplating as this notion applies to all visitors generated by refastor templates.

Have you considered any alternatives or workarounds?

Apart from hand-coding the recipes instead (which would violate the „If it can be declarative, it should be declarative“-convention, nothing comes to mind.

Are you interested in contributing this feature to OpenRewrite?

Potentially.

[timtebeek]

Hi! Linking a concrete example to get a little wider view on to an example generated visitMethodInvocation

rewrite-templating/src/test/resources/refaster/EscapesRecipes.java

Lines 99 to 110 in 347af90

	public J visitMethodInvocation(J.MethodInvocation elem, ExecutionContext ctx) {
	JavaTemplate.Matcher matcher;
	if ((matcher = before.matcher(getCursor())).find()) {
	maybeRemoveImport("com.sun.tools.javac.util.Convert");
	return embed(
	after.apply(getCursor(), elem.getCoordinates().replace(), matcher.parameter(0)),
	getCursor(),
	ctx,
	SHORTEN_NAMES
	);
	}
	return super.visitMethodInvocation(elem, ctx);

If we were to not invoke super.visitMethodInvocation I think we'd miss out on visiting arguments that might themselves contain method invocations. For instance with assertAll(Executable... executables), there an argument could be a lambda that contains Assert.assertEquals statements that you might want to replace. I'm not sure if there's a right/more performant way to exclude unnecessary visits to speed up execution, but open to suggestions.

[timtebeek]

For now I don't immediately see any points at which we could return early, as much as I'd love to be proven wrong. Closing this issue until a concrete example comes in, however much as the idea is appreciated!

[knutwannheden]

I also think that the generated code for the Refaster recipes is quite readable and easy to adapt. I think that is quite an acceptable workaround.

The alternative would be an annotation which would instruct the generator to not generate the super call. What fo you think about that @timtebeek ? As it doesn't really add API surface area, I think that would be OK and if someone is willing to work on it, we could have that solution quite soon.