Chaining UseCollectionInterfaces and RemoveUnusedPrivateFields incorrectly removes field

[MagicalAsh]

What version of OpenRewrite are you using?

I am using

• OpenRewrite v8.31.1
• Gradle plugin v6.17.1
• rewrite-recipe-bom v2.15.0
• rewrite-java

How are you running OpenRewrite?

I am using the Gradle plugin, and my project is a single module project. I've attached a copy of the project directory.

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

Using this config file:

type: specs.openrewrite.org/v1beta/recipe
name: com.example.BaseFormat
displayName: Base Java Format
description: +
  This is a recipe that defines our formatting base
recipeList:
  - org.openrewrite.java.RemoveUnusedImports
  - org.openrewrite.staticanalysis.RemoveUnusedPrivateFields
  - org.openrewrite.staticanalysis.UseCollectionInterfaces
---
type: specs.openrewrite.org/v1beta/recipe
name: com.example.Migrate1
displayName: Migration recipe 1
description: +
  Migrate recipe 1
recipeList:
  - com.example.BaseFormat
---
type: specs.openrewrite.org/v1beta/recipe
name: com.example.Migrate2
displayName: Migration Recipe 2
description: +
  Migrate recipe 2
recipeList:
  - com.example.BaseFormat
  - com.example.Migrate1
---
type: specs.openrewrite.org/v1beta/recipe
name: com.example.Migrate3
displayName: Migration Recipe 3
description: +
  Migrate recipe 3
recipeList:
  - com.example.BaseFormat
  - com.example.Migrate2

We see errors in the following file after running ./gradlew rewriteRun when our project is configured with activeRecipe("com.example.Migrate3").

package org.example;

import jakarta.servlet.http.HttpServletRequest;
import org.springframework.http.HttpMethod;

import java.util.Arrays;
import java.util.HashSet;

public class Main {
    private static final HashSet<String> allowedMethods = new HashSet<>(Arrays.asList(
            HttpMethod.GET.name(), HttpMethod.HEAD.name(), HttpMethod.TRACE.name(), HttpMethod.OPTIONS.name()));

    public boolean matches(HttpServletRequest request) {
        if (allowedMethods.contains(request.getMethod())) {
            return false;
        }
        return true;
    }
}

We were able to correct the incorrect behavior by removing the duplicated invocations of the com.example.BaseFormat recipe in our instance, but the expected behavior here would be that it simply runs the recipes correctly. Figuring out that this was the issue took a while.

What did you expect to see?

The expected behavior would be to see all recipes executed correctly, i.e. the output should be nearly identical.

package org.example;

import jakarta.servlet.http.HttpServletRequest;
import org.springframework.http.HttpMethod;

import java.util.Arrays;
import java.util.Set;

public class Main {
    private static final Set<String> allowedMethods = new HashSet<>(Arrays.asList(
            HttpMethod.GET.name(), HttpMethod.HEAD.name(), HttpMethod.TRACE.name(), HttpMethod.OPTIONS.name()));

    public boolean matches(HttpServletRequest request) {
        if (allowedMethods.contains(request.getMethod())) {
            return false;
        }
        return true;
    }
}

What did you see instead?

package org.example;

import jakarta.servlet.http.HttpServletRequest;
import java.util.HashSet;

public class Main {

    public boolean matches(HttpServletRequest request) {
        if (allowedMethods.contains(request.getMethod())) {
            return false;
        }
        return true;
    }
}

Note that the in-use allowedMethods private constant was removed, and that there's now a leftover unused import.

What is the full stack trace of any errors you encountered?

No explicit errors occured in the execution of OpenRewrite, however the following was output by the plugin:

All sources parsed, running active recipes: com.example.Migrate3
Changes have been made to src/main/java/org/example/Main.java by:
    com.example.BaseFormat
        org.openrewrite.staticanalysis.UseCollectionInterfaces
        com.example.Migrate2
            com.example.BaseFormat
                org.openrewrite.staticanalysis.RemoveUnusedPrivateFields
            com.example.Migrate1
                com.example.BaseFormat
                    org.openrewrite.java.RemoveUnusedImports
Please review and commit the results.
Estimate time saved: 10m

Are you interested in contributing a fix to OpenRewrite?

I unfortunately don't have time to both investigate and submit a fix for this at the moment, sorry.

[timtebeek]

Thanks for the detailed report @MagicalAsh ; That must not have been easy to pin and reduce down. Know that it is appreciated.

I don't quite know what would cause this to start failing, but it's definitely concerning. I suspect it's to do with one of those recipes not updating the model correctly, but that would have to be explored.

[timtebeek]

As a first step I've taken the recipes you've provided and replicated the problem in a unit test.

@Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/321")
@Test
void repeatedApplicationOfRecipe() {
    rewriteRun(
      spec -> spec.recipes(
        new UseCollectionInterfaces(),
        new RemoveUnusedPrivateFields()
      ),
      //language=java
      java(
        """
          import java.util.Arrays;
          import java.util.HashSet;

          class Main {
              private static final HashSet<String> allowedMethods = new HashSet<>(Arrays.asList(
                      "GET", "HEAD", "TRACE", "OPTIONS"));

              public boolean matches() {
                  return allowedMethods.contains("GET");
              }
          }
          """,
        """
          import java.util.Arrays;
          import java.util.HashSet;
          import java.util.Set;

          class Main {
              private static final Set<String> allowedMethods = new HashSet<>(Arrays.asList(
                      "GET", "HEAD", "TRACE", "OPTIONS"));

              public boolean matches() {
                  return allowedMethods.contains("GET");
              }
          }
          """
      )
    );
}

Note that I was able to remove the chained recipes, as tests already run recipes repeatedly.

[timtebeek]

I've been able to narrow down your example quite a bit; it looks like this exact combination and minimal code above is enough to reproduce the issue; now to figure out why it's happening. 😕