RemoveMethodInvocationsVisitor can remove static method

[dralagen]

What's changed?

Add test to verify if RemoveMethodInvocationsVisitor can delete static method

What's your motivation?

After moving recipe org.openrewrite.java.migrate.RemoveMethodInvocation defined on project rewrite-migrate-java to org.openrewrite.java.RemoveMethodInvocations in `rewrite-java' some custom recipe now failed

type: specs.openrewrite.org/v1beta/recipe
name: org.openrewrite.java.testing.easymock.EasymockRemoveReplay
displayName: Remove EasyMock `replay()`"
description: Remove EasyMock `replay()` method calls.
recipeList:
  - org.openrewrite.java.migrate.RemoveMethodInvocation:
      methodPattern: org.easymock..EasyMock replay(..)

this exemple work but the recipe has been removed :

• recipe : https://github.com/dralagen/rewrite-testing-frameworks/blob/java-migrate-removeMethodInvocation/src/main/resources/META-INF/rewrite/easymock.yml
• test : https://github.com/dralagen/rewrite-testing-frameworks/blob/java-migrate-removeMethodInvocation/src/test/java/org/openrewrite/java/testing/easymock/EasymockRemoveReplayTest.java

this exemple break TU : https://github.com/dralagen/rewrite-testing-frameworks/blob/removeMethodInvocations/src/main/resources/META-INF/rewrite/easymock.yml

Anyone you would like to review specifically?

@timtebeek I created the PR as draft with test the remove Method static invocation detect on the Migration from easymock to mockito

Have you considered any alternatives or workarounds?

I see the same issue on PR :

• Migration from Easymock to Mockito rewrite-testing-frameworks#639

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[timtebeek]

Thanks for calling that out @dralagen ! The old recipe in rewrite-migrate-java used this visitor that still exists in rewrite-static-analysis.
https://github.com/openrewrite/rewrite-static-analysis/blob/b412e1d6b0b6fd8bba17e6f29dbb6af63749e638/src/main/java/org/openrewrite/staticanalysis/RemoveMethodCallVisitor.java#L36-L93

We'll need to work out the difference between that and the new RemoveMethodInvocationsVisitor that lives here.

rewrite/rewrite-java/src/main/java/org/openrewrite/java/RemoveMethodInvocationsVisitor.java

Lines 36 to 48 in 104f705

	public class RemoveMethodInvocationsVisitor extends JavaVisitor<ExecutionContext> {
	private final Map<MethodMatcher, Predicate<List<Expression>>> matchers;
	public RemoveMethodInvocationsVisitor(Map<MethodMatcher, Predicate<List<Expression>>> matchers) {
	this.matchers = matchers;
	}
	public RemoveMethodInvocationsVisitor(List<String> methodSignatures) {
	this(methodSignatures.stream().collect(Collectors.toMap(
	MethodMatcher::new,
	signature -> args -> true
	)));
	}

Any help appreciated, but not expected or required; just let me know if you're open to explore this further and know that we already appreciate you logging this as a runnable issue.

[knutwannheden]

Looking at the tests I believe the static method call is not removed by RemoveMethodInvocationsVisitor as the call is part of a local variable assignment. That is of course something that could be changed (e.g. completely remove the initializer or replace it with null), but as it currently stands this visitor will just do nothing in this situation:

rewrite/rewrite-java/src/main/java/org/openrewrite/java/RemoveMethodInvocationsVisitor.java

Lines 124 to 132 in cf71b0d

	private boolean isStatement() {
	return getCursor().dropParentUntil(p -> p instanceof J.Block ||
	p instanceof J.Assignment ||
	p instanceof J.VariableDeclarations.NamedVariable ||
	p instanceof J.Return ||
	p instanceof JContainer ||
	p == Cursor.ROOT_VALUE
	).getValue() instanceof J.Block;
	}

[jevanlingen]

Looking at the tests I believe the static method call is not removed by RemoveMethodInvocationsVisitor as the call is part of a local variable assignment. That is of course something that could be changed (e.g. completely remove the initializer or replace it with null), but as it currently stands this visitor will just do nothing in this situation:

rewrite/rewrite-java/src/main/java/org/openrewrite/java/RemoveMethodInvocationsVisitor.java

Lines 124 to 132 in cf71b0d

	private boolean isStatement() {
	return getCursor().dropParentUntil(p -> p instanceof J.Block ||
	p instanceof J.Assignment ||
	p instanceof J.VariableDeclarations.NamedVariable ||
	p instanceof J.Return ||
	p instanceof JContainer ||
	p == Cursor.ROOT_VALUE
	).getValue() instanceof J.Block;
	}

That's right, I just pushed a first implementation.

[timtebeek]

Thanks for taking this on @jevanlingen ! Did you happen to check if this also resolves the outstanding items on

• Migration from Easymock to Mockito rewrite-testing-frameworks#639

And does this mean the visitor in rewrite-static-analysis can now be replaced & removed?

[jevanlingen]

Thanks for taking this on @jevanlingen ! Did you happen to check if this also resolves the outstanding items on

• Migration from easymock to mockito rewrite-testing-frameworks#639

And does this mean the visitor in rewrite-static-analysis can now be replaced & removed?

Yes and yes, I just tested it locally, all test of rewrite-testing-frameworks succeed:

[dralagen]

Thank you very much for your responsiveness and the time taken to fix it.