internal/lsp/fake: fix some messiness around client hooks

While writing the fake editor, I added some state tracking without using
it (log messages, events etc). We have since duplicated this logic in
the regtest package using client hooks.

Fix two messy aspects of this:
 - remove the state tracking in the editor
 - pass in the client hooks when connecting, so that they may be used
   without locking, and so that we do not miss any hooks that may fire
   during session initialization.

Change-Id: I24c17a28e9cfa4fca32b7ddd17c7bf01cbb12e0f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/232744
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

internal/lsp/fake/client.go

@@ -10,59 +10,25 @@ import (
 	"golang.org/x/tools/internal/lsp/protocol"
 )
 
+// ClientHooks are called to handle the corresponding client LSP method.
+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
+}
+
 // Client is an adapter that converts an *Editor into an LSP Client. It mosly
 // delegates functionality to hooks that can be configured by tests.
 type Client struct {
-	*Editor
+	editor *Editor
-
+	hooks  ClientHooks
-	// Hooks for testing. Add additional hooks here as needed for testing.
-	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
-}
-
-// OnShowMessage sets the hook to run when the editor receives a showMessage notification
-func (c *Client) OnShowMessage(hook func(context.Context, *protocol.ShowMessageParams) error) {
-	c.mu.Lock()
-	c.onShowMessage = hook
-	c.mu.Unlock()
-}
-
-// OnLogMessage sets the hook to run when the editor receives a log message.
-func (c *Client) OnLogMessage(hook func(context.Context, *protocol.LogMessageParams) error) {
-	c.mu.Lock()
-	c.onLogMessage = hook
-	c.mu.Unlock()
-}
-
-// OnDiagnostics sets the hook to run when the editor receives diagnostics
-// published from the language server.
-func (c *Client) OnDiagnostics(hook func(context.Context, *protocol.PublishDiagnosticsParams) error) {
-	c.mu.Lock()
-	c.onDiagnostics = hook
-	c.mu.Unlock()
-}
-
-func (c *Client) OnWorkDoneProgressCreate(hook func(context.Context, *protocol.WorkDoneProgressCreateParams) error) {
-	c.mu.Lock()
-	c.onWorkDoneProgressCreate = hook
-	c.mu.Unlock()
-}
-
-func (c *Client) OnProgress(hook func(context.Context, *protocol.ProgressParams) error) {
-	c.mu.Lock()
-	c.onProgress = hook
-	c.mu.Unlock()
 }
 
 func (c *Client) ShowMessage(ctx context.Context, params *protocol.ShowMessageParams) error {
-	c.mu.Lock()
+	if c.hooks.OnShowMessage != nil {
-	c.lastMessage = params
+		return c.hooks.OnShowMessage(ctx, params)
-	c.mu.Unlock()
-	if c.onShowMessage != nil {
-		return c.onShowMessage(ctx, params)
 	}
 	return nil
 }
@@ -72,30 +38,19 @@ func (c *Client) ShowMessageRequest(ctx context.Context, params *protocol.ShowMe
 }
 
 func (c *Client) LogMessage(ctx context.Context, params *protocol.LogMessageParams) error {
-	c.mu.Lock()
+	if c.hooks.OnLogMessage != nil {
-	c.logs = append(c.logs, params)
+		return c.hooks.OnLogMessage(ctx, params)
-	onLogMessage := c.onLogMessage
-	c.mu.Unlock()
-	if onLogMessage != nil {
-		return onLogMessage(ctx, params)
 	}
 	return nil
 }
 
 func (c *Client) Event(ctx context.Context, event *interface{}) error {
-	c.mu.Lock()
-	c.events = append(c.events, event)
-	c.mu.Unlock()
 	return nil
 }
 
 func (c *Client) PublishDiagnostics(ctx context.Context, params *protocol.PublishDiagnosticsParams) error {
-	c.mu.Lock()
+	if c.hooks.OnDiagnostics != nil {
-	c.diagnostics = params
+		return c.hooks.OnDiagnostics(ctx, params)
-	onPublishDiagnostics := c.onDiagnostics
-	c.mu.Unlock()
-	if onPublishDiagnostics != nil {
-		return onPublishDiagnostics(ctx, params)
 	}
 	return nil
 }
@@ -110,7 +65,7 @@ func (c *Client) Configuration(_ context.Context, p *protocol.ParamConfiguration
 		if item.Section != "gopls" {
 			continue
 		}
-		results[i] = c.configuration()
+		results[i] = c.editor.configuration()
 	}
 	return results, nil
 }
@@ -124,21 +79,15 @@ func (c *Client) UnregisterCapability(context.Context, *protocol.UnregistrationP
 }
 
 func (c *Client) Progress(ctx context.Context, params *protocol.ProgressParams) error {
-	c.mu.Lock()
+	if c.hooks.OnProgress != nil {
-	onProgress := c.onProgress
+		return c.hooks.OnProgress(ctx, params)
-	c.mu.Unlock()
-	if onProgress != nil {
-		return onProgress(ctx, params)
 	}
 	return nil
 }
 
 func (c *Client) WorkDoneProgressCreate(ctx context.Context, params *protocol.WorkDoneProgressCreateParams) error {
-	c.mu.Lock()
+	if c.hooks.OnWorkDoneProgressCreate != nil {
-	onCreate := c.onWorkDoneProgressCreate
+		return c.hooks.OnWorkDoneProgressCreate(ctx, params)
-	c.mu.Unlock()
-	if onCreate != nil {
-		return onCreate(ctx, params)
 	}
 	return nil
 }
@@ -150,9 +99,9 @@ func (c *Client) ApplyEdit(ctx context.Context, params *protocol.ApplyWorkspaceE
 		return &protocol.ApplyWorkspaceEditResponse{FailureReason: "Edit.Changes is unsupported"}, nil
 	}
 	for _, change := range params.Edit.DocumentChanges {
-		path := c.sandbox.Workdir.URIToPath(change.TextDocument.URI)
+		path := c.editor.sandbox.Workdir.URIToPath(change.TextDocument.URI)
 		edits := convertEdits(change.Edits)
-		c.EditBuffer(ctx, path, edits)
+		c.editor.EditBuffer(ctx, path, edits)
 	}
 	return &protocol.ApplyWorkspaceEditResponse{Applied: true}, nil
 }

internal/lsp/fake/editor.go

@@ -32,11 +32,7 @@ type Editor struct {
 	// locking.
 	mu sync.Mutex
 	// Editor state.
-	buffers     map[string]buffer
+	buffers map[string]buffer
-	lastMessage *protocol.ShowMessageParams
-	logs        []*protocol.LogMessageParams
-	diagnostics *protocol.PublishDiagnosticsParams
-	events      []interface{}
 	// Capabilities / Options
 	serverCapabilities protocol.ServerCapabilities
 }
@@ -80,9 +76,9 @@ func NewEditor(ws *Sandbox, config EditorConfig) *Editor {
 //
 // It returns the editor, so that it may be called as follows:
 //   editor, err := NewEditor(s).Connect(ctx, conn)
-func (e *Editor) Connect(ctx context.Context, conn *jsonrpc2.Conn) (*Editor, error) {
+func (e *Editor) Connect(ctx context.Context, conn *jsonrpc2.Conn, hooks ClientHooks) (*Editor, error) {
 	e.server = protocol.ServerDispatcher(conn)
-	e.client = &Client{Editor: e}
+	e.client = &Client{editor: e, hooks: hooks}
 	go conn.Run(ctx,
 		protocol.Handlers(
 			protocol.ClientHandler(e.client,

internal/lsp/lsprpc/lsprpc_test.go

@@ -218,13 +218,13 @@ func TestDebugInfoLifecycle(t *testing.T) {
 	tsForwarder := servertest.NewPipeServer(clientCtx, forwarder)
 
 	conn1 := tsForwarder.Connect(clientCtx)
-	ed1, err := fake.NewEditor(sb, fake.EditorConfig{}).Connect(clientCtx, conn1)
+	ed1, err := fake.NewEditor(sb, fake.EditorConfig{}).Connect(clientCtx, conn1, fake.ClientHooks{})
 	if err != nil {
 		t.Fatal(err)
 	}
 	defer ed1.Shutdown(clientCtx)
 	conn2 := tsBackend.Connect(baseCtx)
-	ed2, err := fake.NewEditor(sb, fake.EditorConfig{}).Connect(baseCtx, conn2)
+	ed2, err := fake.NewEditor(sb, fake.EditorConfig{}).Connect(baseCtx, conn2, fake.ClientHooks{})
 	if err != nil {
 		t.Fatal(err)
 	}

internal/lsp/regtest/env.go

@@ -105,15 +105,10 @@ type condition struct {
 func NewEnv(ctx context.Context, t *testing.T, scratch *fake.Sandbox, ts servertest.Connector, editorConfig fake.EditorConfig) *Env {
 	t.Helper()
 	conn := ts.Connect(ctx)
-	editor, err := fake.NewEditor(scratch, editorConfig).Connect(ctx, conn)
-	if err != nil {
-		t.Fatal(err)
-	}
 	env := &Env{
 		T:       t,
 		Ctx:     ctx,
 		Sandbox: scratch,
-		Editor:  editor,
 		Server:  ts,
 		Conn:    conn,
 		state: State{
@@ -123,11 +118,18 @@ func NewEnv(ctx context.Context, t *testing.T, scratch *fake.Sandbox, ts servert
 		},
 		waiters: make(map[int]*condition),
 	}
-	env.Editor.Client().OnDiagnostics(env.onDiagnostics)
+	hooks := fake.ClientHooks{
-	env.Editor.Client().OnLogMessage(env.onLogMessage)
+		OnDiagnostics:            env.onDiagnostics,
-	env.Editor.Client().OnWorkDoneProgressCreate(env.onWorkDoneProgressCreate)
+		OnLogMessage:             env.onLogMessage,
-	env.Editor.Client().OnProgress(env.onProgress)
+		OnWorkDoneProgressCreate: env.onWorkDoneProgressCreate,
-	env.Editor.Client().OnShowMessage(env.onShowMessage)
+		OnProgress:               env.onProgress,
+		OnShowMessage:            env.onShowMessage,
+	}
+	editor, err := fake.NewEditor(scratch, editorConfig).Connect(ctx, conn, hooks)
+	if err != nil {
+		t.Fatal(err)
+	}
+	env.Editor = editor
 	return env
 }