openrewrite / rewrite-migrate-java

OpenRewrite recipes for migrating to newer versions of Java.
Apache License 2.0
99 stars 69 forks source link

Convert Guava Optional to JDK #197

Closed timtebeek closed 1 year ago

timtebeek commented 1 year ago

We already have quite some recipes to convert Guava classes to standard JDK equivalents: https://github.com/openrewrite/rewrite-migrate-java/blob/main/src/main/resources/META-INF/rewrite/no-guava.yml Seems like Guava's Optional could be a nice addition to the list of supported migrations.

Guava

https://guava.dev/releases/19.0/api/docs/com/google/common/base/Optional.html

Modifier and Type Method and Description
static  Optional absent()Returns an Optional instance with no contained reference.
abstract Set asSet()Returns an immutable singleton Set whose only element is the contained instance if it is present; an empty immutable Set otherwise.
static  Optional fromNullable(T nullableReference)If nullableReference is non-null, returns an Optional instance containing that reference; otherwise returns absent().
abstract T get()Returns the contained instance, which must be present.
abstract boolean isPresent()Returns true if this holder contains a (non-null) instance.
static  Optional of(T reference)Returns an Optional instance containing the given non-null reference.
abstract Optional or(Optional<? extends T> secondChoice)Returns this Optional if it has a value present; secondChoice otherwise.
abstract T or(Supplier<? extends T> supplier)Returns the contained instance if it is present; supplier.get() otherwise.
abstract T or(T defaultValue)Returns the contained instance if it is present; defaultValue otherwise.
abstract T orNull()Returns the contained instance if it is present; null otherwise.
static  Iterable presentInstances(Iterable<? extends Optional<? extends T>> optionals)Returns the value of each present instance from the supplied optionals, in order, skipping over occurrences of absent().
abstract  Optional transform(Function<? super T,V> function)If the instance is present, it is transformed with the given Function; otherwise, absent() is returned.

JDK

https://docs.oracle.com/javase/8/docs/api/java/util/Optional.html

Modifier and Type Method and Description
static  Optional empty()Returns an empty Optional instance.
Optional filter(Predicate<? super T> predicate)If a value is present, and the value matches the given predicate, return an Optional describing the value, otherwise return an empty Optional.
 Optional flatMap(Function<? super T,Optional> mapper)If a value is present, apply the provided Optional-bearing mapping function to it, return that result, otherwise return an empty Optional.
T get()If a value is present in this Optional, returns the value, otherwise throws NoSuchElementException.
void ifPresent(Consumer<? super T> consumer)If a value is present, invoke the specified consumer with the value, otherwise do nothing.
boolean isPresent()Return true if there is a value present, otherwise false.
 Optional map(Function<? super T,? extends U> mapper)If a value is present, apply the provided mapping function to it, and if the result is non-null, return an Optional describing the result.
static  Optional of(T value)Returns an Optional with the specified present non-null value.
static  Optional ofNullable(T value)Returns an Optional describing the specified value, if non-null, otherwise returns an empty Optional.
T orElse(T other)Return the value if present, otherwise return other.
T orElseGet(Supplier<? extends T> other)Return the value if present, otherwise invoke other and return the result of that invocation.
<X extends Throwable>T orElseThrow(Supplier<? extends X> exceptionSupplier)Return the contained value, if present, otherwise throw an exception to be created by the provided supplier.

Quick analysis

There's a few cases that will need to be looked at in detail, such as:

knutwannheden commented 1 year ago

There could be a few more small corner cases. The Guava Javadoc actually tell you what the difference is and what the Java equivalent is. Also note that version 21 added the utilities fromJavaUtil() and toJavaUtil() which should be easy to replace :-)

timtebeek commented 1 year ago

That Comparison to java.util.Optional note is helpful indeed; hadn't noticed that yet; think I arrived at mostly the same, but good to check that when implementing.

I think we can cover most of these quite nicely with declarative recipes already; the remaining ones might be doable with rewrite templates once that's further along, and don't seem to be as highly used as some of the other ones.

knutwannheden commented 1 year ago

I thought a bit about this recipe and it seems like it could be a bit difficult to implement if we have to ensure that the type attribution is still correct everywhere. But I may think about it the wrong way.

timtebeek commented 1 year ago

Looking at some of the other recipes we seem to mostly call out to ChangeType in similar cases. This one would be slightly different as it would also require a first few ChangeMethodName calls before ChangeType, and dedicated recipes for the not-one-to-one cases. To start with I don't there's much else right?

https://github.com/openrewrite/rewrite-migrate-java/blob/bb9015bac09d66e861aa92827fb5e4bd893d8b51/src/main/resources/META-INF/rewrite/no-guava.yml#L61-L95

knutwannheden commented 1 year ago

I guess you are right. It probably just gets a bit weird when we rewrite uses of Guava Optional where some called third-party API expects a Guava Optional. Especially if this is just some other Guava API. But I guess this kind of problem can happen everywhere and is hard to do anything about. Of course, in this specific case, we could use the fromJavaUtil() and toJavaUtil() utilities in these places. But this sounds non-trivial.

knutwannheden commented 1 year ago

The other problem I was thinking of is what happens to the type attribution between the ChangeMethodName, ChangeMethodTargetToStatic, and ChangeType recipes and where in this order we insert the imperative recipes. Will the imperative recipe be able to rely on the type attribution to do its work?

Cases like Optional.fromNullable(null).or(Optional.of("foo")) may require a very careful ordering of the recipes so that this works properly.

timtebeek commented 1 year ago

Mostly implemented in https://github.com/openrewrite/rewrite-migrate-java/pull/202 ; propose we close this one and pick up the asSet and presentInstances methods only when asked. Preferably by then we're further along with rewrite-templating to make that even easier to do.