internal/diff: ToUnified may fail

LineEdits has similar consistency preconditions to ApplyEdits.
Previously they were assumed, and bad input would create bad
output or crashes; now it uses the same validation logic
as ApplyEdits. Since it reports an error, computation of a
unified diff can also fail if the edits are inconsistent.

The ToUnified([]Edit) function now returns an error. For
convenience we also provide a wrapper (Unified) that cannot
fail since it calls Strings and ToUnified consistently.

LineEdits itself is now private.

Change-Id: I3780827f501d7d5c9665ec8be5656331c0dcda8e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/440175
Reviewed-by: Robert Findley <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Alan Donovan <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>

go/analysis/analysistest/analysistest.go

@@ -198,9 +198,9 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns
 								t.Errorf("%s: error formatting edited source: %v\n%s", file.Name(), err, out)
 								continue
 							}
-							if want != string(formatted) {
-								edits := diff.Strings(want, string(formatted))
-								t.Errorf("suggested fixes failed for %s:\n%s", file.Name(), diff.Unified(fmt.Sprintf("%s.golden [%s]", file.Name(), sf), "actual", want, edits))
+							if got := string(formatted); got != want {
+								unified := diff.Unified(fmt.Sprintf("%s.golden [%s]", file.Name(), sf), "actual", want, got)
+								t.Errorf("suggested fixes failed for %s:\n%s", file.Name(), unified)
 							}
 							break
 						}
@@ -229,9 +229,9 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns
 					t.Errorf("%s: error formatting resulting source: %v\n%s", file.Name(), err, out)
 					continue
 				}
-				if want != string(formatted) {
-					edits := diff.Strings(want, string(formatted))
-					t.Errorf("suggested fixes failed for %s:\n%s", file.Name(), diff.Unified(file.Name()+".golden", "actual", want, edits))
+				if got := string(formatted); got != want {
+					unified := diff.Unified(file.Name()+".golden", "actual", want, got)
+					t.Errorf("suggested fixes failed for %s:\n%s", file.Name(), unified)
 				}
 			}
 		}

gopls/api-diff/api_diff.go

@@ -256,12 +256,10 @@ func formatBlock(str string) string {
 }
 
 func diffStr(before, after string) string {
-	// Add newlines to avoid newline messages in diff.
 	if before == after {
 		return ""
 	}
-	before += "\n"
-	after += "\n"
-	edits := diffpkg.Strings(before, after)
-	return fmt.Sprintf("%q", diffpkg.Unified("previous", "current", before, edits))
+	// Add newlines to avoid newline messages in diff.
+	unified := diffpkg.Unified("previous", "current", before+"\n", after+"\n")
+	return fmt.Sprintf("%q", unified)
 }

gopls/internal/lsp/cache/parse.go

