Fix some issues with suggestions (#437)

I discovered a few bugs in the suggestions rendering logic, which I've
fixed in this PR. I've also included a reproducer test.

1. There were cases where multiple edits in one suggestion could cause
the renderer to index out of bounds.
2. There were cases where, in multiline mode, extra newline would
appear, because newlines ended up at the start or end of a hunk
boundary.

experimental/report/diff.go

@@ -15,6 +15,7 @@
 package report
 
 import (
+	"slices"
 	"sort"
 	"strings"
 
@@ -56,40 +57,44 @@ func (h hunk) bold(ss *styleSheet) string {
 }
 
 // hunkDiff computes edit hunks for a diff.
-func hunkDiff(span Span, edits []Edit) []hunk {
+func hunkDiff(span Span, edits []Edit) (Span, []hunk) {
 	out := make([]hunk, 0, len(edits)*3+1)
 	var prev int
-	src, offsets := offsetsForDiffing(span, edits)
-	for i, edit := range edits {
-		start := offsets[i][0]
-		end := offsets[i][1]
-
+	span, edits = offsetsForDiffing(span, edits)
+	src := span.Text()
+	for _, edit := range edits {
 		out = append(out,
-			hunk{hunkUnchanged, src[prev:start]},
-			hunk{hunkDelete, src[start:end]},
+			hunk{hunkUnchanged, src[prev:edit.Start]},
+			hunk{hunkDelete, src[edit.Start:edit.End]},
 			hunk{hunkAdd, edit.Replace},
 		)
-		prev = end
+		prev = edit.End
 	}
-	return append(out, hunk{hunkUnchanged, src[prev:]})
+	return span, append(out, hunk{hunkUnchanged, src[prev:]})
 }
 
 // unifiedDiff computes whole-line hunks for this diff, for producing a unified
 // edit.
 //
 // Each slice will contain one or more lines that should be displayed together.
-func unifiedDiff(span Span, edits []Edit) []hunk {
+func unifiedDiff(span Span, edits []Edit) (Span, []hunk) {
 	// Sort the edits such that they are ordered by starting offset.
-	src, offsets := offsetsForDiffing(span, edits)
+	span, edits = offsetsForDiffing(span, edits)
+	src := span.Text()
 	sort.Slice(edits, func(i, j int) bool {
-		return offsets[i][0] < offsets[j][0]
+		return edits[i].Start < edits[j].End
 	})
 
 	// Partition offsets into overlapping lines. That is, this connects together
 	// all edit spans whose end and start are not separated by a newline.
-	prev := &offsets[0]
-	parts := slicesx.Partition(offsets, func(_, next *[2]int) bool {
-		if next == prev || !strings.Contains(src[prev[1]:next[0]], "\n") {
+	prev := &edits[0]
+	parts := slicesx.Partition(edits, func(_, next *Edit) bool {
+		if next == prev {
+			return false
+		}
+
+		chunk := src[prev.End:next.Start]
+		if !strings.Contains(chunk, "\n") {
 			return false
 		}
 
@@ -99,12 +104,12 @@ func unifiedDiff(span Span, edits []Edit) []hunk {
 
 	var out []hunk
 	var prevHunk int
-	parts(func(i int, offsets [][2]int) bool {
+	parts(func(_ int, edits []Edit) bool {
 		// First, figure out the start and end of the modified region.
-		start, end := offsets[0][0], offsets[0][1]
-		for _, offset := range offsets[1:] {
-			start = min(start, offset[0])
-			end = max(end, offset[1])
+		start, end := edits[0].Start, edits[0].End
+		for _, edit := range edits[1:] {
+			start = min(start, edit.Start)
+			end = max(end, edit.End)
 		}
 		// Then, snap the region to be newline delimited. This is the unedited
 		// lines.
@@ -114,10 +119,10 @@ func unifiedDiff(span Span, edits []Edit) []hunk {
 		// Now, apply the edits to original to produce the modified result.
 		var buf strings.Builder
 		prev := 0
-		for j, offset := range offsets {
-			buf.WriteString(original[prev:offset[0]])
-			buf.WriteString(edits[i+j].Replace)
-			prev = offset[1]
+		for _, edit := range edits {
+			buf.WriteString(original[prev : edit.Start-start])
+			buf.WriteString(edit.Replace)
+			prev = edit.End - start
 		}
 		buf.WriteString(original[prev:])
 
@@ -131,20 +136,31 @@ func unifiedDiff(span Span, edits []Edit) []hunk {
 		prevHunk = end
 		return true
 	})
-	return append(out, hunk{hunkUnchanged, src[prevHunk:]})
+	return span, append(out, hunk{hunkUnchanged, src[prevHunk:]})
 }
 
 // offsetsForDiffing pre-calculates information needed for diffing:
-// the line-snapped span, and the offsetsForDiffing of each edit as indices into
-// that span.
-func offsetsForDiffing(span Span, edits []Edit) (string, [][2]int) {
-	start, end := adjustLineOffsets(span.File.Text(), span.Start, span.End)
-	delta := span.Start - start
-
-	offsets := make([][2]int, len(edits))
-	for i, edit := range edits {
-		offsets[i] = [2]int{edit.Start + delta, edit.End + delta}
+// the line-snapped span, and edits which are adjusted to conform to that
+// span.
+func offsetsForDiffing(span Span, edits []Edit) (Span, []Edit) {
+	edits = slices.Clone(edits)
+	var start, end int
+	for i := range edits {
+		e := &edits[i]
+		e.Start += span.Start
+		e.End += span.Start
+		if i == 0 {
+			start, end = e.Start, e.End
+		} else {
+			start, end = min(e.Start, start), max(e.End, end)
+		}
+	}
+
+	start, end = adjustLineOffsets(span.File.Text(), start, end)
+	for i := range edits {
+		edits[i].Start -= start
+		edits[i].End -= start
 	}
 
-	return span.File.Text()[start:end], offsets
+	return span.File.Span(start, end), edits
 }

experimental/report/renderer.go

@@ -25,6 +25,7 @@ import (
 	"unicode"
 
 	"github.com/bufbuild/protocompile/internal/ext/slicesx"
+	"github.com/bufbuild/protocompile/internal/ext/stringsx"
 )
 
 // Renderer configures a diagnostic rendering operation.
@@ -253,7 +254,7 @@ func (r Renderer) diagnostic(report *Report, d Diagnostic) string {
 			if i > 0 {
 				out.WriteByte('\n')
 			}
-			suggestion(snippets[0], locations[i][0].Line, lineBarWidth, &ss, &out)
+			suggestion(snippets[0], lineBarWidth, &ss, &out)
 			return true
 		}
 
@@ -917,7 +918,7 @@ func renderSidebar(bars, lineno, slashAt int, ss *styleSheet, multis []*multilin
 }
 
 // suggestion renders a single suggestion window.
-func suggestion(snip snippet, startLine int, lineBarWidth int, ss *styleSheet, out *strings.Builder) {
+func suggestion(snip snippet, lineBarWidth int, ss *styleSheet, out *strings.Builder) {
 	out.WriteString(ss.nAccent)
 	padBy(out, lineBarWidth)
 	out.WriteString("help: ")
@@ -939,12 +940,29 @@ func suggestion(snip snippet, startLine int, lineBarWidth int, ss *styleSheet, o
 		strings.Contains(snip.Span.Text(), "\n")
 
 	if multiline {
-		aLine := startLine
-		bLine := startLine
-		for _, hunk := range unifiedDiff(snip.Span, snip.edits) {
+		span, hunks := unifiedDiff(snip.Span, snip.edits)
+		aLine := span.StartLoc().Line
+		bLine := aLine
+		for i, hunk := range hunks {
+			// Trim a single newline before and after hunk. This helps deal with
+			// cases where a newline gets duplicated across hunks of different
+			// type.
+			hunk.content, _ = strings.CutPrefix(hunk.content, "\n")
+			hunk.content, _ = strings.CutSuffix(hunk.content, "\n")
+
 			if hunk.content == "" {
 				continue
 			}
+
+			// Skip addition lines that only contain whitespace, if the previous
+			// hunk was a deletion. This helps avoid cases where a whole line
+			// was deleted and some indentation was left over.
+			if prev, _ := slicesx.Get(hunks, i-1); prev.kind == hunkDelete &&
+				hunk.kind == hunkAdd &&
+				stringsx.EveryFunc(hunk.content, unicode.IsSpace) {
+				continue
+			}
+
 			for _, line := range strings.Split(hunk.content, "\n") {
 				lineno := aLine
 				if hunk.kind == '+' {
@@ -960,12 +978,12 @@ func suggestion(snip snippet, startLine int, lineBarWidth int, ss *styleSheet, o
 				)
 
 				switch hunk.kind {
-				case ' ':
+				case hunkUnchanged:
 					aLine++
 					bLine++
-				case '-':
+				case hunkDelete:
 					aLine++
-				case '+':
+				case hunkAdd:
 					bLine++
 				}
 			}
@@ -978,8 +996,8 @@ func suggestion(snip snippet, startLine int, lineBarWidth int, ss *styleSheet, o
 		return
 	}
 
-	fmt.Fprintf(out, "\n%s%*d | ", ss.nAccent, lineBarWidth, startLine)
-	hunks := hunkDiff(snip.Span, snip.edits)
+	span, hunks := hunkDiff(snip.Span, snip.edits)
+	fmt.Fprintf(out, "\n%s%*d | ", ss.nAccent, lineBarWidth, span.StartLoc().Line)
 	var column int
 	for _, hunk := range hunks {
 		if hunk.content == "" {

experimental/report/testdata/suggestions.yaml

@@ -104,4 +104,26 @@ diagnostics:
             replace: "{\n    option "
           - start: 10
             end: 12
-            replace: ";\n  }"
+            replace: ";\n  }"
+
+  - message: 'delete some stuff'
+    level: LEVEL_ERROR
+    annotations:
+      - file: 0
+        start: 38
+        end: 153
+        edits:
+          - start: 0
+            end: 13
+          - start: 114
+            end: 115
+
+  - message: 'delete this method'
+    level: LEVEL_ERROR
+    annotations:
+      - file: 0
+        start: 38
+        end: 153
+        edits:
+          - start: 59
+            end: 113

experimental/report/testdata/suggestions.yaml.color.txt

@@ -46,5 +46,22 @@
 ⟨blu⟩ 9 | ⟨b.grn⟩+⟨grn⟩   }
 ⟨blu⟩   | ⟨reset⟩
 
-⟨b.red⟩encountered 2 errors and 1 warning⟨reset⟩
+⟨b.red⟩error: delete some stuff⟨reset⟩
+⟨blu⟩  --> foo.proto:5:1
+⟨blu⟩  help: 
+   | 
+⟨blu⟩ 5 | ⟨b.red⟩-⟨red⟩ service Foo {
+⟨blu⟩ 6 | ⟨reset⟩ ⟨reset⟩   rpc Get(GetRequest) returns GetResponse;
+⟨blu⟩ 7 | ⟨reset⟩ ⟨reset⟩   rpc Put(PutRequest) returns (PutResponse) [foo = bar];
+⟨blu⟩ 8 | ⟨b.red⟩-⟨red⟩ }
+⟨blu⟩   | ⟨reset⟩
+
+⟨b.red⟩error: delete this method⟨reset⟩
+⟨blu⟩  --> foo.proto:5:1
+⟨blu⟩  help: 
+   | 
+⟨blu⟩ 7 | ⟨b.red⟩-⟨red⟩   rpc Put(PutRequest) returns (PutResponse) [foo = bar];
+⟨blu⟩   | ⟨reset⟩
+
+⟨b.red⟩encountered 4 errors and 1 warning⟨reset⟩
 ⟨reset⟩

experimental/report/testdata/suggestions.yaml.fancy.txt

@@ -46,4 +46,21 @@ error: method options must go in a block
  9 | +   }
    | 
 
-encountered 2 errors and 1 warning
+error: delete some stuff
+  --> foo.proto:5:1
+  help: 
+   | 
+ 5 | - service Foo {
+ 6 |     rpc Get(GetRequest) returns GetResponse;
+ 7 |     rpc Put(PutRequest) returns (PutResponse) [foo = bar];
+ 8 | - }
+   | 
+
+error: delete this method
+  --> foo.proto:5:1
+  help: 
+   | 
+ 7 | -   rpc Put(PutRequest) returns (PutResponse) [foo = bar];
+   | 
+
+encountered 4 errors and 1 warning

experimental/report/testdata/suggestions.yaml.simple.txt

@@ -3,3 +3,5 @@ remark: foo.proto:1:10: let protocompile pick a syntax for you
 warning: foo.proto:5:9: services should have a `Service` suffix
 error: foo.proto:6:31: missing (...) around return type
 error: foo.proto:7:45: method options must go in a block
+error: foo.proto:5:1: delete some stuff
+error: foo.proto:5:1: delete this method

internal/ext/iterx/iterx.go

@@ -40,6 +40,17 @@ func First[T any](seq iter.Seq[T]) (v T, ok bool) {
 	return v, ok
 }
 
+// All returns whether every element of an iterator satisfies the given
+// predicate. Returns true if seq yields no values.
+func All[T any](seq iter.Seq[T], p func(T) bool) bool {
+	all := true
+	seq(func(v T) bool {
+		all = p(v)
+		return all
+	})
+	return all
+}
+
 // Map returns a new iterator applying f to each element of seq.
 func Map[T, U any](seq iter.Seq[T], f func(T) U) iter.Seq[U] {
 	return func(yield func(U) bool) {

internal/ext/stringsx/stringsx.go

@@ -0,0 +1,53 @@
+// Copyright 2020-2025 Buf Technologies, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// package stringsx contains extensions to Go's package strings.
+package stringsx
+
+import (
+	"github.com/bufbuild/protocompile/internal/ext/iterx"
+	"github.com/bufbuild/protocompile/internal/ext/unsafex"
+	"github.com/bufbuild/protocompile/internal/iter"
+)
+
+// EveryFunc verifies that all runes in the string satisfy the given predicate.
+func EveryFunc(s string, p func(rune) bool) bool {
+	return iterx.All(Runes(s), p)
+}
+
+// Runes returns an iterator over the runes in a string.
+//
+// Each non-UTF-8 byte in the string is yielded as a replacement character (U+FFFD).
+func Runes(s string) iter.Seq[rune] {
+	return func(yield func(r rune) bool) {
+		for _, r := range s {
+			if !yield(r) {
+				return
+			}
+		}
+	}
+}
+
+// Bytes returns an iterator over the bytes in a string.
+func Bytes(s string) iter.Seq[byte] {
+	return func(yield func(byte) bool) {
+		for i := 0; i < len(s); i++ {
+			// Avoid performing a bounds check each loop step.
+			b := *unsafex.Add(unsafex.StringData(s), i)
+			if !yield(b) {
+				return
+			}
+		}
+	}
+}