From 86d412b4c6eacdfb118ca50fe39b3b006b76de02 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Thu, 9 Jan 2020 22:45:06 -0500 Subject: [PATCH] internal/lsp: fix support for watching changed files This is the beginning of the CLs to refactor the file watching code with the normal text synchronization code. This hasn't yet been tested other than with some minimal local testing, so follow-up CLs will be needed. Updates golang/go#31553 Change-Id: Id081ecc59dd2903057164171bd95f0dc07baa5f1 Reviewed-on: https://go-review.googlesource.com/c/tools/+/214277 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- internal/lsp/cache/overlay.go | 25 +++--- internal/lsp/cache/session.go | 56 ++++++++------ internal/lsp/cache/snapshot.go | 18 ++--- internal/lsp/cache/view.go | 16 ++-- internal/lsp/diagnostics.go | 5 +- internal/lsp/source/view.go | 17 ++--- internal/lsp/text_synchronization.go | 46 +++++++++++ internal/lsp/watched_files.go | 110 --------------------------- 8 files changed, 121 insertions(+), 172 deletions(-) delete mode 100644 internal/lsp/watched_files.go diff --git a/internal/lsp/cache/overlay.go b/internal/lsp/cache/overlay.go index ad07a18f794..42ce1eabe8c 100644 --- a/internal/lsp/cache/overlay.go +++ b/internal/lsp/cache/overlay.go @@ -20,9 +20,9 @@ type overlay struct { version float64 kind source.FileKind - // sameContentOnDisk is true if a file has been saved on disk, + // 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. - sameContentOnDisk bool + saved bool } func (o *overlay) FileSystem() source.FileSystem { @@ -42,6 +42,11 @@ func (o *overlay) Read(ctx context.Context) ([]byte, string, error) { } func (s *session) updateOverlay(ctx context.Context, c source.FileModification) (source.FileKind, error) { + // Make sure that the file was not changed on disk. + if c.OnDisk { + return source.UnknownKind, errors.Errorf("updateOverlay called for an on-disk change: %s", c.URI) + } + s.overlayMu.Lock() defer s.overlayMu.Unlock() @@ -90,13 +95,13 @@ func (s *session) updateOverlay(ctx context.Context, c source.FileModification) sameContentOnDisk = true } s.overlays[c.URI] = &overlay{ - session: s, - uri: c.URI, - version: c.Version, - text: text, - kind: kind, - hash: hash, - sameContentOnDisk: sameContentOnDisk, + session: s, + uri: c.URI, + version: c.Version, + text: text, + kind: kind, + hash: hash, + saved: sameContentOnDisk, } return kind, nil } @@ -119,7 +124,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 { + if overlay.saved { continue } overlays[uri.Filename()] = overlay.text diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 6b4be1c5c37..495987e9adf 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -258,24 +258,47 @@ 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) ([]source.Snapshot, error) { +func (s *session) DidModifyFile(ctx context.Context, c source.FileModification) (snapshots []source.Snapshot, err error) { ctx = telemetry.URI.With(ctx, c.URI) - // Perform the session-specific updates. - kind, err := s.updateOverlay(ctx, c) - if err != nil { - return nil, err + // Update overlays only if the file was changed in the editor. + var kind source.FileKind + if !c.OnDisk { + kind, err = s.updateOverlay(ctx, c) + if err != nil { + return nil, err + } } - - var snapshots []source.Snapshot for _, view := range s.viewsOf(c.URI) { if view.Ignore(c.URI) { return nil, errors.Errorf("ignored file %v", c.URI) } - // Make sure to add the file to the view. - if _, err := view.getFileLocked(c.URI); err != nil { + // If the file was changed or deleted on disk, + // do nothing if the view isn't already aware of the file. + if c.OnDisk { + switch c.Action { + case source.Change, source.Delete: + if !view.knownFile(c.URI) { + continue + } + } + } + // Make sure that the file is added to the view. + f, err := view.getFileLocked(c.URI) + if err != nil { return nil, err } + // If the file change was on disk, the file kind is not known. + if c.OnDisk { + // If the file was already known in the snapshot, + // then use the already known file kind. Otherwise, + // detect the file kind. This should only be needed for file creates. + if fh := view.getSnapshot().findFileHandle(ctx, f); fh != nil { + kind = fh.Identity().Kind + } else { + kind = source.DetectLanguage("", c.URI.Filename()) + } + } snapshots = append(snapshots, view.invalidateContent(ctx, c.URI, kind, c.Action)) } return snapshots, nil @@ -296,18 +319,3 @@ 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) DidChangeOutOfBand(ctx context.Context, uri span.URI, action source.FileAction) bool { - view, err := s.viewOf(uri) - if err != nil { - return false - } - // Make sure that the file is part of the view. - if _, err := view.getFileLocked(uri); err != nil { - return false - } - // TODO(golang/go#31553): Remove this when this issue has been resolved. - kind := source.DetectLanguage("", uri.Filename()) - view.invalidateContent(ctx, uri, kind, action) - return true -} diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 8a5dd2ac672..d1b6d51d8b7 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -522,15 +522,6 @@ func (s *snapshot) getFileURIs() []span.URI { return uris } -// FindFile returns the file if the given URI is already a part of the view. -func (s *snapshot) FindFile(uri span.URI) source.FileHandle { - f, err := s.view.findFileLocked(uri) - if f == nil || err != nil { - return nil - } - return s.getFileHandle(f) -} - // GetFile returns a File for the given URI. It will always succeed because it // adds the file to the managed set if needed. func (s *snapshot) GetFile(uri span.URI) (source.FileHandle, error) { @@ -551,7 +542,14 @@ func (s *snapshot) getFileHandle(f *fileBase) source.FileHandle { return s.files[f.URI()] } -func (s *snapshot) clone(ctx context.Context, withoutURI span.URI, withoutFileKind source.FileKind) *snapshot { +func (s *snapshot) findFileHandle(ctx context.Context, f *fileBase) source.FileHandle { + s.mu.Lock() + defer s.mu.Unlock() + + return s.files[f.URI()] +} + +func (s *snapshot) clone(ctx context.Context, withoutURI span.URI) *snapshot { s.mu.Lock() defer s.mu.Unlock() diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 16555e36a12..4aa338b91ac 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -342,12 +342,13 @@ func basename(filename string) string { return strings.ToLower(filepath.Base(filename)) } -// FindFile returns the file if the given URI is already a part of the view. -func (v *view) findFileLocked(uri span.URI) (*fileBase, error) { +// knownFile returns true if the given URI is already a part of the view. +func (v *view) knownFile(uri span.URI) bool { v.mu.Lock() defer v.mu.Unlock() - return v.findFile(uri) + f, err := v.findFile(uri) + return f != nil && err == nil } // getFileLocked returns a File for the given URI. It will always succeed because it @@ -510,8 +511,13 @@ func (v *view) invalidateContent(ctx context.Context, uri span.URI, kind source. // Cancel all still-running previous requests, since they would be // operating on stale data. + // + // TODO(rstambler): All actions should lead to cancellation, + // but this will only be possible when all text synchronization events + // trigger diagnostics. switch action { - case source.Change, source.Close: + case source.Save: + default: v.cancelBackground() } @@ -522,7 +528,7 @@ func (v *view) invalidateContent(ctx context.Context, uri span.URI, kind source. v.snapshotMu.Lock() defer v.snapshotMu.Unlock() - v.snapshot = v.snapshot.clone(ctx, uri, kind) + v.snapshot = v.snapshot.clone(ctx, uri) return v.snapshot } diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 4887eb44cae..17ff4870d0c 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -29,7 +29,7 @@ func (s *Server) diagnoseSnapshot(ctx context.Context, snapshot source.Snapshot) go func(id string) { ph, err := snapshot.PackageHandle(ctx, id) if err != nil { - log.Error(ctx, "diagnoseSnapshot: no PackageHandle for workspace package", err, telemetry.Package.Of(id)) + log.Error(ctx, "diagnoseSnapshot: no PackageHandle for package", err, telemetry.Package.Of(id)) return } reports, _, err := source.PackageDiagnostics(ctx, snapshot, ph, false, snapshot.View().Options().DisabledAnalyses) @@ -37,7 +37,6 @@ func (s *Server) diagnoseSnapshot(ctx context.Context, snapshot source.Snapshot) log.Error(ctx, "diagnoseSnapshot: no diagnostics", err, telemetry.Package.Of(ph.ID())) return } - // Don't publish empty diagnostics. s.publishReports(ctx, reports, false) }(id) } @@ -122,7 +121,7 @@ func (s *Server) publishReports(ctx context.Context, reports map[source.FileIden // If the file is open, and we've already delivered diagnostics for // a later version, do nothing. This only works for open files, // since their contents in the editor are the source of truth. - if s.session.IsOpen(fileID.URI) && fileID.Version < delivered.version { + if s.session.IsOpen(fileID.URI); fileID.Version < delivered.version { continue } geqVersion := fileID.Version >= delivered.version && delivered.version > 0 diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 4687c9880a6..2eebc076b01 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -28,10 +28,6 @@ type Snapshot interface { // if it is not already part of the view. GetFile(uri span.URI) (FileHandle, error) - // FindFile returns the file object for a given URI if it is - // already part of the view. - FindFile(uri span.URI) FileHandle - // Analyze runs the analyses for the given package at this snapshot. Analyze(ctx context.Context, id string, analyzers []*analysis.Analyzer) ([]*Error, error) @@ -157,16 +153,13 @@ type Session interface { // content from the underlying cache if no overlay is present. FileSystem - // IsOpen returns whether the editor currently has a file open. + // 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. 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. - DidChangeOutOfBand(ctx context.Context, uri span.URI, action FileAction) bool - // Options returns a copy of the SessionOptions for this session. Options() Options @@ -179,8 +172,12 @@ type FileModification struct { URI span.URI Action FileAction + // OnDisk is true if a watched file is changed on disk. + // If true, Version will be -1 and Text will be nil. + OnDisk bool + // Version will be -1 and Text will be nil when they are not supplied, - // specifically on textDocument/didClose. + // specifically on textDocument/didClose and for on-disk changes. Version float64 Text []byte diff --git a/internal/lsp/text_synchronization.go b/internal/lsp/text_synchronization.go index 6b4ff3058e9..93e588d97a6 100644 --- a/internal/lsp/text_synchronization.go +++ b/internal/lsp/text_synchronization.go @@ -12,6 +12,7 @@ import ( "golang.org/x/tools/internal/jsonrpc2" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" + "golang.org/x/tools/internal/lsp/telemetry" "golang.org/x/tools/internal/span" errors "golang.org/x/xerrors" ) @@ -87,6 +88,39 @@ func (s *Server) didChange(ctx context.Context, params *protocol.DidChangeTextDo return nil } +func (s *Server) didChangeWatchedFiles(ctx context.Context, params *protocol.DidChangeWatchedFilesParams) error { + // Keep track of each change's view and final snapshot. + views := make(map[source.View]source.Snapshot) + for _, change := range params.Changes { + uri := span.NewURI(change.URI) + ctx := telemetry.File.With(ctx, uri) + + // Do nothing if the file is open in the editor. + // The editor is the source of truth. + if s.session.IsOpen(uri) { + continue + } + snapshots, err := s.session.DidModifyFile(ctx, source.FileModification{ + URI: uri, + Action: changeTypeToFileAction(change.Type), + OnDisk: true, + }) + if err != nil { + return err + } + snapshot, _, err := snapshotOf(s.session, uri, snapshots) + if err != nil { + return err + } + views[snapshot.View()] = snapshot + } + // Diagnose all resulting snapshots. + for view, snapshot := range views { + go s.diagnoseSnapshot(view.BackgroundContext(), snapshot) + } + return nil +} + func (s *Server) didSave(ctx context.Context, params *protocol.DidSaveTextDocumentParams) error { c := source.FileModification{ URI: span.NewURI(params.TextDocument.URI), @@ -189,3 +223,15 @@ func (s *Server) applyIncrementalChanges(ctx context.Context, uri span.URI, chan } return content, nil } + +func changeTypeToFileAction(ct protocol.FileChangeType) source.FileAction { + switch ct { + case protocol.Changed: + return source.Change + case protocol.Created: + return source.Create + case protocol.Deleted: + return source.Delete + } + return source.UnknownFileAction +} diff --git a/internal/lsp/watched_files.go b/internal/lsp/watched_files.go deleted file mode 100644 index f2c8b2f2835..00000000000 --- a/internal/lsp/watched_files.go +++ /dev/null @@ -1,110 +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 lsp - -import ( - "context" - - "golang.org/x/tools/internal/lsp/protocol" - "golang.org/x/tools/internal/lsp/source" - "golang.org/x/tools/internal/lsp/telemetry" - "golang.org/x/tools/internal/span" - "golang.org/x/tools/internal/telemetry/log" -) - -func (s *Server) didChangeWatchedFiles(ctx context.Context, params *protocol.DidChangeWatchedFilesParams) error { - for _, change := range params.Changes { - uri := span.NewURI(change.URI) - ctx := telemetry.File.With(ctx, uri) - - for _, view := range s.session.Views() { - action := toFileAction(change.Type) - switch action { - case source.Change, source.Create: - // If client has this file open, don't do anything. - // The client's contents must remain the source of truth. - if s.session.IsOpen(uri) { - break - } - if s.session.DidChangeOutOfBand(ctx, uri, action) { - // If we had been tracking the given file, - // recompute diagnostics to reflect updated file contents. - snapshot := view.Snapshot() - fh, err := snapshot.GetFile(uri) - if err != nil { - return err - } - switch fh.Identity().Kind { - case source.Go: - go s.diagnoseFile(snapshot, fh) - case source.Mod: - go s.diagnoseModfile(snapshot) - } - - return nil - } - case source.Delete: - snapshot := view.Snapshot() - fh := snapshot.FindFile(uri) - // If we have never seen this file before, there is nothing to do. - if fh == nil { - continue - } - phs, err := snapshot.PackageHandles(ctx, fh) - if err != nil { - log.Error(ctx, "didChangeWatchedFiles: CheckPackageHandles", err, telemetry.File) - continue - } - ph, err := source.WidestCheckPackageHandle(phs) - if err != nil { - log.Error(ctx, "didChangeWatchedFiles: WidestCheckPackageHandle", err, telemetry.File) - continue - } - // Find a different file in the same package we can use to trigger diagnostics. - // TODO(rstambler): Allow diagnostics to be called per-package to avoid this. - var otherFile source.FileHandle - for _, pgh := range ph.CompiledGoFiles() { - if pgh.File().Identity().URI == fh.Identity().URI { - continue - } - if f := snapshot.FindFile(pgh.File().Identity().URI); f != nil && s.session.IsOpen(fh.Identity().URI) { - otherFile = f - break - } - } - - // Notify the view of the deletion of the file. - s.session.DidChangeOutOfBand(ctx, uri, action) - - // If this was the only file in the package, clear its diagnostics. - if otherFile == nil { - if err := s.client.PublishDiagnostics(ctx, &protocol.PublishDiagnosticsParams{ - URI: protocol.NewURI(uri), - Version: fh.Identity().Version, - }); err != nil { - log.Error(ctx, "failed to clear diagnostics", err, telemetry.URI.Of(uri)) - } - return nil - } - - // Refresh diagnostics for the package the file belonged to. - go s.diagnoseFile(view.Snapshot(), otherFile) - } - } - } - return nil -} - -func toFileAction(ct protocol.FileChangeType) source.FileAction { - switch ct { - case protocol.Changed: - return source.Change - case protocol.Created: - return source.Create - case protocol.Deleted: - return source.Delete - } - return source.UnknownFileAction -}