Skip to content

Commit 008e477

Browse files
stamblerrestamblerre
committed
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>
1 parent 48e5bd1 commit 008e477

23 files changed

+126
-65
lines changed

‎go/analysis/analysistest/analysistest.go

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,10 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns
195195
continue
196196
}
197197
if want != string(formatted) {
198-
d := myers.ComputeEdits("", want, string(formatted))
198+
d, err := myers.ComputeEdits("", want, string(formatted))
199+
if err != nil {
200+
t.Errorf("failed to compute suggested fixes: %v", err)
201+
}
199202
t.Errorf("suggested fixes failed for %s:\n%s", file.Name(), diff.ToUnified(fmt.Sprintf("%s.golden [%s]", file.Name(), sf), "actual", want, d))
200203
}
201204
break
@@ -221,7 +224,10 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns
221224
continue
222225
}
223226
if want != string(formatted) {
224-
d := myers.ComputeEdits("", want, string(formatted))
227+
d, err := myers.ComputeEdits("", want, string(formatted))
228+
if err != nil {
229+
t.Errorf("failed to compute edits: %s", err)
230+
}
225231
t.Errorf("suggested fixes failed for %s:\n%s", file.Name(), diff.ToUnified(file.Name()+".golden", "actual", want, d))
226232
}
227233
}

‎gopls/internal/hooks/diff.go

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,25 @@
55
package hooks
66

77
import (
8+
"fmt"
9+
810
"github.com/sergi/go-diff/diffmatchpatch"
911
"golang.org/x/tools/internal/lsp/diff"
1012
"golang.org/x/tools/internal/span"
1113
)
1214

13-
func ComputeEdits(uri span.URI, before, after string) []diff.TextEdit {
15+
func ComputeEdits(uri span.URI, before, after string) (edits []diff.TextEdit, err error) {
16+
// The go-diff library has an unresolved panic (see golang/go#278774).
17+
// TOOD(rstambler): Remove the recover once the issue has been fixed
18+
// upstream.
19+
defer func() {
20+
if r := recover(); r != nil {
21+
edits = nil
22+
err = fmt.Errorf("unable to compute edits for %s: %s", uri.Filename(), r)
23+
}
24+
}()
1425
diffs := diffmatchpatch.New().DiffMain(before, after, true)
15-
edits := make([]diff.TextEdit, 0, len(diffs))
26+
edits = make([]diff.TextEdit, 0, len(diffs))
1627
offset := 0
1728
for _, d := range diffs {
1829
start := span.NewPoint(0, 0, offset)
@@ -26,5 +37,5 @@ func ComputeEdits(uri span.URI, before, after string) []diff.TextEdit {
2637
edits = append(edits, diff.TextEdit{Span: span.New(uri, start, span.Point{}), NewText: d.Text})
2738
}
2839
}
29-
return edits
40+
return edits, nil
3041
}

‎gopls/internal/regtest/codelens_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ go 1.12
146146
require golang.org/x/hello v1.3.3
147147
`
148148
if got != wantGoMod {
149-
t.Fatalf("go.mod upgrade failed:\n%s", tests.Diff(wantGoMod, got))
149+
t.Fatalf("go.mod upgrade failed:\n%s", tests.Diff(t, wantGoMod, got))
150150
}
151151
})
152152
})
@@ -208,7 +208,7 @@ go 1.14
208208
require golang.org/x/hello v1.0.0
209209
`
210210
if got != wantGoMod {
211-
t.Fatalf("go.mod tidy failed:\n%s", tests.Diff(wantGoMod, got))
211+
t.Fatalf("go.mod tidy failed:\n%s", tests.Diff(t, wantGoMod, got))
212212
}
213213
}, ProxyFiles(proxy))
214214
}

‎gopls/internal/regtest/definition_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ func main() {
128128
}
129129
want := "```go\nfunc (error).Error() string\n```"
130130
if content.Value != want {
131-
t.Fatalf("hover failed:\n%s", tests.Diff(want, content.Value))
131+
t.Fatalf("hover failed:\n%s", tests.Diff(t, want, content.Value))
132132
}
133133
})
134134
}

