internal/diff: abolish errors
Computing the difference between two strings is logically an
infallible operation. This change makes the code reflect that. The
actual failures were unreachable given consistent inputs, but that was
hard to see from the complexity of the logic surrounding span.Span.
(The problem only occurs when converting offsets beyond the end of the
file to Spans, but the code preserves the integrity of offsets.)
gopls' "old" hooks.ComputeEdits impl (based on sergi/go-diff) now
reports a bug and returns a single diff for the entire file if it
panics.
Also, first steps towards simpler API and a
reusable diff package in x/tools:
- add TODO for new API. In particular, the diff package shouldn't care
about filenames, spans, and URIs. These are gopls concerns.
- diff.String is the main diff function.
- diff.Unified prints edits in unified form;
all its internals are now hidden.
- the ComputeEdits func type is moved to gopls (source.DiffFunction)
- remove all non-critical uses of myers.ComputeEdits. The only
remaining one is in gopls' defaults, but perhaps that gets
overridden by the default GoDiff setting in hooks, to BothDiffs
(sergi + pjw impls), so maybe it's now actually unused in practice?
Change-Id: I6ceb5c670897abbf285b243530a7372dfa41edf6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/436778
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
diff --git a/go/analysis/analysistest/analysistest.go b/go/analysis/analysistest/analysistest.go
index 1fd0a84..e65a695 100644
--- a/go/analysis/analysistest/analysistest.go
+++ b/go/analysis/analysistest/analysistest.go
@@ -25,7 +25,6 @@
"golang.org/x/tools/go/analysis/internal/checker"
"golang.org/x/tools/go/packages"
"golang.org/x/tools/internal/diff"
- "golang.org/x/tools/internal/diff/myers"
"golang.org/x/tools/internal/span"
"golang.org/x/tools/internal/testenv"
"golang.org/x/tools/txtar"
@@ -201,11 +200,8 @@
continue
}
if want != string(formatted) {
- d, err := myers.ComputeEdits("", want, string(formatted))
- if err != nil {
- t.Errorf("failed to compute suggested fix diff: %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))
+ edits := diff.Strings("", want, string(formatted))
+ t.Errorf("suggested fixes failed for %s:\n%s", file.Name(), diff.Unified(fmt.Sprintf("%s.golden [%s]", file.Name(), sf), "actual", want, edits))
}
break
}
@@ -231,11 +227,8 @@
continue
}
if want != string(formatted) {
- d, err := myers.ComputeEdits("", want, string(formatted))
- if err != nil {
- t.Errorf("%s: failed to compute suggested fix diff: %s", file.Name(), err)
- }
- t.Errorf("suggested fixes failed for %s:\n%s", file.Name(), diff.ToUnified(file.Name()+".golden", "actual", want, d))
+ edits := diff.Strings("", want, string(formatted))
+ t.Errorf("suggested fixes failed for %s:\n%s", file.Name(), diff.Unified(file.Name()+".golden", "actual", want, edits))
}
}
}
diff --git a/gopls/api-diff/api_diff.go b/gopls/api-diff/api_diff.go
index 8e6c09e..8aa333e 100644
--- a/gopls/api-diff/api_diff.go
+++ b/gopls/api-diff/api_diff.go
@@ -22,8 +22,7 @@
"strings"
"golang.org/x/tools/gopls/internal/lsp/source"
- difflib "golang.org/x/tools/internal/diff"
- "golang.org/x/tools/internal/diff/myers"
+ diffpkg "golang.org/x/tools/internal/diff"
"golang.org/x/tools/internal/gocommand"
)
@@ -125,7 +124,7 @@
c, p := bytes.NewBuffer(nil), bytes.NewBuffer(nil)
prev.Write(p)
current.Write(c)
- if diff, err := diffStr(p.String(), c.String()); err == nil && diff != "" {
+ if diff := diffStr(p.String(), c.String()); diff != "" {
diffFunc(b, prev, current)
b.WriteString("\n--\n")
}
@@ -225,11 +224,8 @@
func diffOptions(b *strings.Builder, previous, current *source.OptionJSON) {
b.WriteString(fmt.Sprintf("Changes to option %s:\n\n", current.Name))
if previous.Doc != current.Doc {
- diff, err := diffStr(previous.Doc, current.Doc)
- if err != nil {
- panic(err)
- }
- b.WriteString(fmt.Sprintf("Documentation changed:\n%s\n", diff))
+ diff := diffStr(previous.Doc, current.Doc)
+ fmt.Fprintf(b, "Documentation changed:\n%s\n", diff)
}
if previous.Default != current.Default {
b.WriteString(fmt.Sprintf("Default changed from %q to %q\n", previous.Default, current.Default))
@@ -259,16 +255,13 @@
return "\n```\n" + str + "\n```\n"
}
-func diffStr(before, after string) (string, error) {
+func diffStr(before, after string) string {
// Add newlines to avoid newline messages in diff.
if before == after {
- return "", nil
+ return ""
}
before += "\n"
after += "\n"
- d, err := myers.ComputeEdits("", before, after)
- if err != nil {
- return "", err
- }
- return fmt.Sprintf("%q", difflib.ToUnified("previous", "current", before, d)), err
+ edits := diffpkg.Strings("irrelevant", before, after)
+ return fmt.Sprintf("%q", diffpkg.Unified("previous", "current", before, edits))
}
diff --git a/gopls/internal/hooks/diff.go b/gopls/internal/hooks/diff.go
index 46faae1..25b1d95 100644
--- a/gopls/internal/hooks/diff.go
+++ b/gopls/internal/hooks/diff.go
@@ -8,6 +8,7 @@
"crypto/rand"
"encoding/json"
"fmt"
+ "go/token"
"io"
"log"
"math/big"
@@ -20,6 +21,7 @@
"unicode"
"github.com/sergi/go-diff/diffmatchpatch"
+ "golang.org/x/tools/internal/bug"
"golang.org/x/tools/internal/diff"
"golang.org/x/tools/internal/span"
)
@@ -142,48 +144,67 @@
// BothDiffs edits calls both the new and old diffs, checks that the new diffs
// change before into after, and attempts to preserve some statistics.
-func BothDiffs(uri span.URI, before, after string) (edits []diff.TextEdit, err error) {
+func BothDiffs(uri span.URI, before, after string) (edits []diff.TextEdit) {
// The new diff code contains a lot of internal checks that panic when they
// fail. This code catches the panics, or other failures, tries to save
// the failing example (and ut wiykd ask the user to send it back to us, and
// changes options.newDiff to 'old', if only we could figure out how.)
stat := diffstat{Before: len(before), After: len(after)}
now := time.Now()
- Oldedits, oerr := ComputeEdits(uri, before, after)
- if oerr != nil {
- stat.Msg += fmt.Sprintf("old:%v", oerr)
- }
- stat.Oldedits = len(Oldedits)
+ oldedits := ComputeEdits(uri, before, after)
+ stat.Oldedits = len(oldedits)
stat.Oldtime = time.Since(now)
defer func() {
if r := recover(); r != nil {
disaster(before, after)
- edits, err = Oldedits, oerr
+ edits = oldedits
}
}()
now = time.Now()
- Newedits, rerr := diff.NComputeEdits(uri, before, after)
- stat.Newedits = len(Newedits)
+ newedits := diff.Strings(uri, before, after)
+ stat.Newedits = len(newedits)
stat.Newtime = time.Now().Sub(now)
- got := diff.ApplyEdits(before, Newedits)
+ got := diff.ApplyEdits(before, newedits)
if got != after {
stat.Msg += "FAIL"
disaster(before, after)
stat.save()
- return Oldedits, oerr
+ return oldedits
}
stat.save()
- return Newedits, rerr
+ return newedits
}
-func ComputeEdits(uri span.URI, before, after string) (edits []diff.TextEdit, err error) {
+// ComputeEdits computes a diff using the github.com/sergi/go-diff implementation.
+func ComputeEdits(uri span.URI, before, after string) (edits []diff.TextEdit) {
// The go-diff library has an unresolved panic (see golang/go#278774).
// TODO(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)
+ bug.Reportf("unable to compute edits for %s: %s", uri.Filename(), r)
+
+ // Report one big edit for the whole file.
+
+ // Reuse the machinery of go/token to convert (content, offset)
+ // to (line, column).
+ tf := token.NewFileSet().AddFile(uri.Filename(), -1, len(before))
+ tf.SetLinesForContent([]byte(before))
+ offsetToPoint := func(offset int) span.Point {
+ // Re-use span.ToPosition since it contains more than
+ // just token.File operations (bug workarounds).
+ // But it can only fail if the diff was ill-formed.
+ line, col, err := span.ToPosition(tf, offset)
+ if err != nil {
+ log.Fatalf("invalid offset: %v", err)
+ }
+ return span.NewPoint(line, col, offset)
+ }
+ all := span.New(uri, offsetToPoint(0), offsetToPoint(len(before)))
+ edits = []diff.TextEdit{{
+ Span: all,
+ NewText: after,
+ }}
}
}()
diffs := diffmatchpatch.New().DiffMain(before, after, true)
@@ -201,5 +222,5 @@
edits = append(edits, diff.TextEdit{Span: span.New(uri, start, span.Point{}), NewText: d.Text})
}
}
- return edits, nil
+ return edits
}
diff --git a/gopls/internal/hooks/hooks.go b/gopls/internal/hooks/hooks.go
index 7c4ac9d..085fa53 100644
--- a/gopls/internal/hooks/hooks.go
+++ b/gopls/internal/hooks/hooks.go
@@ -23,7 +23,7 @@
case "old":
options.ComputeEdits = ComputeEdits
case "new":
- options.ComputeEdits = diff.NComputeEdits
+ options.ComputeEdits = diff.Strings
default:
options.ComputeEdits = BothDiffs
}
diff --git a/gopls/internal/lsp/cache/mod_tidy.go b/gopls/internal/lsp/cache/mod_tidy.go
index 1bbc623..5570417 100644
--- a/gopls/internal/lsp/cache/mod_tidy.go
+++ b/gopls/internal/lsp/cache/mod_tidy.go
@@ -16,13 +16,12 @@
"strings"
"golang.org/x/mod/modfile"
- "golang.org/x/tools/internal/event"
- "golang.org/x/tools/internal/gocommand"
"golang.org/x/tools/gopls/internal/lsp/command"
- "golang.org/x/tools/internal/event/tag"
- "golang.org/x/tools/internal/diff"
"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/gopls/internal/lsp/source"
+ "golang.org/x/tools/internal/event"
+ "golang.org/x/tools/internal/event/tag"
+ "golang.org/x/tools/internal/gocommand"
"golang.org/x/tools/internal/memoize"
"golang.org/x/tools/internal/span"
)
@@ -313,7 +312,7 @@
// directnessDiagnostic extracts errors when a dependency is labeled indirect when
// it should be direct and vice versa.
-func directnessDiagnostic(m *protocol.ColumnMapper, req *modfile.Require, computeEdits diff.ComputeEdits) (*source.Diagnostic, error) {
+func directnessDiagnostic(m *protocol.ColumnMapper, req *modfile.Require, computeEdits source.DiffFunction) (*source.Diagnostic, error) {
rng, err := rangeFromPositions(m, req.Syntax.Start, req.Syntax.End)
if err != nil {
return nil, err
@@ -386,7 +385,7 @@
// switchDirectness gets the edits needed to change an indirect dependency to
// direct and vice versa.
-func switchDirectness(req *modfile.Require, m *protocol.ColumnMapper, computeEdits diff.ComputeEdits) ([]protocol.TextEdit, error) {
+func switchDirectness(req *modfile.Require, m *protocol.ColumnMapper, computeEdits source.DiffFunction) ([]protocol.TextEdit, error) {
// We need a private copy of the parsed go.mod file, since we're going to
// modify it.
copied, err := modfile.Parse("", m.Content, nil)
@@ -420,11 +419,8 @@
return nil, err
}
// Calculate the edits to be made due to the change.
- diff, err := computeEdits(m.URI, string(m.Content), string(newContent))
- if err != nil {
- return nil, err
- }
- return source.ToProtocolEdits(m, diff)
+ edits := computeEdits(m.URI, string(m.Content), string(newContent))
+ return source.ToProtocolEdits(m, edits)
}
// missingModuleForImport creates an error for a given import path that comes
diff --git a/gopls/internal/lsp/cache/parse.go b/gopls/internal/lsp/cache/parse.go
index 981e698..5c084fa 100644
--- a/gopls/internal/lsp/cache/parse.go
+++ b/gopls/internal/lsp/cache/parse.go
@@ -22,7 +22,6 @@
"golang.org/x/tools/gopls/internal/lsp/safetoken"
"golang.org/x/tools/gopls/internal/lsp/source"
"golang.org/x/tools/internal/diff"
- "golang.org/x/tools/internal/diff/myers"
"golang.org/x/tools/internal/event"
"golang.org/x/tools/internal/event/tag"
"golang.org/x/tools/internal/memoize"
@@ -158,13 +157,9 @@
// it is likely we got stuck in a loop somehow. Log out a diff
// of the last changes we made to aid in debugging.
if i == 9 {
- edits, err := myers.ComputeEdits(fh.URI(), string(src), string(newSrc))
- if err != nil {
- event.Error(ctx, "error generating fixSrc diff", err, tag.File.Of(tok.Name()))
- } else {
- unified := diff.ToUnified("before", "after", string(src), edits)
- event.Log(ctx, fmt.Sprintf("fixSrc loop - last diff:\n%v", unified), tag.File.Of(tok.Name()))
- }
+ edits := diff.Strings(fh.URI(), string(src), string(newSrc))
+ unified := diff.Unified("before", "after", string(src), edits)
+ event.Log(ctx, fmt.Sprintf("fixSrc loop - last diff:\n%v", unified), tag.File.Of(tok.Name()))
}
newFile, _ := parser.ParseFile(fset, fh.URI().Filename(), newSrc, parserMode)
diff --git a/gopls/internal/lsp/cmd/format.go b/gopls/internal/lsp/cmd/format.go
index 4098506..b2f2afa 100644
--- a/gopls/internal/lsp/cmd/format.go
+++ b/gopls/internal/lsp/cmd/format.go
@@ -10,9 +10,9 @@
"fmt"
"io/ioutil"
- "golang.org/x/tools/internal/diff"
"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/gopls/internal/lsp/source"
+ "golang.org/x/tools/internal/diff"
"golang.org/x/tools/internal/span"
)
@@ -96,7 +96,7 @@
}
if c.Diff {
printIt = false
- u := diff.ToUnified(filename+".orig", filename, string(file.mapper.Content), sedits)
+ u := diff.Unified(filename+".orig", filename, string(file.mapper.Content), sedits)
fmt.Print(u)
}
if printIt {
diff --git a/gopls/internal/lsp/cmd/imports.go b/gopls/internal/lsp/cmd/imports.go
index d6a968d..eb564ee 100644
--- a/gopls/internal/lsp/cmd/imports.go
+++ b/gopls/internal/lsp/cmd/imports.go
@@ -10,9 +10,9 @@
"fmt"
"io/ioutil"
- "golang.org/x/tools/internal/diff"
"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/gopls/internal/lsp/source"
+ "golang.org/x/tools/internal/diff"
"golang.org/x/tools/internal/span"
"golang.org/x/tools/internal/tool"
)
@@ -94,7 +94,7 @@
ioutil.WriteFile(filename, []byte(newContent), 0644)
}
case t.Diff:
- diffs := diff.ToUnified(filename+".orig", filename, string(file.mapper.Content), sedits)
+ diffs := diff.Unified(filename+".orig", filename, string(file.mapper.Content), sedits)
fmt.Print(diffs)
default:
fmt.Print(string(newContent))
diff --git a/gopls/internal/lsp/cmd/rename.go b/gopls/internal/lsp/cmd/rename.go
index 5f16003..a330bae 100644
--- a/gopls/internal/lsp/cmd/rename.go
+++ b/gopls/internal/lsp/cmd/rename.go
@@ -112,7 +112,7 @@
}
ioutil.WriteFile(filename, []byte(newContent), 0644)
case r.Diff:
- diffs := diff.ToUnified(filename+".orig", filename, string(cmdFile.mapper.Content), renameEdits)
+ diffs := diff.Unified(filename+".orig", filename, string(cmdFile.mapper.Content), renameEdits)
fmt.Print(diffs)
default:
if len(orderedURIs) > 1 {
diff --git a/gopls/internal/lsp/cmd/suggested_fix.go b/gopls/internal/lsp/cmd/suggested_fix.go
index 048dcd6..faf681f 100644
--- a/gopls/internal/lsp/cmd/suggested_fix.go
+++ b/gopls/internal/lsp/cmd/suggested_fix.go
@@ -10,9 +10,9 @@
"fmt"
"io/ioutil"
- "golang.org/x/tools/internal/diff"
"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/gopls/internal/lsp/source"
+ "golang.org/x/tools/internal/diff"
"golang.org/x/tools/internal/span"
"golang.org/x/tools/internal/tool"
)
@@ -155,7 +155,7 @@
ioutil.WriteFile(filename, []byte(newContent), 0644)
}
case s.Diff:
- diffs := diff.ToUnified(filename+".orig", filename, string(file.mapper.Content), sedits)
+ diffs := diff.Unified(filename+".orig", filename, string(file.mapper.Content), sedits)
fmt.Print(diffs)
default:
fmt.Print(string(newContent))
diff --git a/gopls/internal/lsp/cmd/test/imports.go b/gopls/internal/lsp/cmd/test/imports.go
index fd41626..67826a4 100644
--- a/gopls/internal/lsp/cmd/test/imports.go
+++ b/gopls/internal/lsp/cmd/test/imports.go
@@ -8,7 +8,6 @@
"testing"
"golang.org/x/tools/internal/diff"
- "golang.org/x/tools/internal/diff/myers"
"golang.org/x/tools/internal/span"
)
@@ -20,10 +19,7 @@
return []byte(got), nil
}))
if 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))
+ edits := diff.Strings(uri, want, got)
+ t.Errorf("imports failed for %s, expected:\n%s", filename, diff.Unified("want", "got", want, edits))
}
}
diff --git a/gopls/internal/lsp/command.go b/gopls/internal/lsp/command.go
index 5ded346..e9483b6 100644
--- a/gopls/internal/lsp/command.go
+++ b/gopls/internal/lsp/command.go
@@ -390,10 +390,7 @@
return nil, err
}
// Calculate the edits to be made due to the change.
- diff, err := snapshot.View().Options().ComputeEdits(pm.URI, string(pm.Mapper.Content), string(newContent))
- if err != nil {
- return nil, err
- }
+ diff := snapshot.View().Options().ComputeEdits(pm.URI, string(pm.Mapper.Content), string(newContent))
return source.ToProtocolEdits(pm.Mapper, diff)
}
@@ -615,10 +612,7 @@
}
m := protocol.NewColumnMapper(fh.URI(), oldContent)
- diff, err := snapshot.View().Options().ComputeEdits(uri, string(oldContent), string(newContent))
- if err != nil {
- return nil, err
- }
+ diff := snapshot.View().Options().ComputeEdits(uri, string(oldContent), string(newContent))
edits, err := source.ToProtocolEdits(m, diff)
if err != nil {
return nil, err
diff --git a/gopls/internal/lsp/mod/format.go b/gopls/internal/lsp/mod/format.go
index d2bce4c..010d2ac 100644
--- a/gopls/internal/lsp/mod/format.go
+++ b/gopls/internal/lsp/mod/format.go
@@ -7,9 +7,9 @@
import (
"context"
- "golang.org/x/tools/internal/event"
"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/gopls/internal/lsp/source"
+ "golang.org/x/tools/internal/event"
)
func Format(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle) ([]protocol.TextEdit, error) {
@@ -25,9 +25,6 @@
return nil, err
}
// Calculate the edits to be made due to the change.
- diff, err := snapshot.View().Options().ComputeEdits(fh.URI(), string(pm.Mapper.Content), string(formatted))
- if err != nil {
- return nil, err
- }
- return source.ToProtocolEdits(pm.Mapper, diff)
+ diffs := snapshot.View().Options().ComputeEdits(fh.URI(), string(pm.Mapper.Content), string(formatted))
+ return source.ToProtocolEdits(pm.Mapper, diffs)
}
diff --git a/gopls/internal/lsp/source/format.go b/gopls/internal/lsp/source/format.go
index e230117..9080595 100644
--- a/gopls/internal/lsp/source/format.go
+++ b/gopls/internal/lsp/source/format.go
@@ -16,12 +16,12 @@
"strings"
"text/scanner"
- "golang.org/x/tools/internal/event"
- "golang.org/x/tools/internal/imports"
- "golang.org/x/tools/internal/diff"
"golang.org/x/tools/gopls/internal/lsp/lsppos"
"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/gopls/internal/lsp/safetoken"
+ "golang.org/x/tools/internal/diff"
+ "golang.org/x/tools/internal/event"
+ "golang.org/x/tools/internal/imports"
)
// Format formats a file with a given range.
@@ -199,10 +199,7 @@
if fixedData == nil || fixedData[len(fixedData)-1] != '\n' {
fixedData = append(fixedData, '\n') // ApplyFixes may miss the newline, go figure.
}
- edits, err := snapshot.View().Options().ComputeEdits(pgf.URI, left, string(fixedData))
- if err != nil {
- return nil, err
- }
+ edits := snapshot.View().Options().ComputeEdits(pgf.URI, left, string(fixedData))
return ProtocolEditsFromSource([]byte(left), edits, pgf.Mapper.TokFile)
}
@@ -312,10 +309,7 @@
_, done := event.Start(ctx, "source.computeTextEdits")
defer done()
- edits, err := snapshot.View().Options().ComputeEdits(pgf.URI, string(pgf.Src), formatted)
- if err != nil {
- return nil, err
- }
+ edits := snapshot.View().Options().ComputeEdits(pgf.URI, string(pgf.Src), formatted)
return ToProtocolEdits(pgf.Mapper, edits)
}
diff --git a/gopls/internal/lsp/source/options.go b/gopls/internal/lsp/source/options.go
index 93d0948..8159221 100644
--- a/gopls/internal/lsp/source/options.go
+++ b/gopls/internal/lsp/source/options.go
@@ -67,6 +67,7 @@
"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/internal/diff"
"golang.org/x/tools/internal/diff/myers"
+ "golang.org/x/tools/internal/span"
)
var (
@@ -167,6 +168,7 @@
ChattyDiagnostics: true,
},
Hooks: Hooks{
+ // TODO(adonovan): switch to new diff.Strings implementation.
ComputeEdits: myers.ComputeEdits,
URLRegexp: urlRegexp(),
DefaultAnalyzers: defaultAnalyzers(),
@@ -497,6 +499,10 @@
}
}
+// DiffFunction is the type for a function that produces a set of edits that
+// convert from the before content to the after content.
+type DiffFunction func(uri span.URI, before, after string) []diff.TextEdit
+
// Hooks contains configuration that is provided to the Gopls command by the
// main package.
type Hooks struct {
@@ -510,7 +516,7 @@
StaticcheckSupported bool
// ComputeEdits is used to compute edits between file versions.
- ComputeEdits diff.ComputeEdits
+ ComputeEdits DiffFunction
// URLRegexp is used to find potential URLs in comments/strings.
//
diff --git a/gopls/internal/lsp/source/stub.go b/gopls/internal/lsp/source/stub.go
index 9a17802..d0dff4f 100644
--- a/gopls/internal/lsp/source/stub.go
+++ b/gopls/internal/lsp/source/stub.go
@@ -80,12 +80,9 @@
if err != nil {
return nil, fmt.Errorf("format.Node: %w", err)
}
- diffEdits, err := snapshot.View().Options().ComputeEdits(parsedConcreteFile.URI, string(parsedConcreteFile.Src), source.String())
- if err != nil {
- return nil, err
- }
+ diffs := snapshot.View().Options().ComputeEdits(parsedConcreteFile.URI, string(parsedConcreteFile.Src), source.String())
var edits []analysis.TextEdit
- for _, edit := range diffEdits {
+ for _, edit := range diffs {
rng, err := edit.Span.Range(parsedConcreteFile.Mapper.TokFile)
if err != nil {
return nil, err
diff --git a/gopls/internal/lsp/tests/compare/text.go b/gopls/internal/lsp/tests/compare/text.go
index a302396..66d2e62 100644
--- a/gopls/internal/lsp/tests/compare/text.go
+++ b/gopls/internal/lsp/tests/compare/text.go
@@ -5,8 +5,6 @@
package compare
import (
- "fmt"
-
"golang.org/x/tools/internal/diff"
)
@@ -24,17 +22,8 @@
want += "\n"
got += "\n"
- d, err := diff.NComputeEdits("", want, got)
-
- // Panic on errors.
- //
- // TODO(rfindley): refactor so that this function doesn't need to panic.
- // Computing diffs should never fail.
- if err != nil {
- panic(fmt.Sprintf("computing edits failed: %v", err))
- }
-
- diff := diff.ToUnified("want", "got", want, d).String()
+ edits := diff.Strings("irrelevant", want, got)
+ diff := diff.Unified("want", "got", want, edits)
// Defensively assert that we get an actual diff, so that we guarantee the
// invariant that we return "" if and only if want == got.
diff --git a/gopls/internal/lsp/tests/util.go b/gopls/internal/lsp/tests/util.go
index aa9381d..d463ba0 100644
--- a/gopls/internal/lsp/tests/util.go
+++ b/gopls/internal/lsp/tests/util.go
@@ -21,7 +21,6 @@
"golang.org/x/tools/gopls/internal/lsp/source"
"golang.org/x/tools/gopls/internal/lsp/source/completion"
"golang.org/x/tools/internal/diff"
- "golang.org/x/tools/internal/diff/myers"
"golang.org/x/tools/internal/span"
)
@@ -231,11 +230,8 @@
w := want.Signatures[0]
if NormalizeAny(w.Label) != NormalizeAny(g.Label) {
wLabel := w.Label + "\n"
- d, err := myers.ComputeEdits("", wLabel, g.Label+"\n")
- if err != nil {
- return "", err
- }
- return decorate("mismatched labels:\n%q", diff.ToUnified("want", "got", wLabel, d)), err
+ edits := diff.Strings("", wLabel, g.Label+"\n")
+ return decorate("mismatched labels:\n%q", diff.Unified("want", "got", wLabel, edits)), nil
}
var paramParts []string
for _, p := range g.Parameters {
diff --git a/gopls/internal/lsp/work/format.go b/gopls/internal/lsp/work/format.go
index ef5ac46..bc84464 100644
--- a/gopls/internal/lsp/work/format.go
+++ b/gopls/internal/lsp/work/format.go
@@ -8,9 +8,9 @@
"context"
"golang.org/x/mod/modfile"
- "golang.org/x/tools/internal/event"
"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/gopls/internal/lsp/source"
+ "golang.org/x/tools/internal/event"
)
func Format(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle) ([]protocol.TextEdit, error) {
@@ -23,9 +23,6 @@
}
formatted := modfile.Format(pw.File.Syntax)
// Calculate the edits to be made due to the change.
- diff, err := snapshot.View().Options().ComputeEdits(fh.URI(), string(pw.Mapper.Content), string(formatted))
- if err != nil {
- return nil, err
- }
- return source.ToProtocolEdits(pw.Mapper, diff)
+ diffs := snapshot.View().Options().ComputeEdits(fh.URI(), string(pw.Mapper.Content), string(formatted))
+ return source.ToProtocolEdits(pw.Mapper, diffs)
}
diff --git a/internal/diff/diff.go b/internal/diff/diff.go
index 8fd6824..b00ffbc 100644
--- a/internal/diff/diff.go
+++ b/internal/diff/diff.go
@@ -12,6 +12,19 @@
"golang.org/x/tools/internal/span"
)
+// TODO(adonovan): simplify this package to just:
+//
+// package diff
+// type Edit struct { Start, End int; New string }
+// func Strings(old, new string) []Edit
+// func Unified(oldLabel, newLabel string, old string, edits []Edit) string
+// func Apply(old string, edits []Edit) (string, error)
+//
+// and move everything else into gopls, including the concepts of filenames and spans.
+// Observe that TextEdit.URI is irrelevant to Unified.
+// - delete LineEdits? (used only by Unified and test)
+// - delete Lines (unused except by its test)
+
// TextEdit represents a change to a section of a document.
// The text within the specified span should be replaced by the supplied new text.
type TextEdit struct {
@@ -19,10 +32,6 @@
NewText string
}
-// 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, 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
// be reordered.
@@ -37,6 +46,9 @@
// content.
// It may panic or produce garbage if the edits are not valid for the provided
// before content.
+// TODO(adonovan): this function must not panic! Make it either cope
+// or report an error. We should not trust that (e.g.) patches supplied
+// as RPC inputs to gopls are consistent.
func ApplyEdits(before string, edits []TextEdit) string {
// Preconditions:
// - all of the edits apply to before
diff --git a/internal/diff/diff_test.go b/internal/diff/diff_test.go
index b199298..11930de 100644
--- a/internal/diff/diff_test.go
+++ b/internal/diff/diff_test.go
@@ -33,10 +33,7 @@
func TestNEdits(t *testing.T) {
for i, tc := range difftest.TestCases {
sp := fmt.Sprintf("file://%s.%d", tc.Name, i)
- edits, err := diff.NComputeEdits(span.URI(sp), tc.In, tc.Out)
- if err != nil {
- t.Fatal(err)
- }
+ edits := diff.Strings(span.URI(sp), tc.In, tc.Out)
got := diff.ApplyEdits(tc.In, edits)
if got != tc.Out {
t.Fatalf("%s: got %q wanted %q", tc.Name, got, tc.Out)
@@ -53,10 +50,7 @@
fname := fmt.Sprintf("file://%x", i)
a := randstr("abω", 16)
b := randstr("abωc", 16)
- edits, err := diff.NComputeEdits(span.URI(fname), a, b)
- if err != nil {
- t.Fatalf("%q,%q %v", a, b, err)
- }
+ edits := diff.Strings(span.URI(fname), a, b)
got := diff.ApplyEdits(a, edits)
if got != b {
t.Fatalf("%d: got %q, wanted %q, starting with %q", i, got, b, a)
@@ -84,10 +78,7 @@
y = y[:len(y)-1]
}
a, b := strings.SplitAfter(x, "\n"), strings.SplitAfter(y, "\n")
- edits, err := diff.NComputeLineEdits(span.URI(fname), a, b)
- if err != nil {
- t.Fatalf("%q,%q %v", a, b, err)
- }
+ edits := diff.Lines(span.URI(fname), a, b)
got := diff.ApplyEdits(x, edits)
if got != y {
t.Fatalf("%d: got\n%q, wanted\n%q, starting with %q", i, got, y, a)
@@ -113,12 +104,12 @@
func TestUnified(t *testing.T) {
for _, tc := range difftest.TestCases {
t.Run(tc.Name, func(t *testing.T) {
- unified := fmt.Sprint(diff.ToUnified(difftest.FileA, difftest.FileB, tc.In, tc.Edits))
+ unified := diff.Unified(difftest.FileA, difftest.FileB, tc.In, tc.Edits)
if unified != tc.Unified {
t.Errorf("Unified(Edits): got diff:\n%v\nexpected:\n%v", unified, tc.Unified)
}
if tc.LineEdits != nil {
- unified := fmt.Sprint(diff.ToUnified(difftest.FileA, difftest.FileB, tc.In, tc.LineEdits))
+ unified := diff.Unified(difftest.FileA, difftest.FileB, tc.In, tc.LineEdits)
if unified != tc.Unified {
t.Errorf("Unified(LineEdits): got diff:\n%v\nexpected:\n%v", unified, tc.Unified)
}
@@ -131,10 +122,7 @@
a := "// Copyright 2019 The Go Authors. All rights reserved.\n// Use of this source code is governed by a BSD-style\n// license that can be found in the LICENSE file.\n\npackage diff_test\n\nimport (\n\t\"fmt\"\n\t\"math/rand\"\n\t\"strings\"\n\t\"testing\"\n\n\t\"golang.org/x/tools/gopls/internal/lsp/diff\"\n\t\"golang.org/x/tools/internal/diff/difftest\"\n\t\"golang.org/x/tools/internal/span\"\n)\n"
b := "// Copyright 2019 The Go Authors. All rights reserved.\n// Use of this source code is governed by a BSD-style\n// license that can be found in the LICENSE file.\n\npackage diff_test\n\nimport (\n\t\"fmt\"\n\t\"math/rand\"\n\t\"strings\"\n\t\"testing\"\n\n\t\"github.com/google/safehtml/template\"\n\t\"golang.org/x/tools/gopls/internal/lsp/diff\"\n\t\"golang.org/x/tools/internal/diff/difftest\"\n\t\"golang.org/x/tools/internal/span\"\n)\n"
- diffs, err := diff.NComputeEdits(span.URI("file://one"), a, b)
- if err != nil {
- t.Error(err)
- }
+ diffs := diff.Strings(span.URI("file://one"), a, b)
got := diff.ApplyEdits(a, diffs)
if got != b {
i := 0
@@ -148,10 +136,7 @@
func TestRegressionOld002(t *testing.T) {
a := "n\"\n)\n"
b := "n\"\n\t\"golang.org/x//nnal/stack\"\n)\n"
- diffs, err := diff.NComputeEdits(span.URI("file://two"), a, b)
- if err != nil {
- t.Error(err)
- }
+ diffs := diff.Strings(span.URI("file://two"), a, b)
got := diff.ApplyEdits(a, diffs)
if got != b {
i := 0
diff --git a/internal/diff/difftest/difftest.go b/internal/diff/difftest/difftest.go
index e42e0d7..c9808a5 100644
--- a/internal/diff/difftest/difftest.go
+++ b/internal/diff/difftest/difftest.go
@@ -8,7 +8,6 @@
package difftest
import (
- "fmt"
"testing"
"golang.org/x/tools/internal/diff"
@@ -246,15 +245,12 @@
}
}
-func DiffTest(t *testing.T, compute diff.ComputeEdits) {
+func DiffTest(t *testing.T, compute func(uri span.URI, before, after string) []diff.TextEdit) {
for _, test := range TestCases {
t.Run(test.Name, func(t *testing.T) {
- edits, err := compute(span.URIFromPath("/"+test.Name), test.In, test.Out)
- if err != nil {
- t.Fatal(err)
- }
+ edits := compute(span.URIFromPath("/"+test.Name), test.In, test.Out)
got := diff.ApplyEdits(test.In, edits)
- unified := fmt.Sprint(diff.ToUnified(FileA, FileB, test.In, edits))
+ unified := diff.Unified(FileA, FileB, test.In, edits)
if got != test.Out {
t.Errorf("Apply: got patched:\n%v\nfrom diff:\n%v\nexpected:\n%v", got, unified, test.Out)
}
diff --git a/internal/diff/myers/diff.go b/internal/diff/myers/diff.go
index 07e4028..2188c0d 100644
--- a/internal/diff/myers/diff.go
+++ b/internal/diff/myers/diff.go
@@ -16,7 +16,7 @@
// 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, error) {
+func ComputeEdits(uri span.URI, before, after string) []diff.TextEdit {
ops := operations(splitLines(before), splitLines(after))
edits := make([]diff.TextEdit, 0, len(ops))
for _, op := range ops {
@@ -32,7 +32,7 @@
}
}
}
- return edits, nil
+ return edits
}
type operation struct {
diff --git a/internal/diff/ndiff.go b/internal/diff/ndiff.go
index e537b72..e369f46 100644
--- a/internal/diff/ndiff.go
+++ b/internal/diff/ndiff.go
@@ -5,6 +5,8 @@
package diff
import (
+ "go/token"
+ "log"
"strings"
"unicode/utf8"
@@ -16,51 +18,66 @@
// the value is just a guess
const maxDiffs = 30
-// NComputeEdits computes TextEdits for strings
-// (both it and the diff in the myers package have type ComputeEdits, which
+// Strings computes the differences between two strings.
+// (Both it and the diff in the myers package have type ComputeEdits, which
// is why the arguments are strings, not []bytes.)
-func NComputeEdits(uri span.URI, before, after string) ([]TextEdit, error) {
+// TODO(adonovan): opt: consider switching everything to []bytes, if
+// that's the more common type in practice. Or provide both flavors?
+func Strings(uri span.URI, before, after string) []TextEdit {
if before == after {
// very frequently true
- return nil, nil
+ return nil
}
// the diffs returned by the lcs package use indexes into whatever slice
- // was passed in. TextEdits need a span.Span which is computed with
+ // was passed in. TextEdits need a span.Span which is computed with
// byte offsets, so rune or line offsets need to be converted.
+ // TODO(adonovan): opt: eliminate all the unnecessary allocations.
if needrunes(before) || needrunes(after) {
diffs, _ := lcs.Compute([]rune(before), []rune(after), maxDiffs/2)
diffs = runeOffsets(diffs, []rune(before))
- ans, err := convertDiffs(uri, diffs, []byte(before))
- return ans, err
+ return convertDiffs(uri, diffs, []byte(before))
} else {
diffs, _ := lcs.Compute([]byte(before), []byte(after), maxDiffs/2)
- ans, err := convertDiffs(uri, diffs, []byte(before))
- return ans, err
+ return convertDiffs(uri, diffs, []byte(before))
}
}
-// NComputeLineEdits computes TextEdits for []strings
-func NComputeLineEdits(uri span.URI, before, after []string) ([]TextEdit, error) {
+// Lines computes the differences between two list of lines.
+// TODO(adonovan): unused except by its test. Do we actually need it?
+func Lines(uri span.URI, before, after []string) []TextEdit {
diffs, _ := lcs.Compute(before, after, maxDiffs/2)
diffs = lineOffsets(diffs, before)
- ans, err := convertDiffs(uri, diffs, []byte(strJoin(before)))
+ return convertDiffs(uri, diffs, []byte(strJoin(before)))
// the code is not coping with possible missing \ns at the ends
- return ans, err
}
// convert diffs with byte offsets into diffs with line and column
-func convertDiffs(uri span.URI, diffs []lcs.Diff, src []byte) ([]TextEdit, error) {
+func convertDiffs(uri span.URI, diffs []lcs.Diff, src []byte) []TextEdit {
ans := make([]TextEdit, len(diffs))
- tf := span.NewTokenFile(uri.Filename(), src)
- for i, d := range diffs {
- s := newSpan(uri, d.Start, d.End)
- s, err := s.WithPosition(tf)
+
+ // Reuse the machinery of go/token to convert (content, offset) to (line, column).
+ tf := token.NewFileSet().AddFile("", -1, len(src))
+ tf.SetLinesForContent(src)
+
+ offsetToPoint := func(offset int) span.Point {
+ // Re-use span.ToPosition's EOF workaround.
+ // It is infallible if the diffs are consistent with src.
+ line, col, err := span.ToPosition(tf, offset)
if err != nil {
- return nil, err
+ log.Fatalf("invalid offset: %v", err)
}
- ans[i] = TextEdit{s, d.Text}
+ return span.NewPoint(line, col, offset)
}
- return ans, nil
+
+ for i, d := range diffs {
+ start := offsetToPoint(d.Start)
+ end := start
+ if d.End != d.Start {
+ end = offsetToPoint(d.End)
+ }
+ ans[i] = TextEdit{span.New(uri, start, end), d.Text}
+ }
+ return ans
}
// convert diffs with rune offsets into diffs with byte offsets
@@ -114,10 +131,6 @@
return b.String()
}
-func newSpan(uri span.URI, left, right int) span.Span {
- return span.New(uri, span.NewPoint(0, 0, left), span.NewPoint(0, 0, right))
-}
-
// need runes is true if the string needs to be converted to []rune
// for random access
func needrunes(s string) bool {
diff --git a/internal/diff/unified.go b/internal/diff/unified.go
index 3ea0697..13ef677 100644
--- a/internal/diff/unified.go
+++ b/internal/diff/unified.go
@@ -9,28 +9,34 @@
"strings"
)
-// Unified represents a set of edits as a unified diff.
-type Unified struct {
+// Unified applies the edits to oldContent and presents a unified diff.
+// The two labels are the names of the old and new files.
+func Unified(oldLabel, newLabel string, oldContent string, edits []TextEdit) string {
+ return toUnified(oldLabel, newLabel, oldContent, edits).String()
+}
+
+// unified represents a set of edits as a unified diff.
+type unified struct {
// From is the name of the original file.
From string
// To is the name of the modified file.
To string
// Hunks is the set of edit hunks needed to transform the file content.
- Hunks []*Hunk
+ Hunks []*hunk
}
// Hunk represents a contiguous set of line edits to apply.
-type Hunk struct {
+type hunk struct {
// The line in the original source where the hunk starts.
FromLine int
// The line in the original source where the hunk finishes.
ToLine int
// The set of line based edits to apply.
- Lines []Line
+ Lines []line
}
// Line represents a single line operation to apply as part of a Hunk.
-type Line struct {
+type line struct {
// Kind is the type of line this represents, deletion, insertion or copy.
Kind OpKind
// Content is the content of this line.
@@ -40,6 +46,7 @@
}
// OpKind is used to denote the type of operation a line represents.
+// TODO(adonovan): hide this once the myers package no longer references it.
type OpKind int
const (
@@ -73,10 +80,10 @@
gap = edge * 2
)
-// ToUnified takes a file contents and a sequence of edits, and calculates
+// toUnified takes a file contents and a sequence of edits, and calculates
// a unified diff that represents those edits.
-func ToUnified(fromName, toName string, content string, edits []TextEdit) Unified {
- u := Unified{
+func toUnified(fromName, toName string, content string, edits []TextEdit) unified {
+ u := unified{
From: fromName,
To: toName,
}
@@ -88,7 +95,7 @@
edits = lineEdits(content, edits)
}
lines := splitLines(content)
- var h *Hunk
+ var h *hunk
last := 0
toLine := 0
for _, edit := range edits {
@@ -108,7 +115,7 @@
u.Hunks = append(u.Hunks, h)
}
toLine += start - last
- h = &Hunk{
+ h = &hunk{
FromLine: start + 1,
ToLine: toLine + 1,
}
@@ -119,12 +126,12 @@
}
last = start
for i := start; i < end; i++ {
- h.Lines = append(h.Lines, Line{Kind: Delete, Content: lines[i]})
+ h.Lines = append(h.Lines, line{Kind: Delete, Content: lines[i]})
last++
}
if edit.NewText != "" {
- for _, line := range splitLines(edit.NewText) {
- h.Lines = append(h.Lines, Line{Kind: Insert, Content: line})
+ for _, content := range splitLines(edit.NewText) {
+ h.Lines = append(h.Lines, line{Kind: Insert, Content: content})
toLine++
}
}
@@ -145,7 +152,7 @@
return lines
}
-func addEqualLines(h *Hunk, lines []string, start, end int) int {
+func addEqualLines(h *hunk, lines []string, start, end int) int {
delta := 0
for i := start; i < end; i++ {
if i < 0 {
@@ -154,25 +161,15 @@
if i >= len(lines) {
return delta
}
- h.Lines = append(h.Lines, Line{Kind: Equal, Content: lines[i]})
+ h.Lines = append(h.Lines, line{Kind: Equal, Content: lines[i]})
delta++
}
return delta
}
-// Format write a textual representation of u to f (see the String method).
-//
-// TODO(rfindley): investigate (and possibly remove) this method. It's not
-// clear why Unified implements fmt.Formatter, since the formatting rune is not
-// used. Probably it is sufficient to only implement Stringer, but this method
-// was left here defensively.
-func (u Unified) Format(f fmt.State, r rune) {
- fmt.Fprintf(f, "%s", u.String())
-}
-
// String converts a unified diff to the standard textual form for that diff.
// The output of this function can be passed to tools like patch.
-func (u Unified) String() string {
+func (u unified) String() string {
if len(u.Hunks) == 0 {
return ""
}
