Add ChangeMethodParameter for modify parameters in method declaration

[ckcd]

What's changed?

Add a ChangeMethodParameter recipe for modify parameters in method declaration.
For now we have AddMethodParameter which is for add new parameter in method declaration, but didn't have the one to modify current parameters.

@timtebeek as we discussed in slack, I tried to add this new ChangeMethodParameter recipe, please take a look at this if you are available, thanks a lot.

What's your motivation?

I want to find any recipe in java can replace a parameter's type, for example:

// From:
public void m(int a) {}

// To:
public void m(long a) {}

but currently we don't have. So I try to add this new recipe.

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 the work done here already @ckcd ! I've pointed out a concern on a unit test which might be harder to fully resolve. Would you mind explaining the intended use cases for this recipe? That way we get a better sense of what's possible and needed here.

Renaming the parameter would either need to be handled fully, or not be offered as an option for instance. Any limitations beyond that might need to be documented such that they are clearer to users, if this is even generic enough of a use case to offer as a recipe.

[timtebeek]

I'm not so sure about changing the parameter name here, as we do not appear to change any usages. And even if we do change the usage, then there's quickly potential to break compilation, as seen in this example.

Suggested change

	public void bar(int i) {
	}
	public void bar(int i) {
	int plusOne = i + 1;
	}

Right now the usage of i is not converted into j, and if it were, then the project would fail to compile. What are your thoughts here?

[ckcd]

@timtebeek Thanks for your reply!

The case I met is, when the method definition of an interface changes, the corresponding implementation also needs to be modified accordingly. Many times, these parameters are not used in the implementation, and the problem can be solved by simply modifying the method definition of the implementation.

For example, for the shouldSkip method in spring-batch's interface: SkipPolicy, in spring-batch 4.x it has a parameter with int type:
https://docs.spring.io/spring-batch/docs/4.2.0.RELEASE/api/index.html?org/springframework/batch/core/step/skip/SkipPolicy.html

shouldSkip(Throwable t, int skipCount)

But when upgrade to spring-batch 5.x it become long:
https://docs.spring.io/spring-batch/docs/current/api/org/springframework/batch/core/step/skip/SkipPolicy.html

shouldSkip(Throwable t, long skipCount)

And actually for many users the implementation of this interface is only return true/false:

public class MySkipPolicy implements SkipPolicy {
    @Override
    public boolean shouldSkip(Throwable throwable, int skipCount) {
        return true;
    }
}

So a recipe like ChangeMethodParameter can easily help us to fix these kind of issues by change int to long, and it won't break compilation because we didn't use the parameter.

And yes I agree that this recipe should only works for some cases. If the business logic uses the old parameter we replaced, then it might break compilation, it may need to to be documented.

[timtebeek]

Thanks for that context! I do indeed see the value in being able to express some initial migration steps in this way. I've also added a note to openrewrite/rewrite-spring#265 for the Spring Batch migration step you pointed out.

Documentation would help in part to limit when to use the recipe; I think not offering the option to rename the parameter just yet might be easiest to start with.

There's also the option of only offering this as a visitor for now, and then have dedicated named recipes that use the visitor, knowing full well the limitations. That might prevent accidental incorrect use, but would also make it more cumbersome to define new steps in larger migrations that use this functionality.

Are you only planning this for Spring Batch for now? If so we could let it mature in rewrite-spring to start with, before pulling it up to rewrite-java at a later stage.

[ckcd]

@timtebeek Thanks again!

I agree that we can only allow to change parameter type and don't offering the option to rename the parameter, it's much safer.

And yes currently I'm planning for Spring Batch cases. For move to rewrite-spring, should I open a new PR at there? Which folder should I put new recipe in? Maybe under this src/main/java/org/openrewrite/java/spring?

[timtebeek]

Yes that would work, thanks! I look forward to continuing the conversation there.

[ckcd]

Move to openrewrite/rewrite-spring#631

[timtebeek]

Thanks a lot! Let's continue there, and then we can close this here for now.