From c4d4ea9c797c3729eaa83e0994d29784eb221bbd Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Tue, 18 Feb 2020 20:59:37 -0500 Subject: [PATCH] internal/lsp/cache: return concrete types where possible For testability, and to allow the exchange of debug information when forwarding the LSP, it will be necessary to access debug information stored in cache objects. This is cumbersome to do if our constructors return source interfaces rather than concrete types. This CL changes cache.New and (*Cache).NewSession to return concrete types. This required removing NewSession from source.Cache. I would argue that this makes sense from a philosophical perspective: everything in the source package operates in a context where the Session and Cache already exist. Updates golang/go#34111 Change-Id: I01721db827d51117f9479f1544b15cedae0c5921 Reviewed-on: https://go-review.googlesource.com/c/tools/+/220077 Run-TryBot: Robert Findley TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick Reviewed-by: Rebecca Stambler --- internal/lsp/cache/cache.go | 20 +++++++-------- internal/lsp/cache/debug.go | 2 +- internal/lsp/cache/parse.go | 2 +- internal/lsp/cache/session.go | 46 +++++++++++++++++------------------ internal/lsp/cache/view.go | 2 +- internal/lsp/lsprpc/lsprpc.go | 4 +-- internal/lsp/source/view.go | 3 --- 7 files changed, 38 insertions(+), 41 deletions(-) diff --git a/internal/lsp/cache/cache.go b/internal/lsp/cache/cache.go index 6363025e4c..29549d8a74 100644 --- a/internal/lsp/cache/cache.go +++ b/internal/lsp/cache/cache.go @@ -19,9 +19,9 @@ import ( "golang.org/x/tools/internal/span" ) -func New(options func(*source.Options)) source.Cache { +func New(options func(*source.Options)) *Cache { index := atomic.AddInt64(&cacheIndex, 1) - c := &cache{ + c := &Cache{ fs: &nativeFileSystem{}, id: strconv.FormatInt(index, 10), fset: token.NewFileSet(), @@ -31,7 +31,7 @@ func New(options func(*source.Options)) source.Cache { return c } -type cache struct { +type Cache struct { fs source.FileSystem id string fset *token.FileSet @@ -45,7 +45,7 @@ type fileKey struct { } type fileHandle struct { - cache *cache + cache *Cache underlying source.FileHandle handle *memoize.Handle } @@ -57,7 +57,7 @@ type fileData struct { err error } -func (c *cache) GetFile(uri span.URI) source.FileHandle { +func (c *Cache) GetFile(uri span.URI) source.FileHandle { underlying := c.fs.GetFile(uri) key := fileKey{ identity: underlying.Identity(), @@ -74,9 +74,9 @@ func (c *cache) GetFile(uri span.URI) source.FileHandle { } } -func (c *cache) NewSession() source.Session { +func (c *Cache) NewSession() *Session { index := atomic.AddInt64(&sessionIndex, 1) - s := &session{ + s := &Session{ cache: c, id: strconv.FormatInt(index, 10), options: source.DefaultOptions(), @@ -86,7 +86,7 @@ func (c *cache) NewSession() source.Session { return s } -func (c *cache) FileSet() *token.FileSet { +func (c *Cache) FileSet() *token.FileSet { return c.fset } @@ -115,8 +115,8 @@ func hashContents(contents []byte) string { var cacheIndex, sessionIndex, viewIndex int64 -type debugCache struct{ *cache } +type debugCache struct{ *Cache } -func (c *cache) ID() string { return c.id } +func (c *Cache) ID() string { return c.id } func (c debugCache) FileSet() *token.FileSet { return c.fset } func (c debugCache) MemStats() map[reflect.Type]int { return c.store.Stats() } diff --git a/internal/lsp/cache/debug.go b/internal/lsp/cache/debug.go index 926fca06d7..65dcb5ecff 100644 --- a/internal/lsp/cache/debug.go +++ b/internal/lsp/cache/debug.go @@ -17,7 +17,7 @@ func (v debugView) ID() string { return v.id } func (v debugView) Session() debug.Session { return debugSession{v.session} } func (v debugView) Env() []string { return v.Options().Env } -type debugSession struct{ *session } +type debugSession struct{ *Session } func (s debugSession) ID() string { return s.id } func (s debugSession) Cache() debug.Cache { return debugCache{s.cache} } diff --git a/internal/lsp/cache/parse.go b/internal/lsp/cache/parse.go index 4eb8a05707..8ddd56ee80 100644 --- a/internal/lsp/cache/parse.go +++ b/internal/lsp/cache/parse.go @@ -47,7 +47,7 @@ type parseGoData struct { err error } -func (c *cache) ParseGoHandle(fh source.FileHandle, mode source.ParseMode) source.ParseGoHandle { +func (c *Cache) ParseGoHandle(fh source.FileHandle, mode source.ParseMode) source.ParseGoHandle { key := parseKey{ file: fh.Identity(), mode: mode, diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 77d19f3463..843e507b04 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -19,8 +19,8 @@ import ( errors "golang.org/x/xerrors" ) -type session struct { - cache *cache +type Session struct { + cache *Cache id string options source.Options @@ -34,7 +34,7 @@ type session struct { } type overlay struct { - session *session + session *Session uri span.URI text []byte hash string @@ -62,15 +62,15 @@ func (o *overlay) Read(ctx context.Context) ([]byte, string, error) { return o.text, o.hash, nil } -func (s *session) Options() source.Options { +func (s *Session) Options() source.Options { return s.options } -func (s *session) SetOptions(options source.Options) { +func (s *Session) SetOptions(options source.Options) { s.options = options } -func (s *session) Shutdown(ctx context.Context) { +func (s *Session) Shutdown(ctx context.Context) { s.viewMu.Lock() defer s.viewMu.Unlock() for _, view := range s.views { @@ -81,11 +81,11 @@ func (s *session) Shutdown(ctx context.Context) { debug.DropSession(debugSession{s}) } -func (s *session) Cache() source.Cache { +func (s *Session) Cache() source.Cache { return s.cache } -func (s *session) NewView(ctx context.Context, name string, folder span.URI, options source.Options) (source.View, source.Snapshot, error) { +func (s *Session) NewView(ctx context.Context, name string, folder span.URI, options source.Options) (source.View, source.Snapshot, error) { s.viewMu.Lock() defer s.viewMu.Unlock() v, snapshot, err := s.createView(ctx, name, folder, options) @@ -98,7 +98,7 @@ func (s *session) NewView(ctx context.Context, name string, folder span.URI, opt return v, snapshot, nil } -func (s *session) createView(ctx context.Context, name string, folder span.URI, options source.Options) (*view, *snapshot, error) { +func (s *Session) createView(ctx context.Context, name string, folder span.URI, options source.Options) (*view, *snapshot, error) { index := atomic.AddInt64(&viewIndex, 1) // We want a true background context and not a detached context here // the spans need to be unrelated and no tag values should pollute it. @@ -147,7 +147,7 @@ func (s *session) createView(ctx context.Context, name string, folder span.URI, } // View returns the view by name. -func (s *session) View(name string) source.View { +func (s *Session) View(name string) source.View { s.viewMu.Lock() defer s.viewMu.Unlock() for _, view := range s.views { @@ -160,11 +160,11 @@ func (s *session) View(name string) source.View { // ViewOf returns a view corresponding to the given URI. // If the file is not already associated with a view, pick one using some heuristics. -func (s *session) ViewOf(uri span.URI) (source.View, error) { +func (s *Session) ViewOf(uri span.URI) (source.View, error) { return s.viewOf(uri) } -func (s *session) viewOf(uri span.URI) (*view, error) { +func (s *Session) viewOf(uri span.URI) (*view, error) { s.viewMu.Lock() defer s.viewMu.Unlock() @@ -181,7 +181,7 @@ func (s *session) viewOf(uri span.URI) (*view, error) { return v, nil } -func (s *session) viewsOf(uri span.URI) []*view { +func (s *Session) viewsOf(uri span.URI) []*view { s.viewMu.Lock() defer s.viewMu.Unlock() @@ -194,7 +194,7 @@ func (s *session) viewsOf(uri span.URI) []*view { return views } -func (s *session) Views() []source.View { +func (s *Session) Views() []source.View { s.viewMu.Lock() defer s.viewMu.Unlock() result := make([]source.View, len(s.views)) @@ -206,7 +206,7 @@ func (s *session) Views() []source.View { // bestView finds the best view to associate a given URI with. // viewMu must be held when calling this method. -func (s *session) bestView(uri span.URI) (*view, error) { +func (s *Session) bestView(uri span.URI) (*view, error) { if len(s.views) == 0 { return nil, errors.Errorf("no views in the session") } @@ -227,7 +227,7 @@ func (s *session) bestView(uri span.URI) (*view, error) { return s.views[0], nil } -func (s *session) removeView(ctx context.Context, view *view) error { +func (s *Session) removeView(ctx context.Context, view *view) error { s.viewMu.Lock() defer s.viewMu.Unlock() i, err := s.dropView(ctx, view) @@ -242,7 +242,7 @@ func (s *session) removeView(ctx context.Context, view *view) error { return nil } -func (s *session) updateView(ctx context.Context, view *view, options source.Options) (*view, *snapshot, error) { +func (s *Session) updateView(ctx context.Context, view *view, options source.Options) (*view, *snapshot, error) { s.viewMu.Lock() defer s.viewMu.Unlock() i, err := s.dropView(ctx, view) @@ -263,7 +263,7 @@ func (s *session) updateView(ctx context.Context, view *view, options source.Opt return v, snapshot, nil } -func (s *session) dropView(ctx context.Context, v *view) (int, error) { +func (s *Session) dropView(ctx context.Context, v *view) (int, error) { // we always need to drop the view map s.viewMap = make(map[span.URI]*view) for i := range s.views { @@ -277,7 +277,7 @@ func (s *session) dropView(ctx context.Context, v *view) (int, error) { return -1, errors.Errorf("view %s for %v not found", v.Name(), v.Folder()) } -func (s *session) DidModifyFiles(ctx context.Context, changes []source.FileModification) ([]source.Snapshot, error) { +func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModification) ([]source.Snapshot, error) { views := make(map[*view]map[span.URI]source.FileHandle) overlays, err := s.updateOverlays(ctx, changes) @@ -322,7 +322,7 @@ func (s *session) DidModifyFiles(ctx context.Context, changes []source.FileModif return snapshots, nil } -func (s *session) isOpen(uri span.URI) bool { +func (s *Session) isOpen(uri span.URI) bool { s.overlayMu.Lock() defer s.overlayMu.Unlock() @@ -330,7 +330,7 @@ func (s *session) isOpen(uri span.URI) bool { return open } -func (s *session) updateOverlays(ctx context.Context, changes []source.FileModification) (map[span.URI]*overlay, error) { +func (s *Session) updateOverlays(ctx context.Context, changes []source.FileModification) (map[span.URI]*overlay, error) { s.overlayMu.Lock() defer s.overlayMu.Unlock() @@ -406,7 +406,7 @@ func (s *session) updateOverlays(ctx context.Context, changes []source.FileModif return overlays, nil } -func (s *session) GetFile(uri span.URI) source.FileHandle { +func (s *Session) GetFile(uri span.URI) source.FileHandle { if overlay := s.readOverlay(uri); overlay != nil { return overlay } @@ -414,7 +414,7 @@ func (s *session) GetFile(uri span.URI) source.FileHandle { return s.cache.GetFile(uri) } -func (s *session) readOverlay(uri span.URI) *overlay { +func (s *Session) readOverlay(uri span.URI) *overlay { s.overlayMu.Lock() defer s.overlayMu.Unlock() diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index e0b5ac830d..88489d58a6 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -34,7 +34,7 @@ import ( ) type view struct { - session *session + session *Session id string options source.Options diff --git a/internal/lsp/lsprpc/lsprpc.go b/internal/lsp/lsprpc/lsprpc.go index 346d5e6bb4..02bbd777c3 100644 --- a/internal/lsp/lsprpc/lsprpc.go +++ b/internal/lsp/lsprpc/lsprpc.go @@ -17,8 +17,8 @@ import ( "golang.org/x/sync/errgroup" "golang.org/x/tools/internal/jsonrpc2" "golang.org/x/tools/internal/lsp" + "golang.org/x/tools/internal/lsp/cache" "golang.org/x/tools/internal/lsp/protocol" - "golang.org/x/tools/internal/lsp/source" ) // The StreamServer type is a jsonrpc2.StreamServer that handles incoming @@ -33,7 +33,7 @@ type StreamServer struct { // NewStreamServer creates a StreamServer using the shared cache. If // withTelemetry is true, each session is instrumented with telemetry that // records RPC statistics. -func NewStreamServer(cache source.Cache, withTelemetry bool) *StreamServer { +func NewStreamServer(cache *cache.Cache, withTelemetry bool) *StreamServer { s := &StreamServer{ withTelemetry: withTelemetry, } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 7e824e33d1..b59a33817e 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -228,9 +228,6 @@ type Cache interface { // A FileSystem that reads file contents from external storage. FileSystem - // NewSession creates a new Session manager and returns it. - NewSession() Session - // FileSet returns the shared fileset used by all files in the system. FileSet() *token.FileSet