internal/lsp: handle on-disk file deletions for opened files

Previously, we only updated the opened file's overlay, but not the
snapshot. This meant that the snapshot was still operating with stale
data. Invalidating the snapshot creates a new snapshot with the correct
set of overlays.

The test is skipped because it will flake until we have a better caching
strategy for `go mod tidy` results.

Updates golang/go#40269

Change-Id: Ia8d1ae75127a1d18d8877923e7a5b25b7bd965ac
Reviewed-on: https://go-review.googlesource.com/c/tools/+/243537
Run-TryBot: Rebecca Stambler <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Heschi Kreinick <[email protected]>

Author: stamblerre

internal/lsp/cache/session.go

@@ -6,6 +6,7 @@ package cache
 
 import (
 	"context"
+	"fmt"
 	"strconv"
 	"strings"
 	"sync"
@@ -328,11 +329,6 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif
 		if c.Action == source.InvalidateMetadata {
 			forceReloadMetadata = true
 		}
-		// Do nothing if the file is open in the editor and we receive
-		// an on-disk action. The editor is the source of truth.
-		if s.isOpen(c.URI) && c.OnDisk {
-			continue
-		}
 		// Look through all of the session's views, invalidating the file for
 		// all of the views to which it is known.
 		for _, view := range s.views {
@@ -379,13 +375,20 @@ func (s *Session) updateOverlays(ctx context.Context, changes []source.FileModif
 	defer s.overlayMu.Unlock()
 
 	for _, c := range changes {
-		// Don't update overlays for on-disk changes or metadata invalidations.
-		if c.OnDisk || c.Action == source.InvalidateMetadata {
+		// Don't update overlays for metadata invalidations.
+		if c.Action == source.InvalidateMetadata {
 			continue
 		}
 
 		o, ok := s.overlays[c.URI]
 
+		// If the file is not opened in an overlay and the change is on disk,
+		// there's no need to update an overlay. If there is an overlay, we
+		// may need to update the overlay's saved value.
+		if !ok && c.OnDisk {
+			continue
+		}
+
 		// Determine the file kind on open, otherwise, assume it has been cached.
 		var kind source.FileKind
 		switch c.Action {
@@ -408,35 +411,46 @@ func (s *Session) updateOverlays(ctx context.Context, changes []source.FileModif
 		}
 
 		// If the file is on disk, check if its content is the same as in the
-		// overlay. Saves don't necessarily come with the file's content.
+		// overlay. Saves and on-disk file changes don't come with the file's
+		// content.
 		text := c.Text
-		if text == nil && c.Action == source.Save {
+		if text == nil && (c.Action == source.Save || c.OnDisk) {
+			if !ok {
+				return nil, fmt.Errorf("no known content for overlay for %s", c.Action)
+			}
 			text = o.text
 		}
+		// On-disk changes don't come with versions.
+		version := c.Version
+		if c.OnDisk {
+			version = o.version
+		}
 		hash := hashContents(text)
 		var sameContentOnDisk bool
 		switch c.Action {
-		case source.Open:
-			fh, err := s.cache.getFile(ctx, c.URI)
-			if err != nil {
-				return nil, err
-			}
-			_, readErr := fh.Read()
-			sameContentOnDisk = (readErr == nil && fh.Identity().Identifier == hash)
+		case source.Delete:
+			// Do nothing. sameContentOnDisk should be false.
 		case source.Save:
 			// Make sure the version and content (if present) is the same.
-			if o.version != c.Version {
+			if o.version != version {
 				return nil, errors.Errorf("updateOverlays: saving %s at version %v, currently at %v", c.URI, c.Version, o.version)
 			}
 			if c.Text != nil && o.hash != hash {
 				return nil, errors.Errorf("updateOverlays: overlay %s changed on save", c.URI)
 			}
 			sameContentOnDisk = true
+		default:
+			fh, err := s.cache.getFile(ctx, c.URI)
+			if err != nil {
+				return nil, err
+			}
+			_, readErr := fh.Read()
+			sameContentOnDisk = (readErr == nil && fh.Identity().Identifier == hash)
 		}
 		o = &overlay{
 			session: s,
 			uri:     c.URI,
-			version: c.Version,
+			version: version,
 			text:    text,
 			kind:    kind,
 			hash:    hash,
@@ -445,7 +459,8 @@ func (s *Session) updateOverlays(ctx context.Context, changes []source.FileModif
 		s.overlays[c.URI] = o
 	}
 
-	// Get the overlays for each change while the session's overlay map is locked.
+	// Get the overlays for each change while the session's overlay map is
+	// locked.
 	overlays := make(map[span.URI]*overlay)
 	for _, c := range changes {
 		if o, ok := s.overlays[c.URI]; ok {

internal/lsp/regtest/modfile_test.go

@@ -37,28 +37,58 @@ package main
 import "example.com/blah"
 
 func main() {
-	fmt.Println(blah.Name)
-}`
-	withOptions(WithProxy(proxy)).run(t, untidyModule, func(t *testing.T, env *Env) {
-		// Open the file and make sure that the initial workspace load does not
-		// modify the go.mod file.
-		goModContent := env.ReadWorkspaceFile("go.mod")
-		env.OpenFile("main.go")
-		env.Await(
-			env.DiagnosticAtRegexp("main.go", "\"example.com/blah\""),
-		)
-		if got := env.ReadWorkspaceFile("go.mod"); got != goModContent {
-			t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(goModContent, got))
-		}
-		// Save the buffer, which will format and organize imports.
-		// Confirm that the go.mod file still does not change.
-		env.SaveBuffer("main.go")
-		env.Await(
-			env.DiagnosticAtRegexp("main.go", "\"example.com/blah\""),
-		)
-		if got := env.ReadWorkspaceFile("go.mod"); got != goModContent {
-			t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(goModContent, got))
-		}
+	println(blah.Name)
+}
+`
+	t.Run("basic", func(t *testing.T) {
+		withOptions(WithProxy(proxy)).run(t, untidyModule, func(t *testing.T, env *Env) {
+			// Open the file and make sure that the initial workspace load does not
+			// modify the go.mod file.
+			goModContent := env.ReadWorkspaceFile("go.mod")
+			env.OpenFile("main.go")
+			env.Await(
+				env.DiagnosticAtRegexp("main.go", "\"example.com/blah\""),
+			)
+			if got := env.ReadWorkspaceFile("go.mod"); got != goModContent {
+				t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(goModContent, got))
+			}
+			// Save the buffer, which will format and organize imports.
+			// Confirm that the go.mod file still does not change.
+			env.SaveBuffer("main.go")
+			env.Await(
+				env.DiagnosticAtRegexp("main.go", "\"example.com/blah\""),
+			)
+			if got := env.ReadWorkspaceFile("go.mod"); got != goModContent {
+				t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(goModContent, got))
+			}
+		})
+	})
+
+	// Reproduce golang/go#40269 by deleting and recreating main.go.
+	t.Run("delete main.go", func(t *testing.T) {
+		t.Skipf("This test will be flaky until golang/go#40269 is resolved.")
+
+		withOptions(WithProxy(proxy)).run(t, untidyModule, func(t *testing.T, env *Env) {
+			goModContent := env.ReadWorkspaceFile("go.mod")
+			mainContent := env.ReadWorkspaceFile("main.go")
+			env.OpenFile("main.go")
+			env.SaveBuffer("main.go")
+
+			env.RemoveWorkspaceFile("main.go")
+			env.Await(
+				CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidOpen), 1),
+				CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidSave), 1),
+				CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChangeWatchedFiles), 2),
+			)
+
+			env.WriteWorkspaceFile("main.go", mainContent)
+			env.Await(
+				env.DiagnosticAtRegexp("main.go", "\"example.com/blah\""),
+			)
+			if got := env.ReadWorkspaceFile("go.mod"); got != goModContent {
+				t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(goModContent, got))
+			}
+		})
 	})
 }
 

internal/lsp/source/view.go

@@ -274,6 +274,27 @@ const (
 	InvalidateMetadata
 )
 
+func (a FileAction) String() string {
+	switch a {
+	case Open:
+		return "Open"
+	case Change:
+		return "Change"
+	case Close:
+		return "Close"
+	case Save:
+		return "Save"
+	case Create:
+		return "Create"
+	case Delete:
+		return "Delete"
+	case InvalidateMetadata:
+		return "InvalidateMetadata"
+	default:
+		return "Unknown"
+	}
+}
+
 // Cache abstracts the core logic of dealing with the environment from the
 // higher level logic that processes the information to produce results.
 // The cache provides access to files and their contents, so the source