Unnecessary `return` as last statement in `void` method

[mccartney]

What's changed?

Adding a new recipe to remove return if it's the last statement of a void method.

What's your motivation?

• Closes #75

Checklist

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

[mccartney]

I think some more tests would be good.

Added some in 3325148629f54c5362a96df3bc59f4418051f123.

Kindly requesting another review.

[mccartney]

BTW, is there some easy way to run the Code Suggester locally to avoid the noise in the pull request itself?

[timtebeek]

BTW, is there some easy way to run the Code Suggester locally to avoid the noise in the pull request itself?

Yes sure! What I've done is take this file and put it in scratch, then run it through the IntelliJ Ultimate bundled support for OpenRewrite.

[timtebeek]

Had another look; thanks so far @mccartney ; Since I think you'll enjoy the puzzle I've added a new unit test that fails. Let me know if you can't or won't enjoy working it out. :)

[mccartney]

Since I think you'll enjoy the puzzle I've added a new unit test that fails.

b48dd6a

[timtebeek]

Saving others a click: unrelated test failure now in:

RemoveRedundantTypeCastTest > changeTypeCastInReturn() FAILED
    org.opentest4j.AssertionFailedError: [Expected recipe to complete in 0 cycles, but took at least one more cycle. Between the last two executed cycles there were changes to "Test.java"] 

• Possibly following https://github.com/openrewrite/rewrite/pull/4688

[knutwannheden]

Saving others a click: unrelated test failure now in:

RemoveRedundantTypeCastTest > changeTypeCastInReturn() FAILED
    org.opentest4j.AssertionFailedError: [Expected recipe to complete in 0 cycles, but took at least one more cycle. Between the last two executed cycles there were changes to "Test.java"] 

• Possibly following Make FindMissingTypes stricter for method invocations rewrite#4688

Indeed. I've been struggling for a while with that TypeUtils#isAssignableTo(), where I've so far neglected the fact that we also need to consider the "direction" of the assignability. While we can assign a List<String> type to <T extends Collection<String>> when passing in a parameter, the same doesn't work when assigning a local variable. I will add an overload with an additional direction parameter, so that we stay backwards compatible.

[knutwannheden]

The type assignability check in Java is tricky. I think we might even have some cases where we can't do it correctly, as we don't have all the information we need in our type attribution. In any case I think this PR should now fix the test failure we have here:

• https://github.com/openrewrite/rewrite/pull/4696

[mccartney]

If I read the GHA output correctly, the failure is with:

InstanceOfPatternMatchTest > If > typeParameters_3() FAILED
    org.opentest4j.AssertionFailedError: [Expected recipe to complete in 0 cycles, but took at least one more cycle. Between the last two executed cycles there were changes to "A.java"] 
    expected: 

now.

[knutwannheden]

That was resolved on main. I just merged in the main branch.

[timtebeek]

@knutwannheden any last thoughts before we merge?

[timtebeek]

Thanks both! As a quick check I'm running this recipe against all Apache projects: https://app.moderne.io/results/5TXPCrrF0 Welcome to use the platform to explore the results, or even push the recipe suggested improvements to other projects! :)

[knutwannheden]

All the changes in that recipe run look good to me.

[mccartney]

As a quick check I'm running this recipe against all Apache projects: https://app.moderne.io/results/5TXPCrrF0 Welcome to use the platform to explore the results, or even push the recipe suggested improvements to other projects!

I am hesitant to do this as the Moderne app requires kind of generous permissions (read/write code to all my private repos) to even see the changes.

[timtebeek]

Rest assured we're not after your private projects at all; we merely need access to your email address to identify you, and then access to your projects if you choose to fork any OSS projects. I do see how the dialog could come across differently; we're exploring options on our side for that in parallel, but there should be no reason not to use the platform.