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

Replace Guava Optional with JDK Optional #202

Merged
merged 15 commits into from Mar 31, 2023
Merged

Replace Guava Optional with JDK Optional #202

merged 15 commits into from Mar 31, 2023

Conversation

timtebeek
Copy link
Contributor

For #197

Thought to try my hand at a first attempt; This should cover the most common cases.
Uncovered cases are added as tests annotated with @ExpectedToFail.

@timtebeek timtebeek added the recipe Recipe requested label Mar 25, 2023
@timtebeek timtebeek self-assigned this Mar 25, 2023
@timtebeek timtebeek changed the title Guava optional Replace Guava Optional with JDK Optional Mar 25, 2023
Co-authored-by: Aurélien Mino <155828+murdos@users.noreply.github.com>
Copy link
Contributor

@knutwannheden knutwannheden left a comment

Choose a reason for hiding this comment

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

There appear to be some minor errors in the type attribution. I think we can however fix that in a follow-up commit.

src/main/resources/META-INF/rewrite/no-guava.yml Outdated Show resolved Hide resolved
J.MethodInvocation mi = super.visitMethodInvocation(method, executionContext);
if (guavaCreateTempDirMatcher.matches(mi)) {
return mi.withName(mi.getName().withSimpleName("orElse"))
.withTemplate(JavaTemplate.builder(this::getCursor, "null").build(), mi.getCoordinates().replaceArguments());
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect this is actually the problem of the ChangeType recipe (I will investigate this further, when I find time), but as you can see in the following screenshot from the debugger, the type attribution of the optional parameter refers to the correct new type, but as its owner still refers to the old JavaType.Method object:
image


class A {
String foo(Optional<String> optional) {
return optional.orElse(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the type attribution is slightly messed up by the recipe. In the following screenshot you see the method type of the method invocation of the orElse() method. I was slightly surprised by the String return type, but I think this is normal for OpenRewrite as it uses JavaType.Method both for method declarations and method invocations and they are slightly different in this respect. What is clearly missing, however, is the parameter type.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

This one is still weird, but I think we can fix that later.
image

@timtebeek
Copy link
Contributor Author

After the addition of the applicability test in 98739f2 the tests now fail with:

PreferJavaUtilOptionalTest > orToOrElse() FAILED
java.lang.AssertionError: [Recipe validation must have no failures]
Expecting empty but was: [Invalid{property='org.openrewrite.java.search.UsesJavaVersion', value='{majorVersionMin=9}', message='Recipe class org.openrewrite.java.search.UsesJavaVersion cannot be found'}]

Don't immediately know why; but working on other things at the moment.

@timtebeek
Copy link
Contributor Author

Fun to do some recipe development again; Are you OK to pickup the identified type issues separately @knutwannheden ?

Copy link
Contributor

@knutwannheden knutwannheden left a comment

Choose a reason for hiding this comment

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

👍

@timtebeek timtebeek merged commit 50e7478 into main Mar 31, 2023
1 check passed
@timtebeek timtebeek deleted the guava_optional branch March 31, 2023 08:17
@timtebeek timtebeek mentioned this pull request Mar 31, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recipe Recipe requested
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants