internal/lsp: don't queue content changes

This updates overlays immeditely, and uses the handle identity change to
correctly update the content on demand.

Fixes golang/go#32348

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

internal/lsp/cache/file.go

@@ -29,6 +29,7 @@ type fileBase struct {
 	fname string
 
 	view  *view
+	fh    source.FileHandle
 	fc    *source.FileContent
 	token *token.File
 }
@@ -70,20 +71,12 @@ func (f *fileBase) read(ctx context.Context) {
 		f.fc = &source.FileContent{Error: err}
 		return
 	}
-	if f.fc != nil {
-		if len(f.view.contentChanges) == 0 {
-			return
-		}
-
-		f.view.mcache.mu.Lock()
-		err := f.view.applyContentChanges(ctx)
-		f.view.mcache.mu.Unlock()
-
-		if err != nil {
-			f.fc = &source.FileContent{Error: err}
-			return
-		}
+	oldFH := f.fh
+	f.fh = f.view.Session().GetFile(f.URI())
+	// do we already have the right contents?
+	if f.fc != nil && f.fh.Identity() == oldFH.Identity() {
+		return
 	}
-	// We don't know the content yet, so read it.
-	f.fc = f.view.Session().GetFile(f.URI()).Read(ctx)
+	// update the contents
+	f.fc = f.fh.Read(ctx)
 }

internal/lsp/cache/gofile.go

@@ -85,7 +85,7 @@ func (f *goFile) GetPackage(ctx context.Context) source.Package {
 // isDirty is true if the file needs to be type-checked.
 // It assumes that the file's view's mutex is held by the caller.
 func (f *goFile) isDirty() bool {
-	return f.meta == nil || f.imports == nil || f.token == nil || f.ast == nil || f.pkg == nil || len(f.view.contentChanges) > 0
+	return f.meta == nil || f.imports == nil || f.token == nil || f.ast == nil || f.pkg == nil
 }
 
 func (f *goFile) astIsTrimmed() bool {

internal/lsp/cache/load.go

@@ -13,11 +13,6 @@ func (v *view) loadParseTypecheck(ctx context.Context, f *goFile) ([]packages.Er
 	v.mcache.mu.Lock()
 	defer v.mcache.mu.Unlock()
 
-	// Apply any queued-up content changes.
-	if err := v.applyContentChanges(ctx); err != nil {
-		return nil, err
-	}
-
 	// If the package for the file has not been invalidated by the application
 	// of the pending changes, there is no need to continue.
 	if !f.isDirty() {

internal/lsp/cache/session.go

@@ -65,17 +65,16 @@ func (s *session) NewView(name string, folder span.URI) source.View {
 	ctx := context.Background()
 	backgroundCtx, cancel := context.WithCancel(ctx)
 	v := &view{
-		session:        s,
-		id:             strconv.FormatInt(index, 10),
-		baseCtx:        ctx,
-		backgroundCtx:  backgroundCtx,
-		cancel:         cancel,
-		name:           name,
-		env:            os.Environ(),
-		folder:         folder,
-		filesByURI:     make(map[span.URI]viewFile),
-		filesByBase:    make(map[string][]viewFile),
-		contentChanges: make(map[span.URI]func()),
+		session:       s,
+		id:            strconv.FormatInt(index, 10),
+		baseCtx:       ctx,
+		backgroundCtx: backgroundCtx,
+		cancel:        cancel,
+		name:          name,
+		env:           os.Environ(),
+		folder:        folder,
+		filesByURI:    make(map[span.URI]viewFile),
+		filesByBase:   make(map[string][]viewFile),
 		mcache: &metadataCache{
 			packages: make(map[string]*metadata),
 		},

internal/lsp/cache/view.go

@@ -56,13 +56,6 @@ type view struct {
 	filesByURI  map[span.URI]viewFile
 	filesByBase map[string][]viewFile
 
-	// contentChanges saves the content changes for a given state of the view.
-	// When type information is requested by the view, all of the dirty changes
-	// are applied, potentially invalidating some data in the caches. The
-	// closures  in the dirty slice assume that their caller is holding the
-	// view's mutex.
-	contentChanges map[span.URI]func()
-
 	// mcache caches metadata for the packages of the opened files in a view.
 	mcache *metadataCache
 
@@ -224,33 +217,14 @@ func (v *view) SetContent(ctx context.Context, uri span.URI, content []byte) err
 	v.cancel()
 	v.backgroundCtx, v.cancel = context.WithCancel(v.baseCtx)
 
-	v.contentChanges[uri] = func() {
-		v.session.SetOverlay(uri, content)
-	}
-
-	return nil
-}
-
-// applyContentChanges applies all of the changed content stored in the view.
-// It is assumed that the caller has locked both the view's and the mcache's
-// mutexes.
-func (v *view) applyContentChanges(ctx context.Context) error {
-	if ctx.Err() != nil {
-		return ctx.Err()
-	}
-
-	v.pcache.mu.Lock()
-	defer v.pcache.mu.Unlock()
-
-	for uri, change := range v.contentChanges {
-		change()
-		delete(v.contentChanges, uri)
-	}
+	v.session.SetOverlay(uri, content)
 
 	return nil
 }
 
 func (f *goFile) invalidate() {
+	f.view.pcache.mu.Lock()
+	defer f.view.pcache.mu.Unlock()
 	// TODO(rstambler): Should we recompute these here?
 	f.ast = nil
 	f.token = nil