openrewrite / rewrite-migrate-java

OpenRewrite recipes for migrating to newer versions of Java.
Apache License 2.0
102 stars 71 forks source link

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

Open delanym opened 4 months ago

delanym commented 4 months ago

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.

timtebeek commented 4 months ago

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.