internal/lsp: modify approach to watching changed files

This change modifies the invalidContent function to take a file change
type. This allows us to eliminate the separate invalidateMetadata
function. The logic of watching changed files is then further pushed
into the caching layer.

Updates golang/go#34218

Change-Id: Id31b3931c45ec408b6e7b4a362e00f9091ba4f70
Reviewed-on: https://go-review.googlesource.com/c/tools/+/201221
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>

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 {

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))
 }

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:

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 {

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

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

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
 }

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),
 				}},
 			},
 		})

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

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
-}