Fix code quality issues in DocumentDirtyTracker: mutable static, redundant I/O, test coverage

[Copilot]

Three code quality issues in the DocumentDirtyTracker feature: a mutable object shared as static final, redundant document buffer connections, and tests only exercising bulk replacement instead of incremental edits.

Changes

Removed shared mutable static field

CleanUpPostSaveListener had private static final NullProgressMonitor NULL_PROGRESS_MONITOR. Since NullProgressMonitor.setCanceled(true) mutates state, sharing one instance across all calls creates potential for leaked state. Replaced with new NullProgressMonitor() at each use site.

Consolidated duplicate getDocument() calls

The saved() method called getDocument(unit) twice:

• Once at line 340 to get dirty regions from tracker
• Again at line 444 to clear dirty lines

Each call does a connect/disconnect cycle. Now cache document and tracker references to eliminate the second I/O round-trip.

Added incremental edit tests

Existing tests only used document.set() which replaces entire document content. Added four tests using document.replace(offset, length, text) to exercise:

• Single line edits
• Line insertion
• Line deletion
• Multiple edits on non-consecutive lines

These test the actual DocumentDirtyTracker behavior during real editing scenarios where changes are incremental.

Original prompt

Code Quality Fixes for PR eclipse-jdt#2778

This PR fixes three code quality issues in the copilot/fix-string-index-out-of-bounds branch that were introduced by the DocumentDirtyTracker feature.

1. Mutable NullProgressMonitor as static final field

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

Problem (line 309):

private static final NullProgressMonitor NULL_PROGRESS_MONITOR = new NullProgressMonitor();

NullProgressMonitor is mutable — methods like setCanceled(true) can change its state. If shared as a static final field, a call to setCanceled() could affect all subsequent uses. The rest of the class already correctly creates new NullProgressMonitor() each time.

Fix: Remove the static final field and use new NullProgressMonitor() in each call site (lines 698 and 703 in getDocument()).

2. getDocument() connect/disconnect lifecycle issue

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

Problem (lines 692-705):

private IDocument getDocument(ICompilationUnit unit) throws CoreException {
    final ITextFileBufferManager manager= FileBuffers.getTextFileBufferManager();
    final IPath path= unit.getResource().getFullPath();
    ITextFileBuffer buffer= null;
    try {
        manager.connect(path, LocationKind.IFILE, NULL_PROGRESS_MONITOR);
        buffer= manager.getTextFileBuffer(path, LocationKind.IFILE);
        return buffer != null ? buffer.getDocument() : null;
    } finally {
        if (buffer != null)
            manager.disconnect(path, LocationKind.IFILE, NULL_PROGRESS_MONITOR);
    }
}

The document is returned but the buffer is immediately disconnected in the finally block. The document object stays in memory but is conceptually no longer connected to the buffer. Additionally, getDocument() is called twice in the saved() method (once at line 340 and once at line 444), each time doing a connect/disconnect cycle.

Fix: Refactor so that the method is more robust. The simplest approach: since this code runs during a save operation where the document is already open in an editor, the buffer is already connected by the editor infrastructure. We should still connect/disconnect properly but the pattern should be clearer. At minimum, consolidate the two getDocument() calls to avoid redundant connect/disconnect cycles.

3. Tests use document.set() instead of incremental edits

File: org.eclipse.jdt.text.tests/src/org/eclipse/jdt/text/tests/DocumentDirtyTrackerTest.java

Problem: The tests use document.set("...") which replaces the entire document content. This fires only a single documentChanged event with offset=0 and length=entire document. In reality, edits are done via document.replace(offset, length, text) which exercises the tracking very differently. The tests could pass even if the tracker doesn't work correctly for real incremental edits.

Fix: Add additional test methods (or modify existing ones) that use document.replace(offset, length, text) for incremental edits. Keep some of the document.set() tests as they test the bulk-replacement case, but add tests like:

@Test
public void testIncrementalSingleLineEdit() throws BadLocationException {
    document.set("line1\nline2\nline3\n");
    tracker.clearDirtyLines();
    
    // Incremental edit on line 0: replace "line1" with "modified1"
    document.replace(0, 5, "modified1");
    
    IRegion[] regions = tracker.getDirtyRegions();
    assertNotNull(regions, "Should have dirty regions after incremental edit");
    assertEquals(1, regions.length, "Should have 1 dirty region");
}

@Test
public void testIncrementalInsertNewLine() throws BadLocationException {
    document.set("line1\nline2\nline3\n");
    tracker.clearDirtyLines();
    
    // Insert a new line after "line1\n" (offset 6)
    document.replace(6, 0, "inserted\n");
    
    IRegion[] regions = tracker.getDirtyRegions();
    assertNotNull(regions, "Should have dirty regions after line insertion");
}

@Test
public void testIncrementalDeleteLine() throws BadLocationException {
    document.set("line1\nline2\nline3\n");
    tracker.clearDirtyLines();
    
    // Mark line 2 as dirty first
    tracker.markLinesDirty(2);
    
    // Delete "line2\n" (offset 6, length 6)
    document.replace(6, 6, "");
    
    IRegion[] regions = tracker.getDirtyRegions();
    assertNotNull(regions, "Should still have dirty regions after deletion");
}

@Test
public void testIncrementalMultipleEditsOnDifferentLines() throws BadLocationException {
    document.set("line1\nline2\nline3\nline4\n");
    tracker.clearDirtyLines();
    
    // Edit line 0
    document.replace(0, 5, "mod1");
    // Edit line 2 (offsets shifted because line 0 is now shorter)
    int line2Offset = document.getLineOffset(2);
    document.replace(line2Offset, 5, "mod3");
    ...

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

*This pull request was created from Copilot chat.*
>

<!-- START COPILOT CODING AGENT TIPS -->
---

✨ Let Copilot coding agent [set things up for you](https://github.com/carstenartur/eclipse.jdt.ui/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo.