fix(LambdaBlockToExpression): convert lambda with method invocation in assertThrows as well

[timo-abele]

What's changed?

This PR will enable the transformation of lambdas with method invocation as body within assertThrows, a feature that unexpectedly wasn't fixed with

• https://github.com/openrewrite/rewrite-static-analysis/pull/241.

What's your motivation?

• Fixes https://github.com/openrewrite/rewrite-static-analysis/issues/236

Anything in particular you'd like reviewers to focus on?

Anyone you would like to review specifically?

@timtebeek

Have you considered any alternatives or workarounds?

Any additional context

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]

Welcome back! :) I've changed the way your PR uses the recipe dependencies such that it's more in line with what we use elsewhere; also briefly stepped through with a debugger and noticed indeed that the recipe isn't triggered, and not yet clear why.

[timtebeek]

It's not converted because of this block https://github.com/openrewrite/rewrite-static-analysis/blob/f1e53d203de026938615fee8b5175cba379dc279/src/main/java/org/openrewrite/staticanalysis/LambdaBlockToExpression.java#L88-L108

Not sure how much we can improve this here without potentially breaking things elsewhere. 🤔

[timtebeek]

Fun puzzle, thanks! I think it would be fixed with https://github.com/openrewrite/rewrite-static-analysis/pull/245/commits/1ac607fc4574effa2efbdeaadbe27ab747d2dcfc

[timtebeek]

@knutwannheden would you agree with the isolated change in https://github.com/openrewrite/rewrite-static-analysis/pull/245/commits/1ac607fc4574effa2efbdeaadbe27ab747d2dcfc ? I know we've been careful where we convert blocks and remove type casts, but I feel this minimal change might add a lot more reductions without additional risk that I can see now.

[timtebeek]

I wouldn't necessarily merge the tests we have here now, as I'd prefer to keep the tests in rewrite-static-analysis free from dependencies, but it did help bring to light why were not replacing these instances, so thanks for that @timo-abele !

[timo-abele]

Hmm, discriminating by "same number of arguments" solves my use case (thank you so much!), because it excludes the problematic case from https://github.com/openrewrite/rewrite-static-analysis/issues/162

void aMethod(Consumer<Integer> consumer)
void aMethod(Function<Integer,String> function)

whereas the heterogeneous assertThrows with

assertThrows(Class<T> expectedType, Executable, executable)
assertThrows(Class<T> expectedType, Executable, executable, String message)
assertThrows(Class<T> expectedType, Executable, executable, Supplier<String> messageSupplier)

would be included. However, a hypothetical

void bMethod(Consumer<Integer> consumer, String message)
void bMethod(Function<Integer,String> function, String message)

would be included again when it shouldn't be. Maybe we should instead discriminate by "The parameter at the position where the lambda is should always have the same type"?

[timtebeek]

That seems better indeed, and thanks for noticing that potential issue. Would you want to wire that up yourself?

[timo-abele]

I've sketched something up, all tests run through but please review diligently I ended up rewriting more then I planned.

[github-actions[bot]]

This PR is stale because it has been open for 90 days with no activity. Remove stale label or comment or this will be closed in 7 days.