search

openrewrite / rewrite-migrate-java

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

Support everything that modernizer-maven-plugin does OOTB #39

Closed jkschneider closed 1 month ago

jkschneider commented 3 years ago

...and maybe ask to be included in resources list on its README?

https://github.com/gaul/modernizer-maven-plugin/blob/master/modernizer-maven-plugin/src/main/resources/modernizer.xml

csemrau commented 3 years ago

Just ran a test for org.openrewrite.java.migrate.guava.PreferJavaUtilObjectsHashCode because I was curious about how one would migrate a static method of one class to a differently named static method of another class. It did not work as expected.

Class to be rewrittten:

package com.acme.product;

import com.google.common.base.Objects;

public class MyPojo {
    String x;

    @Override
    public int hashCode() {
        return Objects.hashCode(x);
    }
}

The import is replaced, but the hashCode method reference is not rewrittten to hash.

Seems that the second inner recipe (ChangeMethodName) must build on top of the result of the first inner recipe, i.e. refer to the (non-existent!) method java.util.Objects hashCode(..), instead of the original method com.google.common.base.Objects hashCode(..).

There are a few other recipes in no-guava.yml that change target type and name of a method, and all of them change the name first and then the target type.

csemrau commented 3 years ago

I see some problems/risks with replacing ImmutableSet#of() with Set#of() as part of the NoGuava recipe:

  1. Guava recomments that ImmutableSet should be treated like an interface, implying that fields and variables should be typed as ImmutableSet instead of Set to denote the immutability guarantee. If I use it like in the following class, the result won't compile:
import com.google.common.collect.ImmutableSet;

public class UseSet {
    ImmutableSet<String> s = ImmutableSet.of("1", "2");
}
  1. ImmutableSet guarantees that iteration order equals creation order. The set returned by Set#of intentionally shuffles the iteration order. Depending on the fixed iteration order is wrong, but there are situations where such dependency is present nontheless.
csemrau commented 3 years ago

The recipe org.openrewrite.java.migrate.guava.PreferJavaUtilObjectsEquals misses to rewrite the method name as well. Guava uses equal, while java.util.Objects uses equals.

aegershman commented 3 years ago

Good catches @csemrau 👍 I haven't taken a deeper look the ImmutableSet comment yet, but I believe I've addressed the two guava comments you discussed above. This is good feedback, thanks again.

aegershman commented 3 years ago

Hey @csemrau I agree with you about this ImmutableSet comment https://github.com/openrewrite/rewrite-migrate-java/issues/39#issuecomment-909639206

as per https://docs.oracle.com/javase/9/docs/api/java/util/Set.html :

The iteration order of set elements is unspecified and is subject to change.

Whereas guava ImmutableCollection types preserve iteration order, as per https://github.com/google/guava/wiki/ImmutableCollectionsExplained#how

Except for sorted collections, order is preserved from construction time. For example,

So, aside from the fact assigning ImmutableSet as the type would break when it's changed to Set.of, here's some options of what I think we could do:

Do you have thoughts on this? If the discussion is more involved we'll probably be better off creating this as a separate github issue, which is no problem either.

csemrau commented 3 years ago

The type mismatch in ImmutableSet<String> s = Set.of("") would be noticed at compile time. The change can then either be reverted or the variable type can be updated. OTOH, the changed iteration order will only show up at test time, and only if you're lucky. So I would leave it out of the composite recipe, because the changes may get drowned in all the other changes performed by the recipe.

I think you should leave the recipe as a standalone one, to be invoked on its own. It is still useful as a stepping stone in refactoring a code base.

aegershman commented 3 years ago

Taken care of in https://github.com/openrewrite/rewrite-migrate-java/commit/c2e690b738e4948a11066eb3aa510bf3c5b6a34e thanks @csemrau -- if you find anything else or have other issues or suggestions, definitely let us know; but just to stop this issue from getting more cluttered, could you log them as separate issues? That'll make it easier to detangle stuff in the future 👍

Thanks again. Good catches.

yeikel commented 2 years ago

This is a nice idea but I think that if we had separate issues it would be easier for the community to start adding them and to track what we have vs the upstream modernizer plugin

Also, we will never be able to close this issue because modernizer is constantly evolving

timtebeek commented 1 year ago

A lot of time has passed between this ticket being opened and improvements and recipes since.

We can check what we currently cover of their migrations away from

Especially now that we have Refaster template support, it should be easy to cover additional recipes: https://github.com/openrewrite/rewrite-migrate-java/blob/v2.1.0/src/main/java/org/openrewrite/java/migrate/apache/commons/lang/ApacheCommonsStringUtils.java#L28-L39

I'd lean towards maybe a quick check and creation of additional separate issues, and then (or already) close this issue as mostly completed.

timtebeek commented 1 month ago

With the Joda time recipes now also merged in https://github.com/openrewrite/rewrite-migrate-java/pull/567 I propose we close this issue and open new specific items if there's anything we'd still like to cover.