Fix Iterator import removal bug for combined loops

[carstenartur]

Remove premature applyRemoves() call in WhileToForEach that was clearing ImportRemover state before all operations completed

What it does

When multiple cleanups process the same compilation unit, each creates its own ImportRemover instance. If cleanup A removes one Iterator reference and cleanup B removes another, neither ImportRemover sees the full picture, so the Iterator import persists.

Changes

CleanUpContextCore

• Added fSharedImportRemover field with getter/setter to hold shared instance

CompilationUnitRewrite

• Added setImportRemover() to accept external ImportRemover instead of creating new instance

CleanUpRefactoring.calculateChange()

• Creates single ImportRemover upfront, stores in context
• After each cleanup creates a fix, passes shared ImportRemover to CompilationUnitRewriteOperationsFixCore instances

CompilationUnitRewriteOperationsFixCore

• Added fSharedImportRemover field with setter
• Modified createChange() to use shared ImportRemover if available

Example

// Before: Iterator import not removed despite all references eliminated
for (Iterator<String> it = list.iterator(); it.hasNext();) { ... }  // cleanup A
Iterator<String> it2 = list2.iterator(); 
while (it2.hasNext()) { ... }  // cleanup B
// Result: import java.util.Iterator; still present

// After: Single ImportRemover tracks all removals
for (String s : list) { ... }  // cleanup A registers removal
for (String s : list2) { ... }  // cleanup B registers removal  
// Result: import java.util.Iterator; correctly removed

Test coverage: CleanUpTest1d8.testIssue121_CombinedForAndWhileLoopIteratorImportRemoval

Original prompt

Bug: Clean up "Convert to enhanced 'for' loops" does not detect unused imports for combined enhanced for- and while-loops

When applying the clean-up Convert to enhanced 'for' loops (without the option only if the loop variable is used), the unused java.util.Iterator import is not removed when the code contains both an enhanced for-loop and a while-loop that are converted.

Steps to Reproduce:

Apply the clean-up to the following code:

package test;

import java.util.Date;
import java.util.Iterator;
import java.util.List;
import java.util.Map;

public class Test {
    private void method(List<String> list, Map<Date, List<String>> map) {
        Iterator<String> removed = list.iterator();
        while (removed.hasNext()) {
            System.out.println(removed.next());
        }
        for (Iterator<List<String>> value = map.values().iterator(); value.hasNext();) {
            System.out.println(value.next());
        }
    }
}

Expected Result:

The java.util.Iterator import should be removed since it is no longer used after the conversion.

Actual Result:

The java.util.Iterator import remains in the code even though it is unused:

package test;

import java.util.Date;
import java.util.Iterator; // <-- Is now unused but NOT removed
import java.util.List;
import java.util.Map;

public class Test {
    private void method(List<String> list, Map<Date, List<String>> map) {
        for (String element : list) {
            System.out.println(element);
        }
        for (List<String> list2 : map.values()) {
            System.out.println(list2);
        }
    }
}

Additional Notes:

• If the initial code contains only one of the loops (either the for-loop or the while-loop), the unused import is removed correctly.
• The bug only occurs when both loop types are present and converted together.

Context:

This feature request is based on issue #121.

Original prompt

Problem

When multiple cleanups run on the same compilation unit (e.g., ConvertLoopCleanUpCore for for-loops and UseIteratorToForLoopCleanUpCore for while-loops), each creates its own CompilationUnitRewrite with its own ImportRemover. This means neither ImportRemover sees that ALL Iterator references are removed, so the import is not removed.

Solution: Share ImportRemover Across Cleanups

Modify the cleanup framework to share a single ImportRemover instance across all cleanups processing the same compilation unit.

Changes Required:

1. Modify CleanUpContext to hold a shared ImportRemover

File: org.eclipse.jdt.ui/ui/org/eclipse/jdt/ui/cleanup/CleanUpContext.java

Add a field and methods:

private ImportRemover fSharedImportRemover;

public void setSharedImportRemover(ImportRemover remover) {
    fSharedImportRemover = remover;
}

public ImportRemover getSharedImportRemover() {
    return fSharedImportRemover;
}

2. Modify CompilationUnitRewrite.getImportRemover() to accept external ImportRemover

File: org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/structure/CompilationUnitRewrite.java

Add a method to set an external ImportRemover:

public void setImportRemover(ImportRemover remover) {
    fImportRemover = remover;
}

3. Modify CleanUpRefactoring.calculateChange() to create and share ImportRemover

File: org.eclipse.jdt.ui/core extension/org/eclipse/jdt/internal/corext/fix/CleanUpRefactoring.java

In the calculateChange method, create an ImportRemover before the loop and set it on the context:

public static CleanUpChange calculateChange(CleanUpContext context, ICleanUp[] cleanUps, List<ICleanUp> undoneCleanUps, HashSet<ICleanUp> slowCleanUps) throws CoreException {
    if (cleanUps.length == 0)
        return null;

    // Create shared ImportRemover for all cleanups on this compilation unit
    CompilationUnit ast = context.getAST();
    if (ast != null) {
        IJavaProject project = context.getCompilationUnit().getJavaProject();
        ImportRemover sharedRemover = new ImportRemover(project, ast);
        context.setSharedImportRemover(sharedRemover);
    }
    
    // ... rest of existing code ...
}

4. Modify CompilationUnitRewriteOperationsFixCore.createChange() to use shared ImportRemover

File: org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/fix/CompilationUnitRewriteOperationsFixCore.java

The fix needs access to the shared ImportRemover. This could be done by:

• Passing the CleanUpContext to the fix, or
• Adding a way to set the ImportRemover on the fix before createChange() is called

One approach is to add a field and setter:

private ImportRemover fSharedImportRemover;

public void setSharedImportRemover(ImportRemover remover) {
    fSharedImportRemover = remover;
}

@Override
public CompilationUnitChange createChange(IProgressMonitor progressMonitor) throws CoreException {
    CompilationUnitRewrite cuRewrite = new CompilationUnitRewrite((ICompilationUnit)fCompilationUnit.getJavaElement(), fCompilationUnit);
    
    // Use shared ImportRemover if available
    if (fSharedImportRemover != null) {
        cuRewrite.setImportRemover(fSharedImportRemover);
    }
    
    // ... rest of existing code ...
}

5. Wire it up in CleanUpRefactoring.calculateChange()

After creating the fix, set the shared ImportRemover on it:

ICleanUpFix fix = cleanUp.createFix(context);
if (fix != null) {
    // Pass shared ImportRemover to fix if it supports it
    if (fix instanceof CompilationUnitRewriteOperationsFixCore && context.getSharedImportRemover() != null) {
        ((CompilationUnitRewriteOperationsFixCore) fix).setSharedImportRemover(context.getSharedImportRemover());
    }
    CompilationUnitChange current = fix.createChange(null);
    // ...
}

Test Case

The existing test in CleanUpTest1d8.java (testIssue121_CombinedForAndWhileLoopIteratorImportRemoval) should pass after these changes, with the Iterator import being correctly removed when both for-loops and while-loops are converted.

Summary

This architectural change allows any combination of cleanups to share import removal state, making the fix general-purpose rather than specific to the loop conversion case.

How to test

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)