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 <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
diff --git a/internal/lsp/cache/overlay.go b/internal/lsp/cache/overlay.go
index ad07a18..42ce1ea 100644
--- a/internal/lsp/cache/overlay.go
+++ b/internal/lsp/cache/overlay.go
@@ -20,9 +20,9 @@
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 (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 @@
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 @@
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 6b4be1c..495987e 100644
--- a/internal/lsp/cache/session.go
+++ b/internal/lsp/cache/session.go
@@ -258,24 +258,47 @@
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 @@
// 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 8a5dd2a..d1b6d51 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -522,15 +522,6 @@
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 @@
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 16555e3..4aa338b 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -342,12 +342,13 @@
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 @@
// 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 @@
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 4887eb4..17ff487 100644
--- a/internal/lsp/diagnostics.go
+++ b/internal/lsp/diagnostics.go
@@ -29,7 +29,7 @@
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 @@
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 @@
// 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 4687c98..2eebc07 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -28,10 +28,6 @@
// 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 @@
// 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 @@
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 6b4ff30..93e588d 100644
--- a/internal/lsp/text_synchronization.go
+++ b/internal/lsp/text_synchronization.go
@@ -12,6 +12,7 @@
"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 @@
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 @@
}
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 f2c8b2f..0000000
--- 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
-}
