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

LambdaBlockToExpression does not work in assertThrows (or generally when no value is returned) #236

Open
timo-abele opened this issue Jan 12, 2024 · 4 comments · Fixed by #241 · May be fixed by #245
Open

LambdaBlockToExpression does not work in assertThrows (or generally when no value is returned) #236

timo-abele opened this issue Jan 12, 2024 · 4 comments · Fixed by #241 · May be fixed by #245
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@timo-abele
Copy link
Contributor

What version of OpenRewrite are you using?

I am using

  • Maven plugin v5.18.0
  • rewrite-static-analysis v1.2.0

How are you running OpenRewrite?

I am using the Maven plugin, and my project is a single module project.

<plugin>
    <groupId>org.openrewrite.maven</groupId>
    <artifactId>rewrite-maven-plugin</artifactId>
    <version>5.18.0</version>
    <configuration>
        <activeRecipes>
            <recipe>org.openrewrite.staticanalysis.LambdaBlockToExpression</recipe>
        </activeRecipes>
    </configuration>
    <dependencies>
        <dependency>
            <groupId>org.openrewrite.recipe</groupId>
            <artifactId>rewrite-static-analysis</artifactId>
            <version>1.2.0</version>
        </dependency>
    </dependencies>
</plugin>

What is the smallest, simplest way to reproduce the problem?

import static org.junit.jupiter.api.Assertions.assertThrows;

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

public class SimpleTest {

  @Test
  void shouldFailOnDivisionByZero() {
    assertThrows(ArithmeticException.class, () -> {
      BigDecimal.ONE.divide(BigDecimal.ZERO);
    });
  }

}

What did you expect to see?

import static org.junit.jupiter.api.Assertions.assertThrows;

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

public class SimpleTest {

  @Test
  void shouldFailOnDivisionByZero() {
    assertThrows(ArithmeticException.class, () -> BigDecimal.ONE.divide(BigDecimal.ZERO));
  }

}

What did you see instead?

no change whatsoever.

Notes

The example above is how I encountered this bug. The examplee below are a bit more minimal but less common in my opinion.

package org.example;

import javax.swing.JButton;

public class Main {

  public static void main(String[] args) {
    Runnable runHelloWorld = () -> {
      System.out.println("Hello world!");
    };
    runHelloWorld.run();

    JButton jb = new JButton();
    jb.addActionListener(event -> {
      System.out.println(7);
    });
  }
}

All three examples have in common that they don't return a value, so the description in the docs:

Single-line statement lambdas returning a value can be replaced with expression lambdas.

would render this issue a feature request. However, if I were to write a separate recipe I wouldn't know what else to call it but "LambdaBlockToExpression", so I declare the description part of the bug 😃.
In addition, IntelliJ suggests for all three examples to "Replace with expression lambda", just like for lambdas that return a value.

What is the full stack trace of any errors you encountered?

No errors, works as (IMO incompletely) designed

Are you interested in contributing a fix to OpenRewrite?

I'm somewhat interested in contributing, but not sure if I have the time. It would help me to know if the the functionality described does indeed belong in this recipe, or what to call such a separate recipe.

@timo-abele timo-abele added the bug Something isn't working label Jan 12, 2024
@timo-abele timo-abele changed the title LambdaBlockToExpression does not work in assertThrows (or generally on Runnables) LambdaBlockToExpression does not work in assertThrows (or generally when no value is returned) Jan 12, 2024
@timtebeek timtebeek added the enhancement New feature or request label Jan 15, 2024
@timtebeek timtebeek moved this to Backlog in OpenRewrite Jan 15, 2024
@timtebeek
Copy link
Contributor

Hi @timo-abele Thanks for the offer to help! I think it ought to be easy to covert some of the examples you've given into a minimal unit test as a start, as for example seen here

@Test
void simplifyLambdaBlockToExpression() {
rewriteRun(
//language=java
java(
"""
import java.util.function.Function;
class Test {
Function<Integer, Integer> f = n -> {
return n+1;
};
}
""",
"""
import java.util.function.Function;
class Test {
Function<Integer, Integer> f = n -> n+1;
}
"""
)
);
}

When you then run that new test in a debugger you'll quite quickly find that you're not getting past the second if statement here

if (lambda.getBody() instanceof J.Block) {
List<Statement> statements = ((J.Block) lambda.getBody()).getStatements();
if (statements.size() == 1 && statements.get(0) instanceof J.Return) {
Space prefix = statements.get(0).getPrefix();
if (prefix.getComments().isEmpty()) {
return l.withBody(((J.Return) statements.get(0)).getExpression());
} else {
return l.withBody(((J.Return) statements.get(0)).getExpression().withPrefix(prefix));
}
}
}

That statements.get(0) instanceof J.Return can then likely be selectively expanded to also allow your use case to pass & make changes. More than happy to help review any attempts you make towards this. Feel free to open up a draft PR containing just a test if you want to get started.

@timo-abele
Copy link
Contributor Author

Could you please take a look at my draft PR @timtebeek?

@timtebeek timtebeek moved this from Backlog to In Progress in OpenRewrite Jan 16, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in OpenRewrite Jan 16, 2024
@timo-abele
Copy link
Contributor Author

Hi @timtebeek, can you please reopen this issue? I just tried 1.3.0-SNAPSHOT and my original use case with assertThrows is not transformed.
I tried to reproduce it with the standard library but this is transformed as desired:

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

public class SimpleTestHomeMade {

  @Test
  void shouldFailOnDivisionByZero() {

    myAssertThrows(UnsupportedOperationException.class, () -> {
      BigDecimal.ONE.divide(BigDecimal.ZERO);
    });
  }

  public static <T extends Throwable> T myAssertThrows(Class<T> expectedType, SimpleTestHomeMade.MyExecutable executable) {
    return (T) null;
  }

  public interface MyExecutable {
    void execute() throws java.lang.Throwable;
  }

}

whereas this is not:

import static org.junit.jupiter.api.Assertions.assertThrows;

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

public class SimpleTest {

  @Test
  void shouldFailOnDivisionByZero() {
    assertThrows(ArithmeticException.class, () -> {
      BigDecimal.ONE.divide(BigDecimal.ZERO);
    });
  }

}

@timtebeek timtebeek reopened this Jan 19, 2024
@github-project-automation github-project-automation bot moved this from Done to In Progress in OpenRewrite Jan 19, 2024
@timtebeek
Copy link
Contributor

Sorry to hear it's not yet working as expected ; guess we'd need to refine with another unit test that more closely matches your intended use. Would you want to get that started?

@timtebeek timtebeek moved this from In Progress to Ready to Review in OpenRewrite Jan 19, 2024
@timtebeek timtebeek moved this from Ready to Review to Backlog in OpenRewrite Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment