gopls/internal/lsp/regtest: add @suggestedfix marker

This change adds a suggestedfix marker type similar to the one
in the LSP tests. Like @diag, it creates an expectation of a
diagnostic, which it then consumes; but it goes on to apply
the suggested fix of the specified kind and assert (like a
@rename marker) that the files were edited correctly.

Porting the existing test cases (and adding suggestedfixerr) will
be done in a follow-up.

Also:
- define an OnApplyEdit client hook that intercepts server downcalls.
  The marker test uses this to capture edits that occur as a side
  effect of an RPC (such as the ExecuteCommand of a suggestedfix)
  rather than the result of an RPC (such as rename).
  The existing main regtest runner does not use the hook.
  (Can we avoid this subtlety?)
- fix two latent "loopclosure" bugs

Change-Id: I5b352f8108f630b4be46aa1d25fd9a88938448ad
Reviewed-on: https://go-review.googlesource.com/c/tools/+/470677
Reviewed-by: Robert Findley <[email protected]>
Run-TryBot: Alan Donovan <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
gopls-CI: kokoro <[email protected]>

gopls/internal/lsp/code_action.go

@@ -418,17 +418,7 @@ func codeActionsForDiagnostic(ctx context.Context, snapshot source.Snapshot, sd
 			if err != nil {
 				return nil, err
 			}
-			changes = append(changes, protocol.DocumentChanges{
-				TextDocumentEdit: &protocol.TextDocumentEdit{
-					TextDocument: protocol.OptionalVersionedTextDocumentIdentifier{
-						Version: fh.Version(),
-						TextDocumentIdentifier: protocol.TextDocumentIdentifier{
-							URI: protocol.URIFromSpanURI(fh.URI()),
-						},
-					},
-					Edits: edits,
-				},
-			})
+			changes = append(changes, documentChanges(fh, edits)...)
 		}
 		action := protocol.CodeAction{
 			Title: fix.Title,

gopls/internal/lsp/command.go

@@ -157,6 +157,7 @@ func (c *commandHandler) ApplyFix(ctx context.Context, args command.ApplyFixArgs
 		}
 		var changes []protocol.DocumentChanges
 		for _, edit := range edits {
+			edit := edit
 			changes = append(changes, protocol.DocumentChanges{
 				TextDocumentEdit: &edit,
 			})
@@ -588,6 +589,7 @@ func (s *Server) runGoModUpdateCommands(ctx context.Context, snapshot source.Sna
 	}
 	var documentChanges []protocol.DocumentChanges
 	for _, change := range changes {
+		change := change
 		documentChanges = append(documentChanges, protocol.DocumentChanges{
 			TextDocumentEdit: &change,
 		})

gopls/internal/lsp/fake/client.go

@@ -13,23 +13,28 @@ import (
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
 )
 
-// ClientHooks are called to handle the corresponding client LSP method.
+// ClientHooks are a set of optional hooks called during handling of
+// the corresponding client method (see protocol.Client for the the
+// LSP server-to-client RPCs) in order to make test expectations
+// awaitable.
 type ClientHooks struct {
 	OnLogMessage             func(context.Context, *protocol.LogMessageParams) error
 	OnDiagnostics            func(context.Context, *protocol.PublishDiagnosticsParams) error
 	OnWorkDoneProgressCreate func(context.Context, *protocol.WorkDoneProgressCreateParams) error
 	OnProgress               func(context.Context, *protocol.ProgressParams) error
 	OnShowMessage            func(context.Context, *protocol.ShowMessageParams) error
 	OnShowMessageRequest     func(context.Context, *protocol.ShowMessageRequestParams) error
-	OnRegistration           func(context.Context, *protocol.RegistrationParams) error
-	OnUnregistration         func(context.Context, *protocol.UnregistrationParams) error
+	OnRegisterCapability     func(context.Context, *protocol.RegistrationParams) error
+	OnUnregisterCapability   func(context.Context, *protocol.UnregistrationParams) error
+	OnApplyEdit              func(context.Context, *protocol.ApplyWorkspaceEditParams) error
 }
 
-// Client is an adapter that converts an *Editor into an LSP Client. It mosly
+// Client is an adapter that converts an *Editor into an LSP Client. It mostly
 // delegates functionality to hooks that can be configured by tests.
 type Client struct {
-	editor *Editor
-	hooks  ClientHooks
+	editor         *Editor
+	hooks          ClientHooks
+	skipApplyEdits bool // don't apply edits from ApplyEdit downcalls to Editor
 }
 
 func (c *Client) CodeLensRefresh(context.Context) error { return nil }
@@ -98,8 +103,8 @@ func (c *Client) Configuration(_ context.Context, p *protocol.ParamConfiguration
 }
 
 func (c *Client) RegisterCapability(ctx context.Context, params *protocol.RegistrationParams) error {
-	if c.hooks.OnRegistration != nil {
-		if err := c.hooks.OnRegistration(ctx, params); err != nil {
+	if c.hooks.OnRegisterCapability != nil {
+		if err := c.hooks.OnRegisterCapability(ctx, params); err != nil {
 			return err
 		}
 	}
@@ -138,8 +143,8 @@ func (c *Client) RegisterCapability(ctx context.Context, params *protocol.Regist
 }
 
 func (c *Client) UnregisterCapability(ctx context.Context, params *protocol.UnregistrationParams) error {
-	if c.hooks.OnUnregistration != nil {
-		return c.hooks.OnUnregistration(ctx, params)
+	if c.hooks.OnUnregisterCapability != nil {
+		return c.hooks.OnUnregisterCapability(ctx, params)
 	}
 	return nil
 }
@@ -162,15 +167,21 @@ func (c *Client) ShowDocument(context.Context, *protocol.ShowDocumentParams) (*p
 	return nil, nil
 }
 
-// ApplyEdit applies edits sent from the server.
 func (c *Client) ApplyEdit(ctx context.Context, params *protocol.ApplyWorkspaceEditParams) (*protocol.ApplyWorkspaceEditResult, error) {
 	if len(params.Edit.Changes) != 0 {
 		return &protocol.ApplyWorkspaceEditResult{FailureReason: "Edit.Changes is unsupported"}, nil
 	}
-	for _, change := range params.Edit.DocumentChanges {
-		if err := c.editor.applyDocumentChange(ctx, change); err != nil {
+	if c.hooks.OnApplyEdit != nil {
+		if err := c.hooks.OnApplyEdit(ctx, params); err != nil {
 			return nil, err
 		}
 	}
+	if !c.skipApplyEdits {
+		for _, change := range params.Edit.DocumentChanges {
+			if err := c.editor.applyDocumentChange(ctx, change); err != nil {
+				return nil, err
+			}
+		}
+	}
 	return &protocol.ApplyWorkspaceEditResult{Applied: true}, nil
 }

gopls/internal/lsp/fake/editor.go

@@ -123,14 +123,14 @@ func NewEditor(sandbox *Sandbox, config EditorConfig) *Editor {
 // It returns the editor, so that it may be called as follows:
 //
 //	editor, err := NewEditor(s).Connect(ctx, conn, hooks)
-func (e *Editor) Connect(ctx context.Context, connector servertest.Connector, hooks ClientHooks) (*Editor, error) {
+func (e *Editor) Connect(ctx context.Context, connector servertest.Connector, hooks ClientHooks, skipApplyEdits bool) (*Editor, error) {
 	bgCtx, cancelConn := context.WithCancel(xcontext.Detach(ctx))
 	conn := connector.Connect(bgCtx)
 	e.cancelConn = cancelConn
 
 	e.serverConn = conn
 	e.Server = protocol.ServerDispatcher(conn)
-	e.client = &Client{editor: e, hooks: hooks}
+	e.client = &Client{editor: e, hooks: hooks, skipApplyEdits: skipApplyEdits}
 	conn.Go(bgCtx,
 		protocol.Handlers(
 			protocol.ClientHandler(e.client,

gopls/internal/lsp/lsprpc/lsprpc_test.go

@@ -225,12 +225,13 @@ func TestDebugInfoLifecycle(t *testing.T) {
 	}
 	tsForwarder := servertest.NewPipeServer(forwarder, nil)
 
-	ed1, err := fake.NewEditor(sb, fake.EditorConfig{}).Connect(clientCtx, tsForwarder, fake.ClientHooks{})
+	const skipApplyEdits = false
+	ed1, err := fake.NewEditor(sb, fake.EditorConfig{}).Connect(clientCtx, tsForwarder, fake.ClientHooks{}, skipApplyEdits)
 	if err != nil {
 		t.Fatal(err)
 	}
 	defer ed1.Close(clientCtx)
-	ed2, err := fake.NewEditor(sb, fake.EditorConfig{}).Connect(baseCtx, tsBackend, fake.ClientHooks{})
+	ed2, err := fake.NewEditor(sb, fake.EditorConfig{}).Connect(baseCtx, tsBackend, fake.ClientHooks{}, skipApplyEdits)
 	if err != nil {
 		t.Fatal(err)
 	}

gopls/internal/lsp/regtest/env.go

@@ -62,6 +62,7 @@ func NewAwaiter(workdir *fake.Workdir) *Awaiter {
 	}
 }
 
+// Hooks returns LSP client hooks required for awaiting asynchronous expectations.
 func (a *Awaiter) Hooks() fake.ClientHooks {
 	return fake.ClientHooks{
 		OnDiagnostics:            a.onDiagnostics,
@@ -70,8 +71,9 @@ func (a *Awaiter) Hooks() fake.ClientHooks {
 		OnProgress:               a.onProgress,
 		OnShowMessage:            a.onShowMessage,
 		OnShowMessageRequest:     a.onShowMessageRequest,
-		OnRegistration:           a.onRegistration,
-		OnUnregistration:         a.onUnregistration,
+		OnRegisterCapability:     a.onRegisterCapability,
+		OnUnregisterCapability:   a.onUnregisterCapability,
+		OnApplyEdit:              a.onApplyEdit,
 	}
 }
 
@@ -86,6 +88,7 @@ type State struct {
 	registrations          []*protocol.RegistrationParams
 	registeredCapabilities map[string]protocol.Registration
 	unregistrations        []*protocol.UnregistrationParams
+	documentChanges        []protocol.DocumentChanges // collected from ApplyEdit downcalls
 
 	// outstandingWork is a map of token->work summary. All tokens are assumed to
 	// be string, though the spec allows for numeric tokens as well.  When work
@@ -179,6 +182,15 @@ type condition struct {
 	verdict      chan Verdict
 }
 
+func (a *Awaiter) onApplyEdit(_ context.Context, params *protocol.ApplyWorkspaceEditParams) error {
+	a.mu.Lock()
+	defer a.mu.Unlock()
+
+	a.state.documentChanges = append(a.state.documentChanges, params.Edit.DocumentChanges...)
+	a.checkConditionsLocked()
+	return nil
+}
+
 func (a *Awaiter) onDiagnostics(_ context.Context, d *protocol.PublishDiagnosticsParams) error {
 	a.mu.Lock()
 	defer a.mu.Unlock()
@@ -255,7 +267,7 @@ func (a *Awaiter) onProgress(_ context.Context, m *protocol.ProgressParams) erro
 	return nil
 }
 
-func (a *Awaiter) onRegistration(_ context.Context, m *protocol.RegistrationParams) error {
+func (a *Awaiter) onRegisterCapability(_ context.Context, m *protocol.RegistrationParams) error {
 	a.mu.Lock()
 	defer a.mu.Unlock()
 
@@ -270,7 +282,7 @@ func (a *Awaiter) onRegistration(_ context.Context, m *protocol.RegistrationPara
 	return nil
 }
 
-func (a *Awaiter) onUnregistration(_ context.Context, m *protocol.UnregistrationParams) error {
+func (a *Awaiter) onUnregisterCapability(_ context.Context, m *protocol.UnregistrationParams) error {
 	a.mu.Lock()
 	defer a.mu.Unlock()
 
@@ -288,6 +300,17 @@ func (a *Awaiter) checkConditionsLocked() {
 	}
 }
 
+// takeDocumentChanges returns any accumulated document changes (from
+// server ApplyEdit RPC downcalls) and resets the list.
+func (a *Awaiter) takeDocumentChanges() []protocol.DocumentChanges {
+	a.mu.Lock()
+	defer a.mu.Unlock()
+
+	res := a.state.documentChanges
+	a.state.documentChanges = nil
+	return res
+}
+
 // checkExpectations reports whether s meets all expectations.
 func checkExpectations(s State, expectations []Expectation) (Verdict, string) {
 	finalVerdict := Met