From 0a1579a33b0268e5c6cc226b86d3b89350a73cf7 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Thu, 9 Jan 2020 18:18:27 -0500 Subject: [PATCH] internal/lsp: don't run full workspace diagnostics on mod file change Minimize the issues at master by not running workspace-level diagnostics on mod file changes. Once the initial workspace load stabilizes we will be able to go back to that approach. Also, a couple of minor changes along the way while debugging. Change-Id: Ib3510e15171326a1b89f08ef0031a3ef7d9ac4ec Reviewed-on: https://go-review.googlesource.com/c/tools/+/214257 Run-TryBot: Rebecca Stambler Reviewed-by: Heschi Kreinick TryBot-Result: Gobot Gobot --- internal/lsp/cache/analysis.go | 2 +- internal/lsp/cache/snapshot.go | 10 ++-------- internal/lsp/cache/view.go | 8 ++------ internal/lsp/diagnostics.go | 11 ----------- internal/lsp/text_synchronization.go | 19 +++++++++++++++---- internal/lsp/watched_files.go | 9 ++++++++- 6 files changed, 28 insertions(+), 31 deletions(-) diff --git a/internal/lsp/cache/analysis.go b/internal/lsp/cache/analysis.go index a1b39ac7f7..f54f3c8afc 100644 --- a/internal/lsp/cache/analysis.go +++ b/internal/lsp/cache/analysis.go @@ -206,7 +206,7 @@ func runAnalysis(ctx context.Context, fset *token.FileSet, analyzer *analysis.An defer func() { if r := recover(); r != nil { log.Print(ctx, fmt.Sprintf("analysis panicked: %s", r), telemetry.Package.Of(pkg.PkgPath)) - data.err = errors.Errorf("analysis %s for package %s panicked: %v", analyzer.Name, pkg.PkgPath()) + data.err = errors.Errorf("analysis %s for package %s panicked: %v", analyzer.Name, pkg.PkgPath(), r) } }() diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 94814ee5ce..ea4bef8f40 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -524,10 +524,7 @@ func (s *snapshot) getFileURIs() []span.URI { // FindFile returns the file if the given URI is already a part of the view. func (s *snapshot) FindFile(ctx context.Context, uri span.URI) source.FileHandle { - s.view.mu.Lock() - defer s.view.mu.Unlock() - - f, err := s.view.findFile(uri) + f, err := s.view.findFileLocked(ctx, uri) if f == nil || err != nil { return nil } @@ -537,10 +534,7 @@ func (s *snapshot) FindFile(ctx context.Context, uri span.URI) source.FileHandle // 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(ctx context.Context, uri span.URI) (source.FileHandle, error) { - s.view.mu.Lock() - defer s.view.mu.Unlock() - - f, err := s.view.getFile(ctx, uri) + f, err := s.view.getFileLocked(ctx, uri) if err != nil { return nil, err } diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 65ef24f0a8..a184582407 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -344,15 +344,11 @@ func basename(filename string) string { } // FindFile returns the file if the given URI is already a part of the view. -func (v *view) findFileLocked(ctx context.Context, uri span.URI) *fileBase { +func (v *view) findFileLocked(ctx context.Context, uri span.URI) (*fileBase, error) { v.mu.Lock() defer v.mu.Unlock() - f, err := v.findFile(uri) - if err != nil { - return nil - } - return f + return v.findFile(uri) } // getFileLocked returns a File for the given URI. It will always succeed because it diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index cad56fbb21..4887eb44ca 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -16,17 +16,6 @@ import ( "golang.org/x/tools/internal/telemetry/trace" ) -func (s *Server) diagnose(snapshot source.Snapshot, fh source.FileHandle) error { - switch fh.Identity().Kind { - case source.Go: - go s.diagnoseFile(snapshot, fh) - case source.Mod: - ctx := snapshot.View().BackgroundContext() - go s.diagnoseSnapshot(ctx, snapshot) - } - return nil -} - func (s *Server) diagnoseSnapshot(ctx context.Context, snapshot source.Snapshot) { ctx, done := trace.StartSpan(ctx, "lsp:background-worker") defer done() diff --git a/internal/lsp/text_synchronization.go b/internal/lsp/text_synchronization.go index 2f2ad81082..6d5732b4a3 100644 --- a/internal/lsp/text_synchronization.go +++ b/internal/lsp/text_synchronization.go @@ -37,8 +37,12 @@ func (s *Server) didOpen(ctx context.Context, params *protocol.DidOpenTextDocume if err != nil { return err } - // Always run diagnostics when a file is opened. - return s.diagnose(snapshot, fh) + // Only run diagnostics on file open for Go files. + switch fh.Identity().Kind { + case source.Go: + go s.diagnoseFile(snapshot, fh) + } + return nil } func (s *Server) didChange(ctx context.Context, params *protocol.DidChangeTextDocumentParams) error { @@ -72,8 +76,15 @@ func (s *Server) didChange(ctx context.Context, params *protocol.DidChangeTextDo if err != nil { return err } - // Always update diagnostics after a file change. - return s.diagnose(snapshot, fh) + // Run diagnostics for Go files and for mod files. + switch fh.Identity().Kind { + case source.Go: + go s.diagnoseFile(snapshot, fh) + case source.Mod: + // TODO(rstambler): Modifying the go.mod file should trigger workspace-level diagnostics. + go s.diagnoseModfile(snapshot) + } + return nil } func (s *Server) didSave(ctx context.Context, params *protocol.DidSaveTextDocumentParams) error { diff --git a/internal/lsp/watched_files.go b/internal/lsp/watched_files.go index f32074c3f3..537cd1d29d 100644 --- a/internal/lsp/watched_files.go +++ b/internal/lsp/watched_files.go @@ -36,7 +36,14 @@ func (s *Server) didChangeWatchedFiles(ctx context.Context, params *protocol.Did if err != nil { return err } - return s.diagnose(snapshot, fh) + 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()