internal/lsp: fix support for watching changed files

This is the beginning of the CLs to refactor the file watching code with
the normal text synchronization code. This hasn't yet been tested other
than with some minimal local testing, so follow-up CLs will be needed.

Updates golang/go#31553

Change-Id: Id081ecc59dd2903057164171bd95f0dc07baa5f1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/214277
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

internal/lsp/cache/overlay.go

@@ -20,9 +20,9 @@ type overlay struct {
 	version float64
 	kind    source.FileKind
 
-	// sameContentOnDisk is true if a file has been saved on disk,
+	// saved is true if a file has been saved on disk,
 	// and therefore does not need to be part of the overlay sent to go/packages.
-	sameContentOnDisk bool
+	saved bool
 }
 
 func (o *overlay) FileSystem() source.FileSystem {
@@ -42,6 +42,11 @@ func (o *overlay) Read(ctx context.Context) ([]byte, string, error) {
 }
 
 func (s *session) updateOverlay(ctx context.Context, c source.FileModification) (source.FileKind, error) {
+	// Make sure that the file was not changed on disk.
+	if c.OnDisk {
+		return source.UnknownKind, errors.Errorf("updateOverlay called for an on-disk change: %s", c.URI)
+	}
+
 	s.overlayMu.Lock()
 	defer s.overlayMu.Unlock()
 
@@ -90,13 +95,13 @@ func (s *session) updateOverlay(ctx context.Context, c source.FileModification)
 		sameContentOnDisk = true
 	}
 	s.overlays[c.URI] = &overlay{
-		session:           s,
-		uri:               c.URI,
-		version:           c.Version,
-		text:              text,
-		kind:              kind,
-		hash:              hash,
-		sameContentOnDisk: sameContentOnDisk,
+		session: s,
+		uri:     c.URI,
+		version: c.Version,
+		text:    text,
+		kind:    kind,
+		hash:    hash,
+		saved:   sameContentOnDisk,
 	}
 	return kind, nil
 }
