Inconsistency around com.google.common.base.Predicate in the "NoGuava" recipe

[lredor]

Before my issues regarding com.google.common.base.Predicate, the rule seemed fairly consistent: com.google.common.base.Predicate was migrated to java.util.function.Predicate, but many methods using com.google.common.base.Predicate as a parameter were not migrated; which caused numerous compilation errors.

• With my recent tickets (including my own PRs and those from @timtebeek — PR Fix NoGuavaSetsNewHashSet to skip Iterable-only arguments #882, PR Convert Guava Predicates.and and .or #885, PR Convert Guava Predicates.equalTo and .not #887, PR Convert Guava Iterables.any and .filter #896, PR Convert Guava Sets.filter #898 and PR Support varargs for Predicates.and() and Predicates.or() #901), several of these methods have now been migrated.

• However, with PR Only convert Guava Predicate when not used as method argument #902, we’ve partially reverted the migration of com.google.common.base.Predicate, keeping it only for certain cases.

• As a result, new compilation errors have appeared, and PR Only convert Guava Predicate when not used as method argument #902 now contradicts several of the PRs mentioned above.
  So, I’m not sure whether the current state is actually "more stable" or "more consistent".

• In addition, PR Only convert Guava Predicate when not used as method argument #902 only affects the type migration, but the rule for renaming the method apply → test has remained unchanged; whereas it should follow the same logic as the type.
  This block in no-guava.yml should probably be extracted and moved into its own recipe, or in NoGuavaPredicate recipe:

  - org.openrewrite.java.ChangeMethodName:
      methodPattern: com.google.common.base.Predicate apply(..)
      newMethodName: test
      matchOverrides: true

I’ve been on sick leave for a few weeks now, including this week, and I’ve used the time to explore OpenRewrite by testing the NoGuava recipe on the Sirius Desktop codebase.
I’ll try to dig a bit deeper into this over the next few days, but my availability will decrease soon.

I’d like at least to clarify the current status around Predicate, though given its wide variety of use cases, that might be a challenging goal.
I’ll keep listing here the different cases I encounter.

[timtebeek]

Thanks for digging in here with all the reports and fixes; I've tried to replicate the issues you're describing with an update to our unit tests, but even with 166202b I'm not seeing any immediate issues. Would you have an example (ideally as a failing test) of a case where you now see compilation issues?

[lredor]

PR #909 illustrates the compilation problem for the apply method, which is converted even though its type is not.

[lredor]

Thanks for digging in here with all the reports and fixes; I've tried to replicate the issues you're describing with an update to our unit tests, but even with 166202b I'm not seeing any immediate issues. Would you have an example (ideally as a failing test) of a case where you now see compilation issues?

I think I may have had a misunderstanding about how the recipes are executed. Therefore, PR #898 might not actually conflict with #902 (contrary to what I thought). The problem is probably a combination of different Predicate usages. I’ll try to analyze all of this.
In the meantime, I have two questions.
1- Is the order of recipes in the no-guava.yml file important, since the result of one recipe can depend on the result of the previous one?
2- If that’s the case, why was it necessary to exclude the Predicates.not case in NoGuavaPredicate? Indeed, the replacement ChangeMethodTargetToStatic Predicates.not is executed before org.openrewrite.java.migrate.guava.NoGuavaPredicate.

[timtebeek]

1. Yes the order is important; Each recipes hands of a (potentially) modified copy of the lossless semantic tree model to the next recipes; at intermediate stages there might be partial migrations, but as long as the end result is consistent it should all work out. It's relevant when say you first change a type, and then want to rename a method; that second recipe should then use the already modified type when matching the method pattern.
2. In this case the ChangeMethodTargetToStatic moves the method to a different type, but does not yet change the type or the argument going into that method (even if that's the same here). That's why we explicitly allow not in NoGuavaPredicate since it might still match based on the method argument. I hope that's clear! :)

[lredor]

I cover a new case for Predicates.and() and Predicates.or() in commit "Support Lambda for Predicates.and() and Predicates.or()" of the PR #909.