From 744a51dd885aeae6b2ab02191a8dc51b3c83c805 Mon Sep 17 00:00:00 2001 From: Ian Cottrell Date: Mon, 8 Apr 2019 09:22:58 -0400 Subject: [PATCH] internal/lsp: normalise and make public diff<->edit conversions This allows us to use the diff.ApplyEdits in tests, saving us from a different implementation. It also prepares for command lines that need to use diff features based on the results of a protocol message. Splitting content into lines is too easy to get wrong, and needs to be done correctly or the diff results make no sense. This adds the SplitLines function to the diff pacakge to do it right and then uses it everwhere we we already doing it wrong. It also makes all the diff tests external black box tests. Change-Id: I698227d5769a2bfbfd22a64ea42906b1df9268d9 Reviewed-on: https://go-review.googlesource.com/c/tools/+/171027 Run-TryBot: Ian Cottrell TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/code_action.go | 2 +- internal/lsp/diff/diff.go | 10 +++++++ internal/lsp/diff/diff_test.go | 52 ++++++++++++++++++---------------- internal/lsp/diff/unified.go | 3 -- internal/lsp/format.go | 22 ++++++++++++-- internal/lsp/lsp_test.go | 33 ++++----------------- internal/lsp/source/format.go | 18 ++---------- internal/lsp/source/view.go | 48 +++++++++++++++++++++++++++++++ 8 files changed, 115 insertions(+), 73 deletions(-) diff --git a/internal/lsp/code_action.go b/internal/lsp/code_action.go index e95d1672..177c1452 100644 --- a/internal/lsp/code_action.go +++ b/internal/lsp/code_action.go @@ -81,7 +81,7 @@ func organizeImports(ctx context.Context, v source.View, s span.Span) (*protocol if err != nil { return nil, err } - protocolEdits, err := toProtocolEdits(m, edits) + protocolEdits, err := ToProtocolEdits(m, edits) if err != nil { return nil, err } diff --git a/internal/lsp/diff/diff.go b/internal/lsp/diff/diff.go index 5e06ec41..f894b867 100644 --- a/internal/lsp/diff/diff.go +++ b/internal/lsp/diff/diff.go @@ -5,6 +5,8 @@ // Package diff implements the Myers diff algorithm. package diff +import "strings" + // Sources: // 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 @@ -208,3 +210,11 @@ func shortestEditSequence(a, b []string) ([][]int, int) { } return nil, 0 } + +func SplitLines(text string) []string { + lines := strings.SplitAfter(text, "\n") + if lines[len(lines)-1] == "" { + lines = lines[:len(lines)-1] + } + return lines +} diff --git a/internal/lsp/diff/diff_test.go b/internal/lsp/diff/diff_test.go index 1eb32f4d..1fe42929 100644 --- a/internal/lsp/diff/diff_test.go +++ b/internal/lsp/diff/diff_test.go @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -package diff +package diff_test import ( "flag" @@ -13,6 +13,8 @@ import ( "reflect" "strings" "testing" + + "golang.org/x/tools/internal/lsp/diff" ) const ( @@ -26,22 +28,22 @@ var verifyDiff = flag.Bool("verify-diff", false, "Check that the unified diff ou func TestDiff(t *testing.T) { for _, test := range []struct { a, b string - lines []*Op - operations []*Op + lines []*diff.Op + operations []*diff.Op unified string nodiff bool }{ { a: "A\nB\nC\n", b: "A\nB\nC\n", - operations: []*Op{}, + operations: []*diff.Op{}, unified: ` `[1:]}, { a: "A\n", b: "B\n", - operations: []*Op{ - &Op{Kind: Delete, I1: 0, I2: 1, J1: 0}, - &Op{Kind: Insert, Content: []string{"B\n"}, I1: 1, I2: 1, J1: 0}, + operations: []*diff.Op{ + &diff.Op{Kind: diff.Delete, I1: 0, I2: 1, J1: 0}, + &diff.Op{Kind: diff.Insert, Content: []string{"B\n"}, I1: 1, I2: 1, J1: 0}, }, unified: ` @@ -1 +1 @@ @@ -50,9 +52,9 @@ func TestDiff(t *testing.T) { `[1:]}, { a: "A", b: "B", - operations: []*Op{ - &Op{Kind: Delete, I1: 0, I2: 1, J1: 0}, - &Op{Kind: Insert, Content: []string{"B"}, I1: 1, I2: 1, J1: 0}, + operations: []*diff.Op{ + &diff.Op{Kind: diff.Delete, I1: 0, I2: 1, J1: 0}, + &diff.Op{Kind: diff.Insert, Content: []string{"B"}, I1: 1, I2: 1, J1: 0}, }, unified: ` @@ -1 +1 @@ @@ -63,12 +65,12 @@ func TestDiff(t *testing.T) { `[1:]}, { a: "A\nB\nC\nA\nB\nB\nA\n", b: "C\nB\nA\nB\nA\nC\n", - operations: []*Op{ - &Op{Kind: Delete, I1: 0, I2: 1, J1: 0}, - &Op{Kind: Delete, I1: 1, I2: 2, J1: 0}, - &Op{Kind: Insert, Content: []string{"B\n"}, I1: 3, I2: 3, J1: 1}, - &Op{Kind: Delete, I1: 5, I2: 6, J1: 4}, - &Op{Kind: Insert, Content: []string{"C\n"}, I1: 7, I2: 7, J1: 5}, + operations: []*diff.Op{ + &diff.Op{Kind: diff.Delete, I1: 0, I2: 1, J1: 0}, + &diff.Op{Kind: diff.Delete, I1: 1, I2: 2, J1: 0}, + &diff.Op{Kind: diff.Insert, Content: []string{"B\n"}, I1: 3, I2: 3, J1: 1}, + &diff.Op{Kind: diff.Delete, I1: 5, I2: 6, J1: 4}, + &diff.Op{Kind: diff.Insert, Content: []string{"C\n"}, I1: 7, I2: 7, J1: 5}, }, unified: ` @@ -1,7 +1,6 @@ @@ -87,10 +89,10 @@ func TestDiff(t *testing.T) { { a: "A\nB\n", b: "A\nC\n\n", - operations: []*Op{ - &Op{Kind: Delete, I1: 1, I2: 2, J1: 1}, - &Op{Kind: Insert, Content: []string{"C\n"}, I1: 2, I2: 2, J1: 1}, - &Op{Kind: Insert, Content: []string{"\n"}, I1: 2, I2: 2, J1: 2}, + operations: []*diff.Op{ + &diff.Op{Kind: diff.Delete, I1: 1, I2: 2, J1: 1}, + &diff.Op{Kind: diff.Insert, Content: []string{"C\n"}, I1: 2, I2: 2, J1: 1}, + &diff.Op{Kind: diff.Insert, Content: []string{"\n"}, I1: 2, I2: 2, J1: 2}, }, unified: ` @@ -1,2 +1,3 @@ @@ -118,9 +120,9 @@ func TestDiff(t *testing.T) { +K `[1:]}, } { - a := strings.SplitAfter(test.a, "\n") - b := strings.SplitAfter(test.b, "\n") - ops := Operations(a, b) + a := diff.SplitLines(test.a) + b := diff.SplitLines(test.b) + ops := diff.Operations(a, b) if test.operations != nil { if len(ops) != len(test.operations) { t.Fatalf("expected %v operations, got %v", len(test.operations), len(ops)) @@ -132,7 +134,7 @@ func TestDiff(t *testing.T) { } } } - applied := ApplyEdits(a, ops) + applied := diff.ApplyEdits(a, ops) for i, want := range applied { got := b[i] if got != want { @@ -140,7 +142,7 @@ func TestDiff(t *testing.T) { } } if test.unified != "" { - diff := ToUnified(fileA, fileB, a, ops) + diff := diff.ToUnified(fileA, fileB, a, ops) got := fmt.Sprint(diff) if !strings.HasPrefix(got, unifiedPrefix) { t.Errorf("expected prefix:\n%s\ngot:\n%s", unifiedPrefix, got) diff --git a/internal/lsp/diff/unified.go b/internal/lsp/diff/unified.go index ab014aae..427a8719 100644 --- a/internal/lsp/diff/unified.go +++ b/internal/lsp/diff/unified.go @@ -38,9 +38,6 @@ func ToUnified(from, to string, lines []string, ops []*Op) Unified { if len(ops) == 0 { return u } - if lines[len(lines)-1] == "" { - lines = lines[:len(lines)-1] - } var h *Hunk last := -(gap + 2) for _, op := range ops { diff --git a/internal/lsp/format.go b/internal/lsp/format.go index 5145e2a2..0277f5ce 100644 --- a/internal/lsp/format.go +++ b/internal/lsp/format.go @@ -31,10 +31,10 @@ func formatRange(ctx context.Context, v source.View, s span.Span) ([]protocol.Te if err != nil { return nil, err } - return toProtocolEdits(m, edits) + return ToProtocolEdits(m, edits) } -func toProtocolEdits(m *protocol.ColumnMapper, edits []source.TextEdit) ([]protocol.TextEdit, error) { +func ToProtocolEdits(m *protocol.ColumnMapper, edits []source.TextEdit) ([]protocol.TextEdit, error) { if edits == nil { return nil, nil } @@ -52,6 +52,24 @@ func toProtocolEdits(m *protocol.ColumnMapper, edits []source.TextEdit) ([]proto return result, nil } +func FromProtocolEdits(m *protocol.ColumnMapper, edits []protocol.TextEdit) ([]source.TextEdit, error) { + if edits == nil { + return nil, nil + } + result := make([]source.TextEdit, len(edits)) + for i, edit := range edits { + spn, err := m.RangeSpan(edit.Range) + if err != nil { + return nil, err + } + result[i] = source.TextEdit{ + Span: spn, + NewText: edit.NewText, + } + } + return result, nil +} + func newColumnMap(ctx context.Context, v source.View, uri span.URI) (source.File, *protocol.ColumnMapper, error) { f, err := v.GetFile(ctx, uri) if err != nil { diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index 60ff67fa..22a502b3 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -20,6 +20,7 @@ import ( "golang.org/x/tools/go/packages/packagestest" "golang.org/x/tools/internal/lsp/cache" + "golang.org/x/tools/internal/lsp/diff" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/lsp/xlog" @@ -413,17 +414,18 @@ func (f formats) test(t *testing.T, s *Server) { } continue } - f, m, err := newColumnMap(ctx, s.findView(ctx, uri), uri) + _, m, err := newColumnMap(ctx, s.findView(ctx, uri), uri) if err != nil { t.Error(err) } - buf, err := applyEdits(m, f.GetContent(context.Background()), edits) + sedits, err := FromProtocolEdits(m, edits) if err != nil { t.Error(err) } - got := string(buf) + ops := source.EditsToDiff(sedits) + got := strings.Join(diff.ApplyEdits(diff.SplitLines(string(m.Content)), ops), "") if gofmted != got { - t.Errorf("format failed for %s: expected '%v', got '%v'", filename, gofmted, got) + t.Errorf("format failed for %s, expected:\n%v\ngot:\n%v", filename, gofmted, got) } } } @@ -660,26 +662,3 @@ func TestBytesOffset(t *testing.T) { } } } - -func applyEdits(m *protocol.ColumnMapper, content []byte, edits []protocol.TextEdit) ([]byte, error) { - prev := 0 - result := make([]byte, 0, len(content)) - for _, edit := range edits { - spn, err := m.RangeSpan(edit.Range) - if err != nil { - return nil, err - } - offset := spn.Start().Offset() - if offset > prev { - result = append(result, content[prev:offset]...) - } - if len(edit.NewText) > 0 { - result = append(result, []byte(edit.NewText)...) - } - prev = spn.End().Offset() - } - if prev < len(content) { - result = append(result, content[prev:]...) - } - return result, nil -} diff --git a/internal/lsp/source/format.go b/internal/lsp/source/format.go index cc7d72a1..bcc0d7b5 100644 --- a/internal/lsp/source/format.go +++ b/internal/lsp/source/format.go @@ -10,7 +10,6 @@ import ( "context" "fmt" "go/format" - "strings" "golang.org/x/tools/go/ast/astutil" "golang.org/x/tools/go/packages" @@ -62,18 +61,7 @@ func Imports(ctx context.Context, f File, rng span.Range) ([]TextEdit, error) { } func computeTextEdits(ctx context.Context, file File, formatted string) (edits []TextEdit) { - u := strings.SplitAfter(string(file.GetContent(ctx)), "\n") - f := strings.SplitAfter(formatted, "\n") - for _, op := range diff.Operations(u, f) { - s := span.New(file.URI(), span.NewPoint(op.I1+1, 1, 0), span.NewPoint(op.I2+1, 1, 0)) - switch op.Kind { - case diff.Delete: - // Delete: unformatted[i1:i2] is deleted. - edits = append(edits, TextEdit{Span: s}) - case diff.Insert: - // Insert: formatted[j1:j2] is inserted at unformatted[i1:i1]. - edits = append(edits, TextEdit{Span: s, NewText: strings.Join(op.Content, "")}) - } - } - return edits + u := diff.SplitLines(string(file.GetContent(ctx))) + f := diff.SplitLines(formatted) + return DiffToEdits(file.URI(), diff.Operations(u, f)) } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 243857f5..896ac8e0 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -9,9 +9,11 @@ import ( "go/ast" "go/token" "go/types" + "strings" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/packages" + "golang.org/x/tools/internal/lsp/diff" "golang.org/x/tools/internal/lsp/xlog" "golang.org/x/tools/internal/span" ) @@ -58,3 +60,49 @@ type TextEdit struct { Span span.Span NewText string } + +// DiffToEdits converts from a sequence of diff operations to a sequence of +// source.TextEdit +func DiffToEdits(uri span.URI, ops []*diff.Op) []TextEdit { + edits := make([]TextEdit, 0, len(ops)) + for _, op := range ops { + s := span.New(uri, span.NewPoint(op.I1+1, 1, 0), span.NewPoint(op.I2+1, 1, 0)) + switch op.Kind { + case diff.Delete: + // Delete: unformatted[i1:i2] is deleted. + edits = append(edits, TextEdit{Span: s}) + case diff.Insert: + // Insert: formatted[j1:j2] is inserted at unformatted[i1:i1]. + if content := strings.Join(op.Content, ""); content != "" { + edits = append(edits, TextEdit{Span: s, NewText: content}) + } + } + } + return edits +} + +func EditsToDiff(edits []TextEdit) []*diff.Op { + iToJ := 0 + ops := make([]*diff.Op, len(edits)) + for i, edit := range edits { + i1 := edit.Span.Start().Line() - 1 + i2 := edit.Span.End().Line() - 1 + kind := diff.Insert + if edit.NewText == "" { + kind = diff.Delete + } + ops[i] = &diff.Op{ + Kind: kind, + Content: diff.SplitLines(edit.NewText), + I1: i1, + I2: i2, + J1: i1 + iToJ, + } + if kind == diff.Insert { + iToJ += len(ops[i].Content) + } else { + iToJ -= i2 - i1 + } + } + return ops +}