Skip to content

RemoveUnusedImports creating ambiguous imports #5714

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 26, 2025
Merged

RemoveUnusedImports creating ambiguous imports #5714

merged 5 commits into from
Jul 26, 2025

Conversation

knutwannheden
Copy link
Contributor

@knutwannheden knutwannheden commented Jul 4, 2025

RemoveUnusedImports creating ambiguous imports

Problem

The RemoveUnusedImports recipe was incorrectly creating ambiguous imports by unfolding wildcard imports for
types that were only used in fully qualified form. For example, when java.util.Date was used as new java.util.Date(), the wildcard import import java.util.*; would be replaced with import java.util.Date;
instead of being removed entirely.

Root Cause

The original implementation lacked proper detection of qualified vs unqualified type references. During
wildcard import unfolding, the recipe would add imports for all types found in the typesInUse collection
without checking whether those types were actually used in unqualified form or only in fully qualified form.

Solution

Enhanced the RemoveUnusedImports recipe in
/rewrite-java/src/main/java/org/openrewrite/java/RemoveUnusedImports.java with:

1. Efficient Unqualified Reference Collection

  • Added collectUnqualifiedTypeNames() method: Performs a single AST traversal to collect all fully
    qualified names of types used in unqualified form
  • Performance optimization: Changed from O(n*m) complexity to O(m + n) by replacing multiple AST traversals
    with a single pass and fast set lookups
  • Smart qualified reference detection: Uses isPartOfQualifiedReference() helper to walk up the AST cursor
    and detect when an identifier is part of a J.FieldAccess chain

2. Improved Wildcard Unfolding Logic

  • Line 246-247: Modified wildcard unfolding to filter types using
    unqualifiedTypeNames.contains(fqType.getFullyQualifiedName())
  • Lines 258-262: Enhanced logic to completely remove wildcard imports when no types are used unqualified
  • Lines 282-284, 317: Added defensive checks to prevent IndexOutOfBoundsException

3. Edge Case Handling

  • Added test case: keepImportWhenBothQualifiedAndUnqualifiedUsageExists() verifies that imports are
    preserved when a type is used both qualified and unqualified in the same file
  • Mixed usage support: Correctly handles scenarios where Date and java.util.Date appear in the same
    file

Key Changes

RemoveUnusedImports.java:

  • Lines 87-88: Added upfront collection of unqualified type names
  • Lines 473-528: New collectUnqualifiedTypeNames() method with AST traversal logic
  • Lines 502-524: Helper method isPartOfQualifiedReference() for qualified reference detection
  • Line 246: Replaced inefficient per-type checking with fast set lookup

RemoveUnusedImportsTest.java:

  • Lines 2215-2230: Added edge case test for mixed qualified/unqualified usage

Verification

  • ✅ Original failing test onlyFullyQualifiedUsageShouldRemoveWildcard() now passes
  • ✅ New edge case test keepImportWhenBothQualifiedAndUnqualifiedUsageExists() passes
  • ✅ All 63 existing RemoveUnusedImports tests continue to pass with no regressions
  • ✅ Significant performance improvement for files with multiple wildcard imports

Test Cases Covered

  1. Fully qualified only: java.util.Date usage → wildcard import removed entirely
  2. Unqualified only: Date usage → wildcard import unfolded to specific import
  3. Mixed usage: Both Date and java.util.Date → import preserved due to unqualified reference
  4. Conflicting names: Multiple packages with same class name → correct import precedence maintained

## Problem
The `RemoveUnusedImports` recipe was incorrectly creating ambiguous imports by unfolding wildcard imports for
types that were only used in fully qualified form. For example, when `java.util.Date` was used as `new
  java.util.Date()`, the wildcard import `import java.util.*;` would be replaced with `import java.util.Date;`
instead of being removed entirely.

## Root Cause
The original implementation lacked proper detection of qualified vs unqualified type references. During
wildcard import unfolding, the recipe would add imports for all types found in the `typesInUse` collection
without checking whether those types were actually used in unqualified form or only in fully qualified form.

## Solution
Enhanced the `RemoveUnusedImports` recipe in
`/rewrite-java/src/main/java/org/openrewrite/java/RemoveUnusedImports.java` with:

### 1. Efficient Unqualified Reference Collection
- **Added `collectUnqualifiedTypeNames()` method**: Performs a single AST traversal to collect all fully
  qualified names of types used in unqualified form
- **Performance optimization**: Changed from O(n*m) complexity to O(m + n) by replacing multiple AST traversals
  with a single pass and fast set lookups
- **Smart qualified reference detection**: Uses `isPartOfQualifiedReference()` helper to walk up the AST cursor
  and detect when an identifier is part of a `J.FieldAccess` chain

### 2. Improved Wildcard Unfolding Logic
- **Line 246-247**: Modified wildcard unfolding to filter types using
  `unqualifiedTypeNames.contains(fqType.getFullyQualifiedName())`
- **Lines 258-262**: Enhanced logic to completely remove wildcard imports when no types are used unqualified
- **Lines 282-284, 317**: Added defensive checks to prevent IndexOutOfBoundsException

### 3. Edge Case Handling
- **Added test case**: `keepImportWhenBothQualifiedAndUnqualifiedUsageExists()` verifies that imports are
  preserved when a type is used both qualified and unqualified in the same file
- **Mixed usage support**: Correctly handles scenarios where `Date` and `java.util.Date` appear in the same
  file

## Key Changes

**RemoveUnusedImports.java:**
- Lines 87-88: Added upfront collection of unqualified type names
- Lines 473-528: New `collectUnqualifiedTypeNames()` method with AST traversal logic
- Lines 502-524: Helper method `isPartOfQualifiedReference()` for qualified reference detection
- Line 246: Replaced inefficient per-type checking with fast set lookup

**RemoveUnusedImportsTest.java:**
- Lines 2215-2230: Added edge case test for mixed qualified/unqualified usage

## Verification
- ✅ Original failing test `onlyFullyQualifiedUsageShouldRemoveWildcard()` now passes
- ✅ New edge case test `keepImportWhenBothQualifiedAndUnqualifiedUsageExists()` passes
- ✅ All 63 existing `RemoveUnusedImports` tests continue to pass with no regressions
- ✅ Significant performance improvement for files with multiple wildcard imports

## Test Cases Covered
1. **Fully qualified only**: `java.util.Date` usage → wildcard import removed entirely
2. **Unqualified only**: `Date` usage → wildcard import unfolded to specific import
3. **Mixed usage**: Both `Date` and `java.util.Date` → import preserved due to unqualified reference
4. **Conflicting names**: Multiple packages with same class name → correct import precedence maintained

Fixes:
- #5703
@github-project-automation github-project-automation bot moved this to In Progress in OpenRewrite Jul 4, 2025
@timtebeek timtebeek self-requested a review July 4, 2025 09:36
@timtebeek timtebeek added the bug Something isn't working label Jul 4, 2025
@timtebeek timtebeek marked this pull request as ready for review July 26, 2025 15:36
@timtebeek timtebeek moved this from In Progress to Ready to Review in OpenRewrite Jul 26, 2025
Copy link
Member

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see some of these longer standing issues resolved! I've applied the recipe here as well, and seems like we can now safely enable it going forward. Great!

@timtebeek timtebeek merged commit 2e5fe35 into main Jul 26, 2025
2 checks passed
@timtebeek timtebeek deleted the issue-5703 branch July 26, 2025 16:16
@github-project-automation github-project-automation bot moved this from Ready to Review to Done in OpenRewrite Jul 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

RemoveUnusedImports creating ambiguous imports when there's explicit imports accompanying wildcards
2 participants