@@ -157,8 +157,7 @@ func parseGoImpl(ctx context.Context, fset *token.FileSet, fh source.FileHandle,
 			// it is likely we got stuck in a loop somehow. Log out a diff
 			// of the last changes we made to aid in debugging.
 			if i == 9 {
-				edits := diff.Bytes(src, newSrc)
-				unified := diff.Unified("before", "after", string(src), edits)
+				unified := diff.Unified("before", "after", string(src), string(newSrc))
 				event.Log(ctx, fmt.Sprintf("fixSrc loop - last diff:\n%v", unified), tag.File.Of(tok.Name()))
 			}
 

gopls/internal/lsp/cmd/format.go

@@ -95,8 +95,11 @@ func (c *format) Run(ctx context.Context, args ...string) error {
 		}
 		if c.Diff {
 			printIt = false
-			u := diff.Unified(filename+".orig", filename, string(file.mapper.Content), sedits)
-			fmt.Print(u)
+			unified, err := diff.ToUnified(filename+".orig", filename, string(file.mapper.Content), sedits)
+			if err != nil {
+				return err
+			}
+			fmt.Print(unified)
 		}
 		if printIt {
 			fmt.Print(formatted)

gopls/internal/lsp/cmd/imports.go

@@ -92,8 +92,11 @@ func (t *imports) Run(ctx context.Context, args ...string) error {
 			ioutil.WriteFile(filename, []byte(newContent), 0644)
 		}
 	case t.Diff:
-		diffs := diff.Unified(filename+".orig", filename, string(file.mapper.Content), sedits)
-		fmt.Print(diffs)
+		unified, err := diff.ToUnified(filename+".orig", filename, string(file.mapper.Content), sedits)
+		if err != nil {
+			return err
+		}
+		fmt.Print(unified)
 	default:
 		fmt.Print(string(newContent))
 	}

gopls/internal/lsp/cmd/rename.go

@@ -110,8 +110,11 @@ func (r *rename) Run(ctx context.Context, args ...string) error {
 			}
 			ioutil.WriteFile(filename, []byte(newContent), 0644)
 		case r.Diff:
-			diffs := diff.Unified(filename+".orig", filename, string(cmdFile.mapper.Content), renameEdits)
-			fmt.Print(diffs)
+			unified, err := diff.ToUnified(filename+".orig", filename, string(cmdFile.mapper.Content), renameEdits)
+			if err != nil {
+				return err
+			}
+			fmt.Print(unified)
 		default:
 			if len(orderedURIs) > 1 {
 				fmt.Printf("%s:\n", filepath.Base(filename))

gopls/internal/lsp/cmd/suggested_fix.go

@@ -154,7 +154,10 @@ func (s *suggestedFix) Run(ctx context.Context, args ...string) error {
 			ioutil.WriteFile(filename, []byte(newContent), 0644)
 		}
 	case s.Diff:
-		diffs := diff.Unified(filename+".orig", filename, string(file.mapper.Content), sedits)
+		diffs, err := diff.ToUnified(filename+".orig", filename, string(file.mapper.Content), sedits)
+		if err != nil {
+			return err
+		}
 		fmt.Print(diffs)
 	default:
 		fmt.Print(string(newContent))

gopls/internal/lsp/cmd/test/imports.go

@@ -19,7 +19,7 @@ func (r *runner) Import(t *testing.T, spn span.Span) {
 		return []byte(got), nil
 	}))
 	if want != got {
-		edits := diff.Strings(want, got)
-		t.Errorf("imports failed for %s, expected:\n%s", filename, diff.Unified("want", "got", want, edits))
+		unified := diff.Unified("want", "got", want, got)
+		t.Errorf("imports failed for %s, expected:\n%s", filename, unified)
 	}
 }

gopls/internal/lsp/lsp_test.go

