internal/lsp: return snapshots from text modifications

Eliminate the file watcher, since it led to a lot of confusion and
difficulty reasoning about the flow of a file action. This change splits
a file invalidation into the two logical steps - 1) things that affect
the overlay, and 2) things that affect the view. It is based on top of
CL 211757, so the diffs will look better once that CL is merged.

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

internal/lsp/cache/cache.go

@@ -76,11 +76,10 @@ func (c *cache) GetFile(uri span.URI, kind source.FileKind) source.FileHandle {
 func (c *cache) NewSession(ctx context.Context) source.Session {
 	index := atomic.AddInt64(&sessionIndex, 1)
 	s := &session{
-		cache:         c,
-		id:            strconv.FormatInt(index, 10),
-		options:       source.DefaultOptions,
-		overlays:      make(map[span.URI]*overlay),
-		filesWatchMap: NewWatchMap(),
+		cache:    c,
+		id:       strconv.FormatInt(index, 10),
+		options:  source.DefaultOptions,
+		overlays: make(map[span.URI]*overlay),
 	}
 	debug.AddSession(debugSession{s})
 	return s

internal/lsp/cache/overlay.go

@@ -5,10 +5,12 @@
 package cache
 
 import (
+	"bytes"
 	"context"
 
 	"golang.org/x/tools/internal/lsp/source"
 	"golang.org/x/tools/internal/span"
+	errors "golang.org/x/xerrors"
 )
 
 type overlay struct {
@@ -40,43 +42,50 @@ func (o *overlay) Read(ctx context.Context) ([]byte, string, error) {
 	return o.data, o.hash, nil
 }
 
-func (s *session) setOverlay(f source.File, version float64, data []byte) {
+func (s *session) setOverlay(uri span.URI, version float64, data []byte) error {
 	s.overlayMu.Lock()
-	defer func() {
-		s.overlayMu.Unlock()
-		s.filesWatchMap.Notify(f.URI(), source.Change)
-	}()
+	defer s.overlayMu.Unlock()
 
-	if data == nil {
-		delete(s.overlays, f.URI())
-		return
+	o, ok := s.overlays[uri]
+	if !ok {
+		return errors.Errorf("setting overlay for unopened file %s", uri)
 	}
-
-	s.overlays[f.URI()] = &overlay{
+	s.overlays[uri] = &overlay{
 		session: s,
-		uri:     f.URI(),
-		kind:    f.Kind(),
+		uri:     uri,
+		kind:    o.kind,
 		data:    data,
 		hash:    hashContents(data),
 		version: version,
 	}
+	return nil
 }
 
-func (s *session) clearOverlay(uri span.URI) {
+func (s *session) closeOverlay(uri span.URI) error {
+	s.openFiles.Delete(uri)
+
 	s.overlayMu.Lock()
 	defer s.overlayMu.Unlock()
 
+	_, ok := s.overlays[uri]
+	if !ok {
+		return errors.Errorf("closing unopened overlay %s", uri)
+	}
 	delete(s.overlays, uri)
+	return nil
 }
 
-// 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, version float64, data []byte) {
+func (s *session) openOverlay(ctx context.Context, uri span.URI, languageID string, version float64, data []byte) error {
+	kind := source.DetectLanguage(languageID, uri.Filename())
+	if kind == source.UnknownKind {
+		return errors.Errorf("openOverlay: unknown file kind for %s", uri)
+	}
+
+	s.openFiles.Store(uri, true)
+
 	s.overlayMu.Lock()
-	defer func() {
-		s.overlayMu.Unlock()
-		s.filesWatchMap.Notify(uri, source.Open)
-	}()
+	defer s.overlayMu.Unlock()
+
 	s.overlays[uri] = &overlay{
 		session: s,
 		uri:     uri,
@@ -91,6 +100,30 @@ func (s *session) openOverlay(ctx context.Context, uri span.URI, kind source.Fil
 			s.overlays[uri].sameContentOnDisk = true
 		}
 	}
+	return nil
+}
+
+func (s *session) saveOverlay(uri span.URI, version float64, data []byte) error {
+	s.overlayMu.Lock()
+	defer s.overlayMu.Unlock()
+
+	o, ok := s.overlays[uri]
+	if !ok {
+		return errors.Errorf("saveOverlay: unopened overlay %s", uri)
+	}
+	if o.version != version {
+		return errors.Errorf("saveOverlay: saving %s at version %v, currently at %v", uri, version, o.version)
+	}
+	if data != nil {
+		if !bytes.Equal(o.data, data) {
+			return errors.Errorf("saveOverlay: overlay %s changed on save", uri)
+		}
+		o.data = data
+	}
+	o.sameContentOnDisk = true
+	o.version = version
+
+	return nil
 }
 
 func (s *session) readOverlay(uri span.URI) *overlay {
@@ -110,6 +143,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 {
 			continue
 		}

internal/lsp/cache/session.go

@@ -6,7 +6,6 @@ package cache
 
 import (
 	"context"
-	"path/filepath"
 	"strconv"
 	"strings"
 	"sync"
@@ -35,8 +34,7 @@ type session struct {
 	overlayMu sync.Mutex
 	overlays  map[span.URI]*overlay
 
-	openFiles     sync.Map
-	filesWatchMap *WatchMap
+	openFiles sync.Map
 }
 
 func (s *session) Options() source.Options {
@@ -275,79 +273,41 @@ 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) error {
+func (s *session) DidModifyFile(ctx context.Context, c source.FileModification) ([]source.Snapshot, error) {
 	ctx = telemetry.URI.With(ctx, c.URI)
+
+	// Perform session-specific actions.
+	switch c.Action {
+	case source.Open:
+		if err := s.openOverlay(ctx, c.URI, c.LanguageID, c.Version, c.Text); err != nil {
+			return nil, err
+		}
+	case source.Change:
+		if err := s.setOverlay(c.URI, c.Version, c.Text); err != nil {
+			return nil, err
+		}
+	case source.Save:
+		if err := s.saveOverlay(c.URI, c.Version, c.Text); err != nil {
+			return nil, err
+		}
+	case source.Close:
+		if err := s.closeOverlay(c.URI); err != nil {
+			return nil, err
+		}
+	}
+	var snapshots []source.Snapshot
 	for _, view := range s.viewsOf(c.URI) {
-		switch c.Action {
-		case source.Open:
-			kind := source.DetectLanguage(c.LanguageID, c.URI.Filename())
-			if kind == source.UnknownKind {
-				return errors.Errorf("DidModifyFile: unknown file kind for %s", c.URI)
-			}
-			return s.didOpen(ctx, c.URI, kind, c.Version, c.Text)
-		case source.Save:
-			s.didSave(c.URI, c.Version)
-		case source.Close:
-			s.didClose(c.URI)
+		if view.Ignore(c.URI) {
+			return nil, errors.Errorf("ignored file %v", c.URI)
 		}
-
 		// Set the content for the file, only for didChange and didClose events.
-		switch c.Action {
-		case source.Change, source.Close:
-			f, err := view.GetFile(ctx, c.URI)
-			if err != nil {
-				return err
-			}
-			view.setContent(ctx, f, c.Version, c.Text)
+		f, err := view.GetFile(ctx, c.URI)
+		if err != nil {
+			return nil, err
 		}
+		snapshots = append(snapshots, view.invalidateContent(ctx, f, c.Action))
 	}
-	return nil
-}
-
-// TODO: Propagate the language ID through to the view.
-func (s *session) didOpen(ctx context.Context, uri span.URI, kind source.FileKind, version float64, text []byte) error {
-	ctx = telemetry.File.With(ctx, uri)
-
-	// Files with _ prefixes are ignored.
-	if strings.HasPrefix(filepath.Base(uri.Filename()), "_") {
-		for _, view := range s.views {
-			view.ignoredURIsMu.Lock()
-			view.ignoredURIs[uri] = struct{}{}
-			view.ignoredURIsMu.Unlock()
-		}
-		return nil
-	}
-
-	// Make sure that the file gets added to the session's file watch map.
-	view, err := s.bestView(uri)
-	if err != nil {
-		return err
-	}
-	if _, err := view.GetFile(ctx, uri); err != nil {
-		return err
-	}
-
-	// Mark the file as open.
-	s.openFiles.Store(uri, true)
-
-	// 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, version, text)
-	return nil
-}
-
-func (s *session) didSave(uri span.URI, version float64) {
-	s.overlayMu.Lock()
-	defer s.overlayMu.Unlock()
-
-	if overlay, ok := s.overlays[uri]; ok {
-		overlay.sameContentOnDisk = true
-		overlay.version = version
-	}
-}
-
-func (s *session) didClose(uri span.URI) {
-	s.openFiles.Delete(uri)
+	return snapshots, nil
 }
 
 func (s *session) IsOpen(uri span.URI) bool {
@@ -364,5 +324,14 @@ func (s *session) GetFile(uri span.URI, kind source.FileKind) source.FileHandle
 }
 
 func (s *session) DidChangeOutOfBand(ctx context.Context, uri span.URI, action source.FileAction) bool {
-	return s.filesWatchMap.Notify(uri, action)
+	view, err := s.viewOf(uri)
+	if err != nil {
+		return false
+	}
+	f, err := view.GetFile(ctx, uri)
+	if err != nil {
+		return false
+	}
+	view.invalidateContent(ctx, f, action)
+	return true
 }

internal/lsp/cache/snapshot.go

@@ -590,22 +590,6 @@ func (s *snapshot) ID() uint64 {
 	return s.id
 }
 
-// invalidateContent invalidates the content of a Go file,
-// including any position and type information that depends on it.
-// It returns true if we were already tracking the given file, false otherwise.
-func (v *view) invalidateContent(ctx context.Context, f source.File, action source.FileAction) {
-	// 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()
-
-	// If we are deleting a file, make sure to clear out the overlay.
-	if action == source.Delete {
-		v.session.clearOverlay(f.URI())
-	}
-
-	v.snapshot = v.snapshot.clone(ctx, f)
-}
-
 func (s *snapshot) clearAndRebuildImportGraph() {
 	s.mu.Lock()
 	defer s.mu.Unlock()

internal/lsp/cache/view.go

@@ -12,6 +12,7 @@ import (
 	"go/token"
 	"os"
 	"os/exec"
+	"path/filepath"
 	"reflect"
 	"strings"
 	"sync"
@@ -25,7 +26,6 @@ import (
 	"golang.org/x/tools/internal/span"
 	"golang.org/x/tools/internal/telemetry/log"
 	"golang.org/x/tools/internal/telemetry/tag"
-	"golang.org/x/tools/internal/xcontext"
 	errors "golang.org/x/xerrors"
 )
 
@@ -313,6 +313,13 @@ func (v *view) Ignore(uri span.URI) bool {
 	defer v.ignoredURIsMu.Unlock()
 
 	_, ok := v.ignoredURIs[uri]
+
+	// Files with _ prefixes are always ignored.
+	if !ok && strings.HasPrefix(filepath.Base(uri.Filename()), "_") {
+		v.ignoredURIs[uri] = struct{}{}
+		return true
+	}
+
 	return ok
 }
 
@@ -338,21 +345,31 @@ func (v *view) getSnapshot() *snapshot {
 	return v.snapshot
 }
 
-// setContent sets the overlay contents for a file.
-func (v *view) setContent(ctx context.Context, f source.File, version float64, content []byte) {
+// invalidateContent invalidates the content of a Go file,
+// including any position and type information that depends on it.
+// It returns true if we were already tracking the given file, false otherwise.
+func (v *view) invalidateContent(ctx context.Context, f source.File, action source.FileAction) source.Snapshot {
+	// Cancel all still-running previous requests, since they would be
+	// operating on stale data.
+	switch action {
+	case source.Change, source.Close:
+		v.cancelBackground()
+	}
+
+	// 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()
+
+	v.snapshot = v.snapshot.clone(ctx, f)
+	return v.snapshot
+}
+
+func (v *view) cancelBackground() {
 	v.mu.Lock()
 	defer v.mu.Unlock()
 
-	if v.Ignore(f.URI()) {
-		return
-	}
-
-	// Cancel all still-running previous requests, since they would be
-	// operating on stale data.
 	v.cancel()
 	v.backgroundCtx, v.cancel = context.WithCancel(v.baseCtx)
-
-	v.session.setOverlay(f, version, content)
 }
 
 // FindFile returns the file if the given URI is already a part of the view.
@@ -390,11 +407,6 @@ func (v *view) getFile(ctx context.Context, uri span.URI, kind source.FileKind)
 		fname: uri.Filename(),
 		kind:  kind,
 	}
-	v.session.filesWatchMap.Watch(uri, func(action source.FileAction) bool {
-		ctx := xcontext.Detach(ctx)
-		v.invalidateContent(ctx, f, action)
-		return true // we're the only watcher
-	})
 	v.mapFile(uri, f)
 	return f, nil
 }

internal/lsp/cache/watcher.go

@@ -1,67 +0,0 @@
-// 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 cache
-
-import (
-	"sync"
-
-	"golang.org/x/tools/internal/lsp/source"
-)
-
-type watcher struct {
-	id       uint64
-	callback func(action source.FileAction) bool
-}
-
-type WatchMap struct {
-	mu       sync.Mutex
-	nextID   uint64
-	watchers map[interface{}][]watcher
-}
-
-func NewWatchMap() *WatchMap {
-	return &WatchMap{watchers: make(map[interface{}][]watcher)}
-}
-
-func (w *WatchMap) Watch(key interface{}, callback func(source.FileAction) bool) func() {
-	w.mu.Lock()
-	defer w.mu.Unlock()
-	id := w.nextID
-	w.nextID++
-	w.watchers[key] = append(w.watchers[key], watcher{
-		id:       id,
-		callback: callback,
-	})
-	return func() {
-		// unwatch if invoked
-		w.mu.Lock()
-		defer w.mu.Unlock()
-		// find and delete the watcher entry
-		entries := w.watchers[key]
-		for i, entry := range entries {
-			if entry.id == id {
-				// found it
-				entries[i] = entries[len(entries)-1]
-				entries = entries[:len(entries)-1]
-			}
-		}
-	}
-}
-
-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()
-	entries := w.watchers[key]
-	entriesCopy := make([]watcher, len(entries))
-	copy(entriesCopy, entries)
-	w.mu.Unlock()
-
-	var result bool
-	for _, entry := range entriesCopy {
-		result = entry.callback(action) || result
-	}
-	return result
-}

internal/lsp/lsp_test.go

@@ -61,7 +61,7 @@ func testLSP(t *testing.T, exporter packagestest.Exporter) {
 		if kind != source.Go {
 			continue
 		}
-		if err := session.DidModifyFile(ctx, source.FileModification{
+		if _, err := session.DidModifyFile(ctx, source.FileModification{
 			URI:        span.FileURI(filename),
 			Action:     source.Open,
 			Version:    -1,

internal/lsp/source/source_test.go

@@ -65,7 +65,7 @@ func testSource(t *testing.T, exporter packagestest.Exporter) {
 		if kind != source.Go {
 			continue
 		}
-		if err := session.DidModifyFile(ctx, source.FileModification{
+		if _, err := session.DidModifyFile(ctx, source.FileModification{
 			URI:        span.FileURI(filename),
 			Action:     source.Open,
 			Version:    -1,

internal/lsp/source/view.go

@@ -169,7 +169,7 @@ type Session interface {
 	IsOpen(uri span.URI) bool
 
 	// DidModifyFile reports a file modification to the session.
-	DidModifyFile(ctx context.Context, c FileModification) error
+	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.

internal/lsp/text_synchronization.go

@@ -19,16 +19,17 @@ import (
 func (s *Server) didOpen(ctx context.Context, params *protocol.DidOpenTextDocumentParams) error {
 	// Confirm that the file's language ID is related to Go.
 	uri := span.NewURI(params.TextDocument.URI)
-	if err := s.session.DidModifyFile(ctx, source.FileModification{
+	snapshots, err := s.session.DidModifyFile(ctx, source.FileModification{
 		URI:        uri,
 		Action:     source.Open,
 		Version:    params.TextDocument.Version,
 		Text:       []byte(params.TextDocument.Text),
 		LanguageID: params.TextDocument.LanguageID,
-	}); err != nil {
+	})
+	if err != nil {
 		return err
 	}
-	view, err := s.session.ViewOf(uri)
+	snapshot, view, err := snapshotOf(s.session, uri, snapshots)
 	if err != nil {
 		return err
 	}
@@ -37,28 +38,7 @@ func (s *Server) didOpen(ctx context.Context, params *protocol.DidOpenTextDocume
 		return err
 	}
 	// Always run diagnostics when a file is opened.
-	return s.diagnose(view.Snapshot(), f)
-}
-
-func (s *Server) didSave(ctx context.Context, params *protocol.DidSaveTextDocumentParams) error {
-	c := source.FileModification{
-		URI:     span.NewURI(params.TextDocument.URI),
-		Action:  source.Save,
-		Version: params.TextDocument.Version,
-	}
-	if params.Text != nil {
-		c.Text = []byte(*params.Text)
-	}
-	return s.session.DidModifyFile(ctx, c)
-}
-
-func (s *Server) didClose(ctx context.Context, params *protocol.DidCloseTextDocumentParams) error {
-	return s.session.DidModifyFile(ctx, source.FileModification{
-		URI:     span.NewURI(params.TextDocument.URI),
-		Action:  source.Close,
-		Version: -1,
-		Text:    nil,
-	})
+	return s.diagnose(snapshot, f)
 }
 
 func (s *Server) didChange(ctx context.Context, params *protocol.DidChangeTextDocumentParams) error {
@@ -67,15 +47,16 @@ func (s *Server) didChange(ctx context.Context, params *protocol.DidChangeTextDo
 	if err != nil {
 		return err
 	}
-	if err := s.session.DidModifyFile(ctx, source.FileModification{
+	snapshots, err := s.session.DidModifyFile(ctx, source.FileModification{
 		URI:     uri,
 		Action:  source.Change,
 		Version: params.TextDocument.Version,
 		Text:    text,
-	}); err != nil {
+	})
+	if err != nil {
 		return err
 	}
-	view, err := s.session.ViewOf(uri)
+	snapshot, view, err := snapshotOf(s.session, uri, snapshots)
 	if err != nil {
 		return err
 	}
@@ -92,7 +73,44 @@ func (s *Server) didChange(ctx context.Context, params *protocol.DidChangeTextDo
 		return err
 	}
 	// Always update diagnostics after a file change.
-	return s.diagnose(view.Snapshot(), f)
+	return s.diagnose(snapshot, f)
+}
+
+func (s *Server) didSave(ctx context.Context, params *protocol.DidSaveTextDocumentParams) error {
+	c := source.FileModification{
+		URI:     span.NewURI(params.TextDocument.URI),
+		Action:  source.Save,
+		Version: params.TextDocument.Version,
+	}
+	if params.Text != nil {
+		c.Text = []byte(*params.Text)
+	}
+	_, err := s.session.DidModifyFile(ctx, c)
+	return err
+}
+
+func (s *Server) didClose(ctx context.Context, params *protocol.DidCloseTextDocumentParams) error {
+	_, err := s.session.DidModifyFile(ctx, source.FileModification{
+		URI:     span.NewURI(params.TextDocument.URI),
+		Action:  source.Close,
+		Version: -1,
+		Text:    nil,
+	})
+	return err
+}
+
+// snapshotOf returns the snapshot corresponding to the view for the given file URI.
+func snapshotOf(session source.Session, uri span.URI, snapshots []source.Snapshot) (source.Snapshot, source.View, error) {
+	view, err := session.ViewOf(uri)
+	if err != nil {
+		return nil, nil, err
+	}
+	for _, s := range snapshots {
+		if s.View() == view {
+			return s, view, nil
+		}
+	}
+	return nil, nil, errors.Errorf("bestSnapshot: no snapshot for %s", uri)
 }
 
 func (s *Server) wasFirstChange(uri span.URI) bool {