Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ plugins {
group = "org.openrewrite.recipe"
description = "The first Static Analysis and REMEDIATION tool"

recipeDependencies {
parserClasspath("org.junit.jupiter:junit-jupiter-api:latest.release")
}

val rewriteVersion = rewriteRecipe.rewriteVersion.get()
dependencies {
compileOnly("org.projectlombok:lombok:latest.release")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,10 @@
import org.openrewrite.java.tree.*;
import org.openrewrite.staticanalysis.java.JavaFileChecker;

import java.util.Collections;
import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;

public class LambdaBlockToExpression extends Recipe {
@Override
Expand Down Expand Up @@ -70,7 +72,7 @@ public J.Lambda visitLambda(J.Lambda lambda, ExecutionContext ctx) {

@Override
public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) {
if (hasLambdaArgument(method) && hasMethodOverloading(method)) {
if (hasLambdaArgument(method) && hasAmbiguousMethodOverloading(method)) {
return method;
}
return super.visitMethodInvocation(method, ctx);
Expand All @@ -79,42 +81,66 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu
);
}

private static boolean hasLambdaArgument(J.MethodInvocation method) {
boolean hasLambdaArgument = false;
for (Expression arg : method.getArguments()) {
if (arg instanceof J.Lambda) {
hasLambdaArgument = true;
break;
}
}
return hasLambdaArgument;
}

// Check whether a method has overloading methods in the declaring class
private static boolean hasMethodOverloading(J.MethodInvocation method) {
static boolean hasAmbiguousMethodOverloading(MethodCall method) {
JavaType.Method methodType = method.getMethodType();
return methodType != null && hasMethodOverloading(methodType);
return methodType != null
&& hasAmbiguousMethodOverloading(methodType, method.getArguments(), methodType.getName());
}

// 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)
static boolean hasAmbiguousMethodOverloading(JavaType.Method methodType, List<Expression> arguments, String methodName) {
int numberOfArguments = arguments.size();

//all methods of the given type
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)
.map(methods -> {
int overloadingCount = 0;
for (JavaType.Method dm : methods) {
if (dm.getName().equals(methodName)) {
if (++overloadingCount > 1) {
return true;
}
}
}
return false;
})
.orElse(false);
}
.orElse(Collections.emptyList());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


private static boolean hasLambdaArgument(J.MethodInvocation method) {
boolean hasLambdaArgument = false;
for (Expression arg : method.getArguments()) {
if (arg instanceof J.Lambda) {
hasLambdaArgument = true;
break;
List<JavaType.Method> potentiallyOverLoadedMethods = methodsOfType.stream()
.filter(dm -> dm.getName().equals(methodName))
.filter(dm -> dm.getParameterTypes().size() == numberOfArguments)
.collect(Collectors.toList());

//if there are less than 2 such methods, then there is no ambiguity
if(potentiallyOverLoadedMethods.size() <= 1) {
return false;
}

//if there is a position where
// - the argument is a lambda
// - the parameters of all potential methods have the same type
// then there is no ambiguity
//TODO we currently assume that there is at most one lambda among the arguments. Handing several lambdas will
// require a bigger rewrite because `visitMethodInvocation` can only return true or false, but not which lambda
// to replace
for (int i = 0; i < numberOfArguments; i++) {
int finalI = i;
if (arguments.get(i) instanceof J.Lambda) {
long distinctElementsCount = potentiallyOverLoadedMethods.stream()
.map(m -> m.getParameterTypes().get(finalI))
.distinct().count();
if (distinctElementsCount == 1) {
return false;
}
}
}
return hasLambdaArgument;
//otherwise, there must be ambiguity
return true;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import java.util.List;
import java.util.Set;

import static org.openrewrite.staticanalysis.LambdaBlockToExpression.hasMethodOverloading;
import static org.openrewrite.staticanalysis.LambdaBlockToExpression.hasAmbiguousMethodOverloading;

@Incubating(since = "7.23.0")
public class RemoveRedundantTypeCast extends Recipe {
Expand Down Expand Up @@ -74,7 +74,7 @@ public J visitTypeCast(J.TypeCast typeCast, ExecutionContext ctx) {
} else if (parentValue instanceof MethodCall) {
MethodCall methodCall = (MethodCall) parentValue;
JavaType.Method methodType = methodCall.getMethodType();
if (methodType == null || hasMethodOverloading(methodType)) {
if (methodType == null || hasAmbiguousMethodOverloading(methodCall)) {
return visited;
} else if (!methodType.getParameterTypes().isEmpty()) {
List<Expression> arguments = methodCall.getArguments();
Expand Down
Binary file not shown.
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import org.junit.jupiter.api.Test;
import org.openrewrite.DocumentExample;
import org.openrewrite.InMemoryExecutionContext;
import org.openrewrite.Issue;
import org.openrewrite.java.JavaParser;
import org.openrewrite.test.RecipeSpec;
Expand All @@ -25,11 +26,12 @@
import static org.openrewrite.java.Assertions.java;

class LambdaBlockToExpressionTest implements RewriteTest {

@Override
public void defaults(RecipeSpec spec) {
spec.recipe(new LambdaBlockToExpression())
.parser(JavaParser.fromJavaVersion().logCompilationWarningsAndErrors(true));
.parser(JavaParser.fromJavaVersion()
.classpathFromResources(new InMemoryExecutionContext(), "junit-jupiter-api-5.10")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.logCompilationWarningsAndErrors(true));
}

@DocumentExample
Expand Down Expand Up @@ -75,7 +77,7 @@ class Test {
"""
import java.util.function.Function;
class Test {
Function<Integer, Integer> f = n ->
Function<Integer, Integer> f = n ->
// The buttonType will always be "cancel", even if we pressed one of the entry type buttons
n + 1;
}
Expand Down Expand Up @@ -116,7 +118,7 @@ void doTest() {

@Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/236")
@Test
void simplifyLambdaBlockReturningVoidAsWell2() {
void simplifyLambdaBlockReturningVoidAsWellSimple() {
//language=java
rewriteRun(
java(
Expand Down Expand Up @@ -145,4 +147,42 @@ public void run() {
);
}

@Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/236")
@Test
void simplifyLambdaBlockReturningVoidAsWellAssertThrows() {
//language=java
rewriteRun(
java(
"""
import static org.junit.jupiter.api.Assertions.assertThrows;

import java.math.BigDecimal;
import org.junit.jupiter.api.Test;

public class Main {
@Test
public void test() {
assertThrows(ArithmeticException.class, () -> {
BigDecimal.ONE.divide(BigDecimal.ZERO);
});
}
}
""",
"""
import static org.junit.jupiter.api.Assertions.assertThrows;

import java.math.BigDecimal;
import org.junit.jupiter.api.Test;

public class Main {
@Test
public void test() {
assertThrows(ArithmeticException.class, () ->
BigDecimal.ONE.divide(BigDecimal.ZERO));
}
}
"""
)
);
}
}