openrewrite / openrewrite/rewrite
`RemoveImport` must not remove potentially used static imports
Nobody has claimed this yet.
- Dominant language
- Java
- Stars
- 3.7k
- Forks
- 570
- Avg merge
- 13h 12m
- Merged PRs (30d)
- 261
Description
When using RemoveImport directly (or via maybeRemoveImport()) it can end up removing required static field or method imports, if the corresponding method invocations are not properly type attributed. This in turn can for example happen when the user code relies on annotation processors like Lombok.
So for example in the following code using Mockito, the static import for doReturn() would end up being removed when applying the CleanupMockitoImports recipe (from rewrite-testing-frameworks).
import org.mockito.Mockito;
import static org.mockito.Mockito.doReturn;
public class MyTest {
void m() {
doReturn(new Unknown("foo")).when(new Object()).hashCode();
}
}
The problem here is the new Unknown("foo") expression. Assuming the Unknown(String) is a constructor generated by an annotation processor (e.g. when using Lombok's @Value), this code would compile correctly using Maven or Gradle, but result in missing type attributions in OpenRewrite's LST. As a result the type attribution for the doReturn() method invocation would also be missing and when the recipe uses maybeRemoveImport() to check if org.mockito.Mockito can be removed it would also end up removing the org.mockito.Mockito.doReturn import, because RemoveImport has dedicated logic to also remove such "owned" static imports: https://github.com/openrewrite/rewrite/blob/f72dbe517002184dfcf4b7aa83476b9b973560fc/rewrite-java/src/main/java/org/openrewrite/java/RemoveImport.java#L139-L143
RemoveImport should however never remove an import unless this can be done in a completely safe way. So when there for example are potentially matching method invocations without type attribution, then a static import should not be removed.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in rewrite-java/src/main/java/org/openrewrite/java/RemoveImport.java, especially the logic that removes owned static imports, and trace calls from maybeRemoveImport(). Reproduce the Mockito example with an unattributed invocation such as one affected by an annotation-processor-generated constructor. Done means RemoveImport retains a potentially used static import when method attribution is missing, while safe removals still work.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100