Convert lambda block to expression after adopting assertThrows

[shivanisky]

Test case for inlining valid single line assertthrows

What's changed?

Inline valid single line assert throws test case

What's your motivation?

This recipe creates intellij suggestion/warning to inline this code and I much prefer it inlined, saves 2 lines of useless code.

Anyone you would like to review specifically?

@timtebeek

Have you considered any alternatives or workarounds?

Yes - maybe a separate recipe which achieves this is better, or maybe both - change this recipe, and have a separate recipe. Can do separate recipe first as it addresses at a global level.

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 yet another case to cover; think indeed best here to have a separate recipe, as that then keeps the logic cleaner here, and allows us to pick up cases already converted.

[shivanisky]

Should the recipe be in rewrite static analysis? Maybe not just for assertThrows, but for everything? I think it’s just the removing parantheses bit … I will exclude throws statements. Any other single statement cases which could break will be very rare. I hope that this approach is fine. Sorry to bother you when you are so busy. No rush on this one.

…

On Fri, 16 Aug 2024 at 1:59 AM, Tim te Beek ***@***.***> wrote: Thanks for yet another case to cover; think indeed best here to have a separate recipe, as that then keeps the logic cleaner here, and allows us to pick up cases already converted. — Reply to this email directly, view it on GitHub <#582 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BI27SFQF3RQI2KOUMU4PIU3ZRTF7TAVCNFSM6AAAAABMSK4YS2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJRGU4TSNJQG4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[timtebeek]

I think we already have this one; we just need to tie it in here. Nice to be able to leverage existing logic there! :)

https://github.com/openrewrite/rewrite-static-analysis/blob/f1e53d203de026938615fee8b5175cba379dc279/src/test/java/org/openrewrite/staticanalysis/LambdaBlockToExpressionTest.java#L35-L58

[timtebeek]

Did a quick check: we're running up against this safe guard where any overload invalidates the visit of a lambda argument.
https://github.com/openrewrite/rewrite-static-analysis/blob/6d04813574c6b02c8bf7d6e2f66b2bdaa881854c/src/main/java/org/openrewrite/staticanalysis/LambdaBlockToExpression.java#L74

Not quite sure if we can safely change that here. 🤔

[timtebeek]

Specifically, the methods were seeing overloaded at that particular point are these.

org.junit.jupiter.api.Assertions{
    name=assertThrows,
    return=Generic{T extends java.lang.Throwable},
    parameters=[java.lang.Class<Generic{T extends java.lang.Throwable}>,org.junit.jupiter.api.function.Executable,java.util.function.Supplier<java.lang.String>]
}
org.junit.jupiter.api.Assertions{
    name=assertThrows,
    return=Generic{T extends java.lang.Throwable},
    parameters=[java.lang.Class<Generic{T extends java.lang.Throwable}>,org.junit.jupiter.api.function.Executable,java.lang.String]
}
org.junit.jupiter.api.Assertions{
    name=assertThrows,
    return=Generic{T extends java.lang.Throwable},
    parameters=[java.lang.Class<Generic{T extends java.lang.Throwable}>,org.junit.jupiter.api.function.Executable]
}

We could try this out in LambdaBlockToExpressionTest to see if we can safely recognize these cases and allow the replacement, but it'll be tricky not the break any other cases there. I'll leave it at this for now. :)