Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

recipe: replace Arrays#asList() by List#of() #466

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kthoms
Copy link
Contributor

@kthoms kthoms commented Feb 16, 2025

What's changed?

Adds a new recipe

What's your motivation?

We have lots of usages in our code base and they are reported as warnings by default

Anything in particular you'd like reviewers to focus on?

The second test is failing. I need help here with the implementation to fix it.

Failed to run recipe at Cursor{MethodInvocation->JRightPadded(element=Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9, 10), after=Space(comments=<0 comments>, whitespace=<empty>))->Block->MethodDeclaration->JRightPadded(element=MethodDeclaration{A{name=foo,return=void,parameters=[]}}, after=Space(comments=<0 comments>, whitespace=<empty>))->Block->ClassDeclaration->CompilationUnit->root}

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@timtebeek
Copy link
Contributor

Thanks for kicking this off @kthoms ; we had so far held off on a recipe like this because List.of returns immutable collections, whereas Arrays.asList returns a new mutable array list. That means this is not yet a guaranteed safe replacement, and could lead to runtime issues. What are your thoughts on that?

@kthoms
Copy link
Contributor Author

kthoms commented Feb 18, 2025

You are right, Tim. Although most of the time an array would be converted to a iist for the sake to use the values for API that requires lists, it is not always true and might even break code.

Maybe we could think about restricting it to test code? There we are very likely safe and it is where most usages of the method will be.

Also, I forgot that a check/test is needed that List.of() should not be used when Arrays.asList() is used with a single argument that is an array.

Do you think I should extend the code in that sense?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants