openrewrite / rewrite-migrate-java

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

Recipe to transform uses of Collections.unmodifiableXX to Immutable Static Factory Methods #67

Closed yeikel closed 2 years ago

yeikel commented 2 years ago

I believe that this belongs here (otherwise please let me know under what module this should be reported)

it would be nice to add a rule to the Java 8 -> Java 11 migration to apply the following transformation :

Before :

Set<String> s = Collections.unmodifiableSet(new HashSet<>(Arrays.asList("A", "B")));

After

Set<String> s = Set.of("A", "B")

Additional resources :

See : https://docs.oracle.com/en/java/javase/11/core/creating-immutable-lists-sets-and-maps.html#GUID-930A5334-9413-4E87-A1D7-6FF4E9E421B2

yeikel commented 2 years ago

Another similar transformation :

final List<String> elements = Collections.unmodifiableList(Arrays.asList("A","B"));
final List<String> elements = List.of("A","B"));
yeikel commented 2 years ago

Another example :

Map<String,String> elements = new HashMap<>();
elements.put("hello","world");
Map<String, String> s = Collections.unmodifiableMap(elements);

Map<String, String> s = Map.of("hello", "world");

tkvangorder commented 2 years ago

@yeikel This is a very good suggestion.

traceyyoshima commented 2 years ago

@yeikel I added the base recipes for the examples you provided in this commit.

I decided not to handle situations like:

Before

List<String> stringList = Arrays.asList("a", "b", "c");
stringList = Collections.unmodifiableList(stringList);

After

List<String> stringList = List.of("a", "b", "c");

Because it would require removing a statement and there could be relevant code in between lines 1 and 2, a multi-variable declaration, etc.

Dataflow analysis is in development and we'll be able to handle the transformation safely once it's finished.

Thanks again for the suggestion!

yeikel commented 2 years ago

@yeikel I added the base recipes for the examples you provided in this commit.

I decided not to handle situations like:

Before

List<String> stringList = Arrays.asList("a", "b", "c");
stringList = Collections.unmodifiableList(stringList);

After

List<String> stringList = List.of("a", "b", "c");

Because it would require removing a statement and there could be relevant code in between lines 1 and 2, a multi-variable declaration, etc.

Dataflow analysis is in development and we'll be able to handle the transformation safely once it's finished.

Thanks again for the suggestion!

Thar makes sense. Thank you for explaining!

yeikel commented 2 years ago

@traceyyoshima

While testing my changes for #80

I found that the following condition is not covered by your implementation (basically any custom type)

@Issue("https://github.com/openrewrite/rewrite-migrate-java/issues/67")
@Test
    fun unmodifiableSetTyped() = assertChanged(
        before = """
            import java.util.*;
            import java.time.LocalDate;

            class Test {
                Set<Integer> s = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(LocalDate.of(2010,1,1),LocalDate.now())));
            }
        """,
        after = """
            import java.util.Set;
            import java.time.chrono.ChronoLocalDate;

            class Test {
                Set<LocalDate> s = Set.of(LocalDate.of(2010,1,1),LocalDate.now());
            }
        """
    )

I tried fixing it myself, but I am not sure how the MethodMatcher should look like in this case

traceyyoshima commented 2 years ago

@yeikel thanks for letting me know. I'll take a look first thing on Monday :)

yeikel commented 2 years ago

@traceyyoshima Did you get a chance to take a look at this? I noticed it was released today without the fix :(

traceyyoshima commented 2 years ago

Hi @yeikel!

Apologies, not yet. I'm working on a few things related to the release and haven't had time yet.

But I know what the issue is:

traceyyoshima commented 2 years ago

@yeikel just pushed the update. Thanks again!

yeikel commented 2 years ago

@yeikel just pushed the update. Thanks again!

Thank you!