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 Recipe: return is unnecessary as the last statement in a void method  #75

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

[knutwannheden]

One thing I am missing here is the check if the return has an expression or not.

[mccartney]

Right. There's no check for void return type.

[mccartney]

In bcc951a adding two lines of defense:

• the whole method being a void method
• the return expression having an empty expression

Not fully sure if you prefer to have both (I've checked both work alone).

[knutwannheden]

I think some more tests would be good. For example removing the return here would be correct:

        Consumer<Integer> c = i -> {
            return;
        };

while removing the return here would not be correct:

        while (true) {
            return;
        }

Although it could also be argued that changing the first is incorrect, as it is a return from a lambda rather than a method and that isn't how the recipe is documented.

[mccartney]

I think some more tests would be good.

Added some in 3325148.

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]

Approved from my side already; thanks for the thorough tests!

I've replaced the three methods you had with the same name but different arguments with a single recursive method, as the flow was harder to follow across those individual methods.

[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 Make FindMissingTypes stricter for method invocations rewrite#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:

• TypeUtils#isAssignable() improvements rewrite#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?

[knutwannheden]

LGTM.

[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.