gopls/internal/regtest/marker: port half of the suggestedfix markers

Port the fillstruct and self_assignment suggestedfix marker tests.

The fillstruct marker test in particular had incredibly verbose golden
content, which made the tests very difficult to read. To mitigate this,
introduce a new 'codeactionedit' marker which only stores edits in the
golden directory, rather than complete file content. Additionally,
narrow the unified diff to have no edges, for brevity. Since none of the
fillstruct tests require multi-line ranges, use a single location for
the range.

Furthermore, standardize on putting locations before action kind in code
action markers. This is more consistent with other markers.

For golang/go#54845

Change-Id: Id5d713b77fa751bfe8be473b19304376bc3bb139
Reviewed-on: https://go-review.googlesource.com/c/tools/+/539655
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

go/analysis/internal/checker/checker.go

@@ -485,11 +485,11 @@ func diff3Conflict(path string, xlabel, ylabel string, xedits, yedits []diff.Edi
 	}
 	oldlabel, old := "base", string(contents)
 
-	xdiff, err := diff.ToUnified(oldlabel, xlabel, old, xedits)
+	xdiff, err := diff.ToUnified(oldlabel, xlabel, old, xedits, diff.DefaultContextLines)
 	if err != nil {
 		return err
 	}
-	ydiff, err := diff.ToUnified(oldlabel, ylabel, old, yedits)
+	ydiff, err := diff.ToUnified(oldlabel, ylabel, old, yedits, diff.DefaultContextLines)
 	if err != nil {
 		return err
 	}

gopls/internal/lsp/analysis/fillstruct/fillstruct.go

@@ -357,7 +357,7 @@ func populateValue(f *ast.File, pkg *types.Package, typ types.Type) ast.Expr {
 		case u.Kind() == types.UnsafePointer:
 			return ast.NewIdent("nil")
 		default:
-			panic("unknown basic type")
+			panic(fmt.Sprintf("unknown basic type %v", u))
 		}
 
 	case *types.Map:

gopls/internal/lsp/cmd/cmd.go

