internal/lsp, gopls: recover from go-diff panics

This CL handles the panic in the sergi/go-diff library which has not
yet been resolved. We add an error return to the ComputeEdits function
and return an error if there is a panic. I'm not sure if this is the
best approach, but it does seem better than allowing the server to
crash.

A concern would be that the user wouldn't know why their code wasn't
being formatted, but hopefully they might look through the logs and
notice the error message. At least, other features would continue
working. The best fix will definitely be the fix for the panic, but that
is not yet available.

Threading through the error return was not pretty, but I thought it was
probably worth doing since it could be needed in other situations.

Updates golang/go#42927

Change-Id: I7f0c05eb296ef9e93b4de8ef071301cdb9dce152
Reviewed-on: https://go-review.googlesource.com/c/tools/+/278775
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Trust: Rebecca Stambler <rstambler@golang.org>

go/analysis/analysistest/analysistest.go

@@ -195,7 +195,10 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns
 								continue
 							}
 							if want != string(formatted) {
-								d := myers.ComputeEdits("", want, string(formatted))
+								d, err := myers.ComputeEdits("", want, string(formatted))
+								if err != nil {
+									t.Errorf("failed to compute suggested fixes: %v", err)
+								}
 								t.Errorf("suggested fixes failed for %s:\n%s", file.Name(), diff.ToUnified(fmt.Sprintf("%s.golden [%s]", file.Name(), sf), "actual", want, d))
 							}
 							break
@@ -221,7 +224,10 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns
 					continue
 				}
 				if want != string(formatted) {
-					d := myers.ComputeEdits("", want, string(formatted))
+					d, err := myers.ComputeEdits("", want, string(formatted))
+					if err != nil {
+						t.Errorf("failed to compute edits: %s", err)
+					}
 					t.Errorf("suggested fixes failed for %s:\n%s", file.Name(), diff.ToUnified(file.Name()+".golden", "actual", want, d))
 				}
 			}

gopls/internal/hooks/diff.go

@@ -5,14 +5,25 @@
 package hooks
 
 import (
+	"fmt"
+
 	"github.com/sergi/go-diff/diffmatchpatch"
 	"golang.org/x/tools/internal/lsp/diff"
 	"golang.org/x/tools/internal/span"
 )
 
-func ComputeEdits(uri span.URI, before, after string) []diff.TextEdit {
+func ComputeEdits(uri span.URI, before, after string) (edits []diff.TextEdit, err error) {
+	// The go-diff library has an unresolved panic (see golang/go#278774).
+	// TOOD(rstambler): Remove the recover once the issue has been fixed
+	// upstream.
+	defer func() {
+		if r := recover(); r != nil {
+			edits = nil
+			err = fmt.Errorf("unable to compute edits for %s: %s", uri.Filename(), r)
+		}
+	}()
 	diffs := diffmatchpatch.New().DiffMain(before, after, true)
-	edits := make([]diff.TextEdit, 0, len(diffs))
+	edits = make([]diff.TextEdit, 0, len(diffs))
 	offset := 0
 	for _, d := range diffs {
 		start := span.NewPoint(0, 0, offset)
@@ -26,5 +37,5 @@ func ComputeEdits(uri span.URI, before, after string) []diff.TextEdit {
 			edits = append(edits, diff.TextEdit{Span: span.New(uri, start, span.Point{}), NewText: d.Text})
 		}
 	}
-	return edits
+	return edits, nil
 }

gopls/internal/regtest/codelens_test.go

@@ -146,7 +146,7 @@ go 1.12
 require golang.org/x/hello v1.3.3
 `
 				if got != wantGoMod {
-					t.Fatalf("go.mod upgrade failed:\n%s", tests.Diff(wantGoMod, got))
+					t.Fatalf("go.mod upgrade failed:\n%s", tests.Diff(t, wantGoMod, got))
 				}
 			})
 		})
@@ -208,7 +208,7 @@ go 1.14
 require golang.org/x/hello v1.0.0
 `
 		if got != wantGoMod {
-			t.Fatalf("go.mod tidy failed:\n%s", tests.Diff(wantGoMod, got))
+			t.Fatalf("go.mod tidy failed:\n%s", tests.Diff(t, wantGoMod, got))
 		}
 	}, ProxyFiles(proxy))
 }

gopls/internal/regtest/definition_test.go

