rewrite-migrate-java icon indicating copy to clipboard operation
rewrite-migrate-java copied to clipboard

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

Open delanym opened this issue 1 year ago • 1 comments

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 avatar May 14 '24 16:05 delanym

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 avatar May 14 '24 21:05 timtebeek

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

  • https://github.com/openrewrite/rewrite-migrate-java/issues/423
  • https://github.com/openrewrite/rewrite-migrate-java/issues/424

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

timtebeek avatar Nov 02 '24 20:11 timtebeek