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

Safely expand SequencedCollection getFirst/Last to also convert lists returned from method calls #483

Closed
delanym opened this issue May 14, 2024 · 2 comments
Labels
enhancement New feature or request java 21+

Comments

@delanym
Copy link

delanym commented May 14, 2024

The recipe at https://docs.openrewrite.org/recipes/java/migrate/util/sequencedcollection
can recognize a match where a simple index value is used

-     TokenAdaptor tokenAdaptor = cachedTokens.get(0);
+     TokenAdaptor tokenAdaptor = cachedTokens.getFirst();

but it misses code where the size of the collection being operated on is given (which an Intellij inspection will convert) such as

-      getHistory().remove(getHistory().size() - 1);
+      getHistory().removeLast();

or even where a constant is used, such as

-      var storedTransactionLogAdaptor = (StoredTransactionLogAdaptor) resultVector.get(SettlementTransactionTokenSubTransactionConstants.EXPECTED_RESULT_SET_RECORD);
+      var storedTransactionLogAdaptor = (StoredTransactionLogAdaptor) resultVector.getFirst();

Calculating the index based on a constant value may be asking too much.

@delanym delanym added the bug Something isn't working label May 14, 2024
@timtebeek
Copy link
Contributor

timtebeek commented May 14, 2024

getLast() should work when a variable is used, but with method calls we hadn't yet covered those out of caution. In theory there might be side effects to that method call, hence why we're being safe. While it ought to be rare that folks call a method twice and expect a different collection, we can't rule it out and hence have to opt for the safest mode of not making code changes.

@timtebeek timtebeek changed the title Improve SequencedCollection recipe Safely expand SequencedCollection getFirst/Last to also convert lists returned from method calls May 20, 2024
@timtebeek timtebeek added enhancement New feature or request and removed bug Something isn't working labels May 20, 2024
@timtebeek timtebeek transferred this issue from openrewrite/rewrite May 20, 2024
@timtebeek
Copy link
Contributor

Thanks again for the suggestion @delanym , but I don't really see a good way forward to cover this in a guaranteed safe way for add/get/removeLast. Note that we already added support for add/get/removeFirst in

Best then to close this issue such that we don't make any unsafe changes.

@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenRewrite Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request java 21+
Projects
Archived in project
Development

No branches or pull requests

2 participants