From acdb8c158a355fdf5304c8a552216660909dadb4 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Sun, 19 Jul 2020 15:11:07 -0400 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- internal/lsp/cache/session.go | 53 +++++++++++++------- internal/lsp/regtest/modfile_test.go | 74 +++++++++++++++++++--------- internal/lsp/source/view.go | 21 ++++++++ 3 files changed, 107 insertions(+), 41 deletions(-) diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 0c815874e4..bec8fce39a 100644 --- a/internal/lsp/cache/session.go +++ b/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 { diff --git a/internal/lsp/regtest/modfile_test.go b/internal/lsp/regtest/modfile_test.go index 3b5c2f829d..503a789c09 100644 --- a/internal/lsp/regtest/modfile_test.go +++ b/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)) + } + }) }) } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 195cb9012f..036da6cadc 100644 --- a/internal/lsp/source/view.go +++ b/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