[tools] gopls: replace all uses of Myers diff

[Peter Weinberger (Gerrit)]

Peter Weinberger has uploaded the change for review

Commit message

gopls: replace all uses of Myers diff

A prvious CL (go.dev/cl/719160) extended the lcs package to do
line by line diffs.

This CL replaces the last uses of Myers' diff algorithm for
line by line diffs with the lcs algorithm.

As in the previous CL, the main difficulty was existing tests. These
tests contained some code to be modified, and the unified diffs
obtained by applying Myers' diff to the test and modified code.
The lcs diff produces a different (shorter) diff, so its unified diffs
are different. In the first CL, instead of checking the exact value
of the unified diffs, it is checked that the correctly convert the test
code into the modified code. In this CL that is still done for all the
marker tests, so the final check ignores differences in txtar files
containting unified diffs.

The final CL in the sequence will remove the Myers' diff code.

Change-Id: Ie0ea25ded46204adae6d2a5546833c2126585e61

Change diff

diff --git a/gopls/internal/test/marker/marker_test.go b/gopls/internal/test/marker/marker_test.go
index 5f48719..8eee497 100644
--- a/gopls/internal/test/marker/marker_test.go
+++ b/gopls/internal/test/marker/marker_test.go
@@ -43,7 +43,6 @@
 	"golang.org/x/tools/gopls/internal/util/bug"
 	"golang.org/x/tools/gopls/internal/util/safetoken"
 	"golang.org/x/tools/internal/diff"
-	"golang.org/x/tools/internal/diff/myers"
 	"golang.org/x/tools/internal/expect"
 	"golang.org/x/tools/internal/jsonrpc2"
 	"golang.org/x/tools/internal/jsonrpc2/servertest"
@@ -323,7 +322,15 @@
 				// report duplicate mismatches of golden data.
 				// Otherwise, verify that formatted content matches.
 				if diff := compare.NamedText("formatted", "on-disk", string(formatted), string(test.content)); diff != "" {
-					t.Errorf("formatted test does not match on-disk content:\n%s", diff)
+					// at this point, the txtar file created by the test and the expected txtar file on the
+					// disk differ. The differences are irrelevant if they are only the contents
+					// of files from a unified diff. These files are dependent on which diff algorithm was used,
+					// so instead the test code has already checked that they correctly express the difference between
+					// the test input and the test output. (If the test code uses the diff that created them, then
+					// they would not differ, but that diff algorithms has been replaced.)
+					if hasSignificantDifference(t, test.content, formatted) {
+						t.Errorf("formatted test does not match on-disk content:\n%s", diff)
+					}
 				}
 			}
 		})
@@ -334,6 +341,45 @@
 	}
 }
 
+// hasSignificatDifference reports whether the txtars in result and disk
+// differ, other than in the contents of golden files containing unified diffs
+func hasSignificantDifference(t *testing.T, result, disk []byte) bool {
+	tcnt := txtar.Parse(result)
+	fcnt := txtar.Parse(disk)
+	if len(tcnt.Files) != len(fcnt.Files) {
+		t.Errorf("mismatched lens test.content:%d formatted:%d", len(tcnt.Files), len(fcnt.Files))
+		// print out the details
+		nms := strings.Builder{}
+		for _, tf := range tcnt.Files {
+			fmt.Fprintf(&nms, "%s tcnt\n", tf.Name)
+		}
+		for _, tf := range fcnt.Files {
+			fmt.Fprintf(&nms, "%s fcnt\n", tf.Name)
+		}
+		x := strings.Split(nms.String(), "\n")
+		sort.Strings(x)
+		for _, s := range x {
+			t.Log(s)
+		}
+		return true
+	} else {
+		for i, tf := range tcnt.Files {
+			// if they are both golden files (name starts with @) and both unified diffs
+			// (both start with @@, then the difference is insignificant)
+			ff := fcnt.Files[i]
+			if strings.HasPrefix(tf.Name, "@") && strings.HasPrefix(ff.Name, "@") && tf.Name == ff.Name &&
+				bytes.HasPrefix(tf.Data, []byte("@@")) && bytes.HasPrefix(ff.Data, []byte("@@")) {
+				continue
+			}
+			if !bytes.Equal(tf.Data, ff.Data) {
+				log.Printf("%s differs", tf.Name)
+				return true
+			}
+		}
+		return false
+	}
+}
+
 // A marker holds state for the execution of a single @marker
 // annotation in the source.
 type marker struct {
@@ -1468,7 +1514,7 @@
 func checkDiffs(mark marker, changed map[string][]byte, golden *Golden) {
 	for name, after := range changed {
 		before := mark.run.env.FileContent(name)
-		edits := myers.ComputeEdits(before, string(after))
+		edits := diff.Lines(before, string(after))
 		d, err := diff.ToUnified("before", "after", before, edits, 0)
 		if err != nil {
 			// Can't happen: edits are consistent.
diff --git a/internal/diff/myers/diff_test.go b/internal/diff/myers/diff_test.go
index f244455..53ee8e8 100644
--- a/internal/diff/myers/diff_test.go
+++ b/internal/diff/myers/diff_test.go
@@ -7,10 +7,10 @@
 import (
 	"testing"
 
+	"golang.org/x/tools/internal/diff"
 	"golang.org/x/tools/internal/diff/difftest"
-	"golang.org/x/tools/internal/diff/myers"
 )
 
 func TestDiff(t *testing.T) {
-	difftest.DiffTest(t, myers.ComputeEdits)
+	difftest.DiffTest(t, diff.Lines)
 }
diff --git a/internal/drivertest/driver_test.go b/internal/drivertest/driver_test.go
index e1b170e..6a4d781 100644
--- a/internal/drivertest/driver_test.go
+++ b/internal/drivertest/driver_test.go
@@ -14,7 +14,6 @@
 
 	"golang.org/x/tools/go/packages"
 	"golang.org/x/tools/internal/diff"
-	"golang.org/x/tools/internal/diff/myers"
 	"golang.org/x/tools/internal/drivertest"
 	"golang.org/x/tools/internal/packagesinternal"
 	"golang.org/x/tools/internal/testenv"
@@ -139,8 +138,7 @@
 			listJSON := jsons[0]
 			driverJSON := jsons[1]
 
-			// Use the myers package for better line diffs.
-			edits := myers.ComputeEdits(listJSON, driverJSON)
+			edits := diff.Lines(listJSON, driverJSON)
 			d, err := diff.ToUnified("go list", "driver", listJSON, edits, 0)
 			if err != nil {
 				t.Fatal(err)

Change information

Files:

• M gopls/internal/test/marker/marker_test.go
• M internal/diff/myers/diff_test.go
• M internal/drivertest/driver_test.go

Change size: M

Delta: 3 files changed, 52 insertions(+), 8 deletions(-)

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: newchange

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: Ie0ea25ded46204adae6d2a5546833c2126585e61

Gerrit-Change-Number: 736766

Gerrit-PatchSet: 1

Gerrit-Owner: Peter Weinberger <p...@google.com>

Gerrit-Reviewer: Peter Weinberger <p...@google.com>