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 #241.

What's your motivation?

Fixes #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

rewrite-static-analysis/src/main/java/org/openrewrite/staticanalysis/LambdaBlockToExpression.java

Lines 88 to 108 in f1e53d2

	// TODO this is actually more complex in the presence of generics and inheritance
	static boolean hasMethodOverloading(JavaType.Method methodType) {
	String methodName = methodType.getName();
	return Optional.of(methodType)
	.map(JavaType.Method::getDeclaringType)
	.filter(JavaType.Class.class::isInstance)
	.map(JavaType.Class.class::cast)
	.map(JavaType.Class::getMethods)
	.map(methods -> {
	int overloadingCount = 0;
	for (JavaType.Method dm : methods) {
	if (dm.getName().equals(methodName)) {
	if (++overloadingCount > 1) {
	return true;
	}
	}
	}
	return false;
	})
	.orElse(false);
	}

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 1ac607f

[timtebeek]

@knutwannheden would you agree with the isolated change in 1ac607f ? 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 #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.

[knutwannheden]

Since this dependency is only used for tests, we should make sure to not add it to src/main/resources.

[knutwannheden]

Rather than collecting all methods into a list, it would be more efficient to loop over them.

I am also saying this, because we should actually also be checking methods in super types and even default methods on implemented interfaces. But this can quickly get difficult, as I am not convinced that we will be reliable able to tell if a method is an override of some other method (and thus not an overload) when generics are involved. But once we cross that road, a recursive method which returns as soon as it finds a conflicting overload will be more efficient than first gathering all methods up front.

[timo-abele]

I won't find time to finish this and have unassigned me. I trust your judgement that not collecting makes a enough of a difference to justify the loss in readability. I found the previous version very hard to get into (at first I thought it maps over the list elements, but it is always over the Optional):

rewrite-static-analysis/src/main/java/org/openrewrite/staticanalysis/LambdaBlockToExpression.java

Lines 80 to 97 in e57dac0

	String methodName = methodType.getName();
	return Optional.of(methodType)
	.map(JavaType.Method::getDeclaringType)
	.filter(JavaType.Class.class::isInstance)
	.map(JavaType.Class.class::cast)
	.map(JavaType.Class::getMethods)
	.map(methods -> {
	int overloadingCount = 0;
	for (JavaType.Method dm : methods) {
	if (dm.getName().equals(methodName)) {
	if (++overloadingCount > 1) {
	return true;
	}
	}
	}
	return false;
	})
	.orElse(false);

and so I hope the assignment to MethodsOfType can stay:

rewrite-static-analysis/src/main/java/org/openrewrite/staticanalysis/LambdaBlockToExpression.java

Lines 113 to 118 in 758e7cf

	List<JavaType.Method> methodsOfType = Optional.of(methodType)
	.map(JavaType.Method::getDeclaringType)
	.filter(JavaType.Class.class::isInstance)
	.map(JavaType.Class.class::cast)
	.map(JavaType.Class::getMethods)
	.orElse(Collections.emptyList());

Since the list is already there (from getMethods) and Collections.emptyList() is a constant, I think it makes no difference in performance while neatly separating the optional stream from the loop over the elements.

[knutwannheden]

Yes, you are right, there is no additional list being created here. I was already thinking ahead for the use case with inherited method declarations. But let's forget about that for the moment then (a bug will probably get reported at some point).