@@ -128,7 +128,7 @@ func main() {
 		}
 		want := "```go\nfunc (error).Error() string\n```"
 		if content.Value != want {
-			t.Fatalf("hover failed:\n%s", tests.Diff(want, content.Value))
+			t.Fatalf("hover failed:\n%s", tests.Diff(t, want, content.Value))
 		}
 	})
 }

gopls/internal/regtest/diagnostics_test.go

@@ -531,7 +531,7 @@ func _() {
 		env.SaveBuffer("main.go")
 		fixed := env.ReadWorkspaceFile("main.go")
 		if original != fixed {
-			t.Fatalf("generated file was changed by quick fixes:\n%s", tests.Diff(original, fixed))
+			t.Fatalf("generated file was changed by quick fixes:\n%s", tests.Diff(t, original, fixed))
 		}
 	})
 }

gopls/internal/regtest/fix_test.go

@@ -58,7 +58,7 @@ func Foo() {
 }
 `
 		if got := env.Editor.BufferText("main.go"); got != want {
-			t.Fatalf("TestFillStruct failed:\n%s", tests.Diff(want, got))
+			t.Fatalf("TestFillStruct failed:\n%s", tests.Diff(t, want, got))
 		}
 	})
 }

gopls/internal/regtest/formatting_test.go

@@ -32,7 +32,7 @@ func TestFormatting(t *testing.T) {
 		got := env.Editor.BufferText("main.go")
 		want := env.ReadWorkspaceFile("main.go.golden")
 		if got != want {
-			t.Errorf("unexpected formatting result:\n%s", tests.Diff(want, got))
+			t.Errorf("unexpected formatting result:\n%s", tests.Diff(t, want, got))
 		}
 	})
 }
@@ -54,7 +54,7 @@ func f() {}
 		got := env.Editor.BufferText("a.go")
 		want := env.ReadWorkspaceFile("a.go.formatted")
 		if got != want {
-			t.Errorf("unexpected formatting result:\n%s", tests.Diff(want, got))
+			t.Errorf("unexpected formatting result:\n%s", tests.Diff(t, want, got))
 		}
 	})
 }
@@ -78,7 +78,7 @@ func f() { fmt.Println() }
 		got := env.Editor.BufferText("a.go")
 		want := env.ReadWorkspaceFile("a.go.imported")
 		if got != want {
-			t.Errorf("unexpected formatting result:\n%s", tests.Diff(want, got))
+			t.Errorf("unexpected formatting result:\n%s", tests.Diff(t, want, got))
 		}
 	})
 }
@@ -99,7 +99,7 @@ func f() {}
 		got := env.Editor.BufferText("a.go")
 		want := env.ReadWorkspaceFile("a.go.imported")
 		if got != want {
-			t.Errorf("unexpected formatting result:\n%s", tests.Diff(want, got))
+			t.Errorf("unexpected formatting result:\n%s", tests.Diff(t, want, got))
 		}
 	})
 }
@@ -145,7 +145,7 @@ func TestOrganizeImports(t *testing.T) {
 		got := env.Editor.BufferText("main.go")
 		want := env.ReadWorkspaceFile("main.go.organized")
 		if got != want {
-			t.Errorf("unexpected formatting result:\n%s", tests.Diff(want, got))
+			t.Errorf("unexpected formatting result:\n%s", tests.Diff(t, want, got))
 		}
 	})
 }
@@ -157,7 +157,7 @@ func TestFormattingOnSave(t *testing.T) {
 		got := env.Editor.BufferText("main.go")
 		want := env.ReadWorkspaceFile("main.go.formatted")
 		if got != want {
-			t.Errorf("unexpected formatting result:\n%s", tests.Diff(want, got))
+			t.Errorf("unexpected formatting result:\n%s", tests.Diff(t, want, got))
 		}
 	})
 }
@@ -229,7 +229,7 @@ type Tree struct {
 				got := env.Editor.BufferText("main.go")
 				got = strings.ReplaceAll(got, "\r\n", "\n") // convert everything to LF for simplicity
 				if tt.want != got {
-					t.Errorf("unexpected content after save:\n%s", tests.Diff(tt.want, got))
+					t.Errorf("unexpected content after save:\n%s", tests.Diff(t, tt.want, got))
 				}
 			})
 		})

gopls/internal/regtest/modfile_test.go

@@ -68,7 +68,7 @@ func main() {
 				env.DiagnosticAtRegexp("main.go", "\"example.com/blah\""),
 			)
 			if got := env.ReadWorkspaceFile("go.mod"); got != goModContent {
-				t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(goModContent, got))
+				t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(t, goModContent, got))
 			}
 			// Save the buffer, which will format and organize imports.
 			// Confirm that the go.mod file still does not change.
@@ -77,7 +77,7 @@ func main() {
 				env.DiagnosticAtRegexp("main.go", "\"example.com/blah\""),
 			)
 			if got := env.ReadWorkspaceFile("go.mod"); got != goModContent {
-				t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(goModContent, got))
+				t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(t, goModContent, got))
 			}
 		})
 	})
@@ -104,7 +104,7 @@ func main() {
 				env.DiagnosticAtRegexp("main.go", "\"example.com/blah\""),
 			)
 			if got := env.ReadWorkspaceFile("go.mod"); got != goModContent {
-				t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(goModContent, got))
+				t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(t, goModContent, got))
 			}
 		})
 	})
@@ -153,7 +153,7 @@ require example.com v1.2.3
 		}
 		env.ApplyQuickFixes("main.go", []protocol.Diagnostic{goGetDiag})
 		if got := env.ReadWorkspaceFile("go.mod"); got != want {
-			t.Fatalf("unexpected go.mod content:\n%s", tests.Diff(want, got))
+			t.Fatalf("unexpected go.mod content:\n%s", tests.Diff(t, want, got))
 		}
 	})
 }
@@ -200,7 +200,7 @@ require random.org v1.2.3
 		}
 		env.ApplyQuickFixes("main.go", []protocol.Diagnostic{randomDiag})
 		if got := env.ReadWorkspaceFile("go.mod"); got != want {
-			t.Fatalf("unexpected go.mod content:\n%s", tests.Diff(want, got))
+			t.Fatalf("unexpected go.mod content:\n%s", tests.Diff(t, want, got))
 		}
 	})
 }
@@ -243,7 +243,7 @@ require example.com v1.2.3
 		)
 		env.ApplyQuickFixes("go.mod", d.Diagnostics)
 		if got := env.Editor.BufferText("go.mod"); got != want {
-			t.Fatalf("unexpected go.mod content:\n%s", tests.Diff(want, got))
+			t.Fatalf("unexpected go.mod content:\n%s", tests.Diff(t, want, got))
 		}
 	})
 }
@@ -285,7 +285,7 @@ go 1.14
 		)
 		env.ApplyQuickFixes("go.mod", d.Diagnostics)
 		if got := env.ReadWorkspaceFile("go.mod"); got != want {
-			t.Fatalf("unexpected go.mod content:\n%s", tests.Diff(want, got))
+			t.Fatalf("unexpected go.mod content:\n%s", tests.Diff(t, want, got))
 		}
 	})
 }
@@ -353,7 +353,7 @@ require (
 )
 `
 		if got := env.ReadWorkspaceFile("go.mod"); got != want {
-			t.Fatalf("TestNewDepWithUnusedDep failed:\n%s", tests.Diff(want, got))
+			t.Fatalf("TestNewDepWithUnusedDep failed:\n%s", tests.Diff(t, want, got))
 		}
 	})
 }
