ChangeType Recipe Incorrectly Updates from OldType to NewType

[amishra-u]

The ChangeType recipe updates types from OldType to NewType without verifying the correctness, eg. Changed type of a method argument with verifying that new type will be assignable to method invocation.

While running the NoGuava recipe, which includes multiple ChangeType recipes, the project failed to build after applying them.

The ChangeType recipe serves as a foundational building block and is used in many other recipes. We expect such foundational recipes to be 100% accurate, as any bugs in these core recipes can impact numerous others. It would be beneficial to enforce stricter standards for these building block recipes to ensure reliability.

[timtebeek]

hi @amishra-u ; would you happen to have any examples where this is not working out as you had expected? Typically we've found this form to work well to give us all the details we need to start troubleshooting.

[amishra-u]

Here are fews examples, but list is big.

1. Maps.filterKeys(headers, new Predicate() {...} /guava Predicate to java predicate/) ;
2. Iterators.transform(new S3ListObjectIterator(client, s3URIString), new Function<S3ObjectSummary, String>() {,...} /guava Fuction to java Function/)

[knutwannheden]

While we could add an option for this, there are a lot of cases where ChangeType is intentionally used to replace a type with another non-assignable type. There should then of course also be another recipe to compensate for that, but it seems like that isn't always the case. That would then be a bug in that composite recipe.

An option would be possible here, but in many cases this would not be used.

[amishra-u]

I believe each recipe should be fully self-contained. While I find JavaTemplate very useful, it's not defined as a recipe because applying the template without proper checks could leave the project in an inconsistent state. Similarly, if ChangeType can lead to changes that make the project inconsistent, it might not be a good idea to define it as a recipe either.

[knutwannheden]

If we take Guava's Function as an example, ChangeType would first be used to replace it everywhere with Java's Function. This works well as long as no third-party libraries are used, which operate on Function. But that would typically be outside of the recipe's scope and would have to be handled by custom recipes. (Guava may even provide an adapter method.) But if any Guava methods operating on Functions are used, then the NoGuava recipe should also have visitors to deal with that, but that can't be part of ChangeType.

Do you have examples where the NoGuava recipe could be improved for everyone?

[jkschneider]

I don't quite agree that ChangeType is flawed because it is possible to apply it to change A to B in such a way that there is an assignability problem. Applying that same logic, ChangeDependencyVersion couldn't be a recipe if it was possible to select a version that didn't exist.

It sounds to me like the main issue here is in the Guava recipe, which may require a more custom recipe than just an application of ChangeType.

[amishra-u]

The philosophy here is that the ChangeType Recipe will update the change type as requested, but the responsibility for using it appropriately lies with the consumer recipe. Isn't this contradict with "do not harm" best practice? https://docs.openrewrite.org/authoring-recipes/recipe-conventions-and-best-practices#do-no-harm

[knutwannheden]

If you can provide the details about which consuming recipe is making a mistake, then we can create an issue for that and look into a fix. This fix could then either be to not make any change (to do no harm) or to complete the recipe to perform the missing steps.

But as already pointed out, if additional third-party dependencies are involved, they will probably be out of scope.

[amishra-u]

@knutwannheden The problem is with NoGuava Recipe. It includes ChangeType recipe from GuavaFunction to JavaFunction. https://github.com/openrewrite/rewrite-migrate-java/pull/580

Here is the example code for which it will fail.

void foo() {
   java.util.Map<Integer, String> x = Map.of()
   com.google.common.collect.Maps.filterKeys(x, new com.google.common.base.Predicate() { 
           @Override
          public boolean apply(Integer key) {
            return key % 2 == 0
          }
   }
} 

Above code will update the com.google.common.base.Predicate to java.util.function.Predicate but Maps.filterKeys doesn't accept argument of type java.util.function.Predicate.

[okundzich]

Think of ChangeType as a helper recipe, and it is the responsibility of the consumer/recipe author to pair it correctly with other recipes that make corresponding code changes. In this case, it's NoGuave recipe.

Overall, there is always human involvement in reviewing and testing code changes. The effort to make migrations 100% automated and correct has a long tale and diminishing returns. Here is an example of our own Spring Boot 3 migration

On the other hand, there are some recipes, usually security fixes like Log4shell, that are 100% accurate, and can be issued with automation, and do not require additional manual work. So it really depends on the situation.