x/tools/gopls: unified diff has a wrong new-file start line in hunks after the first #81453

Activity

1. gopherbot

   added

   ToolsThis label describes issues relating to any tools in the x/tools repository.This label describes issues relating to any tools in the x/tools repository.

   goplsIssues related to the Go language server, gopls.Issues related to the Go language server, gopls.

   on Sep 10, 2026
2. gopherbot

   added this to the Unreleased milestone on Sep 10, 2026
3. ajvengo commented on Sep 10, 2026

   

   ajvengo

   on Sep 10, 2026

   Author

   More actions

   Cause

   toUnified keeps toLine as the running new-file line counter, but the branch that joins a nearby edit into the current hunk adds the intervening lines to the hunk without advancing it (internal/diff/unified.go, current master):

   		case h != nil && start <= last+gap:
   			//within range of previous lines, add the joiners
   			addEqualLines(h, lines, last, start)   // <-- toLine not advanced
   		default:
   			...
   			toLine += start - last                 // <-- only this path advances it

   The default path advances toLine by start - last, and each inserted line advances it by one, so those two are accounted for. The joiners are not. Every joined edit therefore leaves toLine short by exactly the number of joiner lines, and the error is carried into the header of every subsequent hunk.

   That predicts the magnitudes in the report. In the small reproducer the two edits are joined across one blank line, so the second hunk is off by 1. In x/tools the edits in trie.go are at lines 71, 78 and 81, joined across 6 and 2 lines, so the next hunk is off by 8 — @@ -102,7 +94,7 @@.

   Minimal fix

   --- a/internal/diff/unified.go
   +++ b/internal/diff/unified.go
   @@ -134,7 +134,7 @@ func toUnified(fromName, toName string, content string, edits []Edit, contextLine
    			//direct extension
    		case h != nil && start <= last+gap:
    			//within range of previous lines, add the joiners
   -			addEqualLines(h, lines, last, start)
   +			toLine += addEqualLines(h, lines, last, start)
    		default:

   addEqualLines already returns the number of lines it appended, and it is already used that way for the leading edge of a hunk just below, so the fix needs nothing new.

   Verification

   With that one line changed:

   • the reproducer from the report prints @@ -13,4 +13,4 @@, matching GNU diff byte for byte;
   • the gopls format -d case prints @@ -17,6 +18,6 @@, again matching gofmt + GNU diff -u (note the released binary is wrong in the other direction here, +16);
   • the x/tools rename prints @@ -102,7 +102,7 @@ instead of +94;
   • go test ./internal/... is green, internal/diff, internal/diff/difftest and internal/diff/lcs included.

   All four checks were run against a gopls built from the patched tree, since gopls/go.mod replaces golang.org/x/tools with ...

   A test that would have caught it

   The existing tests compare against golden strings, which is why a wrong header survived. A property is cheap here: every hunk header must point at the real position of its own lines — the old lines at fromLine in the old file, the new lines at toLine in the new file. On 400 randomly generated line-edit pairs (delete, change, insert) that check fails 112 times before the fix and 0 times after.

   var hunkHeader = regexp.MustCompile(`^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@`)
   
   // checkHunkPositions asserts that every hunk header points at the real position
   // of its own lines: the old lines (context and deletions) at fromLine in the old
   // file, the new lines (context and insertions) at toLine in the new file.
   func checkHunkPositions(t *testing.T, old, new string) {
   	t.Helper()
   	u := diff.Unified("old", "new", old, new)
   	if u == "" {
   		return
   	}
   	oldLines, newLines := strings.Split(old, "\n"), strings.Split(new, "\n")
   	var fromLine, toLine int
   	var fromBlock, toBlock []string
   	at := func(lines []string, start int, block []string) bool {
   		return start-1+len(block) <= len(lines) &&
   			strings.Join(lines[start-1:start-1+len(block)], "\n") == strings.Join(block, "\n")
   	}
   	verify := func() {
   		if fromLine == 0 {
   			return
   		}
   		if !at(oldLines, fromLine, fromBlock) {
   			t.Errorf("hunk claims old line %d, the old file has something else there:\n%s", fromLine, u)
   		}
   		if !at(newLines, toLine, toBlock) {
   			t.Errorf("hunk claims new line %d, the new file has something else there:\n%s", toLine, u)
   		}
   	}
   	for _, l := range strings.Split(u, "\n") {
   		switch {
   		case strings.HasPrefix(l, "--- "), strings.HasPrefix(l, "+++ "):
   		case strings.HasPrefix(l, "@@"):
   			verify()
   			m := hunkHeader.FindStringSubmatch(l)
   			if m == nil {
   				t.Fatalf("unparsable hunk header %q", l)
   			}
   			fromLine, _ = strconv.Atoi(m[1])
   			toLine, _ = strconv.Atoi(m[2])
   			fromBlock, toBlock = nil, nil
   		case strings.HasPrefix(l, "-"):
   			fromBlock = append(fromBlock, l[1:])
   		case strings.HasPrefix(l, "+"):
   			toBlock = append(toBlock, l[1:])
   		case strings.HasPrefix(l, " "):
   			fromBlock = append(fromBlock, l[1:])
   			toBlock = append(toBlock, l[1:])
   		}
   	}
   	verify()
   }

   I am happy to send this as a CL (the one-line fix plus the golden-string test for the reproducer and the property test) if that is welcome.

   👍React with 👍1Reacted by Alan Donovan
4. gabyhelp commented on Sep 10, 2026

   

   gabyhelp

   on Sep 10, 2026

   More actions

   Related Issues

   • x/tools/gopls: bug in new diff (unified formatter) #57290 (closed)
   • cmd/fix: -diff produces interleaved -/+ lines instead of grouped hunks #77957 (closed)
   • x/tools/gopls: strange unified diff #60379 (closed)
   • x/tools/internal/diff: suboptimal line edits for a common prefix #64023 (closed)

   _{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}
5. pjweinb commented on Sep 10, 2026

   

   pjweinb

   on Sep 10, 2026

   More actions

   Sure. Fix it.
6. hyangah

   modified the milestones: Unreleased, gopls/backlog on Sep 10, 2026
7. hyangah

   assigned

   ajvengo

   on Sep 10, 2026
8. aputman

   added

   help wanted

   on Sep 17, 2026
9. shuaixr commented on Sep 18, 2026

   

   shuaixr

   on Sep 18, 2026

   More actions

   I'd happy to take a look at this if no one's working on it
10. gopherbot commented on Sep 18, 2026

    

    gopherbot

    on Sep 18, 2026

    Contributor

    More actions

    Change https://go.dev/cl/834825 mentions this issue: cmd/vendor/golang.org/x/tools: fix wrong new-file start line in unified diff