@@ -456,7 +456,7 @@ require (
 `
 		env.Await(EmptyDiagnostics("go.mod"))
 		if got := env.Editor.BufferText("go.mod"); got != want {
-			t.Fatalf("suggested fixes failed:\n%s", tests.Diff(want, got))
+			t.Fatalf("suggested fixes failed:\n%s", tests.Diff(t, want, got))
 		}
 	})
 }
@@ -630,7 +630,7 @@ func main() {
 		)
 		got := env.ReadWorkspaceFile("go.mod")
 		if got != original {
-			t.Fatalf("go.mod file modified:\n%s", tests.Diff(original, got))
+			t.Fatalf("go.mod file modified:\n%s", tests.Diff(t, original, got))
 		}
 		env.RunGoCommand("get", "example.com/blah@v1.2.3")
 		env.RunGoCommand("mod", "tidy")

internal/lsp/cache/mod_tidy.go

@@ -511,7 +511,10 @@ func switchDirectness(req *modfile.Require, m *protocol.ColumnMapper, computeEdi
 		return nil, err
 	}
 	// Calculate the edits to be made due to the change.
-	diff := computeEdits(m.URI, string(m.Content), string(newContent))
+	diff, err := computeEdits(m.URI, string(m.Content), string(newContent))
+	if err != nil {
+		return nil, err
+	}
 	return source.ToProtocolEdits(m, diff)
 }
 

internal/lsp/cmd/test/definition.go

@@ -51,7 +51,10 @@ func (r *runner) Definition(t *testing.T, spn span.Span, d tests.Definition) {
 			return []byte(got), nil
 		})))
 		if expect != "" && !strings.HasPrefix(got, expect) {
-			d := myers.ComputeEdits("", expect, got)
+			d, err := myers.ComputeEdits("", expect, got)
+			if err != nil {
+				t.Fatal(err)
+			}
 			t.Errorf("definition %v failed with %#v\n%s", tag, args, diff.ToUnified("expect", "got", expect, d))
 		}
 	}

internal/lsp/cmd/test/imports.go

@@ -20,7 +20,10 @@ func (r *runner) Import(t *testing.T, spn span.Span) {
 		return []byte(got), nil
 	}))
 	if want != got {
-		d := myers.ComputeEdits(uri, want, got)
+		d, err := myers.ComputeEdits(uri, want, got)
+		if err != nil {
+			t.Fatal(err)
+		}
 		t.Errorf("imports failed for %s, expected:\n%s", filename, diff.ToUnified("want", "got", want, d))
 	}
 }

internal/lsp/cmd/test/suggested_fix.go

@@ -30,6 +30,6 @@ func (r *runner) SuggestedFix(t *testing.T, spn span.Span, actionKinds []string,
 		return []byte(got), nil
 	}))
 	if want != got {
-		t.Errorf("suggested fixes failed for %s:\n%s", filename, tests.Diff(want, got))
+		t.Errorf("suggested fixes failed for %s:\n%s", filename, tests.Diff(t, want, got))
 	}
 }

internal/lsp/cmd/test/workspace_symbol.go

@@ -48,6 +48,6 @@ func (r *runner) runWorkspaceSymbols(t *testing.T, uri span.URI, matcher, query
 	}))
 
 	if expect != got {
-		t.Errorf("workspace_symbol failed for %s:\n%s", query, tests.Diff(expect, got))
+		t.Errorf("workspace_symbol failed for %s:\n%s", query, tests.Diff(t, expect, got))
 	}
 }

internal/lsp/diff/diff.go

@@ -21,7 +21,7 @@ type TextEdit struct {
 
 // ComputeEdits is the type for a function that produces a set of edits that
 // convert from the before content to the after content.
-type ComputeEdits func(uri span.URI, before, after string) []TextEdit
+type ComputeEdits func(uri span.URI, before, after string) ([]TextEdit, error)
 
 // SortTextEdits attempts to order all edits by their starting points.
 // The sort is stable so that edits with the same starting point will not

internal/lsp/diff/difftest/difftest.go

@@ -222,7 +222,10 @@ func DiffTest(t *testing.T, compute diff.ComputeEdits) {
 	for _, test := range TestCases {
 		t.Run(test.Name, func(t *testing.T) {
 			t.Helper()
-			edits := compute(span.URIFromPath("/"+test.Name), test.In, test.Out)
+			edits, err := compute(span.URIFromPath("/"+test.Name), test.In, test.Out)
+			if err != nil {
+				t.Fatal(err)
+			}
 			got := diff.ApplyEdits(test.In, edits)
 			unified := fmt.Sprint(diff.ToUnified(FileA, FileB, test.In, edits))
 			if got != test.Out {

internal/lsp/diff/myers/diff.go

@@ -16,7 +16,7 @@ import (
 // https://blog.jcoglan.com/2017/02/17/the-myers-diff-algorithm-part-3/
 // https://www.codeproject.com/Articles/42279/%2FArticles%2F42279%2FInvestigating-Myers-diff-algorithm-Part-1-of-2
 
-func ComputeEdits(uri span.URI, before, after string) []diff.TextEdit {
+func ComputeEdits(uri span.URI, before, after string) ([]diff.TextEdit, error) {
 	ops := operations(splitLines(before), splitLines(after))
 	edits := make([]diff.TextEdit, 0, len(ops))
 	for _, op := range ops {
@@ -32,7 +32,7 @@ func ComputeEdits(uri span.URI, before, after string) []diff.TextEdit {
 			}
 		}
 	}
-	return edits
+	return edits, nil
 }
 
 type operation struct {