Fix some issues with suggestions

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.
@@ -246,7 +247,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
 		}
 
@@ -911,7 +912,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: ")
@@ -933,12 +934,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 == '+' {
@@ -954,12 +972,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++
 				}
 			}
@@ -972,8 +990,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
+			}
+		}
+	}
+}