openrewrite / rewrite-static-analysis

OpenRewrite recipes for identifying and fixing static analysis issues.
Apache License 2.0
27 stars 43 forks source link

Change mutable public collections to immutable versions #125

Open yeikel opened 1 year ago

yeikel commented 1 year ago

public static final List<String> entries = Arrays.asList("A","B");

public static final List<String> entries = Collections.unmodifiableList(Arrays.asList("A","B"));

While List.of is also an option, it is important to note that List.of throws NPE for null access patterns (like contains) and that can be problematic.


var a = Collections.unmodifiableList(Arrays.asList("A","B"));
var b =Arrays.asList("A","B");
var c = List.of("A","B");

a.contains(null) // false
b.contains(null) // false
c.contains(null) // NPE
coiouhkc commented 1 year ago

Could you please assign it to me? I would attempt it in coming weeks...

timtebeek commented 1 year ago

I have some doubts if this recipe could do harm in certain situations. Consider something like:

@Test
void doNotWrapIfSet() {
    rewriteRun(
      java("""
        import java.util.Arrays;
        import java.util.List;

        class A {
            public static void main(String[] args) {
                List<String> entries = Arrays.asList("A", "B");
                entries.set(0, "C");
                System.out.println(entries);
            }
        }
        """
      )
    );
}

This is allowed currently, as changing an element of the backing array does not change the size of the list.

But this recipe could convert that into Collections.unmodifiableList(Arrays.asList("A", "B")), which no longer allows entries.set(0, "C").

JLLeitschuh commented 1 year ago

You could use data flow to determine if entries is ever used with set