From faa69481e76149c3386932970ba2d389b6155317 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Wed, 13 Nov 2019 13:14:21 -0500 Subject: [PATCH] internal/lsp/cache: add finer-grained control of file changes This change is the first step in centralizing control of modifications to different files, either within the workspace or outside of it. We add a source.FileAction type to pass into the internal/lsp/cache package and handle the difference between opening and creating a file. Now that we load all packages in a workspace by default, we no longer need to re-load a file on open. This CL should enable CL 206883 to work correctly. Change-Id: I2ddb21ca2dd33720d668066e73283f5629d02867 Reviewed-on: https://go-review.googlesource.com/c/tools/+/206888 Run-TryBot: Rebecca Stambler Reviewed-by: Ian Cottrell --- internal/lsp/cache/gofile.go | 13 +++++++++---- internal/lsp/cache/session.go | 9 ++++----- internal/lsp/cache/snapshot.go | 10 ++++------ internal/lsp/cache/view.go | 4 ++-- internal/lsp/cache/watcher.go | 11 ++++++----- internal/lsp/source/view.go | 13 ++++++++++++- internal/lsp/watched_files.go | 23 ++++++++++++++++++----- 7 files changed, 55 insertions(+), 28 deletions(-) diff --git a/internal/lsp/cache/gofile.go b/internal/lsp/cache/gofile.go index f55a818466..39c5a7ca86 100644 --- a/internal/lsp/cache/gofile.go +++ b/internal/lsp/cache/gofile.go @@ -33,25 +33,30 @@ func (s *snapshot) CheckPackageHandles(ctx context.Context, f source.File) ([]so fh := s.Handle(ctx, f) // Determine if we need to type-check the package. - m, cphs, load, check := s.shouldCheck(fh) + metadata, cphs, load, check := s.shouldCheck(fh) // We may need to re-load package metadata. // We only need to this if it has been invalidated, and is therefore unvailable. if load { var err error - m, err = s.load(ctx, source.FileURI(f.URI())) + m, err := s.load(ctx, source.FileURI(f.URI())) if err != nil { return nil, err } // If load has explicitly returned nil metadata and no error, // it means that we should not re-type-check the packages. - if m == nil { + // Return the cached CheckPackageHandles, if we had them. + if m == nil && len(cphs) > 0 { return cphs, nil } + // If metadata was returned, from the load call, use it. + if m != nil { + metadata = m + } } if check { var results []source.CheckPackageHandle - for _, m := range m { + for _, m := range metadata { cph, err := s.checkPackageHandle(ctx, m.id, source.ParseFull) if err != nil { return nil, err diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 1b8cd3522f..eb1c85cd8b 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -15,7 +15,6 @@ import ( "sync/atomic" "golang.org/x/tools/internal/lsp/debug" - "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/lsp/telemetry" "golang.org/x/tools/internal/span" @@ -338,7 +337,7 @@ func (s *session) SetOverlay(uri span.URI, kind source.FileKind, version float64 s.overlayMu.Lock() defer func() { s.overlayMu.Unlock() - s.filesWatchMap.Notify(uri, protocol.Changed) + s.filesWatchMap.Notify(uri, source.Change) }() if data == nil { @@ -374,7 +373,7 @@ func (s *session) openOverlay(ctx context.Context, uri span.URI, kind source.Fil s.overlayMu.Lock() defer func() { s.overlayMu.Unlock() - s.filesWatchMap.Notify(uri, protocol.Created) + s.filesWatchMap.Notify(uri, source.Open) }() s.overlays[uri] = &overlay{ session: s, @@ -418,8 +417,8 @@ func (s *session) buildOverlay() map[string][]byte { return overlays } -func (s *session) DidChangeOutOfBand(ctx context.Context, uri span.URI, changeType protocol.FileChangeType) bool { - return s.filesWatchMap.Notify(uri, changeType) +func (s *session) DidChangeOutOfBand(ctx context.Context, uri span.URI, action source.FileAction) bool { + return s.filesWatchMap.Notify(uri, action) } func (o *overlay) FileSystem() source.FileSystem { diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index db332dc207..dcd95e41a7 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -7,7 +7,6 @@ import ( "sync" "golang.org/x/tools/go/analysis" - "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/span" "golang.org/x/tools/internal/telemetry/log" @@ -352,7 +351,7 @@ func (s *snapshot) clone(ctx context.Context, withoutURI *span.URI, withoutTypes // invalidateContent invalidates the content of a Go file, // including any position and type information that depends on it. -func (v *view) invalidateContent(ctx context.Context, f source.File, changeType protocol.FileChangeType) bool { +func (v *view) invalidateContent(ctx context.Context, f source.File, kind source.FileKind, action source.FileAction) bool { var ( withoutTypes = make(map[span.URI]struct{}) withoutMetadata = make(map[span.URI]struct{}) @@ -370,8 +369,8 @@ func (v *view) invalidateContent(ctx context.Context, f source.File, changeType // Get the original FileHandle for the URI, if it exists. originalFH := v.snapshot.getFile(f.URI()) - switch changeType { - case protocol.Created: + switch action { + case source.Create: // If this is a file we don't yet know about, // then we do not yet know what packages it should belong to. // Make a rough estimate of what metadata to invalidate by finding the package IDs @@ -388,7 +387,6 @@ func (v *view) invalidateContent(ctx context.Context, f source.File, changeType } } } - // If a file has been explicitly created, make sure that its original file handle is nil. originalFH = nil } @@ -402,7 +400,7 @@ func (v *view) invalidateContent(ctx context.Context, f source.File, changeType } // Make sure to clear out the content if there has been a deletion. - if changeType == protocol.Deleted { + if action == source.Delete { v.session.clearOverlay(f.URI()) } diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 155a3743ee..e2a4236640 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -376,9 +376,9 @@ func (v *view) getFile(ctx context.Context, uri span.URI, kind source.FileKind) fname: uri.Filename(), kind: kind, } - v.session.filesWatchMap.Watch(uri, func(changeType protocol.FileChangeType) bool { + v.session.filesWatchMap.Watch(uri, func(action source.FileAction) bool { ctx := xcontext.Detach(ctx) - return v.invalidateContent(ctx, f, changeType) + return v.invalidateContent(ctx, f, kind, action) }) v.mapFile(uri, f) return f, nil diff --git a/internal/lsp/cache/watcher.go b/internal/lsp/cache/watcher.go index 53fe4dc80f..b955e1a4b2 100644 --- a/internal/lsp/cache/watcher.go +++ b/internal/lsp/cache/watcher.go @@ -7,12 +7,12 @@ package cache import ( "sync" - "golang.org/x/tools/internal/lsp/protocol" + "golang.org/x/tools/internal/lsp/source" ) type watcher struct { id uint64 - callback func(changeType protocol.FileChangeType) bool + callback func(action source.FileAction) bool } type WatchMap struct { @@ -24,7 +24,8 @@ type WatchMap struct { func NewWatchMap() *WatchMap { return &WatchMap{watchers: make(map[interface{}][]watcher)} } -func (w *WatchMap) Watch(key interface{}, callback func(protocol.FileChangeType) bool) func() { + +func (w *WatchMap) Watch(key interface{}, callback func(source.FileAction) bool) func() { w.mu.Lock() defer w.mu.Unlock() id := w.nextID @@ -49,7 +50,7 @@ func (w *WatchMap) Watch(key interface{}, callback func(protocol.FileChangeType) } } -func (w *WatchMap) Notify(key interface{}, changeType protocol.FileChangeType) bool { +func (w *WatchMap) Notify(key interface{}, action source.FileAction) bool { // Make a copy of the watcher callbacks so we don't need to hold // the mutex during the callbacks (to avoid deadlocks). w.mu.Lock() @@ -60,7 +61,7 @@ func (w *WatchMap) Notify(key interface{}, changeType protocol.FileChangeType) b var result bool for _, entry := range entriesCopy { - result = entry.callback(changeType) || result + result = entry.callback(action) || result } return result } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index aa1e2a8181..16907b512c 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -194,7 +194,7 @@ type Session interface { // DidChangeOutOfBand is called when a file under the root folder changes. // If the file was open in the editor, it returns true. - DidChangeOutOfBand(ctx context.Context, uri span.URI, change protocol.FileChangeType) bool + DidChangeOutOfBand(ctx context.Context, uri span.URI, action FileAction) bool // Options returns a copy of the SessionOptions for this session. Options() Options @@ -203,6 +203,17 @@ type Session interface { SetOptions(Options) } +type FileAction int + +const ( + Open = FileAction(iota) + Close + Change + Create + Delete + UnknownFileAction +) + // View represents a single workspace. // This is the level at which we maintain configuration like working directory // and build tags. diff --git a/internal/lsp/watched_files.go b/internal/lsp/watched_files.go index b6238e01ab..10e1a9a0fa 100644 --- a/internal/lsp/watched_files.go +++ b/internal/lsp/watched_files.go @@ -23,19 +23,20 @@ func (s *Server) didChangeWatchedFiles(ctx context.Context, params *protocol.Did if !view.Options().WatchFileChanges { continue } - switch change.Type { - case protocol.Changed, protocol.Created: + action := toFileAction(change.Type) + switch action { + case source.Change, source.Create: // If client has this file open, don't do anything. // The client's contents must remain the source of truth. if s.session.IsOpen(uri) { break } - if s.session.DidChangeOutOfBand(ctx, uri, change.Type) { + if s.session.DidChangeOutOfBand(ctx, uri, action) { // If we had been tracking the given file, // recompute diagnostics to reflect updated file contents. go s.diagnostics(view, uri) } - case protocol.Deleted: + case source.Delete: f := view.FindFile(ctx, uri) // If we have never seen this file before, there is nothing to do. if f == nil { @@ -65,7 +66,7 @@ func (s *Server) didChangeWatchedFiles(ctx context.Context, params *protocol.Did } // Notify the view of the deletion of the file. - s.session.DidChangeOutOfBand(ctx, uri, change.Type) + s.session.DidChangeOutOfBand(ctx, uri, action) // If this was the only file in the package, clear its diagnostics. if otherFile == nil { @@ -82,3 +83,15 @@ func (s *Server) didChangeWatchedFiles(ctx context.Context, params *protocol.Did } return nil } + +func toFileAction(ct protocol.FileChangeType) source.FileAction { + switch ct { + case protocol.Changed: + return source.Change + case protocol.Created: + return source.Create + case protocol.Deleted: + return source.Delete + } + return source.UnknownFileAction +}