diff --git a/internal/lsp/cache/overlay.go b/internal/lsp/cache/overlay.go index ad07a18f79..42ce1eabe8 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 6b4be1c5c3..495987e9ad 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 8a5dd2ac67..d1b6d51d8b 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 16555e36a1..4aa338b91a 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 4887eb44ca..17ff4870d0 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 4687c9880a..2eebc076b0 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 6b4ff3058e..93e588d97a 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 f2c8b2f283..0000000000 --- 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 -}