‎gopls/internal/regtest/diagnostics_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -531,7 +531,7 @@ func _() {
531531
env.SaveBuffer("main.go")
532532
fixed := env.ReadWorkspaceFile("main.go")
533533
if original != fixed {
534-
t.Fatalf("generated file was changed by quick fixes:\n%s", tests.Diff(original, fixed))
534+
t.Fatalf("generated file was changed by quick fixes:\n%s", tests.Diff(t, original, fixed))
535535
}
536536
})
537537
}

‎gopls/internal/regtest/fix_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ func Foo() {
5858
}
5959
`
6060
if got := env.Editor.BufferText("main.go"); got != want {
61-
t.Fatalf("TestFillStruct failed:\n%s", tests.Diff(want, got))
61+
t.Fatalf("TestFillStruct failed:\n%s", tests.Diff(t, want, got))
6262
}
6363
})
6464
}

‎gopls/internal/regtest/formatting_test.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ func TestFormatting(t *testing.T) {
3232
got := env.Editor.BufferText("main.go")
3333
want := env.ReadWorkspaceFile("main.go.golden")
3434
if got != want {
35-
t.Errorf("unexpected formatting result:\n%s", tests.Diff(want, got))
35+
t.Errorf("unexpected formatting result:\n%s", tests.Diff(t, want, got))
3636
}
3737
})
3838
}
@@ -54,7 +54,7 @@ func f() {}
5454
got := env.Editor.BufferText("a.go")
5555
want := env.ReadWorkspaceFile("a.go.formatted")
5656
if got != want {
57-
t.Errorf("unexpected formatting result:\n%s", tests.Diff(want, got))
57+
t.Errorf("unexpected formatting result:\n%s", tests.Diff(t, want, got))
5858
}
5959
})
6060
}
@@ -78,7 +78,7 @@ func f() { fmt.Println() }
7878
got := env.Editor.BufferText("a.go")
7979
want := env.ReadWorkspaceFile("a.go.imported")
8080
if got != want {
81-
t.Errorf("unexpected formatting result:\n%s", tests.Diff(want, got))
81+
t.Errorf("unexpected formatting result:\n%s", tests.Diff(t, want, got))
8282
}
8383
})
8484
}
@@ -99,7 +99,7 @@ func f() {}
9999
got := env.Editor.BufferText("a.go")
100100
want := env.ReadWorkspaceFile("a.go.imported")
101101
if got != want {
102-
t.Errorf("unexpected formatting result:\n%s", tests.Diff(want, got))
102+
t.Errorf("unexpected formatting result:\n%s", tests.Diff(t, want, got))
103103
}
104104
})
105105
}
@@ -145,7 +145,7 @@ func TestOrganizeImports(t *testing.T) {
145145
got := env.Editor.BufferText("main.go")
146146
want := env.ReadWorkspaceFile("main.go.organized")
147147
if got != want {
148-
t.Errorf("unexpected formatting result:\n%s", tests.Diff(want, got))
148+
t.Errorf("unexpected formatting result:\n%s", tests.Diff(t, want, got))
149149
}
150150
})
151151
}
@@ -157,7 +157,7 @@ func TestFormattingOnSave(t *testing.T) {
157157
got := env.Editor.BufferText("main.go")
158158
want := env.ReadWorkspaceFile("main.go.formatted")
159159
if got != want {
160-
t.Errorf("unexpected formatting result:\n%s", tests.Diff(want, got))
160+
t.Errorf("unexpected formatting result:\n%s", tests.Diff(t, want, got))
161161
}
162162
})
163163
}
@@ -229,7 +229,7 @@ type Tree struct {
229229
got := env.Editor.BufferText("main.go")
230230
got = strings.ReplaceAll(got, "\r\n", "\n") // convert everything to LF for simplicity
231231
if tt.want != got {
232-
t.Errorf("unexpected content after save:\n%s", tests.Diff(tt.want, got))
232+
t.Errorf("unexpected content after save:\n%s", tests.Diff(t, tt.want, got))
233233
}
234234
})
235235
})

‎gopls/internal/regtest/modfile_test.go

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ func main() {
6868
env.DiagnosticAtRegexp("main.go", "\"example.com/blah\""),
6969
)
7070
if got := env.ReadWorkspaceFile("go.mod"); got != goModContent {
71-
t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(goModContent, got))
71+
t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(t, goModContent, got))
7272
}
7373
// Save the buffer, which will format and organize imports.
7474
// Confirm that the go.mod file still does not change.
@@ -77,7 +77,7 @@ func main() {
7777
env.DiagnosticAtRegexp("main.go", "\"example.com/blah\""),
7878
)
7979
if got := env.ReadWorkspaceFile("go.mod"); got != goModContent {
80-
t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(goModContent, got))
80+
t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(t, goModContent, got))
8181
}
8282
})
8383
})
@@ -104,7 +104,7 @@ func main() {
104104
env.DiagnosticAtRegexp("main.go", "\"example.com/blah\""),
105105
)
106106
if got := env.ReadWorkspaceFile("go.mod"); got != goModContent {
107-
t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(goModContent, got))
107+
t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(t, goModContent, got))
108108
}
109109
})
110110
})
@@ -153,7 +153,7 @@ require example.com v1.2.3
153153
}
154154
env.ApplyQuickFixes("main.go", []protocol.Diagnostic{goGetDiag})
155155
if got := env.ReadWorkspaceFile("go.mod"); got != want {
156-
t.Fatalf("unexpected go.mod content:\n%s", tests.Diff(want, got))
156+
t.Fatalf("unexpected go.mod content:\n%s", tests.Diff(t, want, got))
157157
}
158158
})
159159
}
@@ -200,7 +200,7 @@ require random.org v1.2.3
200200
}
201201
env.ApplyQuickFixes("main.go", []protocol.Diagnostic{randomDiag})
202202
if got := env.ReadWorkspaceFile("go.mod"); got != want {
203-
t.Fatalf("unexpected go.mod content:\n%s", tests.Diff(want, got))
203+
t.Fatalf("unexpected go.mod content:\n%s", tests.Diff(t, want, got))
204204
}
205205
})
206206
}
@@ -243,7 +243,7 @@ require example.com v1.2.3
243243
)
244244
env.ApplyQuickFixes("go.mod", d.Diagnostics)
245245
if got := env.Editor.BufferText("go.mod"); got != want {
246-
t.Fatalf("unexpected go.mod content:\n%s", tests.Diff(want, got))
246+
t.Fatalf("unexpected go.mod content:\n%s", tests.Diff(t, want, got))
247247
}
248248
})
249249
}
@@ -285,7 +285,7 @@ go 1.14
285285
)
286286
env.ApplyQuickFixes("go.mod", d.Diagnostics)
287287
if got := env.ReadWorkspaceFile("go.mod"); got != want {
288-
t.Fatalf("unexpected go.mod content:\n%s", tests.Diff(want, got))
288+
t.Fatalf("unexpected go.mod content:\n%s", tests.Diff(t, want, got))
289289
}
290290
})
291291
}
@@ -353,7 +353,7 @@ require (
353353
)
354354
`
355355
if got := env.ReadWorkspaceFile("go.mod"); got != want {
356-
t.Fatalf("TestNewDepWithUnusedDep failed:\n%s", tests.Diff(want, got))
356+
t.Fatalf("TestNewDepWithUnusedDep failed:\n%s", tests.Diff(t, want, got))
357357
}
358358
})
359359
}
@@ -456,7 +456,7 @@ require (
456456
`
457457
env.Await(EmptyDiagnostics("go.mod"))
458458
if got := env.Editor.BufferText("go.mod"); got != want {
459-
t.Fatalf("suggested fixes failed:\n%s", tests.Diff(want, got))
459+
t.Fatalf("suggested fixes failed:\n%s", tests.Diff(t, want, got))
460460
}
461461
})
462462
}
@@ -630,7 +630,7 @@ func main() {
630630
)
631631
got := env.ReadWorkspaceFile("go.mod")
632632
if got != original {
633-
t.Fatalf("go.mod file modified:\n%s", tests.Diff(original, got))
633+
t.Fatalf("go.mod file modified:\n%s", tests.Diff(t, original, got))
634634
}
635635
env.RunGoCommand("get", "example.com/blah@v1.2.3")
636636
env.RunGoCommand("mod", "tidy")

‎internal/lsp/cache/mod_tidy.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -511,7 +511,10 @@ func switchDirectness(req *modfile.Require, m *protocol.ColumnMapper, computeEdi
511511
return nil, err
512512
}
513513
// Calculate the edits to be made due to the change.
514-
diff := computeEdits(m.URI, string(m.Content), string(newContent))
514+
diff, err := computeEdits(m.URI, string(m.Content), string(newContent))
515+
if err != nil {
516+
return nil, err
517+
}
515518
return source.ToProtocolEdits(m, diff)
516519
}
517520

‎internal/lsp/cmd/test/definition.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,10 @@ func (r *runner) Definition(t *testing.T, spn span.Span, d tests.Definition) {
5151
return []byte(got), nil
5252
})))
5353
if expect != "" && !strings.HasPrefix(got, expect) {
54-
d := myers.ComputeEdits("", expect, got)
54+
d, err := myers.ComputeEdits("", expect, got)
55+
if err != nil {
56+
t.Fatal(err)
57+
}
5558
t.Errorf("definition %v failed with %#v\n%s", tag, args, diff.ToUnified("expect", "got", expect, d))
5659
}
5760
}

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,10 @@ func (r *runner) Import(t *testing.T, spn span.Span) {
2020
return []byte(got), nil
2121
}))
2222
if want != got {
23-
d := myers.ComputeEdits(uri, want, got)
23+
d, err := myers.ComputeEdits(uri, want, got)
24+
if err != nil {
25+
t.Fatal(err)
26+
}
2427
t.Errorf("imports failed for %s, expected:\n%s", filename, diff.ToUnified("want", "got", want, d))
2528
}
2629
}

‎internal/lsp/cmd/test/suggested_fix.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,6 @@ func (r *runner) SuggestedFix(t *testing.T, spn span.Span, actionKinds []string,
3030
return []byte(got), nil
3131
}))
3232
if want != got {
33-
t.Errorf("suggested fixes failed for %s:\n%s", filename, tests.Diff(want, got))
33+
t.Errorf("suggested fixes failed for %s:\n%s", filename, tests.Diff(t, want, got))
3434
}
3535
}

‎internal/lsp/cmd/test/workspace_symbol.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,6 @@ func (r *runner) runWorkspaceSymbols(t *testing.T, uri span.URI, matcher, query
4848
}))
4949

5050
if expect != got {
51-
t.Errorf("workspace_symbol failed for %s:\n%s", query, tests.Diff(expect, got))
51+
t.Errorf("workspace_symbol failed for %s:\n%s", query, tests.Diff(t, expect, got))
5252
}
5353
}

‎internal/lsp/diff/diff.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ type TextEdit struct {
2121

2222
// ComputeEdits is the type for a function that produces a set of edits that
2323
// convert from the before content to the after content.
24-
type ComputeEdits func(uri span.URI, before, after string) []TextEdit
24+
type ComputeEdits func(uri span.URI, before, after string) ([]TextEdit, error)
2525

2626
// SortTextEdits attempts to order all edits by their starting points.
2727
// The sort is stable so that edits with the same starting point will not

‎internal/lsp/diff/difftest/difftest.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,10 @@ func DiffTest(t *testing.T, compute diff.ComputeEdits) {
222222
for _, test := range TestCases {
223223
t.Run(test.Name, func(t *testing.T) {
224224
t.Helper()
225-
edits := compute(span.URIFromPath("/"+test.Name), test.In, test.Out)
225+
edits, err := compute(span.URIFromPath("/"+test.Name), test.In, test.Out)
226+
if err != nil {
227+
t.Fatal(err)
228+
}
226229
got := diff.ApplyEdits(test.In, edits)
227230
unified := fmt.Sprint(diff.ToUnified(FileA, FileB, test.In, edits))
228231
if got != test.Out {

‎internal/lsp/diff/myers/diff.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import (
1616
// https://blog.jcoglan.com/2017/02/17/the-myers-diff-algorithm-part-3/
1717
// https://www.codeproject.com/Articles/42279/%2FArticles%2F42279%2FInvestigating-Myers-diff-algorithm-Part-1-of-2
1818

19-
func ComputeEdits(uri span.URI, before, after string) []diff.TextEdit {
19+
func ComputeEdits(uri span.URI, before, after string) ([]diff.TextEdit, error) {
2020
ops := operations(splitLines(before), splitLines(after))
2121
edits := make([]diff.TextEdit, 0, len(ops))
2222
for _, op := range ops {
@@ -32,7 +32,7 @@ func ComputeEdits(uri span.URI, before, after string) []diff.TextEdit {
3232
}
3333
}
3434
}
35-
return edits
35+
return edits, nil
3636
}
3737

3838
type operation struct {

0 commit comments

Comments
 (0)