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 <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
diff --git a/internal/lsp/diff/diff.go b/internal/lsp/diff/diff.go
index dc6da2f..573190b 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 @@
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 @@
}
}
x++
- if x == len(a) {
+ if x == M {
break
}
}
@@ -125,58 +129,7 @@
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 @@
x = V[kPrev+offset]
y = x - kPrev
}
- // this feels questionable
if x < 0 || y < 0 {
return snakes
}
@@ -254,7 +206,7 @@
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 1c53328..9a615e3 100644
--- a/internal/lsp/diff/diff_test.go
+++ b/internal/lsp/diff/diff_test.go
@@ -18,17 +18,6 @@
{
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 @@
&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 c23a129..5dc80a9 100644
--- a/internal/lsp/lsp_test.go
+++ b/internal/lsp/lsp_test.go
@@ -38,7 +38,7 @@
// 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 @@
}
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 5d1177a..78aaab7 100644
--- a/internal/lsp/source/format.go
+++ b/internal/lsp/source/format.go
@@ -82,21 +82,29 @@
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 0000000..fe597b9
--- /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
