From 3d51b05cfb3803f91156426067b902085ee5bc4e Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Thu, 6 Feb 2020 16:20:50 -0500 Subject: [PATCH] internal/lsp: don't use overlays from the session in the snapshot Hold the session's overlay mutex the whole time we compute new overlays, and then pass these overlays directly into clone. This avoids us calling s.session.GetFile, which can return overlays that the snapshot doesn't yet "know" about. Change-Id: I1a10c78e26f8fec64550bfe0a97b5975ea8f976b Reviewed-on: https://go-review.googlesource.com/c/tools/+/218321 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- internal/lsp/cache/overlay.go | 118 ----------------------- internal/lsp/cache/session.go | 139 +++++++++++++++++++++++++-- internal/lsp/cache/snapshot.go | 21 +++- internal/lsp/cache/view.go | 7 +- internal/lsp/diagnostics.go | 2 +- internal/lsp/source/diagnostics.go | 2 +- internal/lsp/source/view.go | 8 +- internal/lsp/text_synchronization.go | 9 +- 8 files changed, 158 insertions(+), 148 deletions(-) delete mode 100644 internal/lsp/cache/overlay.go diff --git a/internal/lsp/cache/overlay.go b/internal/lsp/cache/overlay.go deleted file mode 100644 index 6d3dbd146c..0000000000 --- a/internal/lsp/cache/overlay.go +++ /dev/null @@ -1,118 +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 ( - "context" - - "golang.org/x/tools/internal/lsp/source" - "golang.org/x/tools/internal/span" - errors "golang.org/x/xerrors" -) - -type overlay struct { - session *session - uri span.URI - text []byte - hash string - version float64 - kind source.FileKind - - // 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. - saved bool -} - -func (o *overlay) FileSystem() source.FileSystem { - return o.session -} - -func (o *overlay) Identity() source.FileIdentity { - return source.FileIdentity{ - URI: o.uri, - Identifier: o.hash, - Version: o.version, - Kind: o.kind, - } -} -func (o *overlay) Read(ctx context.Context) ([]byte, string, error) { - return o.text, o.hash, nil -} - -func (s *session) updateOverlay(ctx context.Context, c source.FileModification) error { - // Make sure that the file was not changed on disk. - if c.OnDisk { - return errors.Errorf("updateOverlay called for an on-disk change: %s", c.URI) - } - - s.overlayMu.Lock() - defer s.overlayMu.Unlock() - - o, ok := s.overlays[c.URI] - - // Determine the file kind on open, otherwise, assume it has been cached. - var kind source.FileKind - switch c.Action { - case source.Open: - kind = source.DetectLanguage(c.LanguageID, c.URI.Filename()) - default: - if !ok { - return errors.Errorf("updateOverlay: modifying unopened overlay %v", c.URI) - } - kind = o.kind - } - if kind == source.UnknownKind { - return errors.Errorf("updateOverlay: unknown file kind for %s", c.URI) - } - - // Closing a file just deletes its overlay. - if c.Action == source.Close { - delete(s.overlays, c.URI) - return nil - } - - // If the file is on disk, check if its content is the same as the overlay. - text := c.Text - if text == nil { - text = o.text - } - hash := hashContents(text) - var sameContentOnDisk bool - switch c.Action { - case source.Open: - _, h, err := s.cache.GetFile(c.URI).Read(ctx) - sameContentOnDisk = (err == nil && h == hash) - case source.Save: - // Make sure the version and content (if present) is the same. - if o.version != c.Version { - return errors.Errorf("updateOverlay: saving %s at version %v, currently at %v", c.URI, c.Version, o.version) - } - if c.Text != nil && o.hash != hash { - return errors.Errorf("updateOverlay: overlay %s changed on save", c.URI) - } - sameContentOnDisk = true - } - s.overlays[c.URI] = &overlay{ - session: s, - uri: c.URI, - version: c.Version, - text: text, - kind: kind, - hash: hash, - saved: sameContentOnDisk, - } - return nil -} - -func (s *session) readOverlay(uri span.URI) *overlay { - s.overlayMu.Lock() - defer s.overlayMu.Unlock() - - // We might have the content saved in an overlay. - if overlay, ok := s.overlays[uri]; ok { - return overlay - } - return nil -} diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 2c009dd963..6111fa5b0c 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -33,6 +33,35 @@ type session struct { overlays map[span.URI]*overlay } +type overlay struct { + session *session + uri span.URI + text []byte + hash string + version float64 + kind source.FileKind + + // 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. + saved bool +} + +func (o *overlay) FileSystem() source.FileSystem { + return o.session +} + +func (o *overlay) Identity() source.FileIdentity { + return source.FileIdentity{ + URI: o.uri, + Identifier: o.hash, + Version: o.version, + Kind: o.kind, + } +} +func (o *overlay) Read(ctx context.Context) ([]byte, string, error) { + return o.text, o.hash, nil +} + func (s *session) Options() source.Options { return s.options } @@ -249,14 +278,17 @@ func (s *session) dropView(ctx context.Context, v *view) (int, error) { } func (s *session) DidModifyFiles(ctx context.Context, changes []source.FileModification) ([]source.Snapshot, error) { - views := make(map[*view][]span.URI) + views := make(map[*view]map[span.URI]source.FileHandle) + overlays, err := s.updateOverlays(ctx, changes) + if err != nil { + return nil, err + } for _, c := range changes { - // Only update overlays for in-editor changes. - if !c.OnDisk { - if err := s.updateOverlay(ctx, c); err != nil { - return nil, err - } + // 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 } for _, view := range s.viewsOf(c.URI) { if view.Ignore(c.URI) { @@ -276,7 +308,14 @@ func (s *session) DidModifyFiles(ctx context.Context, changes []source.FileModif if _, err := view.getFile(c.URI); err != nil { return nil, err } - views[view] = append(views[view], c.URI) + if _, ok := views[view]; !ok { + views[view] = make(map[span.URI]source.FileHandle) + } + if o, ok := overlays[c.URI]; ok { + views[view][c.URI] = o + } else { + views[view][c.URI] = s.cache.GetFile(c.URI) + } } } var snapshots []source.Snapshot @@ -286,7 +325,7 @@ func (s *session) DidModifyFiles(ctx context.Context, changes []source.FileModif return snapshots, nil } -func (s *session) IsOpen(uri span.URI) bool { +func (s *session) isOpen(uri span.URI) bool { s.overlayMu.Lock() defer s.overlayMu.Unlock() @@ -294,6 +333,80 @@ func (s *session) IsOpen(uri span.URI) bool { return open } +func (s *session) updateOverlays(ctx context.Context, changes []source.FileModification) (map[span.URI]*overlay, error) { + s.overlayMu.Lock() + defer s.overlayMu.Unlock() + + for _, c := range changes { + // Don't update overlays for on-disk changes. + if c.OnDisk { + continue + } + + o, ok := s.overlays[c.URI] + + // Determine the file kind on open, otherwise, assume it has been cached. + var kind source.FileKind + switch c.Action { + case source.Open: + kind = source.DetectLanguage(c.LanguageID, c.URI.Filename()) + default: + if !ok { + return nil, errors.Errorf("updateOverlays: modifying unopened overlay %v", c.URI) + } + kind = o.kind + } + if kind == source.UnknownKind { + return nil, errors.Errorf("updateOverlays: unknown file kind for %s", c.URI) + } + + // Closing a file just deletes its overlay. + if c.Action == source.Close { + delete(s.overlays, c.URI) + continue + } + + // If the file is on disk, check if its content is the same as the overlay. + text := c.Text + if text == nil { + text = o.text + } + hash := hashContents(text) + var sameContentOnDisk bool + switch c.Action { + case source.Open: + _, h, err := s.cache.GetFile(c.URI).Read(ctx) + sameContentOnDisk = (err == nil && h == hash) + case source.Save: + // Make sure the version and content (if present) is the same. + if o.version != c.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 + } + o = &overlay{ + session: s, + uri: c.URI, + version: c.Version, + text: text, + kind: kind, + hash: hash, + saved: sameContentOnDisk, + } + s.overlays[c.URI] = o + } + + // Get the overlays for each change while the session's overlay map is locked. + overlays := make(map[span.URI]*overlay) + for _, c := range changes { + overlays[c.URI] = s.overlays[c.URI] + } + return overlays, nil +} + func (s *session) GetFile(uri span.URI) source.FileHandle { if overlay := s.readOverlay(uri); overlay != nil { return overlay @@ -301,3 +414,13 @@ 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) readOverlay(uri span.URI) *overlay { + s.overlayMu.Lock() + defer s.overlayMu.Unlock() + + if overlay, ok := s.overlays[uri]; ok { + return overlay + } + return nil +} diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 7488a98bb1..41a69502df 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -108,6 +108,9 @@ func (s *snapshot) Config(ctx context.Context) *packages.Config { } func (s *snapshot) buildOverlay() map[string][]byte { + s.mu.Lock() + defer s.mu.Unlock() + overlays := make(map[string][]byte) for uri, fh := range s.files { overlay, ok := fh.(*overlay) @@ -569,15 +572,24 @@ func (s *snapshot) GetFile(uri span.URI) (source.FileHandle, error) { if err != nil { return nil, err } + s.mu.Lock() defer s.mu.Unlock() if _, ok := s.files[f.URI()]; !ok { - s.files[f.URI()] = s.view.session.GetFile(f.URI()) + s.files[f.URI()] = s.view.session.cache.GetFile(uri) } return s.files[f.URI()], nil } +func (s *snapshot) IsOpen(uri span.URI) bool { + s.mu.Lock() + defer s.mu.Unlock() + + _, open := s.files[uri].(*overlay) + return open +} + func (s *snapshot) awaitLoaded(ctx context.Context) error { // Do not return results until the snapshot's view has been initialized. s.view.awaitInitialized(ctx) @@ -685,7 +697,7 @@ func contains(views []*view, view *view) bool { return false } -func (s *snapshot) clone(ctx context.Context, withoutURIs []span.URI) *snapshot { +func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.FileHandle) *snapshot { s.mu.Lock() defer s.mu.Unlock() @@ -716,7 +728,7 @@ func (s *snapshot) clone(ctx context.Context, withoutURIs []span.URI) *snapshot // If an ID's value is true, invalidate its metadata too. transitiveIDs := make(map[packageID]bool) - for _, withoutURI := range withoutURIs { + for withoutURI, currentFH := range withoutURIs { directIDs := map[packageID]struct{}{} // Collect all of the package IDs that correspond to the given file. @@ -724,8 +736,7 @@ func (s *snapshot) clone(ctx context.Context, withoutURIs []span.URI) *snapshot for _, id := range s.ids[withoutURI] { directIDs[id] = struct{}{} } - // Get the current and original FileHandles for this URI. - currentFH := s.view.session.GetFile(withoutURI) + // The original FileHandle for this URI is cached on the snapshot. originalFH := s.files[withoutURI] // Check if the file's package name or imports have changed, diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index fc9066bb4c..4525853edc 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -239,8 +239,9 @@ func (v *view) buildBuiltinPackage(ctx context.Context, goFiles []string) error uri := span.FileURI(goFiles[0]) v.addIgnoredFile(uri) // to avoid showing diagnostics for builtin.go - // Get the FileHandle through the session to avoid adding it to the snapshot. - pgh := v.session.cache.ParseGoHandle(v.session.GetFile(uri), source.ParseFull) + // Get the FileHandle through the cache to avoid adding it to the snapshot + // and to get the file content from disk. + pgh := v.session.cache.ParseGoHandle(v.session.cache.GetFile(uri), source.ParseFull) fset := v.session.cache.fset h := v.session.cache.store.Bind(pgh.File().Identity(), func(ctx context.Context) interface{} { data := &builtinPackageData{} @@ -540,7 +541,7 @@ func (v *view) awaitInitialized(ctx context.Context) { // 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, uris []span.URI) source.Snapshot { +func (v *view) invalidateContent(ctx context.Context, uris map[span.URI]source.FileHandle) source.Snapshot { // Detach the context so that content invalidation cannot be canceled. ctx = xcontext.Detach(ctx) diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index fcc777c1c7..411344f3da 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -88,7 +88,7 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, alwaysA // Only run analyses for packages with open files. withAnalyses := alwaysAnalyze for _, fh := range ph.CompiledGoFiles() { - if s.session.IsOpen(fh.File().Identity().URI) { + if snapshot.IsOpen(fh.File().Identity().URI) { withAnalyses = true } } diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index fefbb9aa8a..2c8f3e9396 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -105,7 +105,7 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, ph PackageHandle, missi // If no file is associated with the error, pick an open file from the package. if e.URI.Filename() == "" { for _, ph := range pkg.CompiledGoFiles() { - if snapshot.View().Session().IsOpen(ph.File().Identity().URI) { + if snapshot.IsOpen(ph.File().Identity().URI) { e.URI = ph.File().Identity().URI } } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 3f0e30d02e..21c7a1d416 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -33,6 +33,10 @@ type Snapshot interface { // if it is not already part of the view. GetFile(uri span.URI) (FileHandle, error) + // 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 + // Analyze runs the analyses for the given package at this snapshot. Analyze(ctx context.Context, id string, analyzers []*analysis.Analyzer) ([]*Error, error) @@ -164,10 +168,6 @@ type Session interface { // content from the underlying cache if no overlay is present. FileSystem - // 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. // It returns the resulting snapshots, a guaranteed one per view. DidModifyFiles(ctx context.Context, changes []FileModification) ([]Snapshot, error) diff --git a/internal/lsp/text_synchronization.go b/internal/lsp/text_synchronization.go index 3d95a85291..652132d2c1 100644 --- a/internal/lsp/text_synchronization.go +++ b/internal/lsp/text_synchronization.go @@ -65,15 +65,8 @@ func (s *Server) didChange(ctx context.Context, params *protocol.DidChangeTextDo func (s *Server) didChangeWatchedFiles(ctx context.Context, params *protocol.DidChangeWatchedFilesParams) error { var modifications []source.FileModification for _, change := range params.Changes { - uri := span.NewURI(change.URI) - - // Do nothing if the file is open in the editor. - // The editor is the source of truth. - if s.session.IsOpen(uri) { - continue - } modifications = append(modifications, source.FileModification{ - URI: uri, + URI: span.NewURI(change.URI), Action: changeTypeToFileAction(change.Type), OnDisk: true, })