openrewrite / rewrite

Automated mass refactoring of source code.
https://docs.openrewrite.org
Apache License 2.0
2.22k stars 330 forks source link

`RemoveUnusedImports` removes used imports for annotated method parameters #4473

Open mvitz opened 2 months ago

mvitz commented 2 months ago

What version of OpenRewrite are you using?

I am using

How are you running OpenRewrite?

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

<plugin>
  <groupId>org.openrewrite.maven</groupId>
  <artifactId>rewrite-maven-plugin</artifactId>
  <version>5.39.2</version>
  <configuration>
    <exportDatatables>true</exportDatatables>
    <activeRecipes>
      <recipe>org.openrewrite.java.RemoveUnusedImports</recipe>
    </activeRecipes>
  </configuration>
</plugin>

The complete bug (configuration and code) is also shown within https://github.com/mvitz/openrewrite-bug-removeunusedimports

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

import java.beans.BeanProperty;
record A(@BeanProperty String a) {}

Although @BeanProperty may not make sense at that place, it was the first annotation within the JDK that I found to make the example as small as possible. A more realistic scenario would be Spring Boots @DefaultValue which is used within the reproducer repository.

What did you expect to see?

The import for the @BeanProperty annotation should not be removed.

What did you see instead?

The import for the @BeanProperty annotation is removed, and the code does not compile afterwards because the import is needed and should not be removed.

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

Example output can be seen in the reproducer repository (mentioned above).

Are you interested in contributing a fix to OpenRewrite?

Probably I would like to contribute, but if this is something someone with prior knowledge can fix within a few minutes, I would not inisist in fixing that myself.

timtebeek commented 2 months ago

Thanks for the clear example @mvitz ! I wonder if we register annotations on record parameters as TypesInUse. 🤔

Also: have you tried stripping away any elements not necessary to reproduce the issue? There's a lot of extra class annotations & methods that I hope are not needed to reproduce and fix the issue.

Once we've minimized the steps needed to reproduce it's then often easier to write a unit test , as seen here

https://github.com/openrewrite/rewrite/blob/350454123fc8818208aac5afb665c50e9a95a991/rewrite-java-test/src/test/java/org/openrewrite/java/RemoveUnusedImportsTest.java#L158-L169

mvitz commented 2 months ago

@timtebeek sorry for not stripping it any further.

The bug can be reproduced with:

import java.beans.BeanProperty;
record A(@BeanProperty String a) {}

Edit: I have updated the description accordingly and are working on a fix for that.