@@ -610,7 +610,7 @@ func applyTextEdits(mapper *protocol.Mapper, edits []protocol.TextEdit, flags *E
 	}
 
 	if flags.Diff {
-		unified, err := diff.ToUnified(filename+".orig", filename, string(mapper.Content), renameEdits)
+		unified, err := diff.ToUnified(filename+".orig", filename, string(mapper.Content), renameEdits, diff.DefaultContextLines)
 		if err != nil {
 			return err
 		}

gopls/internal/lsp/regtest/marker.go

@@ -36,6 +36,7 @@ import (
 	"golang.org/x/tools/gopls/internal/lsp/source"
 	"golang.org/x/tools/gopls/internal/lsp/tests"
 	"golang.org/x/tools/gopls/internal/lsp/tests/compare"
+	"golang.org/x/tools/internal/diff"
 	"golang.org/x/tools/internal/jsonrpc2"
 	"golang.org/x/tools/internal/jsonrpc2/servertest"
 	"golang.org/x/tools/internal/testenv"
@@ -152,12 +153,17 @@ var update = flag.Bool("update", false, "if set, update test data during marker
 //     completion candidate produced at the given location with provided label
 //     results in the given golden state.
 //
-//   - codeaction(kind, start, end, golden): specifies a codeaction to request
+//   - codeaction(start, end, kind, golden): specifies a code action to request
 //     for the given range. To support multi-line ranges, the range is defined
 //     to be between start.Start and end.End. The golden directory contains
 //     changed file content after the code action is applied.
 //
-//   - codeactionerr(kind, start, end, wantError): specifies a codeaction that
+//   - codeactionedit(range, kind, golden): a shorter form of codeaction.
+//     Invokes a code action of the given kind for the given in-line range, and
+//     compares the resulting formatted unified *edits* (notably, not the full
+//     file content) with the golden directory.
+//
+//   - codeactionerr(start, end, kind, wantError): specifies a codeaction that
 //     fails with an error that matches the expectation.
 //
 //   - codelens(location, title): specifies that a codelens is expected at the
@@ -243,6 +249,9 @@ var update = flag.Bool("update", false, "if set, update test data during marker
 //     to have exactly one associated code action of the specified kind.
 //     This action is executed for its editing effects on the source files.
 //     Like rename, the golden directory contains the expected transformed files.
+//     TODO(rfindley): we probably only need 'suggestedfix' for quick-fixes. All
+//     other actions should use codeaction markers. In that case, we can remove
+//     the 'kind' parameter.
 //
 //   - rank(location, ...completionItem): executes a textDocument/completion
 //     request at the given location, and verifies that each expected
@@ -708,6 +717,7 @@ var valueMarkerFuncs = map[string]func(marker){
 var actionMarkerFuncs = map[string]func(marker){
 	"acceptcompletion": actionMarkerFunc(acceptCompletionMarker),
 	"codeaction":       actionMarkerFunc(codeActionMarker),
+	"codeactionedit":   actionMarkerFunc(codeActionEditMarker),
 	"codeactionerr":    actionMarkerFunc(codeActionErrMarker),
 	"codelenses":       actionMarkerFunc(codeLensesMarker),
 	"complete":         actionMarkerFunc(completeMarker),
@@ -1420,6 +1430,41 @@ func checkChangedFiles(mark marker, changed map[string][]byte, golden *Golden) {
 	}
 }
 
+// checkDiffs computes unified diffs for each changed file, and compares with
+// the diff content stored in the given golden directory.
+func checkDiffs(mark marker, changed map[string][]byte, golden *Golden) {
+	diffs := make(map[string]string)
+	for name, after := range changed {
+		before := mark.run.env.FileContent(name)
+		edits := diff.Strings(before, string(after))
+		d, err := diff.ToUnified("before", "after", before, edits, 0)
+		if err != nil {
+			// Can't happen: edits are consistent.
+			log.Fatalf("internal error in diff.ToUnified: %v", err)
+		}
+		diffs[name] = d
+	}
+	// Check changed files match expectations.
+	for filename, got := range diffs {
+		if want, ok := golden.Get(mark.run.env.T, filename, []byte(got)); !ok {
+			mark.errorf("%s: unexpected change to file %s; got diff:\n%s",
+				mark.note.Name, filename, got)
+
+		} else if got != string(want) {
+			mark.errorf("%s: wrong diff for %s:\n\ngot:\n%s\n\nwant:\n%s\n",
+				mark.note.Name, filename, got, want)
+		}
+	}
+	// Report unmet expectations.
+	for filename := range golden.data {
+		if _, ok := changed[filename]; !ok {
+			want, _ := golden.Get(mark.run.env.T, filename, nil)
+			mark.errorf("%s: missing change to file %s; want:\n%s",
+				mark.note.Name, filename, want)
+		}
+	}
+}
+
 // ---- marker functions ----
 
 // TODO(rfindley): consolidate documentation of these markers. They are already
@@ -1887,7 +1932,7 @@ func applyDocumentChanges(env *Env, changes []protocol.DocumentChanges, fileChan
 	return nil
 }
 
-func codeActionMarker(mark marker, actionKind string, start, end protocol.Location, golden *Golden) {
+func codeActionMarker(mark marker, start, end protocol.Location, actionKind string, g *Golden) {
 	// Request the range from start.Start to end.End.
 	loc := start
 	loc.Range.End = end.Range.End
@@ -1900,10 +1945,20 @@ func codeActionMarker(mark marker, actionKind string, start, end protocol.Locati
 	}
 
 	// Check the file state.
-	checkChangedFiles(mark, changed, golden)
+	checkChangedFiles(mark, changed, g)
 }
 
-func codeActionErrMarker(mark marker, actionKind string, start, end protocol.Location, wantErr wantError) {
+func codeActionEditMarker(mark marker, loc protocol.Location, actionKind string, g *Golden) {
+	changed, err := codeAction(mark.run.env, loc.URI, loc.Range, actionKind, nil)
+	if err != nil {
+		mark.errorf("codeAction failed: %v", err)
+		return
+	}
+
+	checkDiffs(mark, changed, g)
+}
+
+func codeActionErrMarker(mark marker, start, end protocol.Location, actionKind string, wantErr wantError) {
 	loc := start
 	loc.Range.End = end.Range.End
 	_, err := codeAction(mark.run.env, loc.URI, loc.Range, actionKind, nil)
@@ -2008,6 +2063,21 @@ func suggestedfixMarker(mark marker, loc protocol.Location, re *regexp.Regexp, a
 // applied. Currently, this function does not support code actions that return
 // edits directly; it only supports code action commands.
 func codeAction(env *Env, uri protocol.DocumentURI, rng protocol.Range, actionKind string, diag *protocol.Diagnostic) (map[string][]byte, error) {
+	changes, err := codeActionChanges(env, uri, rng, actionKind, diag)
+	if err != nil {
+		return nil, err
+	}
+	fileChanges := make(map[string][]byte)
+	if err := applyDocumentChanges(env, changes, fileChanges); err != nil {
+		return nil, fmt.Errorf("applying document changes: %v", err)
+	}
+	return fileChanges, nil
+}
+
+// codeActionChanges executes a textDocument/codeAction request for the
+// specified location and kind, and captures the resulting document changes.
+// If diag is non-nil, it is used as the code action context.
+func codeActionChanges(env *Env, uri protocol.DocumentURI, rng protocol.Range, actionKind string, diag *protocol.Diagnostic) ([]protocol.DocumentChanges, error) {
 	// Request all code actions that apply to the diagnostic.
 	// (The protocol supports filtering using Context.Only={actionKind}
 	// but we can give a better error if we don't filter.)
@@ -2047,20 +2117,19 @@ func codeAction(env *Env, uri protocol.DocumentURI, rng protocol.Range, actionKi
 	// Spec:
 	//  "If a code action provides an edit and a command, first the edit is
 	//  executed and then the command."
-	fileChanges := make(map[string][]byte)
 	// An action may specify an edit and/or a command, to be
 	// applied in that order. But since applyDocumentChanges(env,
 	// action.Edit.DocumentChanges) doesn't compose, for now we
-	// assert that all commands used in the @suggestedfix tests
-	// return only a command.
+	// assert that actions return one or the other.
 	if action.Edit != nil {
 		if action.Edit.Changes != nil {
 			env.T.Errorf("internal error: discarding unexpected CodeAction{Kind=%s, Title=%q}.Edit.Changes", action.Kind, action.Title)
 		}
 		if action.Edit.DocumentChanges != nil {
-			if err := applyDocumentChanges(env, action.Edit.DocumentChanges, fileChanges); err != nil {
-				return nil, fmt.Errorf("applying document changes: %v", err)
+			if action.Command != nil {
+				env.T.Errorf("internal error: discarding unexpected CodeAction{Kind=%s, Title=%q}.Command", action.Kind, action.Title)
 			}
+			return action.Edit.DocumentChanges, nil
 		}
 	}
 
@@ -2085,13 +2154,10 @@ func codeAction(env *Env, uri protocol.DocumentURI, rng protocol.Range, actionKi
 		}); err != nil {
 			return nil, err
 		}
-
-		if err := applyDocumentChanges(env, env.Awaiter.takeDocumentChanges(), fileChanges); err != nil {
-			return nil, fmt.Errorf("applying document changes from command: %v", err)
-		}
+		return env.Awaiter.takeDocumentChanges(), nil
 	}
 
-	return fileChanges, nil
+	return nil, nil
 }
 
 // TODO(adonovan): suggestedfixerr

gopls/internal/lsp/regtest/wrappers.go

@@ -114,6 +114,18 @@ func (e *Env) SetBufferContent(name string, content string) {
 	}
 }
 
+// ReadFile returns the file content for name that applies to the current
+// editing session: if the file is open, it returns its buffer content,
+// otherwise it returns on disk content.
+func (e *Env) FileContent(name string) string {
+	e.T.Helper()
+	text, ok := e.Editor.BufferText(name)
+	if ok {
+		return text
+	}
+	return e.ReadWorkspaceFile(name)
+}
+
 // RegexpSearch returns the starting position of the first match for re in the
 // buffer specified by name, calling t.Fatal on any error. It first searches
 // for the position in open buffers, then in workspace files.