Change mutable public collections to immutable versions: ref issue #2413 #3308
Conversation
…AsListWithUnmodifiable' into feature/createRecipeToWrapArraysAsListWithUnmodifiable
|
Thank you for starting this implementation! It's looking quite good already. |
| * This recipe modifies the invocation of a method so that it returns an unmodifiable list, if the | ||
| * method returns a modifiable one. |
There was a problem hiding this comment.
Perhaps this is more of an issue with the original feature request rather than the recipe implementation, but I have some doubts if this recipe could do harm in certain situations. More details here: https://github.com/openrewrite/rewrite/issues/2413#issuecomment-1586767731
rewrite-java/src/main/java/org/openrewrite/java/recipes/ArraysAsListToImmutableRecipe.java
Outdated
Show resolved
Hide resolved
rewrite-java/src/main/java/org/openrewrite/java/recipes/ArraysAsListToImmutableRecipe.java
Show resolved
Hide resolved
| import java.util.Arrays; | ||
|
|
||
| class A { | ||
| public static final List<String> entries = Arrays.asList("A", "B"); |
There was a problem hiding this comment.
IIRC, Arrays.asList already returns a list that is unmodifiable.
There was a problem hiding this comment.
Ah, I'm wrong, you can't append to it, but you can replace existing values.
There was a problem hiding this comment.
Indeed; more details discussed in this thread; I like your suggestion of using data flow to make this recipe safe to execute. I've not used data flow myself before, but would be good to explore here.
There was a problem hiding this comment.
To use data flow, this recipe would need to be moved to the cleanup package in rewrite-static-analysis repository
There was a problem hiding this comment.
That's indeed perhaps a better place for this recipe; sorry about that @marynasavchenko . Would you mind targeting https://github.com/openrewrite/rewrite-static-analysis/tree/main/src/main/java/org/openrewrite/staticanalysis for these files?
As seen above that's necessary to use data flow, which we need to be sure we can safely apply this recipe.
…AsListWithUnmodifiable
…AsListWithUnmodifiable
|
Closed pending a move to rewrite-static-analysis, which we need for the data flow analysis to determine if it's safe to do this replacement: we can't replace when list elements or order is modified. Great start, looking forward to the full implementation. |
What's changed?
Added new recipe to wrap modifiable lists to be unmodifiable.
What's your motivation?
Closes openrewrite/rewrite-static-analysis#125
Any additional context
Thanks for your work! I hope we can support with our small improvements.
Checklist
./gradlew licenseFormat