openrewrite / openrewrite/rewrite

`RemoveImport` must not remove potentially used static imports

Open
#3,111 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.