diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index 6e678a95d0..211734c30e 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -250,7 +250,7 @@ func (imp *importer) Import(pkgPath string) (*types.Package, error) { } cph := imp.snapshot.getPackage(id, source.ParseExported) if cph == nil { - return nil, errors.Errorf("no package for %s", id) + return nil, errors.Errorf("no cached package for %s", id) } pkg, err := cph.check(ctx) if err != nil { diff --git a/internal/lsp/cache/file.go b/internal/lsp/cache/file.go index a40e0244dc..122dcdebd2 100644 --- a/internal/lsp/cache/file.go +++ b/internal/lsp/cache/file.go @@ -31,6 +31,10 @@ type fileBase struct { view *view } +func dir(filename string) string { + return strings.ToLower(filepath.Dir(filename)) +} + func basename(filename string) string { return strings.ToLower(filepath.Base(filename)) } diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 4c3b7d8650..855f598a52 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -105,18 +105,11 @@ func (c *cache) shouldLoad(ctx context.Context, s *snapshot, originalFH, current return true } - // Get the original parsed file in order to check package name and imports. - original, _, _, err := c.ParseGoHandle(originalFH, source.ParseHeader).Parse(ctx) - if err != nil { - log.Error(ctx, "no ParseGoHandle for original FileHandle", err, telemetry.URI.Of(originalFH.Identity().URI)) - return false - } - - // Get the current parsed file in order to check package name and imports. - current, _, _, err := c.ParseGoHandle(currentFH, source.ParseHeader).Parse(ctx) - if err != nil { - log.Error(ctx, "no ParseGoHandle for original FileHandle", err, telemetry.URI.Of(currentFH.Identity().URI)) - return false + // Get the original and current parsed files in order to check package name and imports. + original, _, _, originalErr := c.ParseGoHandle(originalFH, source.ParseHeader).Parse(ctx) + current, _, _, currentErr := c.ParseGoHandle(currentFH, source.ParseHeader).Parse(ctx) + if originalErr != nil || currentErr != nil { + return (originalErr == nil) != (currentErr == nil) } // Check if the package's metadata has changed. The cases handled are: diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index a7ba68d417..6b5b900640 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -217,7 +217,7 @@ func (s *session) removeView(ctx context.Context, view *view) error { } // TODO: Propagate the language ID through to the view. -func (s *session) DidOpen(ctx context.Context, uri span.URI, kind source.FileKind, text []byte) { +func (s *session) DidOpen(ctx context.Context, uri span.URI, kind source.FileKind, text []byte) error { ctx = telemetry.File.With(ctx, uri) // Files with _ prefixes are ignored. @@ -227,7 +227,13 @@ func (s *session) DidOpen(ctx context.Context, uri span.URI, kind source.FileKin view.ignoredURIs[uri] = struct{}{} view.ignoredURIsMu.Unlock() } - return + return nil + } + + // Make sure that the file gets added to the session's file watch map. + view := s.bestView(uri) + if _, err := view.GetFile(ctx, uri); err != nil { + return err } // Mark the file as open. @@ -236,15 +242,7 @@ func (s *session) DidOpen(ctx context.Context, uri span.URI, kind source.FileKin // Read the file on disk and compare it to the text provided. // If it is the same as on disk, we can avoid sending it as an overlay to go/packages. s.openOverlay(ctx, uri, kind, text) - - // Mark the file as just opened so that we know to re-run packages.Load on it. - // We do this because we may not be aware of all of the packages the file belongs to. - // A file may be in multiple views. - for _, view := range s.views { - if strings.HasPrefix(string(uri), string(view.Folder())) { - view.invalidateMetadata(ctx, uri) - } - } + return nil } func (s *session) DidSave(uri span.URI) { @@ -277,7 +275,7 @@ func (s *session) SetOverlay(uri span.URI, kind source.FileKind, data []byte) bo s.overlayMu.Lock() defer func() { s.overlayMu.Unlock() - s.filesWatchMap.Notify(uri) + s.filesWatchMap.Notify(uri, protocol.Changed) }() if data == nil { @@ -299,13 +297,20 @@ func (s *session) SetOverlay(uri span.URI, kind source.FileKind, data []byte) bo return firstChange } +func (s *session) clearOverlay(uri span.URI) { + s.overlayMu.Lock() + defer s.overlayMu.Unlock() + + delete(s.overlays, uri) +} + // openOverlay adds the file content to the overlay. // It also checks if the provided content is equivalent to the file's content on disk. func (s *session) openOverlay(ctx context.Context, uri span.URI, kind source.FileKind, data []byte) { s.overlayMu.Lock() defer func() { s.overlayMu.Unlock() - s.filesWatchMap.Notify(uri) + s.filesWatchMap.Notify(uri, protocol.Created) }() s.overlays[uri] = &overlay{ session: s, @@ -350,16 +355,8 @@ func (s *session) buildOverlay() map[string][]byte { return overlays } -func (s *session) DidChangeOutOfBand(ctx context.Context, uri span.URI, changeType protocol.FileChangeType) { - if changeType == protocol.Deleted { - // After a deletion we must invalidate the package's metadata to - // force a go/packages invocation to refresh the package's file list. - views := s.viewsOf(uri) - for _, v := range views { - v.invalidateMetadata(ctx, uri) - } - } - s.filesWatchMap.Notify(uri) +func (s *session) DidChangeOutOfBand(ctx context.Context, uri span.URI, changeType protocol.FileChangeType) bool { + return s.filesWatchMap.Notify(uri, changeType) } func (o *overlay) FileSystem() source.FileSystem { diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index b0a3ffc54a..d3825f5ad1 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -2,9 +2,11 @@ package cache import ( "context" + "os" "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" ) @@ -286,26 +288,60 @@ 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, uri span.URI, kind source.FileKind) { - withoutTypes := make(map[span.URI]struct{}) - withoutMetadata := make(map[span.URI]struct{}) +func (v *view) invalidateContent(ctx context.Context, f source.File, kind source.FileKind, changeType protocol.FileChangeType) bool { + var ( + withoutTypes = make(map[span.URI]struct{}) + withoutMetadata = make(map[span.URI]struct{}) + ids = make(map[packageID]struct{}) + ) // This should be the only time we hold the view's snapshot lock for any period of time. v.snapshotMu.Lock() defer v.snapshotMu.Unlock() - ids := v.snapshot.getIDs(uri) + for _, id := range v.snapshot.getIDs(f.URI()) { + ids[id] = struct{}{} + } + + switch changeType { + case protocol.Created: + // 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 + // of all of the files in the same directory as this one. + // TODO(rstambler): Speed this up by mapping directories to filenames. + if dirStat, err := os.Stat(dir(f.URI().Filename())); err == nil { + for uri := range v.snapshot.files { + if fdirStat, err := os.Stat(dir(uri.Filename())); err == nil { + if os.SameFile(dirStat, fdirStat) { + for _, id := range v.snapshot.ids[uri] { + ids[id] = struct{}{} + } + } + } + } + } + } + + if len(ids) == 0 { + return false + } // Remove the package and all of its reverse dependencies from the cache. - for _, id := range ids { + for id := range ids { v.snapshot.reverseDependencies(id, withoutTypes, map[packageID]struct{}{}) } // Get the original FileHandle for the URI, if it exists. - originalFH := v.snapshot.getFile(uri) + originalFH := v.snapshot.getFile(f.URI()) + + // Make sure to clear out the content if there has been a deletion. + if changeType == protocol.Deleted { + v.session.clearOverlay(f.URI()) + } // Get the current FileHandle for the URI. - currentFH := v.session.GetFile(uri, kind) + currentFH := v.session.GetFile(f.URI(), kind) // Check if the file's package name or imports have changed, // and if so, invalidate metadata. @@ -315,25 +351,9 @@ func (v *view) invalidateContent(ctx context.Context, uri span.URI, kind source. // TODO: If a package's name has changed, // we should invalidate the metadata for the new package name (if it exists). } + uri := f.URI() v.snapshot = v.snapshot.clone(ctx, &uri, withoutTypes, withoutMetadata) -} - -// invalidateMetadata invalidates package metadata for all files in f's -// package. This forces f's package's metadata to be reloaded next -// time the package is checked. -// -// TODO: This function shouldn't be necessary. -// We should be able to handle its use cases more efficiently. -func (v *view) invalidateMetadata(ctx context.Context, uri span.URI) { - v.snapshotMu.Lock() - defer v.snapshotMu.Unlock() - - withoutMetadata := make(map[span.URI]struct{}) - - for _, id := range v.snapshot.getIDs(uri) { - v.snapshot.reverseDependencies(id, withoutMetadata, map[packageID]struct{}{}) - } - v.snapshot = v.snapshot.clone(ctx, nil, withoutMetadata, withoutMetadata) + return true } // reverseDependencies populates the uris map with file URIs belonging to the diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 9027505b97..227166090a 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -18,6 +18,7 @@ import ( "golang.org/x/tools/go/packages" "golang.org/x/tools/internal/imports" "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/span" "golang.org/x/tools/internal/telemetry/log" @@ -348,9 +349,9 @@ func (v *view) getFile(ctx context.Context, uri span.URI, kind source.FileKind) fname: uri.Filename(), kind: source.Go, } - v.session.filesWatchMap.Watch(uri, func() { + v.session.filesWatchMap.Watch(uri, func(changeType protocol.FileChangeType) bool { ctx := xcontext.Detach(ctx) - v.invalidateContent(ctx, uri, kind) + return v.invalidateContent(ctx, f, kind, changeType) }) v.mapFile(uri, f) return f, nil diff --git a/internal/lsp/cache/watcher.go b/internal/lsp/cache/watcher.go index f788e88e0b..53fe4dc80f 100644 --- a/internal/lsp/cache/watcher.go +++ b/internal/lsp/cache/watcher.go @@ -6,11 +6,13 @@ package cache import ( "sync" + + "golang.org/x/tools/internal/lsp/protocol" ) type watcher struct { id uint64 - callback func() + callback func(changeType protocol.FileChangeType) bool } type WatchMap struct { @@ -22,7 +24,7 @@ type WatchMap struct { func NewWatchMap() *WatchMap { return &WatchMap{watchers: make(map[interface{}][]watcher)} } -func (w *WatchMap) Watch(key interface{}, callback func()) func() { +func (w *WatchMap) Watch(key interface{}, callback func(protocol.FileChangeType) bool) func() { w.mu.Lock() defer w.mu.Unlock() id := w.nextID @@ -47,7 +49,7 @@ func (w *WatchMap) Watch(key interface{}, callback func()) func() { } } -func (w *WatchMap) Notify(key interface{}) { +func (w *WatchMap) Notify(key interface{}, changeType protocol.FileChangeType) 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() @@ -56,7 +58,9 @@ func (w *WatchMap) Notify(key interface{}) { copy(entriesCopy, entries) w.mu.Unlock() + var result bool for _, entry := range entriesCopy { - entry.callback() + result = entry.callback(changeType) || result } + return result } diff --git a/internal/lsp/general.go b/internal/lsp/general.go index 1ecd19374e..0b408dd794 100644 --- a/internal/lsp/general.go +++ b/internal/lsp/general.go @@ -149,7 +149,7 @@ func (s *Server) initialized(ctx context.Context, params *protocol.InitializedPa RegisterOptions: protocol.DidChangeWatchedFilesRegistrationOptions{ Watchers: []protocol.FileSystemWatcher{{ GlobPattern: "**/*.go", - Kind: float64(protocol.WatchChange), + Kind: float64(protocol.WatchChange + protocol.WatchDelete + protocol.WatchCreate), }}, }, }) diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index b0c3f9654d..0f95dc2756 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -164,7 +164,7 @@ type Session interface { FileSystem // DidOpen is invoked each time a file is opened in the editor. - DidOpen(ctx context.Context, uri span.URI, kind FileKind, text []byte) + DidOpen(ctx context.Context, uri span.URI, kind FileKind, text []byte) error // DidSave is invoked each time an open file is saved in the editor. DidSave(uri span.URI) @@ -178,9 +178,9 @@ type Session interface { // Called to set the effective contents of a file from this session. SetOverlay(uri span.URI, kind FileKind, data []byte) (wasFirstChange bool) - // DidChangeOutOfBand is called when a file under the root folder - // changes. The file is not necessarily open in the editor. - DidChangeOutOfBand(ctx context.Context, uri span.URI, change protocol.FileChangeType) + // 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 // Options returns a copy of the SessionOptions for this session. Options() Options diff --git a/internal/lsp/watched_files.go b/internal/lsp/watched_files.go index a923afeeda..e34a729378 100644 --- a/internal/lsp/watched_files.go +++ b/internal/lsp/watched_files.go @@ -1,12 +1,10 @@ // Copyright 2019 The Go Authors. All rights reserved. // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. - package lsp import ( "context" - "sort" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" @@ -16,67 +14,56 @@ import ( ) func (s *Server) didChangeWatchedFiles(ctx context.Context, params *protocol.DidChangeWatchedFilesParams) error { - options := s.session.Options() - if !options.WatchFileChanges { - return nil - } - for _, change := range params.Changes { uri := span.NewURI(change.URI) - ctx := telemetry.File.With(ctx, uri) for _, view := range s.session.Views() { - f := view.FindFile(ctx, uri) - - // If we have never seen this file before, there is nothing to do. - if f == nil { + if !view.Options().WatchFileChanges { continue } - - // 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 - } - switch change.Type { - case protocol.Changed: - log.Print(ctx, "watched file changed", telemetry.File) - - s.session.DidChangeOutOfBand(ctx, uri, change.Type) - - // Refresh diagnostics to reflect updated file contents. - go s.diagnostics(view, uri) - case protocol.Created: - log.Print(ctx, "watched file created", telemetry.File) + case protocol.Changed, protocol.Created: + // 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 we had been tracking the given file, + // recompute diagnostics to reflect updated file contents. + go s.diagnostics(view, uri) + } case protocol.Deleted: - log.Print(ctx, "watched file deleted", telemetry.File) - + f := view.FindFile(ctx, uri) + // If we have never seen this file before, there is nothing to do. + if f == nil { + continue + } _, cphs, err := view.CheckPackageHandles(ctx, f) if err != nil { - log.Error(ctx, "didChangeWatchedFiles: GetPackage", err, telemetry.File) + log.Error(ctx, "didChangeWatchedFiles: CheckPackageHandles", err, telemetry.File) + continue + } + cph, err := source.WidestCheckPackageHandle(cphs) + if err != nil { + log.Error(ctx, "didChangeWatchedFiles: WidestCheckPackageHandle", err, telemetry.File) continue } // Find a different file in the same package we can use to trigger diagnostics. // TODO(rstambler): Allow diagnostics to be called per-package to avoid this. var otherFile source.File - sort.Slice(cphs, func(i, j int) bool { - return len(cphs[i].Files()) > len(cphs[j].Files()) - }) - for _, ph := range cphs[0].Files() { - if len(cphs) > 1 && contains(cphs[1], ph.File()) { + for _, ph := range cph.Files() { + if ph.File().Identity().URI == f.URI() { continue } - ident := ph.File().Identity() - if ident.URI == f.URI() { - continue - } - otherFile := view.FindFile(ctx, ident.URI) - if otherFile != nil { + if f := view.FindFile(ctx, ph.File().Identity().URI); f != nil && s.session.IsOpen(f.URI()) { + otherFile = f break } } + + // Notify the view of the deletion of the file. s.session.DidChangeOutOfBand(ctx, uri, change.Type) // If this was the only file in the package, clear its diagnostics. @@ -86,18 +73,11 @@ func (s *Server) didChangeWatchedFiles(ctx context.Context, params *protocol.Did } return nil } + + // Refresh diagnostics for the package the file belonged to. go s.diagnostics(view, otherFile.URI()) } } } return nil } - -func contains(cph source.CheckPackageHandle, fh source.FileHandle) bool { - for _, ph := range cph.Files() { - if ph.File().Identity().URI == fh.Identity().URI { - return true - } - } - return false -}