gopls/internal: remove myers diff implementation from gopls

CL 548175 removed the sergey/diffmatchpatch diff implementation,
which had been disabled by default for some time.

This change removes the dependency from gopls to the myers
implementation, and the associated configuration plumbing
for alternative implementations. The myers implementation
was not actually used because the GoDiff=true default
in Hooks (see default.go) was always overridden.

Unfortunately we cannot yet delete the myers implementation
completely because the marker tests depend on the details
of its behavior. A follow-up change should apply the diff
and compare the result, instead of comparing diffs.

Fixes golang/go#52967

Change-Id: I67796e260ac00f7edc31ce18fda7b1042e8374f8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/548856
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>

Author: adonovan

gopls/internal/hooks/hooks.go

@@ -9,15 +9,11 @@ package hooks // import "golang.org/x/tools/gopls/internal/hooks"
 
 import (
 	"golang.org/x/tools/gopls/internal/settings"
-	"golang.org/x/tools/internal/diff"
 	"mvdan.cc/xurls/v2"
 )
 
 func Options(options *settings.Options) {
 	options.LicensesText = licensesText
-	if options.GoDiff {
-			options.ComputeEdits = diff.Strings
-	}
 	options.URLRegexp = xurls.Relaxed()
 	updateAnalyzers(options)
 	updateGofumpt(options)

gopls/internal/lsp/cache/mod_tidy.go

@@ -19,7 +19,7 @@ import (
 	"golang.org/x/tools/gopls/internal/file"
 	"golang.org/x/tools/gopls/internal/lsp/command"
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
-	"golang.org/x/tools/gopls/internal/settings"
+	"golang.org/x/tools/internal/diff"
 	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/event/tag"
 	"golang.org/x/tools/internal/gocommand"
@@ -180,7 +180,7 @@ func modTidyDiagnostics(ctx context.Context, snapshot *Snapshot, pm *ParsedModul
 	for _, req := range wrongDirectness {
 		// Handle dependencies that are incorrectly labeled indirect and
 		// vice versa.
-		srcDiag, err := directnessDiagnostic(pm.Mapper, req, snapshot.Options().ComputeEdits)
+		srcDiag, err := directnessDiagnostic(pm.Mapper, req)
 		if err != nil {
 			// We're probably in a bad state if we can't compute a
 			// directnessDiagnostic, but try to keep going so as to not suppress
@@ -357,7 +357,7 @@ func unusedDiagnostic(m *protocol.Mapper, req *modfile.Require, onlyDiagnostic b
 
 // directnessDiagnostic extracts errors when a dependency is labeled indirect when
 // it should be direct and vice versa.
-func directnessDiagnostic(m *protocol.Mapper, req *modfile.Require, computeEdits settings.DiffFunction) (*Diagnostic, error) {
+func directnessDiagnostic(m *protocol.Mapper, req *modfile.Require) (*Diagnostic, error) {
 	rng, err := m.OffsetRange(req.Syntax.Start.Byte, req.Syntax.End.Byte)
 	if err != nil {
 		return nil, err
@@ -378,7 +378,7 @@ func directnessDiagnostic(m *protocol.Mapper, req *modfile.Require, computeEdits
 		}
 	}
 	// If the dependency should be indirect, add the // indirect.
-	edits, err := switchDirectness(req, m, computeEdits)
+	edits, err := switchDirectness(req, m)
 	if err != nil {
 		return nil, err
 	}
@@ -430,7 +430,7 @@ func missingModuleDiagnostic(pm *ParsedModule, req *modfile.Require) (*Diagnosti
 
 // switchDirectness gets the edits needed to change an indirect dependency to
 // direct and vice versa.
-func switchDirectness(req *modfile.Require, m *protocol.Mapper, computeEdits settings.DiffFunction) ([]protocol.TextEdit, error) {
+func switchDirectness(req *modfile.Require, m *protocol.Mapper) ([]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)
@@ -464,7 +464,7 @@ func switchDirectness(req *modfile.Require, m *protocol.Mapper, computeEdits set
 		return nil, err
 	}
 	// Calculate the edits to be made due to the change.
-	edits := computeEdits(string(m.Content), string(newContent))
+	edits := diff.Bytes(m.Content, newContent)
 	return protocol.EditsFromDiffEdits(m, edits)
 }
 

gopls/internal/lsp/source/format.go

@@ -196,7 +196,7 @@ func computeFixEdits(snapshot *cache.Snapshot, pgf *ParsedGoFile, options *impor
 	if fixedData == nil || fixedData[len(fixedData)-1] != '\n' {
 		fixedData = append(fixedData, '\n') // ApplyFixes may miss the newline, go figure.
 	}
-	edits := snapshot.Options().ComputeEdits(left, string(fixedData))
+	edits := diff.Strings(left, string(fixedData))
 	return protocolEditsFromSource([]byte(left), edits)
 }
 
@@ -306,7 +306,7 @@ func computeTextEdits(ctx context.Context, snapshot *cache.Snapshot, pgf *Parsed
 	_, done := event.Start(ctx, "source.computeTextEdits")
 	defer done()
 
-	edits := snapshot.Options().ComputeEdits(string(pgf.Src), formatted)
+	edits := diff.Strings(string(pgf.Src), formatted)
 	return protocol.EditsFromDiffEdits(pgf.Mapper, edits)
 }
 

gopls/internal/lsp/source/rename.go

@@ -704,7 +704,7 @@ func renamePackageName(ctx context.Context, s *cache.Snapshot, f file.Handle, ne
 		}
 
 		// Calculate the edits to be made due to the change.
-		edits := s.Options().ComputeEdits(string(pm.Mapper.Content), string(newContent))
+		edits := diff.Bytes(pm.Mapper.Content, newContent)
 		renamingEdits[pm.URI] = append(renamingEdits[pm.URI], edits...)
 	}
 

gopls/internal/lsp/source/stub.go

@@ -231,13 +231,13 @@ func (%s%s%s) %s%s {
 	}
 
 	// Pretty-print.
-	var output strings.Builder
+	var output bytes.Buffer
 	if err := format.Node(&output, fset, newF); err != nil {
 		return nil, nil, fmt.Errorf("format.Node: %w", err)
 	}
 
 	// Report the diff.
-	diffs := snapshot.Options().ComputeEdits(string(input), output.String())
+	diffs := diff.Bytes(input, output.Bytes())
 	return tokeninternal.FileSetFor(declPGF.Tok), // edits use declPGF.Tok
 		&analysis.SuggestedFix{TextEdits: diffToTextEdits(declPGF.Tok, diffs)},
 		nil

gopls/internal/mod/format.go

@@ -10,6 +10,7 @@ import (
 	"golang.org/x/tools/gopls/internal/file"
 	"golang.org/x/tools/gopls/internal/lsp/cache"
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
+	"golang.org/x/tools/internal/diff"
 	"golang.org/x/tools/internal/event"
 )
 
@@ -26,6 +27,6 @@ func Format(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle) ([]pr
 		return nil, err
 	}
 	// Calculate the edits to be made due to the change.
-	diffs := snapshot.Options().ComputeEdits(string(pm.Mapper.Content), string(formatted))
+	diffs := diff.Bytes(pm.Mapper.Content, formatted)
 	return protocol.EditsFromDiffEdits(pm.Mapper, diffs)
 }

gopls/internal/server/command.go

@@ -37,6 +37,7 @@ import (
 	"golang.org/x/tools/gopls/internal/util/maps"
 	"golang.org/x/tools/gopls/internal/vulncheck"
 	"golang.org/x/tools/gopls/internal/vulncheck/scan"
+	"golang.org/x/tools/internal/diff"
 	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/gocommand"
 	"golang.org/x/tools/internal/tokeninternal"
@@ -490,7 +491,7 @@ func dropDependency(snapshot *cache.Snapshot, pm *cache.ParsedModule, modulePath
 		return nil, err
 	}
 	// Calculate the edits to be made due to the change.
-	diff := snapshot.Options().ComputeEdits(string(pm.Mapper.Content), string(newContent))
+	diff := diff.Bytes(pm.Mapper.Content, newContent)
 	return protocol.EditsFromDiffEdits(pm.Mapper, diff)
 }
 
@@ -693,7 +694,7 @@ func collectFileEdits(ctx context.Context, snapshot *cache.Snapshot, uri protoco
 	}
 
 	m := protocol.NewMapper(fh.URI(), oldContent)
-	diff := snapshot.Options().ComputeEdits(string(oldContent), string(newContent))
+	diff := diff.Bytes(oldContent, newContent)
 	edits, err := protocol.EditsFromDiffEdits(m, diff)
 	if err != nil {
 		return nil, err

gopls/internal/settings/default.go

@@ -11,7 +11,6 @@ import (
 	"golang.org/x/tools/gopls/internal/file"
 	"golang.org/x/tools/gopls/internal/lsp/command"
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
-	"golang.org/x/tools/internal/diff/myers"
 )
 
 var (
@@ -118,14 +117,11 @@ func DefaultOptions(overrides ...func(*Options)) *Options {
 				LinkifyShowMessage:          false,
 			},
 			Hooks: Hooks{
-				// TODO(adonovan): switch to new diff.Strings implementation.
-				ComputeEdits:         myers.ComputeEdits,
 				URLRegexp:            urlRegexp(),
 				DefaultAnalyzers:     defaultAnalyzers(),
 				TypeErrorAnalyzers:   typeErrorAnalyzers(),
 				ConvenienceAnalyzers: convenienceAnalyzers(),
 				StaticcheckAnalyzers: map[string]*Analyzer{},
-				GoDiff:               true,
 			},
 		}
 	})

gopls/internal/settings/settings.go

@@ -69,7 +69,6 @@ import (
 	"golang.org/x/tools/gopls/internal/file"
 	"golang.org/x/tools/gopls/internal/lsp/command"
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
-	"golang.org/x/tools/internal/diff"
 )
 
 type Annotation string
@@ -434,25 +433,15 @@ func (u *UserOptions) SetEnvSlice(env []string) {
 	}
 }
 
-// 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(before, after string) []diff.Edit
-
 // Hooks contains configuration that is provided to the Gopls command by the
 // main package.
 type Hooks struct {
 	// LicensesText holds third party licenses for software used by gopls.
 	LicensesText string
 
-	// GoDiff is used in gopls/hooks to get Myers' diff
-	GoDiff bool
-
 	// Whether staticcheck is supported.
 	StaticcheckSupported bool
 
-	// ComputeEdits is used to compute edits between file versions.
-	ComputeEdits DiffFunction
-
 	// URLRegexp is used to find potential URLs in comments/strings.
 	//
 	// Not all matches are shown to the user: if the matched URL is not detected
@@ -766,9 +755,7 @@ func (o *Options) Clone() *Options {
 		ClientOptions:   o.ClientOptions,
 		InternalOptions: o.InternalOptions,
 		Hooks: Hooks{
-			GoDiff:               o.GoDiff,
 			StaticcheckSupported: o.StaticcheckSupported,
-			ComputeEdits:         o.ComputeEdits,
 			GofumptFormat:        o.GofumptFormat,
 			URLRegexp:            o.URLRegexp,
 		},
@@ -1137,6 +1124,9 @@ func (o *Options) set(name string, value interface{}, seen map[string]struct{})
 		// This setting should be handled before all of the other options are
 		// processed, so do nothing here.
 
+	case "newDiff":
+		result.deprecated("")
+
 	case "subdirWatchPatterns":
 		if s, ok := result.asOneOf(
 			string(SubdirWatchPatternsOn),

gopls/internal/test/marker/marker_test.go

@@ -1149,6 +1149,15 @@ func checkDiffs(mark marker, changed map[string][]byte, golden *Golden) {
 	for name, after := range changed {
 		before := mark.run.env.FileContent(name)
 		// TODO(golang/go#64023): switch back to diff.Strings.
+		// The attached issue is only one obstacle to switching.
+		// Another is that different diff algorithms produce
+		// different results, so if we commit diffs in test
+		// expectations, then we need to either (1) state
+		// which diff implementation they use and never change
+		// it, or (2) don't compare diffs, but instead apply
+		// the "want" diff and check that it produces the
+		// "got" output. Option 2 is more robust, as it allows
+		// the test expectation to use any valid diff.
 		edits := myers.ComputeEdits(before, string(after))
 		d, err := diff.ToUnified("before", "after", before, edits, 0)
 		if err != nil {

gopls/internal/test/marker/testdata/suggestedfix/missingfunction.txt

@@ -1,10 +1,5 @@
 This test checks the quick fix for undefined functions.
 
--- settings.json --
-{
-	"newDiff": "ol"
-}
-
 -- channels.go --
 package missingfunction
 

gopls/internal/work/format.go

@@ -11,6 +11,7 @@ import (
 	"golang.org/x/tools/gopls/internal/file"
 	"golang.org/x/tools/gopls/internal/lsp/cache"
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
+	"golang.org/x/tools/internal/diff"
 	"golang.org/x/tools/internal/event"
 )
 
@@ -24,6 +25,6 @@ func Format(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle) ([]pr
 	}
 	formatted := modfile.Format(pw.File.Syntax)
 	// Calculate the edits to be made due to the change.
-	diffs := snapshot.Options().ComputeEdits(string(pw.Mapper.Content), string(formatted))
+	diffs := diff.Bytes(pw.Mapper.Content, formatted)
 	return protocol.EditsFromDiffEdits(pw.Mapper, diffs)
 }

internal/diff/myers/diff.go

@@ -15,6 +15,15 @@ import (
 // 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
 
+// ComputeEdits returns the diffs of two strings using a simple
+// line-based implementation, like [diff.Strings].
+//
+// Deprecated: this implementation is moribund. However, when diffs
+// appear in marker test expectations, they are the particular diffs
+// produced by this implementation. The marker test framework
+// asserts diff(orig, got)==wantDiff, but ideally it would compute
+// got==apply(orig, wantDiff) so that the notation of the diff
+// is immaterial.
 func ComputeEdits(before, after string) []diff.Edit {
 	beforeLines := splitLines(before)
 	ops := operations(beforeLines, splitLines(after))