From b8469989bc69e50ec6dc4e4513fc3ff9ce48b8af Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Thu, 7 May 2020 12:25:33 -0400 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/fake/client.go | 99 ++++++++---------------------- internal/lsp/fake/editor.go | 10 +-- internal/lsp/lsprpc/lsprpc_test.go | 4 +- internal/lsp/regtest/env.go | 22 ++++--- 4 files changed, 41 insertions(+), 94 deletions(-) diff --git a/internal/lsp/fake/client.go b/internal/lsp/fake/client.go index 4ee164c0b5..9a2d8c9574 100644 --- a/internal/lsp/fake/client.go +++ b/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 - - // 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() + editor *Editor + hooks ClientHooks } func (c *Client) ShowMessage(ctx context.Context, params *protocol.ShowMessageParams) error { - c.mu.Lock() - c.lastMessage = params - c.mu.Unlock() - if c.onShowMessage != nil { - return c.onShowMessage(ctx, params) + if c.hooks.OnShowMessage != nil { + return c.hooks.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() - c.logs = append(c.logs, params) - onLogMessage := c.onLogMessage - c.mu.Unlock() - if onLogMessage != nil { - return onLogMessage(ctx, params) + if c.hooks.OnLogMessage != nil { + return c.hooks.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() - c.diagnostics = params - onPublishDiagnostics := c.onDiagnostics - c.mu.Unlock() - if onPublishDiagnostics != nil { - return onPublishDiagnostics(ctx, params) + if c.hooks.OnDiagnostics != nil { + return c.hooks.OnDiagnostics(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() - onProgress := c.onProgress - c.mu.Unlock() - if onProgress != nil { - return onProgress(ctx, params) + if c.hooks.OnProgress != nil { + return c.hooks.OnProgress(ctx, params) } return nil } func (c *Client) WorkDoneProgressCreate(ctx context.Context, params *protocol.WorkDoneProgressCreateParams) error { - c.mu.Lock() - onCreate := c.onWorkDoneProgressCreate - c.mu.Unlock() - if onCreate != nil { - return onCreate(ctx, params) + if c.hooks.OnWorkDoneProgressCreate != nil { + return c.hooks.OnWorkDoneProgressCreate(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 } diff --git a/internal/lsp/fake/editor.go b/internal/lsp/fake/editor.go index dc9babb88f..8df543be98 100644 --- a/internal/lsp/fake/editor.go +++ b/internal/lsp/fake/editor.go @@ -32,11 +32,7 @@ type Editor struct { // locking. mu sync.Mutex // Editor state. - buffers map[string]buffer - lastMessage *protocol.ShowMessageParams - logs []*protocol.LogMessageParams - diagnostics *protocol.PublishDiagnosticsParams - events []interface{} + buffers map[string]buffer // 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, diff --git a/internal/lsp/lsprpc/lsprpc_test.go b/internal/lsp/lsprpc/lsprpc_test.go index 5014ad82e9..c89e54cb02 100644 --- a/internal/lsp/lsprpc/lsprpc_test.go +++ b/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) } diff --git a/internal/lsp/regtest/env.go b/internal/lsp/regtest/env.go index c8b1a23fef..08fc842100 100644 --- a/internal/lsp/regtest/env.go +++ b/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) - env.Editor.Client().OnLogMessage(env.onLogMessage) - env.Editor.Client().OnWorkDoneProgressCreate(env.onWorkDoneProgressCreate) - env.Editor.Client().OnProgress(env.onProgress) - env.Editor.Client().OnShowMessage(env.onShowMessage) + hooks := fake.ClientHooks{ + OnDiagnostics: env.onDiagnostics, + OnLogMessage: env.onLogMessage, + OnWorkDoneProgressCreate: env.onWorkDoneProgressCreate, + 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 }