@@ -1223,11 +1223,7 @@ func (r *runner) SignatureHelp(t *testing.T, spn span.Span, want *protocol.Signa
 	if got == nil {
 		t.Fatalf("expected %v, got nil", want)
 	}
-	diff, err := tests.DiffSignatures(spn, want, got)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if diff != "" {
+	if diff := tests.DiffSignatures(spn, want, got); diff != "" {
 		t.Error(diff)
 	}
 }

gopls/internal/lsp/source/source_test.go

@@ -844,11 +844,7 @@ func (r *runner) SignatureHelp(t *testing.T, spn span.Span, want *protocol.Signa
 		Signatures:      []protocol.SignatureInformation{*gotSignature},
 		ActiveParameter: uint32(gotActiveParameter),
 	}
-	diff, err := tests.DiffSignatures(spn, want, got)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if diff != "" {
+	if diff := tests.DiffSignatures(spn, want, got); diff != "" {
 		t.Error(diff)
 	}
 }

gopls/internal/lsp/tests/compare/text.go

@@ -19,19 +19,15 @@ func Text(want, got string) string {
 	}
 
 	// Add newlines to avoid verbose newline messages ("No newline at end of file").
-	want += "\n"
-	got += "\n"
-
-	edits := diff.Strings(want, got)
-	diff := diff.Unified("want", "got", want, edits)
+	unified := diff.Unified("want", "got", want+"\n", got+"\n")
 
 	// Defensively assert that we get an actual diff, so that we guarantee the
 	// invariant that we return "" if and only if want == got.
 	//
 	// This is probably unnecessary, but convenient.
-	if diff == "" {
+	if unified == "" {
 		panic("empty diff for non-identical input")
 	}
 
-	return diff
+	return unified
 }

gopls/internal/lsp/tests/util.go

@@ -20,8 +20,8 @@ import (
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
 	"golang.org/x/tools/gopls/internal/lsp/source"
 	"golang.org/x/tools/gopls/internal/lsp/source/completion"
+	"golang.org/x/tools/gopls/internal/lsp/tests/compare"
 	"golang.org/x/tools/gopls/internal/span"
-	"golang.org/x/tools/internal/diff"
 )
 
 // DiffLinks takes the links we got and checks if they are located within the source or a Note.
@@ -213,35 +213,33 @@ func summarizeCodeLens(i int, uri span.URI, want, got []protocol.CodeLens, reaso
 	return msg.String()
 }
 
-func DiffSignatures(spn span.Span, want, got *protocol.SignatureHelp) (string, error) {
+func DiffSignatures(spn span.Span, want, got *protocol.SignatureHelp) string {
 	decorate := func(f string, args ...interface{}) string {
 		return fmt.Sprintf("invalid signature at %s: %s", spn, fmt.Sprintf(f, args...))
 	}
 	if len(got.Signatures) != 1 {
-		return decorate("wanted 1 signature, got %d", len(got.Signatures)), nil
+		return decorate("wanted 1 signature, got %d", len(got.Signatures))
 	}
 	if got.ActiveSignature != 0 {
-		return decorate("wanted active signature of 0, got %d", int(got.ActiveSignature)), nil
+		return decorate("wanted active signature of 0, got %d", int(got.ActiveSignature))
 	}
 	if want.ActiveParameter != got.ActiveParameter {
-		return decorate("wanted active parameter of %d, got %d", want.ActiveParameter, int(got.ActiveParameter)), nil
+		return decorate("wanted active parameter of %d, got %d", want.ActiveParameter, int(got.ActiveParameter))
 	}
 	g := got.Signatures[0]
 	w := want.Signatures[0]
-	if NormalizeAny(w.Label) != NormalizeAny(g.Label) {
-		wLabel := w.Label + "\n"
-		edits := diff.Strings(wLabel, g.Label+"\n")
-		return decorate("mismatched labels:\n%q", diff.Unified("want", "got", wLabel, edits)), nil
+	if diff := compare.Text(NormalizeAny(w.Label), NormalizeAny(g.Label)); diff != "" {
+		return decorate("mismatched labels:\n%s", diff)
 	}
 	var paramParts []string
 	for _, p := range g.Parameters {
 		paramParts = append(paramParts, p.Label)
 	}
 	paramsStr := strings.Join(paramParts, ", ")
 	if !strings.Contains(g.Label, paramsStr) {
-		return decorate("expected signature %q to contain params %q", g.Label, paramsStr), nil
+		return decorate("expected signature %q to contain params %q", g.Label, paramsStr)
 	}
-	return "", nil
+	return ""
 }
 
 // NormalizeAny replaces occurrences of interface{} in input with any.

internal/diff/diff.go

@@ -24,28 +24,14 @@ type Edit struct {
 // Apply returns an error if any edit is out of bounds,
 // or if any pair of edits is overlapping.
 func Apply(src string, edits []Edit) (string, error) {
-	if !sort.IsSorted(editsSort(edits)) {
-		edits = append([]Edit(nil), edits...)
-		sortEdits(edits)
-	}
-
-	// Check validity of edits and compute final size.
-	size := len(src)
-	lastEnd := 0
-	for _, edit := range edits {
-		if !(0 <= edit.Start && edit.Start <= edit.End && edit.End <= len(src)) {
-			return "", fmt.Errorf("diff has out-of-bounds edits")
-		}
-		if edit.Start < lastEnd {
-			return "", fmt.Errorf("diff has overlapping edits")
-		}
-		size += len(edit.New) + edit.Start - edit.End
-		lastEnd = edit.End
+	edits, size, err := validate(src, edits)
+	if err != nil {
+		return "", err
 	}
 
 	// Apply edits.
 	out := make([]byte, 0, size)
-	lastEnd = 0
+	lastEnd := 0
 	for _, edit := range edits {
 		if lastEnd < edit.Start {
 			out = append(out, src[lastEnd:edit.Start]...)
@@ -62,6 +48,32 @@ func Apply(src string, edits []Edit) (string, error) {
 	return string(out), nil
 }
 
+// validate checks that edits are consistent with src,
+// and returns the size of the patched output.
+// It may return a different slice.
+func validate(src string, edits []Edit) ([]Edit, int, error) {
+	if !sort.IsSorted(editsSort(edits)) {
+		edits = append([]Edit(nil), edits...)
+		sortEdits(edits)
+	}
+
+	// Check validity of edits and compute final size.
+	size := len(src)
+	lastEnd := 0
+	for _, edit := range edits {
+		if !(0 <= edit.Start && edit.Start <= edit.End && edit.End <= len(src)) {
+			return nil, 0, fmt.Errorf("diff has out-of-bounds edits")
+		}
+		if edit.Start < lastEnd {
+			return nil, 0, fmt.Errorf("diff has overlapping edits")
+		}
+		size += len(edit.New) + edit.Start - edit.End
+		lastEnd = edit.End
+	}
+
+	return edits, size, nil
+}
+
 // sortEdits orders edits by (start, end) offset.
 // This ordering puts insertions (end=start) before deletions
 // (end>start) at the same point, but uses a stable sort to preserve
@@ -84,8 +96,12 @@ func (a editsSort) Swap(i, j int) { a[i], a[j] = a[j], a[i] }
 
 // lineEdits expands and merges a sequence of edits so that each
 // resulting edit replaces one or more complete lines.
-func lineEdits(src string, edits []Edit) []Edit {
-	sortEdits(edits) // TODO(adonovan): is this necessary? Move burden to caller?
+// See ApplyEdits for preconditions.
+func lineEdits(src string, edits []Edit) ([]Edit, error) {
+	edits, _, err := validate(src, edits)
+	if err != nil {
+		return nil, err
+	}
 
 	// Do all edits begin and end at the start of a line?
 	// TODO(adonovan): opt: is this fast path necessary?
@@ -97,7 +113,7 @@ func lineEdits(src string, edits []Edit) []Edit {
 			goto expand
 		}
 	}
-	return edits // aligned
+	return edits, nil // aligned
 
 expand:
 	expanded := make([]Edit, 0, len(edits)) // a guess
@@ -116,7 +132,7 @@ expand:
 			prev = edit
 		}
 	}
-	return append(expanded, expandEdit(prev, src)) // flush final edit
+	return append(expanded, expandEdit(prev, src)), nil // flush final edit
 }
 
 // expandEdit returns edit expanded to complete whole lines.

internal/diff/diff_test.go

@@ -95,22 +95,32 @@ func TestLineEdits(t *testing.T) {
 			if edits == nil {
 				edits = tc.Edits
 			}
-			if got := diff.LineEdits(tc.In, tc.Edits); diffEdits(got, edits) {
+			got, err := diff.LineEdits(tc.In, tc.Edits)
+			if err != nil {
+				t.Fatalf("LineEdits: %v", err)
+			}
+			if !reflect.DeepEqual(got, edits) {
 				t.Errorf("LineEdits got %q, want %q", got, edits)
 			}
 		})
 	}
 }
 
-func TestUnified(t *testing.T) {
+func TestToUnified(t *testing.T) {
 	for _, tc := range difftest.TestCases {
 		t.Run(tc.Name, func(t *testing.T) {
-			unified := diff.Unified(difftest.FileA, difftest.FileB, tc.In, tc.Edits)
+			unified, err := diff.ToUnified(difftest.FileA, difftest.FileB, tc.In, tc.Edits)
+			if err != nil {
+				t.Fatal(err)
+			}
 			if unified != tc.Unified {
 				t.Errorf("Unified(Edits): got diff:\n%v\nexpected:\n%v", unified, tc.Unified)
 			}
 			if tc.LineEdits != nil {
-				unified := diff.Unified(difftest.FileA, difftest.FileB, tc.In, tc.LineEdits)
+				unified, err := diff.ToUnified(difftest.FileA, difftest.FileB, tc.In, tc.LineEdits)
+				if err != nil {
+					t.Fatal(err)
+				}
 				if unified != tc.Unified {
 					t.Errorf("Unified(LineEdits): got diff:\n%v\nexpected:\n%v", unified, tc.Unified)
 				}
@@ -154,10 +164,6 @@ func TestRegressionOld002(t *testing.T) {
 	}
 }
 
-func diffEdits(got, want []diff.Edit) bool {
-	return !reflect.DeepEqual(got, want)
-}
-
 // return a random string of length n made of characters from s
 func randstr(s string, n int) string {
 	src := []rune(s)

internal/diff/difftest/difftest.go

@@ -249,7 +249,10 @@ func DiffTest(t *testing.T, compute func(before, after string) []diff.Edit) {
 			if err != nil {
 				t.Fatalf("Apply failed: %v", err)
 			}
-			unified := diff.Unified(FileA, FileB, test.In, edits)
+			unified, err := diff.ToUnified(FileA, FileB, test.In, edits)
+			if err != nil {
+				t.Fatalf("ToUnified: %v", err)
+			}
 			if got != test.Out {
 				t.Errorf("Apply: got patched:\n%v\nfrom diff:\n%v\nexpected:\n%v", got, unified, test.Out)
 			}