From 78d067421b028ce0d58277f5b1b4daadb59a0798 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Tue, 21 Jan 2020 16:01:59 -0500 Subject: [PATCH] internal/lsp: remove the Context argument from NewSession The passed-in Context is not used, and creates the illusion of a startup dependency problem: existing code is careful to pass in the context containing the correct Client instance. This allows passing in a source.Session, rather than a source.Cache, into lsp server constructors. Updates golang/go#34111 Change-Id: I081ad6fa800b846b63e04d7164577e3a32966704 Reviewed-on: https://go-review.googlesource.com/c/tools/+/215740 Run-TryBot: Robert Findley TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler Reviewed-by: Ian Cottrell --- internal/lsp/cache/cache.go | 2 +- internal/lsp/cmd/capabilities_test.go | 2 +- internal/lsp/cmd/cmd.go | 4 ++-- internal/lsp/cmd/serve.go | 2 +- internal/lsp/cmd/test/cmdtest.go | 1 + internal/lsp/lsp_test.go | 4 ++-- internal/lsp/mod/mod_test.go | 4 ++-- internal/lsp/server.go | 14 +++++++------- internal/lsp/source/source_test.go | 2 +- internal/lsp/source/view.go | 2 +- 10 files changed, 19 insertions(+), 18 deletions(-) diff --git a/internal/lsp/cache/cache.go b/internal/lsp/cache/cache.go index e6a185a7468..56531ddc32c 100644 --- a/internal/lsp/cache/cache.go +++ b/internal/lsp/cache/cache.go @@ -73,7 +73,7 @@ func (c *cache) GetFile(uri span.URI) source.FileHandle { } } -func (c *cache) NewSession(ctx context.Context) source.Session { +func (c *cache) NewSession() source.Session { index := atomic.AddInt64(&sessionIndex, 1) s := &session{ cache: c, diff --git a/internal/lsp/cmd/capabilities_test.go b/internal/lsp/cmd/capabilities_test.go index da906d83f7e..4d15a2e559a 100644 --- a/internal/lsp/cmd/capabilities_test.go +++ b/internal/lsp/cmd/capabilities_test.go @@ -40,7 +40,7 @@ func TestCapabilities(t *testing.T) { params.Capabilities.Workspace.Configuration = true // Send an initialize request to the server. - ctx, c.Server = lsp.NewClientServer(ctx, cache.New(app.options), c.Client) + ctx, c.Server = lsp.NewClientServer(ctx, cache.New(app.options).NewSession(), c.Client) result, err := c.Server.Initialize(ctx, params) if err != nil { t.Fatal(err) diff --git a/internal/lsp/cmd/cmd.go b/internal/lsp/cmd/cmd.go index 2c5573bc5c9..25df438a9e0 100644 --- a/internal/lsp/cmd/cmd.go +++ b/internal/lsp/cmd/cmd.go @@ -192,7 +192,7 @@ func (app *Application) connect(ctx context.Context) (*connection, error) { switch app.Remote { case "": connection := newConnection(app) - ctx, connection.Server = lsp.NewClientServer(ctx, cache.New(app.options), connection.Client) + ctx, connection.Server = lsp.NewClientServer(ctx, cache.New(app.options).NewSession(), connection.Client) return connection, connection.initialize(ctx, app.options) case "internal": internalMu.Lock() @@ -208,7 +208,7 @@ func (app *Application) connect(ctx context.Context) (*connection, error) { ctx, jc, connection.Server = protocol.NewClient(ctx, jsonrpc2.NewHeaderStream(cr, cw), connection.Client) go jc.Run(ctx) go func() { - ctx, srv := lsp.NewServer(ctx, cache.New(app.options), jsonrpc2.NewHeaderStream(sr, sw)) + ctx, srv := lsp.NewServer(ctx, cache.New(app.options).NewSession(), jsonrpc2.NewHeaderStream(sr, sw)) srv.Run(ctx) }() if err := connection.initialize(ctx, app.options); err != nil { diff --git a/internal/lsp/cmd/serve.go b/internal/lsp/cmd/serve.go index b44d4bd2b37..02b71f0d664 100644 --- a/internal/lsp/cmd/serve.go +++ b/internal/lsp/cmd/serve.go @@ -97,7 +97,7 @@ func (s *Serve) Run(ctx context.Context, args ...string) error { if s.Trace { stream = protocol.LoggingStream(stream, out) } - ctx, srv := lsp.NewServer(ctx, cache.New(s.app.options), stream) + ctx, srv := lsp.NewServer(ctx, cache.New(s.app.options).NewSession(), stream) return prepare(ctx, srv).Run(ctx) } diff --git a/internal/lsp/cmd/test/cmdtest.go b/internal/lsp/cmd/test/cmdtest.go index 06e7da7f5c3..f0aab4b97cc 100644 --- a/internal/lsp/cmd/test/cmdtest.go +++ b/internal/lsp/cmd/test/cmdtest.go @@ -140,6 +140,7 @@ func (r *runner) RunGoplsCmd(t testing.TB, args ...string) (string, string) { return string(stdout), string(stderr) } +// NormalizeGoplsCmd runs the gopls command and normalizes its output. func (r *runner) NormalizeGoplsCmd(t testing.TB, args ...string) (string, string) { stdout, stderr := r.RunGoplsCmd(t, args...) return r.Normalize(stdout), r.Normalize(stderr) diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index 07d44f7c939..71301793209 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -52,7 +52,7 @@ func testLSP(t *testing.T, exporter packagestest.Exporter) { defer data.Exported.Cleanup() cache := cache.New(nil) - session := cache.NewSession(ctx) + session := cache.NewSession() options := tests.DefaultOptions() session.SetOptions(options) options.Env = data.Config.Env @@ -928,7 +928,7 @@ func TestModfileSuggestedFixes(t *testing.T) { ctx := tests.Context(t) cache := cache.New(nil) - session := cache.NewSession(ctx) + session := cache.NewSession() options := tests.DefaultOptions() options.TempModfile = true options.Env = append(os.Environ(), "GOPACKAGESDRIVER=off", "GOROOT=") diff --git a/internal/lsp/mod/mod_test.go b/internal/lsp/mod/mod_test.go index 6876f7d423a..29d0d1b9288 100644 --- a/internal/lsp/mod/mod_test.go +++ b/internal/lsp/mod/mod_test.go @@ -29,7 +29,7 @@ func TestMain(m *testing.M) { func TestModfileRemainsUnchanged(t *testing.T) { ctx := tests.Context(t) cache := cache.New(nil) - session := cache.NewSession(ctx) + session := cache.NewSession() options := tests.DefaultOptions() options.TempModfile = true options.Env = append(os.Environ(), "GOPACKAGESDRIVER=off", "GOROOT=") @@ -67,7 +67,7 @@ func TestModfileRemainsUnchanged(t *testing.T) { func TestDiagnostics(t *testing.T) { ctx := tests.Context(t) cache := cache.New(nil) - session := cache.NewSession(ctx) + session := cache.NewSession() options := tests.DefaultOptions() options.TempModfile = true options.Env = append(os.Environ(), "GOPACKAGESDRIVER=off", "GOROOT=") diff --git a/internal/lsp/server.go b/internal/lsp/server.go index fa3c77fa4db..d6c87cb621f 100644 --- a/internal/lsp/server.go +++ b/internal/lsp/server.go @@ -18,23 +18,23 @@ import ( ) // NewClientServer -func NewClientServer(ctx context.Context, cache source.Cache, client protocol.Client) (context.Context, *Server) { +func NewClientServer(ctx context.Context, session source.Session, client protocol.Client) (context.Context, *Server) { ctx = protocol.WithClient(ctx, client) return ctx, &Server{ client: client, - session: cache.NewSession(ctx), + session: session, delivered: make(map[span.URI]sentDiagnostics), } } -// NewServer starts an LSP server on the supplied stream, and waits until the -// stream is closed. -func NewServer(ctx context.Context, cache source.Cache, stream jsonrpc2.Stream) (context.Context, *Server) { +// NewServer creates an LSP server and binds it to handle incoming client +// messages on on the supplied stream. +func NewServer(ctx context.Context, session source.Session, stream jsonrpc2.Stream) (context.Context, *Server) { s := &Server{ delivered: make(map[span.URI]sentDiagnostics), + session: session, } ctx, s.Conn, s.client = protocol.NewServer(ctx, stream, s) - s.session = cache.NewSession(ctx) return ctx, s } @@ -56,7 +56,7 @@ func RunServerOnAddress(ctx context.Context, cache source.Cache, addr string, h if err != nil { return err } - h(NewServer(ctx, cache, jsonrpc2.NewHeaderStream(conn, conn))) + h(NewServer(ctx, cache.NewSession(), jsonrpc2.NewHeaderStream(conn, conn))) } } diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index bbab05a43b8..e2bdabf69b5 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -48,7 +48,7 @@ func testSource(t *testing.T, exporter packagestest.Exporter) { defer data.Exported.Cleanup() cache := cache.New(nil) - session := cache.NewSession(ctx) + session := cache.NewSession() options := tests.DefaultOptions() options.Env = data.Config.Env view, _, err := session.NewView(ctx, "source_test", span.FileURI(data.Config.Dir), options) diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index ad5676b40cd..8b826362e73 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -215,7 +215,7 @@ type Cache interface { FileSystem // NewSession creates a new Session manager and returns it. - NewSession(ctx context.Context) Session + NewSession() Session // FileSet returns the shared fileset used by all files in the system. FileSet() *token.FileSet