@@ -119,7 +124,7 @@ func (s *session) buildOverlay() map[string][]byte {
 	overlays := make(map[string][]byte)
 	for uri, overlay := range s.overlays {
 		// TODO(rstambler): Make sure not to send overlays outside of the current view.
-		if overlay.sameContentOnDisk {
+		if overlay.saved {
 			continue
 		}
 		overlays[uri.Filename()] = overlay.text

internal/lsp/cache/session.go

@@ -258,24 +258,47 @@ 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) DidModifyFile(ctx context.Context, c source.FileModification) ([]source.Snapshot, error) {
+func (s *session) DidModifyFile(ctx context.Context, c source.FileModification) (snapshots []source.Snapshot, err error) {
 	ctx = telemetry.URI.With(ctx, c.URI)
 
-	// Perform the session-specific updates.
-	kind, err := s.updateOverlay(ctx, c)
-	if err != nil {
-		return nil, err
+	// Update overlays only if the file was changed in the editor.
+	var kind source.FileKind
+	if !c.OnDisk {
+		kind, err = s.updateOverlay(ctx, c)
+		if err != nil {
+			return nil, err
+		}
 	}
-
-	var snapshots []source.Snapshot
 	for _, view := range s.viewsOf(c.URI) {
 		if view.Ignore(c.URI) {
 			return nil, errors.Errorf("ignored file %v", c.URI)
 		}
-		// Make sure to add the file to the view.
-		if _, err := view.getFileLocked(c.URI); err != nil {
+		// If the file was changed or deleted on disk,
+		// do nothing if the view isn't already aware of the file.
+		if c.OnDisk {
+			switch c.Action {
+			case source.Change, source.Delete:
+				if !view.knownFile(c.URI) {
+					continue
+				}
+			}
+		}
+		// Make sure that the file is added to the view.
+		f, err := view.getFileLocked(c.URI)
+		if err != nil {
 			return nil, err
 		}
+		// If the file change was on disk, the file kind is not known.
+		if c.OnDisk {
+			// If the file was already known in the snapshot,
+			// then use the already known file kind. Otherwise,
+			// detect the file kind. This should only be needed for file creates.
+			if fh := view.getSnapshot().findFileHandle(ctx, f); fh != nil {
+				kind = fh.Identity().Kind
+			} else {
+				kind = source.DetectLanguage("", c.URI.Filename())
+			}
+		}
 		snapshots = append(snapshots, view.invalidateContent(ctx, c.URI, kind, c.Action))
 	}
 	return snapshots, nil
@@ -296,18 +319,3 @@ func (s *session) GetFile(uri span.URI) source.FileHandle {
 	// Fall back to the cache-level file system.
 	return s.cache.GetFile(uri)
 }
-
-func (s *session) DidChangeOutOfBand(ctx context.Context, uri span.URI, action source.FileAction) bool {
-	view, err := s.viewOf(uri)
-	if err != nil {
-		return false
-	}
-	// Make sure that the file is part of the view.
-	if _, err := view.getFileLocked(uri); err != nil {
-		return false
-	}
-	// TODO(golang/go#31553): Remove this when this issue has been resolved.
-	kind := source.DetectLanguage("", uri.Filename())
-	view.invalidateContent(ctx, uri, kind, action)
-	return true
-}

internal/lsp/cache/snapshot.go

@@ -522,15 +522,6 @@ func (s *snapshot) getFileURIs() []span.URI {
 	return uris
 }
 
-// FindFile returns the file if the given URI is already a part of the view.
-func (s *snapshot) FindFile(uri span.URI) source.FileHandle {
-	f, err := s.view.findFileLocked(uri)
-	if f == nil || err != nil {
-		return nil
-	}
-	return s.getFileHandle(f)
-}
-
 // GetFile returns a File for the given URI. It will always succeed because it
 // adds the file to the managed set if needed.
 func (s *snapshot) GetFile(uri span.URI) (source.FileHandle, error) {
@@ -551,7 +542,14 @@ func (s *snapshot) getFileHandle(f *fileBase) source.FileHandle {
 	return s.files[f.URI()]
 }
 
-func (s *snapshot) clone(ctx context.Context, withoutURI span.URI, withoutFileKind source.FileKind) *snapshot {
+func (s *snapshot) findFileHandle(ctx context.Context, f *fileBase) source.FileHandle {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	return s.files[f.URI()]
+}
+
+func (s *snapshot) clone(ctx context.Context, withoutURI span.URI) *snapshot {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 

internal/lsp/cache/view.go

@@ -342,12 +342,13 @@ func basename(filename string) string {
 	return strings.ToLower(filepath.Base(filename))
 }
 
-// FindFile returns the file if the given URI is already a part of the view.
-func (v *view) findFileLocked(uri span.URI) (*fileBase, error) {
+// knownFile returns true if the given URI is already a part of the view.
+func (v *view) knownFile(uri span.URI) bool {
 	v.mu.Lock()
 	defer v.mu.Unlock()
 
-	return v.findFile(uri)
+	f, err := v.findFile(uri)
+	return f != nil && err == nil
 }
 
 // getFileLocked returns a File for the given URI. It will always succeed because it
@@ -510,8 +511,13 @@ func (v *view) invalidateContent(ctx context.Context, uri span.URI, kind source.
 
 	// Cancel all still-running previous requests, since they would be
 	// operating on stale data.
+	//
+	// TODO(rstambler): All actions should lead to cancellation,
+	// but this will only be possible when all text synchronization events
+	// trigger diagnostics.
 	switch action {
-	case source.Change, source.Close:
+	case source.Save:
+	default:
 		v.cancelBackground()
 	}
 
@@ -522,7 +528,7 @@ func (v *view) invalidateContent(ctx context.Context, uri span.URI, kind source.
 	v.snapshotMu.Lock()
 	defer v.snapshotMu.Unlock()
 
-	v.snapshot = v.snapshot.clone(ctx, uri, kind)
+	v.snapshot = v.snapshot.clone(ctx, uri)
 	return v.snapshot
 }
 

internal/lsp/diagnostics.go

@@ -29,15 +29,14 @@ func (s *Server) diagnoseSnapshot(ctx context.Context, snapshot source.Snapshot)
 		go func(id string) {
 			ph, err := snapshot.PackageHandle(ctx, id)
 			if err != nil {
-				log.Error(ctx, "diagnoseSnapshot: no PackageHandle for workspace package", err, telemetry.Package.Of(id))
+				log.Error(ctx, "diagnoseSnapshot: no PackageHandle for package", err, telemetry.Package.Of(id))
 				return
 			}
 			reports, _, err := source.PackageDiagnostics(ctx, snapshot, ph, false, snapshot.View().Options().DisabledAnalyses)
 			if err != nil {
 				log.Error(ctx, "diagnoseSnapshot: no diagnostics", err, telemetry.Package.Of(ph.ID()))
 				return
 			}
-			// Don't publish empty diagnostics.
 			s.publishReports(ctx, reports, false)
 		}(id)
 	}
@@ -122,7 +121,7 @@ func (s *Server) publishReports(ctx context.Context, reports map[source.FileIden
 			// If the file is open, and we've already delivered diagnostics for
 			// a later version, do nothing. This only works for open files,
 			// since their contents in the editor are the source of truth.
-			if s.session.IsOpen(fileID.URI) && fileID.Version < delivered.version {
+			if s.session.IsOpen(fileID.URI); fileID.Version < delivered.version {
 				continue
 			}
 			geqVersion := fileID.Version >= delivered.version && delivered.version > 0

internal/lsp/source/view.go

@@ -28,10 +28,6 @@ type Snapshot interface {
 	// if it is not already part of the view.
 	GetFile(uri span.URI) (FileHandle, error)
 
-	// FindFile returns the file object for a given URI if it is
-	// already part of the view.
-	FindFile(uri span.URI) FileHandle
-
 	// Analyze runs the analyses for the given package at this snapshot.
 	Analyze(ctx context.Context, id string, analyzers []*analysis.Analyzer) ([]*Error, error)
 
@@ -157,16 +153,13 @@ type Session interface {
 	// content from the underlying cache if no overlay is present.
 	FileSystem
 
-	// IsOpen returns whether the editor currently has a file open.
+	// IsOpen returns whether the editor currently has a file open,
+	// and if its contents are saved on disk or not.
 	IsOpen(uri span.URI) bool
 
 	// DidModifyFile reports a file modification to the session.
 	DidModifyFile(ctx context.Context, c FileModification) ([]Snapshot, error)
 
-	// 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, action FileAction) bool
-
 	// Options returns a copy of the SessionOptions for this session.
 	Options() Options
 
@@ -179,8 +172,12 @@ type FileModification struct {
 	URI    span.URI
 	Action FileAction
 
+	// OnDisk is true if a watched file is changed on disk.
+	// If true, Version will be -1 and Text will be nil.
+	OnDisk bool
+
 	// Version will be -1 and Text will be nil when they are not supplied,
-	// specifically on textDocument/didClose.
+	// specifically on textDocument/didClose and for on-disk changes.
 	Version float64
 	Text    []byte
 

internal/lsp/text_synchronization.go

@@ -12,6 +12,7 @@ import (
 	"golang.org/x/tools/internal/jsonrpc2"
 	"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"
 	errors "golang.org/x/xerrors"
 )
@@ -87,6 +88,39 @@ func (s *Server) didChange(ctx context.Context, params *protocol.DidChangeTextDo
 	return nil
 }
 
+func (s *Server) didChangeWatchedFiles(ctx context.Context, params *protocol.DidChangeWatchedFilesParams) error {
+	// Keep track of each change's view and final snapshot.
+	views := make(map[source.View]source.Snapshot)
+	for _, change := range params.Changes {
+		uri := span.NewURI(change.URI)
+		ctx := telemetry.File.With(ctx, uri)
+
+		// Do nothing if the file is open in the editor.
+		// The editor is the source of truth.
+		if s.session.IsOpen(uri) {
+			continue
+		}
+		snapshots, err := s.session.DidModifyFile(ctx, source.FileModification{
+			URI:    uri,
+			Action: changeTypeToFileAction(change.Type),
+			OnDisk: true,
+		})
+		if err != nil {
+			return err
+		}
+		snapshot, _, err := snapshotOf(s.session, uri, snapshots)
+		if err != nil {
+			return err
+		}
+		views[snapshot.View()] = snapshot
+	}
+	// Diagnose all resulting snapshots.
+	for view, snapshot := range views {
+		go s.diagnoseSnapshot(view.BackgroundContext(), snapshot)
+	}
+	return nil
+}
+
 func (s *Server) didSave(ctx context.Context, params *protocol.DidSaveTextDocumentParams) error {
 	c := source.FileModification{
 		URI:     span.NewURI(params.TextDocument.URI),
@@ -189,3 +223,15 @@ func (s *Server) applyIncrementalChanges(ctx context.Context, uri span.URI, chan
 	}
 	return content, nil
 }
+
+func changeTypeToFileAction(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
+}