Won't this remove a static method import if the class is not used?

[amirmv2006]

Description

Applying the spring boot 3 migration script, I came across an issue where my static import of assert* methods are removed when they are actually in use. I couldn't create a snippet where this would be reproducible, but I can debug my project locally, and I narrowed it down to this line.

rewrite/rewrite-java/src/main/java/org/openrewrite/java/RemoveImport.java

Line 140 in 53f8095

} else if (TypeUtils.fullyQualifiedNamesAreEqual(typeName, type) && !methodsAndFieldsUsed.contains(imported)) {

It's a bit confusing understanding of "what should happen", so bear with me if these questions are "obvious" 😅

• First of all, if RemoveImport is called for a class with f.q.N, should it remove static method imports from this class? For example should it remove f.q.N.m? IMHO, that shouldn't happen in the first place, because RemoveImport doesn't necessarily mean the class is removed so all its static methods should also be removed... for example, rule org.openrewrite.java.testing.junit5.StaticImports is removing the import for the Assertions class, and adding the import for its used static methods instead.
• Secondly, if it should remove, why is it wrongly assuming my imported static method is not used? I debugged a bit, and what I found is, org.openrewrite.java.internal.TypesInUse is not detecting my imported static method usage. org.openrewrite.java.internal.TypesInUse.FindTypesInUse#visitMethodInvocation is called, and passed method has correct name, but method.getMethodType() is null, but I don't understand why.

What version of OpenRewrite are you using?

I am using

• OpenRewrite v7.40.8 (bom 1.19.4)
• Maven plugin v4.46.0

How are you running OpenRewrite?

I am using the Maven plugin, and my project is a multi-module project.

<plugin>
    <groupId>org.openrewrite.maven</groupId>
    <artifactId>rewrite-maven-plugin</artifactId>
    <version>4.46.0</version>
    <configuration>
        <activeRecipes>
            <recipe>com.x.y.z.MyRecipe</recipe>
        </activeRecipes>
    </configuration>
    <dependencies>
        <dependency>
            <groupId>com.x.y.z</groupId>
            <artifactId>my-recipes</artifactId>
            <version>${my.version}</version>
        </dependency>
    </dependencies>
</plugin>

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

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;

class MyTest {
  void test() {
     ...
     assertFalse(...);
     ...
  }
  void test2() {
     ...
     assertTrue(...);
     ...
  }
  void test3() {
     ...
     assertNotNull(...);
     ...
  }
}

What did you expect to see?

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;

class MyTest {
  void test() {
     ...
     assertFalse(...);
     ...
  }
  void test2() {
     ...
     assertTrue(...);
     ...
  }
  void test3() {
     ...
     assertNotNull(...);
     ...
  }
}

What did you see instead?

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

class MyTest {
  void test() {
     ...
     assertFalse(...);
     ...
  }
  void test2() {
     ...
     assertTrue(...);
     ...
  }
  void test3() {
     ...
     assertNotNull(...);
     ...
  }
}

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

Are you interested in contributing a fix to OpenRewrite?

Yes but I don't know how to fix this.

[timtebeek]

Hi @amirmv2006 ! When getMethodType() is null that usually means OpenRewrite was not able to correctly type attribute the selected element. Usually that means that for instance a classpath entry containing the referenced class is not present, or the method would otherwise be generated by Lombok for instance, which we don't currently support. Could either of those factor in here?

I don't quite know how you're running OpenRewrite, or how you've configured for instance the build plugins. That might factor in here. We usually ask folks to fill out this short form whenever they report issues, such that we have some of the context in which a problem occurs. Would you mind sharing those details such that we can better judge what might factor in here?

I'm not sure in your case which recipe (out of the hundreds involved with a Spring Boot 3 migration) lead to calling RemoveImport in your debugging session. That would already help judge if we need to add a precondition somewhere to not remove imports in the face of missing types.

We can't blindly not remove imports when there are missing types, as there are frequent cases where we would still like to make changes despite missing type attribution.

You're also welcome to join our Slack if you'd rather discuss the details in a mostly synchronous chat.

[amirmv2006]

Hi @timtebeek, thanks for the prompt response!

Usually that means that for instance a classpath entry containing the referenced class is not present, or the method would otherwise be generated by Lombok for instance, which we don't currently support. Could either of those factor in here?

I don't think so, the problem is with Junit 5's org.junit.jupiter.api.Assertions.assertNotNull. Weirdly enough, I also use assertFalse and assertTrue on the same class, but they are not touched (RemoveImport correctly finds out those are used and doesn't remove them) . Only the import of assertNotNull is removed.

I don't quite know how you're running OpenRewrite

I will edit the issue description to the format you've mentioned 🙏.

I'm not sure in your case which recipe (out of the hundreds involved with a Spring Boot 3 migration) lead to calling RemoveImport in your debugging session.

Here's my debugging path:
Upgrade Spring boot 3 -> Upgrade Spring boot 2.7 -> Upgrade Spring boot 2.6 -> Upgrade Spring boot 2.5 -> Upgrade Spring boot 2.4 -> SpringBoot2JUnit4to5Migration
-> RemoveObsoleteSpringRunners -> JUnit5BestPractices -> StaticImports -> UseStaticImport: methodPattern: "org.junit.jupiter.api.Assertions *(..)"

We can't blindly not remove imports when there are missing types

I'd also like to challenge the way RemoveImport is behaving. Is it a good idea for this visitor to remove imported static method calls from the passed type? In this case, new RemoveImport("org.junit.jupiter.api.Assertions") is trying to remove import static org.junit.jupiter.api.Assertions.assertNotNull;.

[amirmv2006]

Sorry, Lombok was actually the problem. I am using Lombok in the class under test, but was not using it in the test class of course. However, I think that was setting the AST parsing off. After "delomboking" the class under test, the recipe also worked out great 👍

[timtebeek]

Thanks for reporting back here! We're tracking support for Lombok in this issue, welcome to follow along there. And if there's anything else we can help with feel free to report here or join our Slack.