Skip to content

Keep early returns in TestMethodsShouldBeVoid and KotlinTestMethodsShouldReturnUnit #1217

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.

Sign up for GitHub

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

Merged
timtebeek merged 4 commits into from
Oct 24, 2026
Merged

Keep early returns in TestMethodsShouldBeVoid and KotlinTestMethodsShouldReturnUnit #1217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
496b8ea
Only change Java sources in `TestMethodsShouldBeVoid`
timtebeek Oct 24, 2026
8c83b7c
Keep early returns in `TestMethodsShouldBeVoid` and `KotlinTestMethod…
timtebeek Oct 24, 2026
6f44114
Use `JavaFileChecker` precondition in `TestMethodsShouldBeVoid`
timtebeek Oct 24, 2026
aedac6a
Merge remote-tracking branch 'origin/main' into tim/test-methods-void…
timtebeek Oct 24, 2026
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
120 changes: 91 additions & 29 deletions src/main/java/org/openrewrite/java/testing/cleanup/KotlinTestMethodsShouldReturnUnit.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,23 @@
package org.openrewrite.java.testing.cleanup;

import lombok.Getter;
import lombok.RequiredArgsConstructor;
import org.jspecify.annotations.Nullable;
import org.openrewrite.*;
import org.openrewrite.internal.ListUtils;
import org.openrewrite.java.AnnotationMatcher;
import org.openrewrite.java.search.UsesType;
import org.openrewrite.java.service.AnnotationService;
import org.openrewrite.java.tree.*;
import org.openrewrite.kotlin.KotlinIsoVisitor;
import org.openrewrite.kotlin.KotlinVisitor;
import org.openrewrite.kotlin.marker.KObject;
import org.openrewrite.kotlin.marker.SingleExpressionBlock;
import org.openrewrite.kotlin.tree.K;
import org.openrewrite.kotlin.tree.KotlinTypeUtils;
import org.openrewrite.marker.Markers;

import java.util.concurrent.atomic.AtomicBoolean;

import static java.util.Collections.emptyList;

public class KotlinTestMethodsShouldReturnUnit extends Recipe {
Expand All @@ -51,7 +54,7 @@ public class KotlinTestMethodsShouldReturnUnit extends Recipe {
@Getter
final String description = "Kotlin test methods annotated with `@Test`, `@ParameterizedTest`, `@RepeatedTest`, `@TestTemplate` " +
"should have `Unit` return type. Other return types can cause test discovery issues, " +
"and warnings as of JUnit 5.13+. This recipe changes the return type to `Unit` and removes `return` statements.";
"and warnings as of JUnit 5.13+. This recipe changes the return type to `Unit` and drops the returned values. Test methods that override a method or are called elsewhere in the same file are left unchanged.";

@Override
public TreeVisitor<?, ExecutionContext> getVisitor() {
Expand All @@ -61,8 +64,9 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, Ex
J.MethodDeclaration m = super.visitMethodDeclaration(method, ctx);

// Skip invalid signatures or already-correct return types
J.Block body = m.getBody();
JavaType.Method methodType = m.getMethodType();
if (m.getBody() == null || methodType == null || KotlinTypeUtils.isKotlinUnit(methodType.getReturnType())) {
if (body == null || methodType == null || KotlinTypeUtils.isKotlinUnit(methodType.getReturnType())) {
return m;
}

Expand All @@ -74,13 +78,16 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, Ex
return m;
}

// Update method and method identifier type.
JavaType.Method newMethodType = methodType.withReturnType(KOTLIN_UNIT);
m = m.withMethodType(newMethodType).withName(m.getName().withType(newMethodType));
if (TypeUtils.isOverride(methodType) || isInvoked(getCursor().firstEnclosingOrThrow(K.CompilationUnit.class), m.getSimpleName())) {
return m;
}

JavaType.Method unitMethodType = methodType.withReturnType(KOTLIN_UNIT);
J.MethodDeclaration unitMethod = m.withMethodType(unitMethodType).withName(m.getName().withType(unitMethodType));

// Add an explicit Unit return type
if (m.getBody() != null && m.getBody().getMarkers().findFirst(SingleExpressionBlock.class).isPresent()) {
return m.withReturnTypeExpression(new J.Identifier(
if (body.getMarkers().findFirst(SingleExpressionBlock.class).isPresent()) {
return unitMethod.withReturnTypeExpression(new J.Identifier(
Tree.randomId(),
Space.SINGLE_SPACE,
Markers.EMPTY,
Expand All @@ -90,41 +97,96 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, Ex
null));
}

// Otherwise, there's no need for a return type expression at all.
return m.withReturnTypeExpression(null)
// Remove return statements that are not in nested classes, objects, or lambdas.
.withBody((J.Block) new RemoveDirectReturns().visit(m.getBody(), ctx));
AtomicBoolean unsafeReturn = new AtomicBoolean();
J.Block withoutLastReturn = body.withStatements(ListUtils.mapLast(body.getStatements(), KotlinTestMethodsShouldReturnUnit::removeLastReturn));
J.Block newBody = (J.Block) new RemoveEarlyReturnValues(m.getSimpleName()).visitNonNull(withoutLastReturn, unsafeReturn, getCursor());
if (unsafeReturn.get()) {
return m;
}
return unitMethod.withReturnTypeExpression(null).withBody(newBody);
}
});
}

private static class RemoveDirectReturns extends KotlinVisitor<ExecutionContext> {
private static @Nullable Statement removeLastReturn(Statement statement) {
if (!(statement instanceof K.Return)) {
return statement;
}
Expression expression = ((K.Return) statement).getExpression().getExpression();
return expression instanceof Statement ? expression.withPrefix(statement.getPrefix()) : null;
}

@RequiredArgsConstructor
private static class RemoveEarlyReturnValues extends KotlinVisitor<AtomicBoolean> {
private final String methodName;

@Override
public J visitClassDeclaration(J.ClassDeclaration classDeclaration, ExecutionContext ctx) {
return classDeclaration.getMarkers().findFirst(KObject.class).isPresent() ?
classDeclaration : // Retain nested object expressions
super.visitClassDeclaration(classDeclaration, ctx);
public J visitClassDeclaration(J.ClassDeclaration classDeclaration, AtomicBoolean unsafeReturn) {
return classDeclaration;
}

@Override
public J.Lambda visitLambda(J.Lambda lambda, ExecutionContext ctx) {
return lambda; // Retain nested returns
public J visitMethodDeclaration(J.MethodDeclaration method, AtomicBoolean unsafeReturn) {
return method;
}

@Override
public J.NewClass visitNewClass(J.NewClass newClass, ExecutionContext ctx) {
return newClass; // Retain nested returns
public J visitNewClass(J.NewClass newClass, AtomicBoolean unsafeReturn) {
return newClass;
}

@Override
public @Nullable J visitReturn(K.Return return_, ExecutionContext ctx) {
Expression returnExpr = return_.getExpression().getExpression();
//noinspection DataFlowIssue
return returnExpr instanceof Statement ?
// Retain any side effects from statements in return expressions
returnExpr.withPrefix(return_.getPrefix()) :
// Remove any other return statements entirely
null;
public J visitReturn(K.Return return_, AtomicBoolean unsafeReturn) {
if (getCursor().dropParentUntil(t -> t instanceof J.Lambda || t instanceof J.MethodDeclaration).getValue() instanceof J.Lambda) {
J.Identifier label = return_.getLabel();
if (label == null || methodName.equals(label.getSimpleName())) {
unsafeReturn.set(true);
}
return return_;
}
Expression expression = return_.getExpression().getExpression();
if (expression == null) {
return return_;
}
if (hasSideEffects(expression)) {
unsafeReturn.set(true);
}
return return_.withExpression(return_.getExpression().withExpression(null));
}
}

private static boolean hasSideEffects(Expression expression) {
return new KotlinIsoVisitor<AtomicBoolean>() {
@Override
public @Nullable J preVisit(J tree, AtomicBoolean found) {
if (tree instanceof J.MethodInvocation || tree instanceof J.NewClass ||
tree instanceof J.Assignment || tree instanceof J.AssignmentOperation ||
tree instanceof J.Unary && ((J.Unary) tree).getOperator().isModifying()) {
found.set(true);
stopAfterPreVisit();
}
return tree;
}
}.reduce(expression, new AtomicBoolean()).get();
}

private static boolean isInvoked(K.CompilationUnit cu, String methodName) {
return new KotlinIsoVisitor<AtomicBoolean>() {
@Override
public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, AtomicBoolean found) {
if (method.getSimpleName().equals(methodName)) {
found.set(true);
}
return super.visitMethodInvocation(method, found);
}

@Override
public J.MemberReference visitMemberReference(J.MemberReference memberRef, AtomicBoolean found) {
if (memberRef.getReference().getSimpleName().equals(methodName)) {
found.set(true);
}
return super.visitMemberReference(memberRef, found);
}
}.reduce(cu, new AtomicBoolean()).get();
}
}
116 changes: 86 additions & 30 deletions src/main/java/org/openrewrite/java/testing/cleanup/TestMethodsShouldBeVoid.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,19 @@
import org.openrewrite.Preconditions;
import org.openrewrite.Recipe;
import org.openrewrite.TreeVisitor;
import org.openrewrite.internal.ListUtils;
import org.openrewrite.java.JavaIsoVisitor;
import org.openrewrite.java.JavaVisitor;
import org.openrewrite.java.search.UsesType;
import org.openrewrite.java.tree.Expression;
import org.openrewrite.java.tree.J;
import org.openrewrite.java.tree.JavaType;
import org.openrewrite.java.tree.Statement;
import org.openrewrite.java.tree.TypeTree;
import org.openrewrite.java.tree.TypeUtils;
import org.openrewrite.staticanalysis.java.JavaFileChecker;

import java.util.concurrent.atomic.AtomicBoolean;

import static java.util.Objects.requireNonNull;

Expand All @@ -41,64 +47,114 @@ public class TestMethodsShouldBeVoid extends Recipe {
@Getter
final String description = "Test methods annotated with `@Test`, `@ParameterizedTest`, `@RepeatedTest`, `@TestTemplate` " +
"should have `void` return type. Non-void return types can cause test discovery issues, " +
"and warnings as of JUnit 5.13+. This recipe changes the return type to `void` and removes `return` statements.";
"and warnings as of JUnit 5.13+. This recipe changes the return type to `void` and drops the returned values. Test methods that override a method or are called elsewhere in the same file are left unchanged.";

@Override
public TreeVisitor<?, ExecutionContext> getVisitor() {
return Preconditions.check(new UsesType<>(TEST_ANNOTATION_TYPE_PATTERN, true), new JavaIsoVisitor<ExecutionContext>() {
return Preconditions.check(Preconditions.and(new JavaFileChecker<>(), new UsesType<>(TEST_ANNOTATION_TYPE_PATTERN, true)), new JavaIsoVisitor<ExecutionContext>() {
@Override
public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, ExecutionContext ctx) {
J.MethodDeclaration m = super.visitMethodDeclaration(method, ctx);
if (!hasJUnit5MethodAnnotation(m)) {
J.Block body = m.getBody();
TypeTree returnType = m.getReturnTypeExpression();
if (body == null || returnType == null || TypeUtils.isOfType(returnType.getType(), JavaType.Primitive.Void) ||
!hasJUnit5MethodAnnotation(m) || TypeUtils.isOverride(m.getMethodType()) ||
isInvoked(getCursor().firstEnclosingOrThrow(J.CompilationUnit.class), m.getSimpleName())) {
return m;
}

// Check if return type is already void
JavaType.Primitive voidType = JavaType.Primitive.Void;
if (m.getReturnTypeExpression() == null || TypeUtils.isOfType(m.getReturnTypeExpression().getType(), voidType)) {
AtomicBoolean earlyReturnHasSideEffects = new AtomicBoolean();
J.Block withoutLastReturn = body.withStatements(ListUtils.mapLast(body.getStatements(), TestMethodsShouldBeVoid::removeLastReturn));
J.Block newBody = (J.Block) new RemoveEarlyReturnValues().visitNonNull(withoutLastReturn, earlyReturnHasSideEffects, getCursor());
if (earlyReturnHasSideEffects.get()) {
return m;
}

// Change return type to void
m = m.withReturnTypeExpression(new J.Primitive(
m.getReturnTypeExpression().getId(),
m.getReturnTypeExpression().getPrefix(),
m.getReturnTypeExpression().getMarkers(),
voidType
));

// Update method type
m = m.withReturnTypeExpression(new J.Primitive(returnType.getId(), returnType.getPrefix(), returnType.getMarkers(), JavaType.Primitive.Void));
if (m.getMethodType() != null) {
m = m.withMethodType(m.getMethodType().withReturnType(voidType));
m = m.withMethodType(m.getMethodType().withReturnType(JavaType.Primitive.Void));
}

// Remove return statements that are not in nested classes or lambdas
return m.withBody((J.Block) new RemoveDirectReturns().visit(requireNonNull(m.getBody()), ctx, getCursor().getParentTreeCursor()));
return m.withBody(newBody);
}
});
}

private static class RemoveDirectReturns extends JavaVisitor<ExecutionContext> {
private static @Nullable Statement removeLastReturn(Statement statement) {
if (!(statement instanceof J.Return) || ((J.Return) statement).getExpression() == null) {
return statement;
}
Expression expression = requireNonNull(((J.Return) statement).getExpression()).unwrap();
return isExpressionStatement(expression) ? expression.withPrefix(statement.getPrefix()) : null;
}

private static class RemoveEarlyReturnValues extends JavaVisitor<AtomicBoolean> {
@Override
public J visitLambda(J.Lambda lambda, AtomicBoolean earlyReturnHasSideEffects) {
return lambda;
}

@Override
public J visitLambda(J.Lambda lambda, ExecutionContext ctx) {
return lambda; // Retain nested returns
public J visitNewClass(J.NewClass newClass, AtomicBoolean earlyReturnHasSideEffects) {
return newClass;
}

@Override
public J visitNewClass(J.NewClass newClass, ExecutionContext ctx) {
return newClass; // Retain nested returns
public J visitClassDeclaration(J.ClassDeclaration classDecl, AtomicBoolean earlyReturnHasSideEffects) {
return classDecl;
}

@Override
public @Nullable J visitReturn(J.Return retrn, ExecutionContext ctx) {
return retrn.getExpression() instanceof Statement ?
// Retain any side effects from expressions in return statements
retrn.getExpression().withPrefix(retrn.getPrefix()) :
// Remove any other return statements
null;
public J visitReturn(J.Return retrn, AtomicBoolean earlyReturnHasSideEffects) {
Expression expression = retrn.getExpression();
if (expression == null) {
return retrn;
}
if (hasSideEffects(expression)) {
earlyReturnHasSideEffects.set(true);
}
return retrn.withExpression(null);
}
}

private static boolean isExpressionStatement(Expression expression) {
return expression instanceof J.MethodInvocation || expression instanceof J.NewClass ||
expression instanceof J.Assignment || expression instanceof J.AssignmentOperation ||
expression instanceof J.Unary && ((J.Unary) expression).getOperator().isModifying();
}

private static boolean hasSideEffects(Expression expression) {
return new JavaIsoVisitor<AtomicBoolean>() {
@Override
public @Nullable J preVisit(J tree, AtomicBoolean found) {
if (tree instanceof Expression && isExpressionStatement((Expression) tree)) {
found.set(true);
stopAfterPreVisit();
}
return tree;
}
}.reduce(expression, new AtomicBoolean()).get();
}

private static boolean isInvoked(J.CompilationUnit cu, String methodName) {
return new JavaIsoVisitor<AtomicBoolean>() {
@Override
public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, AtomicBoolean found) {
if (method.getSimpleName().equals(methodName)) {
found.set(true);
}
return super.visitMethodInvocation(method, found);
}

@Override
public J.MemberReference visitMemberReference(J.MemberReference memberRef, AtomicBoolean found) {
if (memberRef.getReference().getSimpleName().equals(methodName)) {
found.set(true);
}
return super.visitMemberReference(memberRef, found);
}
}.reduce(cu, new AtomicBoolean()).get();
}

private static boolean hasJUnit5MethodAnnotation(J.MethodDeclaration method) {
for (J.Annotation a : method.getLeadingAnnotations()) {
if (TypeUtils.isOfClassType(a.getType(), "org.junit.jupiter.api.Test") ||
Expand Down
Loading
Loading