From 0e66cc6fab8bce4b5783ae1fc63b303910fb8b08 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Tue, 12 Feb 2019 13:51:16 -0500 Subject: [PATCH] internal/lsp/diff: fix bug that adds extra line to files on format Small changes to handle the last line in the diff library, LSP tests, and diff to text edits conversion. Fixes golang/go#30137 Change-Id: Iff0e53a04c2dabf6f54eb7c738b4c0837f16efba Reviewed-on: https://go-review.googlesource.com/c/162217 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Ian Cottrell --- internal/lsp/diff/diff.go | 64 +++---------------- internal/lsp/diff/diff_test.go | 39 ++++------- internal/lsp/lsp_test.go | 17 +++-- internal/lsp/source/format.go | 16 +++-- .../lsp/testdata/format/newline_format.go.in | 2 + 5 files changed, 47 insertions(+), 91 deletions(-) create mode 100644 internal/lsp/testdata/format/newline_format.go.in diff --git a/internal/lsp/diff/diff.go b/internal/lsp/diff/diff.go index dc6da2f0..573190b0 100644 --- a/internal/lsp/diff/diff.go +++ b/internal/lsp/diff/diff.go @@ -5,7 +5,9 @@ // Package diff implements the Myers diff algorithm. package diff -import "strings" +import ( + "strings" +) // Sources: // https://blog.jcoglan.com/2017/02/17/the-myers-diff-algorithm-part-3/ @@ -70,6 +72,8 @@ func Operations(a, b []string) []*Op { trace, offset := shortestEditSequence(a, b) snakes := backtrack(trace, len(a), len(b), offset) + M, N := len(a), len(b) + var i int solution := make([]*Op, len(a)+len(b)) @@ -101,7 +105,7 @@ func Operations(a, b []string) []*Op { } } x++ - if x == len(a) { + if x == M { break } } @@ -125,58 +129,7 @@ func Operations(a, b []string) []*Op { x++ y++ } - if x >= len(a) && y >= len(b) { - break - } - } - return solution[:i] -} - -// Lines returns a list of per-line operations to convert a into b. -func Lines(a, b []string) []*Op { - trace, offset := shortestEditSequence(a, b) - snakes := backtrack(trace, len(a), len(b), offset) - - var i int - solution := make([]*Op, len(a)+len(b)) - - x, y := 0, 0 - for _, snake := range snakes { - if len(snake) < 2 { - continue - } - // horizontal - for snake[0]-snake[1] > x-y { - solution[i] = &Op{ - Kind: Delete, - Content: a[x], - } - i++ - x++ - if x == len(a) { - break - } - } - // vertical - for snake[0]-snake[1] < x-y { - solution[i] = &Op{ - Kind: Insert, - Content: b[y], - } - i++ - y++ - } - // diagonal - for x < snake[0] { - solution[i] = &Op{ - Kind: Equal, - Content: a[x], - } - i++ - x++ - y++ - } - if x >= len(a) && y >= len(b) { + if x >= M && y >= N { break } } @@ -208,7 +161,6 @@ func backtrack(trace [][]int, x, y, offset int) [][]int { x = V[kPrev+offset] y = x - kPrev } - // this feels questionable if x < 0 || y < 0 { return snakes } @@ -254,7 +206,7 @@ func shortestEditSequence(a, b []string) ([][]int, int) { trace[d] = copyV // Return if we've exceeded the maximum values. - if x >= M-1 && y >= N-1 { + if x == M && y == N { return trace, offset } } diff --git a/internal/lsp/diff/diff_test.go b/internal/lsp/diff/diff_test.go index 1c533285..9a615e3b 100644 --- a/internal/lsp/diff/diff_test.go +++ b/internal/lsp/diff/diff_test.go @@ -18,17 +18,6 @@ func TestDiff(t *testing.T) { { a: []string{"A", "B", "C", "A", "B", "B", "A"}, b: []string{"C", "B", "A", "B", "A", "C"}, - lines: []*Op{ - &Op{Kind: Delete, Content: "A"}, - &Op{Kind: Delete, Content: "B"}, - &Op{Kind: Equal, Content: "C"}, - &Op{Kind: Insert, Content: "B"}, - &Op{Kind: Equal, Content: "A"}, - &Op{Kind: Equal, Content: "B"}, - &Op{Kind: Delete, Content: "B"}, - &Op{Kind: Equal, Content: "A"}, - &Op{Kind: Insert, Content: "C"}, - }, operations: []*Op{ &Op{Kind: Delete, I1: 0, I2: 1, J1: 0, J2: 0}, &Op{Kind: Delete, I1: 1, I2: 2, J1: 0, J2: 0}, @@ -37,27 +26,27 @@ func TestDiff(t *testing.T) { &Op{Kind: Insert, Content: "C", I1: 7, I2: 7, J1: 5, J2: 6}, }, }, + { + a: []string{"A", "B"}, + b: []string{"A", "C", ""}, + operations: []*Op{ + &Op{Kind: Delete, I1: 1, I2: 2, J1: 1, J2: 1}, + &Op{Kind: Insert, Content: "C", I1: 2, I2: 2, J1: 1, J2: 2}, + &Op{Kind: Insert, Content: "", I1: 2, I2: 2, J1: 2, J2: 3}, + }, + }, } { - for i, got := range Lines(tt.a, tt.b) { - want := tt.lines[i] - if !reflect.DeepEqual(want, got) { - t.Errorf("expected %v, got %v", want, got) - } + ops := Operations(tt.a, tt.b) + if len(ops) != len(tt.operations) { + t.Fatalf("expected %v operations, got %v", len(tt.operations), len(ops)) } - b := ApplyEdits(tt.a, tt.lines) - for i, want := range tt.b { - got := b[i] - if got != want { - t.Errorf("expected %v got %v", want, got) - } - } - for i, got := range Operations(tt.a, tt.b) { + for i, got := range ops { want := tt.operations[i] if !reflect.DeepEqual(want, got) { t.Errorf("expected %v, got %v", want, got) } } - b = ApplyEdits(tt.a, tt.operations) + b := ApplyEdits(tt.a, tt.operations) for i, want := range tt.b { got := b[i] if got != want { diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index c23a1293..5dc80a96 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -38,7 +38,7 @@ func testLSP(t *testing.T, exporter packagestest.Exporter) { // are being executed. If a test is added, this number must be changed. const expectedCompletionsCount = 63 const expectedDiagnosticsCount = 17 - const expectedFormatCount = 3 + const expectedFormatCount = 4 const expectedDefinitionsCount = 16 const expectedTypeDefinitionsCount = 2 @@ -380,25 +380,30 @@ func (f formats) test(t *testing.T, s *server) { } var ops []*diff.Op for _, edit := range edits { + start := int(edit.Range.Start.Line) + end := int(edit.Range.End.Line) + if start == end && edit.Range.End.Character > 1 { + end++ + } if edit.NewText == "" { // deletion ops = append(ops, &diff.Op{ Kind: diff.Delete, - I1: int(edit.Range.Start.Line), - I2: int(edit.Range.End.Line), + I1: start, + I2: end, }) } else if edit.Range.Start == edit.Range.End { // insertion ops = append(ops, &diff.Op{ Kind: diff.Insert, Content: edit.NewText, - I1: int(edit.Range.Start.Line), - I2: int(edit.Range.End.Line), + I1: start, + I2: end, }) } } split := strings.SplitAfter(string(original), "\n") got := strings.Join(diff.ApplyEdits(split, ops), "") if gofmted != got { - t.Errorf("format failed for %s: expected %v, got %v", filename, gofmted, got) + t.Errorf("format failed for %s: expected '%v', got '%v'", filename, gofmted, got) } } } diff --git a/internal/lsp/source/format.go b/internal/lsp/source/format.go index 5d1177ac..78aaab71 100644 --- a/internal/lsp/source/format.go +++ b/internal/lsp/source/format.go @@ -82,21 +82,29 @@ func computeTextEdits(rng Range, tok *token.File, unformatted, formatted string) u := strings.SplitAfter(unformatted, "\n") f := strings.SplitAfter(formatted, "\n") for _, op := range diff.Operations(u, f) { + start := lineStart(tok, op.I1+1) + if start == token.NoPos && op.I1 == len(u) { + start = tok.Pos(tok.Size()) + } + end := lineStart(tok, op.I2+1) + if end == token.NoPos && op.I2 == len(u) { + end = tok.Pos(tok.Size()) + } switch op.Kind { case diff.Delete: // Delete: unformatted[i1:i2] is deleted. edits = append(edits, TextEdit{ Range: Range{ - Start: lineStart(tok, op.I1+1), - End: lineStart(tok, op.I2+1), + Start: start, + End: end, }, }) case diff.Insert: // Insert: formatted[j1:j2] is inserted at unformatted[i1:i1]. edits = append(edits, TextEdit{ Range: Range{ - Start: lineStart(tok, op.I1+1), - End: lineStart(tok, op.I1+1), + Start: start, + End: start, }, NewText: op.Content, }) diff --git a/internal/lsp/testdata/format/newline_format.go.in b/internal/lsp/testdata/format/newline_format.go.in new file mode 100644 index 00000000..fe597b90 --- /dev/null +++ b/internal/lsp/testdata/format/newline_format.go.in @@ -0,0 +1,2 @@ +package format //@format("package") +func _() {} \ No newline at end of file