internal/lsp: set version correctly after textDocument/didOpen
The early return logic for didOpen events in
(*snapshot).invalidateContent was preventing the creation of a new
snapshot, which was in turn stopping the versions from being updated.
This exposed a fundamental issue in the way we were calculating
workspace diagnostics. Since we weren't waiting for diagnostics to be
completed for an entire snapshot before replying that the server had
been initialized, snapshots were being cloned without any type
information. For quickfix code actions, we assume that we have all
information cached (since we need to have sent the diagnostics that the
quickfix is mapped to), so we were not finding the cached analysis
results.
To handle this in the short-term, we key analyses by their names, and
then regenerate results as-needed for code actions. This is technically
more correct than simply assuming that we have the analyses cached. In a
follow-up CL, I will send a follow-up that will make sure that
snapshots "wait" on each other to be fully constructed before being
cloned.
Change-Id: Ie89fcdb438b6b8b675f87335561bf47b768641ac
Reviewed-on: https://go-review.googlesource.com/c/tools/+/208265
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
diff --git a/gopls/internal/hooks/analysis.go b/gopls/internal/hooks/analysis.go
index ca437ad..0549b4f 100644
--- a/gopls/internal/hooks/analysis.go
+++ b/gopls/internal/hooks/analysis.go
@@ -14,13 +14,13 @@
func updateAnalyzers(options *source.Options) {
if options.StaticCheck {
for _, a := range simple.Analyzers {
- options.Analyzers = append(options.Analyzers, a)
+ options.Analyzers[a.Name] = a
}
for _, a := range staticcheck.Analyzers {
- options.Analyzers = append(options.Analyzers, a)
+ options.Analyzers[a.Name] = a
}
for _, a := range stylecheck.Analyzers {
- options.Analyzers = append(options.Analyzers, a)
+ options.Analyzers[a.Name] = a
}
}
}
diff --git a/internal/lsp/cache/analysis.go b/internal/lsp/cache/analysis.go
index b20dc1a..b38ebd2 100644
--- a/internal/lsp/cache/analysis.go
+++ b/internal/lsp/cache/analysis.go
@@ -76,9 +76,9 @@
}
func (s *snapshot) actionHandle(ctx context.Context, id packageID, mode source.ParseMode, a *analysis.Analyzer) (*actionHandle, error) {
- ah := s.getAction(id, mode, a)
- if ah != nil {
- return ah, nil
+ act := s.getActionHandle(id, mode, a)
+ if act != nil {
+ return act, nil
}
cph := s.getPackage(id, mode)
if cph == nil {
@@ -91,7 +91,7 @@
if err != nil {
return nil, err
}
- ah = &actionHandle{
+ act = &actionHandle{
analyzer: a,
pkg: pkg,
}
@@ -139,10 +139,10 @@
}
return runAnalysis(ctx, fset, a, pkg, results)
})
- ah.handle = h
+ act.handle = h
- s.addAction(ah)
- return ah, nil
+ s.addActionHandle(act)
+ return act, nil
}
func (act *actionHandle) analyze(ctx context.Context) ([]*source.Error, interface{}, error) {
diff --git a/internal/lsp/cache/pkg.go b/internal/lsp/cache/pkg.go
index e753173..915f0f5 100644
--- a/internal/lsp/cache/pkg.go
+++ b/internal/lsp/cache/pkg.go
@@ -110,28 +110,30 @@
return result
}
-func (s *snapshot) FindAnalysisError(ctx context.Context, id string, diag protocol.Diagnostic) (*source.Error, error) {
- acts := s.getActionHandles(packageID(id), source.ParseFull)
- if len(acts) == 0 {
- return nil, errors.Errorf("no action handles for %v", id)
+func (s *snapshot) FindAnalysisError(ctx context.Context, pkgID, analyzerName, msg string, rng protocol.Range) (*source.Error, error) {
+ analyzer, ok := s.View().Options().Analyzers[analyzerName]
+ if !ok {
+ return nil, errors.Errorf("unexpected analyzer: %s", analyzerName)
}
- for _, act := range acts {
- errors, _, err := act.analyze(ctx)
- if err != nil {
- return nil, err
- }
- for _, err := range errors {
- if err.Category != diag.Source {
- continue
- }
- if err.Message != diag.Message {
- continue
- }
- if protocol.CompareRange(err.Range, diag.Range) != 0 {
- continue
- }
- return err, nil
- }
+ act, err := s.actionHandle(ctx, packageID(pkgID), source.ParseFull, analyzer)
+ if err != nil {
+ return nil, err
}
- return nil, errors.Errorf("no matching diagnostic for %v", diag)
+ errs, _, err := act.analyze(ctx)
+ if err != nil {
+ return nil, err
+ }
+ for _, err := range errs {
+ if err.Category != analyzerName {
+ continue
+ }
+ if err.Message != msg {
+ continue
+ }
+ if protocol.CompareRange(err.Range, rng) != 0 {
+ continue
+ }
+ return err, nil
+ }
+ return nil, errors.Errorf("no matching diagnostic for %s:%v", pkgID, analyzerName)
}
diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go
index 4ae68bb..fdc84df 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -17,6 +17,7 @@
id uint64
view *view
+ // mu guards all of the maps in the snapshot.
mu sync.Mutex
// ids maps file URIs to package IDs.
@@ -304,20 +305,7 @@
return s.packages[key]
}
-func (s *snapshot) getActionHandles(id packageID, m source.ParseMode) []*actionHandle {
- s.mu.Lock()
- defer s.mu.Unlock()
-
- var acts []*actionHandle
- for k, v := range s.actions {
- if k.pkg.id == id && k.pkg.mode == m {
- acts = append(acts, v)
- }
- }
- return acts
-}
-
-func (s *snapshot) getAction(id packageID, m source.ParseMode, a *analysis.Analyzer) *actionHandle {
+func (s *snapshot) getActionHandle(id packageID, m source.ParseMode, a *analysis.Analyzer) *actionHandle {
s.mu.Lock()
defer s.mu.Unlock()
@@ -331,7 +319,7 @@
return s.actions[key]
}
-func (s *snapshot) addAction(ah *actionHandle) {
+func (s *snapshot) addActionHandle(ah *actionHandle) {
s.mu.Lock()
defer s.mu.Unlock()
@@ -421,7 +409,7 @@
return s.files[f.URI()]
}
-func (s *snapshot) clone(ctx context.Context, withoutURI *span.URI, withoutTypes, withoutMetadata map[span.URI]struct{}) *snapshot {
+func (s *snapshot) clone(ctx context.Context, withoutURI span.URI, withoutTypes, withoutMetadata map[span.URI]struct{}) *snapshot {
s.mu.Lock()
defer s.mu.Unlock()
@@ -438,7 +426,7 @@
}
// Copy all of the FileHandles except for the one that was invalidated.
for k, v := range s.files {
- if withoutURI != nil && k == *withoutURI {
+ if k == withoutURI {
continue
}
result.files[k] = v
@@ -492,7 +480,7 @@
}
result.metadata[k] = v
}
- // Copy the set of initally loaded packages
+ // Copy the set of initally loaded packages.
for k, v := range s.workspacePackages {
result.workspacePackages[k] = v
}
@@ -508,13 +496,9 @@
// 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.
+//
+// Note: The logic in this function is convoluted. Do not change without significant thought.
func (v *view) invalidateContent(ctx context.Context, f source.File, kind source.FileKind, action source.FileAction) bool {
- // TODO: Handle the possibility of opening a file outside of the current view.
- // For now, return early if we open a file.
- // We assume that we are already tracking any files within the given view.
- if action == source.Open {
- return true
- }
var (
withoutTypes = make(map[span.URI]struct{})
withoutMetadata = make(map[span.URI]struct{})
@@ -525,6 +509,15 @@
v.snapshotMu.Lock()
defer v.snapshotMu.Unlock()
+ // TODO: Handle the possibility of opening a file outside of the current view.
+ // For now, return early if we open a file. Clone the snapshot so that the file's version is updated.
+ // We assume that we are already tracking any files within the given view.
+ if action == source.Open {
+ v.snapshot = v.snapshot.clone(ctx, f.URI(), nil, nil)
+ return true
+ }
+
+ // Collect all of the package IDs that correspond to the given file.
for _, id := range v.snapshot.getIDs(f.URI()) {
ids[id] = struct{}{}
}
@@ -532,13 +525,12 @@
// Get the original FileHandle for the URI, if it exists.
originalFH := v.snapshot.getFile(f.URI())
- switch action {
- case source.Create:
- // If this is a file we don't yet know about,
- // then we do not yet know what packages it should belong to.
- // Make a rough estimate of what metadata to invalidate by finding the package IDs
- // of all of the files in the same directory as this one.
- // TODO(rstambler): Speed this up by mapping directories to filenames.
+ // If this is a file we don't yet know about,
+ // then we do not yet know what packages it should belong to.
+ // Make a rough estimate of what metadata to invalidate by finding the package IDs
+ // of all of the files in the same directory as this one.
+ // TODO(rstambler): Speed this up by mapping directories to filenames.
+ if action == source.Create {
if dirStat, err := os.Stat(dir(f.URI().Filename())); err == nil {
for uri := range v.snapshot.files {
if fdirStat, err := os.Stat(dir(uri.Filename())); err == nil {
@@ -550,9 +542,13 @@
}
}
}
+ // Make sure that the original FileHandle is nil.
originalFH = nil
}
- if len(ids) == 0 {
+
+ // If there is no known FileHandle and no known IDs for the given file,
+ // there is nothing to invalidate.
+ if len(ids) == 0 && originalFH == nil {
return false
}
@@ -568,7 +564,7 @@
}
}
- // Make sure to clear out the content if there has been a deletion.
+ // If we are deleting a file, make sure to clear out the overlay.
if action == source.Delete {
v.session.clearOverlay(f.URI())
}
@@ -584,8 +580,7 @@
// TODO: If a package's name has changed,
// we should invalidate the metadata for the new package name (if it exists).
}
- uri := f.URI()
- v.snapshot = v.snapshot.clone(ctx, &uri, withoutTypes, withoutMetadata)
+ v.snapshot = v.snapshot.clone(ctx, f.URI(), withoutTypes, withoutMetadata)
return true
}
diff --git a/internal/lsp/code_action.go b/internal/lsp/code_action.go
index 9a6bf99..a4c206f 100644
--- a/internal/lsp/code_action.go
+++ b/internal/lsp/code_action.go
@@ -29,7 +29,6 @@
if err != nil {
return nil, err
}
-
snapshot := view.Snapshot()
fh := snapshot.Handle(ctx, f)
@@ -217,7 +216,9 @@
return nil, err
}
for _, diag := range diagnostics {
- srcErr, err := snapshot.FindAnalysisError(ctx, cph.ID(), diag)
+ // This code assumes that the analyzer name is the Source of the diagnostic.
+ // If this ever changes, this will need to be addressed.
+ srcErr, err := snapshot.FindAnalysisError(ctx, cph.ID(), diag.Source, diag.Message, diag.Range)
if err != nil {
continue
}
diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go
index 8334d6d..33f178e 100644
--- a/internal/lsp/diagnostics.go
+++ b/internal/lsp/diagnostics.go
@@ -16,22 +16,25 @@
"golang.org/x/tools/internal/telemetry/trace"
)
-func (s *Server) diagnoseSnapshot(snapshot source.Snapshot, cphs []source.CheckPackageHandle) {
+func (s *Server) diagnoseSnapshot(ctx context.Context, snapshot source.Snapshot, cphs []source.CheckPackageHandle) {
for _, cph := range cphs {
if len(cph.CompiledGoFiles()) == 0 {
continue
}
- f := cph.CompiledGoFiles()[0]
+ // Find a file on which to call diagnostics.
+ uri := cph.CompiledGoFiles()[0].File().Identity().URI
// Run diagnostics on the workspace package.
go func(snapshot source.Snapshot, uri span.URI) {
- s.diagnostics(snapshot, uri)
- }(snapshot, f.File().Identity().URI)
+ if err := s.diagnostics(ctx, snapshot, uri); err != nil {
+ log.Error(snapshot.View().BackgroundContext(), "error computing diagnostics", err, telemetry.URI.Of(uri))
+ }
+
+ }(snapshot, uri)
}
}
-func (s *Server) diagnostics(snapshot source.Snapshot, uri span.URI) error {
- ctx := snapshot.View().BackgroundContext()
+func (s *Server) diagnostics(ctx context.Context, snapshot source.Snapshot, uri span.URI) error {
ctx, done := trace.StartSpan(ctx, "lsp:background-worker")
defer done()
diff --git a/internal/lsp/general.go b/internal/lsp/general.go
index 8a2f1ff..b60d976 100644
--- a/internal/lsp/general.go
+++ b/internal/lsp/general.go
@@ -155,18 +155,27 @@
debug.PrintVersionInfo(buf, true, debug.PlainText)
log.Print(ctx, buf.String())
+ s.addFolders(ctx, s.pendingFolders)
+ s.pendingFolders = nil
+
+ return nil
+}
+
+func (s *Server) addFolders(ctx context.Context, folders []protocol.WorkspaceFolder) {
+ originalViews := len(s.session.Views())
viewErrors := make(map[span.URI]error)
- for _, folder := range s.pendingFolders {
+
+ for _, folder := range folders {
uri := span.NewURI(folder.URI)
view, workspacePackages, err := s.addView(ctx, folder.Name, span.NewURI(folder.URI))
if err != nil {
viewErrors[uri] = err
continue
}
- s.diagnoseSnapshot(view.Snapshot(), workspacePackages)
+ go s.diagnoseSnapshot(ctx, view.Snapshot(), workspacePackages)
}
if len(viewErrors) > 0 {
- errMsg := fmt.Sprintf("Error loading workspace folders (expected %v, got %v)\n", len(s.pendingFolders), len(s.session.Views()))
+ errMsg := fmt.Sprintf("Error loading workspace folders (expected %v, got %v)\n", len(folders), len(s.session.Views())-originalViews)
for uri, err := range viewErrors {
errMsg += fmt.Sprintf("failed to load view for %s: %v\n", uri, err)
}
@@ -175,9 +184,6 @@
Message: errMsg,
})
}
- s.pendingFolders = nil
-
- return nil
}
func (s *Server) fetchConfig(ctx context.Context, name string, folder span.URI, o *source.Options) error {
diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go
index fe39ad6..f049091 100644
--- a/internal/lsp/source/options.go
+++ b/internal/lsp/source/options.go
@@ -104,7 +104,7 @@
ComputeEdits diff.ComputeEdits
- Analyzers []*analysis.Analyzer
+ Analyzers map[string]*analysis.Analyzer
// LocalPrefix is used to specify goimports's -local behavior.
LocalPrefix string
@@ -338,30 +338,31 @@
}
}
-var defaultAnalyzers = []*analysis.Analyzer{
+var defaultAnalyzers = map[string]*analysis.Analyzer{
// The traditional vet suite:
- asmdecl.Analyzer,
- assign.Analyzer,
- atomic.Analyzer,
- atomicalign.Analyzer,
- bools.Analyzer,
- buildtag.Analyzer,
- cgocall.Analyzer,
- composite.Analyzer,
- copylock.Analyzer,
- httpresponse.Analyzer,
- loopclosure.Analyzer,
- lostcancel.Analyzer,
- nilfunc.Analyzer,
- printf.Analyzer,
- shift.Analyzer,
- stdmethods.Analyzer,
- structtag.Analyzer,
- tests.Analyzer,
- unmarshal.Analyzer,
- unreachable.Analyzer,
- unsafeptr.Analyzer,
- unusedresult.Analyzer,
+ asmdecl.Analyzer.Name: asmdecl.Analyzer,
+ assign.Analyzer.Name: assign.Analyzer,
+ atomic.Analyzer.Name: atomic.Analyzer,
+ atomicalign.Analyzer.Name: atomicalign.Analyzer,
+ bools.Analyzer.Name: bools.Analyzer,
+ buildtag.Analyzer.Name: buildtag.Analyzer,
+ cgocall.Analyzer.Name: cgocall.Analyzer,
+ composite.Analyzer.Name: composite.Analyzer,
+ copylock.Analyzer.Name: copylock.Analyzer,
+ httpresponse.Analyzer.Name: httpresponse.Analyzer,
+ loopclosure.Analyzer.Name: loopclosure.Analyzer,
+ lostcancel.Analyzer.Name: lostcancel.Analyzer,
+ nilfunc.Analyzer.Name: nilfunc.Analyzer,
+ printf.Analyzer.Name: printf.Analyzer,
+ shift.Analyzer.Name: shift.Analyzer,
+ stdmethods.Analyzer.Name: stdmethods.Analyzer,
+ structtag.Analyzer.Name: structtag.Analyzer,
+ tests.Analyzer.Name: tests.Analyzer,
+ unmarshal.Analyzer.Name: unmarshal.Analyzer,
+ unreachable.Analyzer.Name: unreachable.Analyzer,
+ unsafeptr.Analyzer.Name: unsafeptr.Analyzer,
+ unusedresult.Analyzer.Name: unusedresult.Analyzer,
+
// Non-vet analyzers
- sortslice.Analyzer,
+ sortslice.Analyzer.Name: sortslice.Analyzer,
}
diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go
index 9f3d480..db2de6c 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -31,7 +31,7 @@
// FindAnalysisError returns the analysis error represented by the diagnostic.
// This is used to get the SuggestedFixes associated with that error.
- FindAnalysisError(ctx context.Context, id string, diag protocol.Diagnostic) (*Error, error)
+ FindAnalysisError(ctx context.Context, pkgID, analyzerName, msg string, rng protocol.Range) (*Error, error)
// PackageHandle returns the CheckPackageHandle for the given package ID.
PackageHandle(ctx context.Context, id string) (CheckPackageHandle, error)
diff --git a/internal/lsp/text_synchronization.go b/internal/lsp/text_synchronization.go
index a979cca..64cfb57 100644
--- a/internal/lsp/text_synchronization.go
+++ b/internal/lsp/text_synchronization.go
@@ -32,10 +32,8 @@
if err != nil {
return err
}
- snapshot := view.Snapshot()
-
// Run diagnostics on the newly-changed file.
- go s.diagnostics(snapshot, uri)
+ go s.diagnostics(view.BackgroundContext(), view.Snapshot(), uri)
return nil
}
@@ -82,7 +80,7 @@
}
// Run diagnostics on the newly-changed file.
- go s.diagnostics(view.Snapshot(), uri)
+ go s.diagnostics(view.BackgroundContext(), view.Snapshot(), uri)
return nil
}
diff --git a/internal/lsp/watched_files.go b/internal/lsp/watched_files.go
index 87fa0f4..f23a115 100644
--- a/internal/lsp/watched_files.go
+++ b/internal/lsp/watched_files.go
@@ -34,7 +34,7 @@
if s.session.DidChangeOutOfBand(ctx, uri, action) {
// If we had been tracking the given file,
// recompute diagnostics to reflect updated file contents.
- go s.diagnostics(view.Snapshot(), uri)
+ go s.diagnostics(view.BackgroundContext(), view.Snapshot(), uri)
}
case source.Delete:
f := view.FindFile(ctx, uri)
@@ -79,7 +79,7 @@
}
// Refresh diagnostics for the package the file belonged to.
- go s.diagnostics(view.Snapshot(), otherFile.URI())
+ go s.diagnostics(view.BackgroundContext(), view.Snapshot(), otherFile.URI())
}
}
}
diff --git a/internal/lsp/workspace.go b/internal/lsp/workspace.go
index 10b082f..9ade18a 100644
--- a/internal/lsp/workspace.go
+++ b/internal/lsp/workspace.go
@@ -22,14 +22,7 @@
return errors.Errorf("view %s for %v not found", folder.Name, folder.URI)
}
}
-
- for _, folder := range event.Added {
- view, cphs, err := s.addView(ctx, folder.Name, span.NewURI(folder.URI))
- if err != nil {
- return err
- }
- go s.diagnoseSnapshot(view.Snapshot(), cphs)
- }
+ s.addFolders(ctx, event.Added)
return nil
}
