NoGuavaJava21 causing invalid code since v2.28.0

[protocol7]

What version of OpenRewrite are you using?

• org.openrewrite:rewrite-core:8.38.0
• org.openrewrite.recipe:rewrite-migrate-java:2.28.0

What is the smallest, simplest way to reproduce the problem?

Since the latest release of OpenRewrite, we're seeing it generate invalid code seemingly related to the Guava collections, perhaps related to

• Fix Guava Immutable{Set|List|Map} #586

I have not been able to reproduce these as tests, so including some samples here in the meantime.

Rewritten by org.openrewrite.java.migrate.guava.NoGuavaJava11:

   public static ImmutableList<Class<? extends Provider<ClientInterceptor>>>
       defaultClientInterceptorProviders() {
-    return ImmutableList.of(
+    return List.of(
         OutgoingRequestLoggingServerInterceptorProvider.class,
         CtxTrackerClientInterceptorProvider.class,
         RequestMetadataClientInterceptorProvider.class,

Note that the method is declared to return ImmutableList but has here been rewritten to actually return a java.util.List.

Here's a more involved example, in this case rewritten by org.openrewrite.staticanalysis.RemoveUnusedPrivateFields and org.openrewrite.java.migrate.guava.NoGuavaJava21:

 public class DiagramRules {
-  private static final ImmutableSet<String> INCLUDE_COMPONENT_TYPES =
-      ImmutableSet.of(
-          "service",
-          "data",
-          "other");

   static boolean includeType(final String type) {
     return INCLUDE_COMPONENT_TYPES.contains(type);

Note how INCLUDE_COMPONENT_TYPES is still referenced. The code is only removed if both recipes are enabled.

Both of these examples causes compiler errors. We have plenty of these examples and have rolled back to the previous version for now.

[timtebeek]

Ah sorry to hear about your issues @protocol7 ! Thanks for the swift report here. Definitely something to fix.
/cc @BramliAK who helped expand what these recipes cover in the #586 linked above.

Not quite sure what's the best way forward here; for the return we could change the return type, but that might just shift the problem, and potentially shift it beyond the SourceFile changed, meaning we can't guarantee correctness beyond that point.

For the interaction with RemoveUnusedPrivateFields I suspect we don't update the references in the same sourcefile to the new field after the type change, which means RemoveUnusedPrivateFields can not find any usages. That's still potentially fixable here.

With the NoGuava recipes there's always a bit of a conflict of interests; some folks want to fully phase these out, and are fine to potential finish up partial changes the recipe might do, whereas others are less tolerant of incomplete migrations, which I expect to be the case here.

For our understanding, could you explain a bit how you're using these recipes @protocol7 ? In particular I'm interested in how and why you're running these repeatedly, and what led to you seeing these changes now in a problematic way fixed with a roll back to an older version, as opposed to rolling forward to complete the move away from Guava.

[BramliAK]

Sorry for creating this issue due to my modification.
There is a compilation error in this part

 public static ImmutableList<Class<? extends Provider<ClientInterceptor>>>
       defaultClientInterceptorProviders() {
 -    return ImmutableList.of(
 +    return List.of(

because I am not changing the return type of the function, but rather changing the return type of the immtableList.
I will try to cover this case or don't make the change like the previous version

[protocol7]

Hey, thanks for the quick responses, despite my struggles turning these into proper test cases.

Yeah, I understand the tradeoffs involved here in terms of how aggressive these types of recipes should be. We use these recipes (some ~200 in total, some custom but most out of the box) as part of a daily, scheduled rewrite in a mostly Java monorepo. Currently with only ~220 kloc, but our hope is to import in our full production code over the next year, which would mean ~5 Mloc when done. We use the recipes to both ensure code consistency and to improve code quality, e.g. remove the use of legacy APIs (like much of Guava in this case). Since we are continuously importing code into the repo, and developers work there every day producing new code, we run this as a daily scheduled job. (We also upgrade OpenRewrite daily, which is why you've seen us being fairly quick in reporting regressions in recipes over the last few months)

Thus, it is crucially important to us that we don't break any code as part of this, including in some cases code that lives outside of the monorepo, where we do not have these rewrites, as that will require manual fixing, sometimes in hundreds or thousands of places. As such, we aim to not break any API compatibility. Thus, as you point out, for the first example here, it would be preferable to us to not change the method signature but in this case simply leave the ImmutableList.of() (as done prior to 2.28.0).

I understand that this might not be the desired tradeoff for everyone though, and if you decide to go with the approach with breaking the method signature, we would most likely need to remove this recipe (we already exclude some other recipes for similar reasons) and consider implementing a custom recipe.

[knutwannheden]

@BramliAK The recipe could only change the return type of private methods and this further only if the callers can all be updated to use List instead. This analysis can quickly get a bit complicated and I am not sure we offer any mechanism with the analysis of that. So in a first step we will probably have to leave the return type as is.