diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go index e1b55b5b13..f915fccbd1 100644 --- a/gopls/internal/regtest/diagnostics/diagnostics_test.go +++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go @@ -384,7 +384,7 @@ func main() {} // completed. OnceMet( env.DoneWithChange(), - NoDiagnostics("a.go"), + EmptyDiagnostics("a.go"), ), ) }) @@ -757,15 +757,20 @@ func _() { env.OpenFile("a/a1.go") env.CreateBuffer("a/a2.go", ``) env.SaveBufferWithoutActions("a/a2.go") + // We can't use OnceMet here (at least, not easily) because the didSave + // races with the didChangeWatchedFiles. + // + // TODO(rfindley): add an AllOf expectation combinator, or an expectation + // that all notifications have been processed. env.Await( - OnceMet( - env.DoneWithSave(), - NoDiagnostics("a/a1.go"), - ), + EmptyDiagnostics("a/a1.go"), ) env.EditBuffer("a/a2.go", fake.NewEdit(0, 0, 0, 0, `package a`)) env.Await( - OnceMet(env.DoneWithChange(), NoDiagnostics("a/a1.go")), + OnceMet( + env.DoneWithChange(), + EmptyDiagnostics("a/a1.go"), + ), ) }) } @@ -914,7 +919,7 @@ var _ = foo.Bar env.Await( OnceMet( env.DoneWithOpen(), - NoDiagnostics("_foo/x.go"), + EmptyDiagnostics("_foo/x.go"), )) }) } @@ -1730,7 +1735,7 @@ package b env.Await( OnceMet( env.DoneWithOpen(), - NoDiagnostics("a/a.go"), + EmptyDiagnostics("a/a.go"), ), NoOutstandingWork(), ) diff --git a/gopls/internal/regtest/diagnostics/invalidation_test.go b/gopls/internal/regtest/diagnostics/invalidation_test.go new file mode 100644 index 0000000000..ea65037644 --- /dev/null +++ b/gopls/internal/regtest/diagnostics/invalidation_test.go @@ -0,0 +1,126 @@ +// Copyright 2022 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 diagnostics + +import ( + "fmt" + "testing" + + "golang.org/x/tools/internal/lsp/protocol" + . "golang.org/x/tools/internal/lsp/regtest" +) + +// Test for golang/go#50267: diagnostics should be re-sent after a file is +// opened. +func TestDiagnosticsAreResentAfterCloseOrOpen(t *testing.T) { + const files = ` +-- go.mod -- +module mod.com + +go 1.16 +-- main.go -- +package main + +func _() { + x := 2 +} +` + Run(t, files, func(_ *testing.T, env *Env) { // Create a new workspace-level directory and empty file. + env.OpenFile("main.go") + var afterOpen protocol.PublishDiagnosticsParams + env.Await( + OnceMet( + env.DoneWithOpen(), + ReadDiagnostics("main.go", &afterOpen), + ), + ) + env.CloseBuffer("main.go") + var afterClose protocol.PublishDiagnosticsParams + env.Await( + OnceMet( + env.DoneWithClose(), + ReadDiagnostics("main.go", &afterClose), + ), + ) + if afterOpen.Version == afterClose.Version { + t.Errorf("publishDiagnostics: got the same version after closing (%d) as after opening", afterOpen.Version) + } + env.OpenFile("main.go") + var afterReopen protocol.PublishDiagnosticsParams + env.Await( + OnceMet( + env.DoneWithOpen(), + ReadDiagnostics("main.go", &afterReopen), + ), + ) + if afterReopen.Version == afterClose.Version { + t.Errorf("pubslishDiagnostics: got the same version after reopening (%d) as after closing", afterClose.Version) + } + }) +} + +// Test for the "chattyDiagnostics" setting: we should get re-published +// diagnostics after every file change, even if diagnostics did not change. +func TestChattyDiagnostics(t *testing.T) { + const files = ` +-- go.mod -- +module mod.com + +go 1.16 +-- main.go -- +package main + +func _() { + x := 2 +} + +// Irrelevant comment #0 +` + + WithOptions( + Settings{ + "chattyDiagnostics": true, + }, + ).Run(t, files, func(_ *testing.T, env *Env) { // Create a new workspace-level directory and empty file. + + env.OpenFile("main.go") + var d protocol.PublishDiagnosticsParams + env.Await( + OnceMet( + env.DoneWithOpen(), + ReadDiagnostics("main.go", &d), + ), + ) + + if len(d.Diagnostics) != 1 { + t.Fatalf("len(Diagnostics) = %d, want 1", len(d.Diagnostics)) + } + msg := d.Diagnostics[0].Message + + for i := 0; i < 5; i++ { + before := d.Version + env.RegexpReplace("main.go", "Irrelevant comment #.", fmt.Sprintf("Irrelevant comment #%d", i)) + env.Await( + OnceMet( + env.DoneWithChange(), + ReadDiagnostics("main.go", &d), + ), + ) + + if d.Version == before { + t.Errorf("after change, got version %d, want new version", d.Version) + } + + // As a sanity check, make sure we have the same diagnostic. + if len(d.Diagnostics) != 1 { + t.Fatalf("len(Diagnostics) = %d, want 1", len(d.Diagnostics)) + } + newMsg := d.Diagnostics[0].Message + if newMsg != msg { + t.Errorf("after change, got message %q, want %q", newMsg, msg) + } + } + }) +} diff --git a/gopls/internal/regtest/misc/configuration_test.go b/gopls/internal/regtest/misc/configuration_test.go index 9629a2382e..433f96e855 100644 --- a/gopls/internal/regtest/misc/configuration_test.go +++ b/gopls/internal/regtest/misc/configuration_test.go @@ -37,7 +37,7 @@ var FooErr = errors.New("foo") env.OpenFile("a/a.go") env.Await( env.DoneWithOpen(), - NoDiagnostics("a/a.go"), + EmptyDiagnostics("a/a.go"), ) cfg := env.Editor.Config() cfg.Settings = map[string]interface{}{ diff --git a/gopls/internal/regtest/template/template_test.go b/gopls/internal/regtest/template/template_test.go index 0fdc3bda6f..292088ce59 100644 --- a/gopls/internal/regtest/template/template_test.go +++ b/gopls/internal/regtest/template/template_test.go @@ -133,7 +133,7 @@ go 1.12 env.Await( OnceMet( env.DoneWithOpen(), - NoDiagnostics("hello.tmpl"), // Don't get spurious errors for empty templates. + EmptyDiagnostics("hello.tmpl"), // Don't get spurious errors for empty templates. ), ) env.SetBufferContent("hello.tmpl", "{{range .Planets}}\nHello {{}}\n{{end}}") diff --git a/gopls/internal/regtest/watch/watch_test.go b/gopls/internal/regtest/watch/watch_test.go index 2890f401a9..31655955e2 100644 --- a/gopls/internal/regtest/watch/watch_test.go +++ b/gopls/internal/regtest/watch/watch_test.go @@ -139,7 +139,7 @@ func _() { }) env.Await( EmptyDiagnostics("a/a.go"), - NoDiagnostics("b/b.go"), + EmptyOrNoDiagnostics("b/b.go"), ) }) } @@ -341,12 +341,12 @@ func _() { env.Await( OnceMet( env.DoneWithChangeWatchedFiles(), - NoDiagnostics("a/a.go"), + EmptyOrNoDiagnostics("a/a.go"), ), ) env.WriteWorkspaceFile("b/b.go", newMethod) env.Await( - NoDiagnostics("a/a.go"), + EmptyOrNoDiagnostics("a/a.go"), ) }) }) @@ -360,9 +360,9 @@ func _() { env.Await( OnceMet( env.DoneWithChangeWatchedFiles(), - NoDiagnostics("a/a.go"), + EmptyOrNoDiagnostics("a/a.go"), ), - NoDiagnostics("b/b.go"), + EmptyOrNoDiagnostics("b/b.go"), ) }) }) @@ -715,11 +715,11 @@ func TestAll(t *testing.T) { env.Await( OnceMet( env.DoneWithChangeWatchedFiles(), - NoDiagnostics("a/a.go"), + EmptyOrNoDiagnostics("a/a.go"), ), OnceMet( env.DoneWithChangeWatchedFiles(), - NoDiagnostics("a/a_test.go"), + EmptyOrNoDiagnostics("a/a_test.go"), ), ) // Now, add a new file to the test variant and use its symbol in the @@ -747,11 +747,11 @@ func TestSomething(t *testing.T) {} env.Await( OnceMet( env.DoneWithChangeWatchedFiles(), - NoDiagnostics("a/a_test.go"), + EmptyOrNoDiagnostics("a/a_test.go"), ), OnceMet( env.DoneWithChangeWatchedFiles(), - NoDiagnostics("a/a2_test.go"), + EmptyOrNoDiagnostics("a/a2_test.go"), ), ) }) diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 372e8e9795..a77deb6211 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -413,6 +413,8 @@ func (s *Session) ModifyFiles(ctx context.Context, changes []source.FileModifica return err } +// TODO(rfindley): fileChange seems redundant with source.FileModification. +// De-dupe into a common representation for changes. type fileChange struct { content []byte exists bool diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 460659f30d..642957cbad 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -41,17 +41,42 @@ const ( // A diagnosticReport holds results for a single diagnostic source. type diagnosticReport struct { - snapshotID uint64 - publishedHash string + snapshotID uint64 // snapshot ID on which the report was computed + publishedHash string // last published hash for this (URI, source) diags map[string]*source.Diagnostic } // fileReports holds a collection of diagnostic reports for a single file, as // well as the hash of the last published set of diagnostics. type fileReports struct { - snapshotID uint64 + // publishedSnapshotID is the last snapshot ID for which we have "published" + // diagnostics (though the publishDiagnostics notification may not have + // actually been sent, if nothing changed). + // + // Specifically, publishedSnapshotID is updated to a later snapshot ID when + // we either: + // (1) publish diagnostics for the file for a snapshot, or + // (2) determine that published diagnostics are valid for a new snapshot. + // + // Notably publishedSnapshotID may not match the snapshot id on individual reports in + // the reports map: + // - we may have published partial diagnostics from only a subset of + // diagnostic sources for which new results have been computed, or + // - we may have started computing reports for an even new snapshot, but not + // yet published. + // + // This prevents gopls from publishing stale diagnostics. + publishedSnapshotID uint64 + + // publishedHash is a hash of the latest diagnostics published for the file. publishedHash string - reports map[diagnosticSource]diagnosticReport + + // If set, mustPublish marks diagnostics as needing publication, independent + // of whether their publishedHash has changed. + mustPublish bool + + // The last stored diagnostics for each diagnostic source. + reports map[diagnosticSource]diagnosticReport } func (d diagnosticSource) String() string { @@ -358,6 +383,24 @@ func (s *Server) diagnosePkg(ctx context.Context, snapshot source.Snapshot, pkg } } +// mustPublishDiagnostics marks the uri as needing publication, independent of +// whether the published contents have changed. +// +// This can be used for ensuring gopls publishes diagnostics after certain file +// events. +func (s *Server) mustPublishDiagnostics(uri span.URI) { + s.diagnosticsMu.Lock() + defer s.diagnosticsMu.Unlock() + + if s.diagnostics[uri] == nil { + s.diagnostics[uri] = &fileReports{ + publishedHash: hashDiagnostics(), // Hash for 0 diagnostics. + reports: map[diagnosticSource]diagnosticReport{}, + } + } + s.diagnostics[uri].mustPublish = true +} + // storeDiagnostics stores results from a single diagnostic source. If merge is // true, it merges results into any existing results for this snapshot. func (s *Server) storeDiagnostics(snapshot source.Snapshot, uri span.URI, dsource diagnosticSource, diags []*source.Diagnostic) { @@ -367,6 +410,7 @@ func (s *Server) storeDiagnostics(snapshot source.Snapshot, uri span.URI, dsourc if fh == nil { return } + s.diagnosticsMu.Lock() defer s.diagnosticsMu.Unlock() if s.diagnostics[uri] == nil { @@ -512,7 +556,7 @@ func (s *Server) publishDiagnostics(ctx context.Context, final bool, snapshot so // If we've already delivered diagnostics for a future snapshot for this // file, do not deliver them. - if r.snapshotID > snapshot.ID() { + if r.publishedSnapshotID > snapshot.ID() { continue } anyReportsChanged := false @@ -541,10 +585,10 @@ func (s *Server) publishDiagnostics(ctx context.Context, final bool, snapshot so } source.SortDiagnostics(diags) hash := hashDiagnostics(diags...) - if hash == r.publishedHash { + if hash == r.publishedHash && !r.mustPublish { // Update snapshotID to be the latest snapshot for which this diagnostic // hash is valid. - r.snapshotID = snapshot.ID() + r.publishedSnapshotID = snapshot.ID() continue } var version int32 @@ -558,7 +602,8 @@ func (s *Server) publishDiagnostics(ctx context.Context, final bool, snapshot so }); err == nil { published++ r.publishedHash = hash - r.snapshotID = snapshot.ID() + r.mustPublish = false // diagnostics have been successfully published + r.publishedSnapshotID = snapshot.ID() for dsource, hash := range reportHashes { report := r.reports[dsource] report.publishedHash = hash diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go index a695feaf26..8abcc7452c 100644 --- a/internal/lsp/source/options.go +++ b/internal/lsp/source/options.go @@ -573,6 +573,16 @@ type InternalOptions struct { // that the new algorithm has worked, and write some summary // statistics to a file in os.TmpDir() NewDiff string + + // ChattyDiagnostics controls whether to report file diagnostics for each + // file change. If unset, gopls only reports diagnostics when they change, or + // when a file is opened or closed. + // + // TODO(rfindley): is seems that for many clients this should be true by + // default. For example, coc.nvim seems to get confused if diagnostics are + // not re-published. Switch the default to true after some period of internal + // testing. + ChattyDiagnostics bool } type ImportShortcut string @@ -805,6 +815,7 @@ func (o *Options) EnableAllExperiments() { o.ExperimentalUseInvalidMetadata = true o.ExperimentalWatchedFileDelay = 50 * time.Millisecond o.NewDiff = "checked" + o.ChattyDiagnostics = true } func (o *Options) enableAllExperimentMaps() { @@ -1075,6 +1086,9 @@ func (o *Options) set(name string, value interface{}, seen map[string]struct{}) case "newDiff": result.setString(&o.NewDiff) + case "chattyDiagnostics": + result.setBool(&o.ChattyDiagnostics) + // Replaced settings. case "experimentalDisabledAnalyses": result.deprecated("analyses") diff --git a/internal/lsp/text_synchronization.go b/internal/lsp/text_synchronization.go index 514834b077..dd6714553c 100644 --- a/internal/lsp/text_synchronization.go +++ b/internal/lsp/text_synchronization.go @@ -286,16 +286,35 @@ func (s *Server) processModifications(ctx context.Context, modifications []sourc return errors.New("server is shut down") } s.stateMu.Unlock() + // If the set of changes included directories, expand those directories // to their files. modifications = s.session.ExpandModificationsToDirectories(ctx, modifications) + // Build a lookup map for file modifications, so that we can later join + // with the snapshot file associations. + modMap := make(map[span.URI]source.FileModification) + for _, mod := range modifications { + modMap[mod.URI] = mod + } + snapshots, release, err := s.session.DidModifyFiles(ctx, modifications) if err != nil { close(diagnoseDone) return err } + // golang/go#50267: diagnostics should be re-sent after an open or close. For + // some clients, it may be helpful to re-send after each change. + for snapshot, uris := range snapshots { + for _, uri := range uris { + mod := modMap[uri] + if snapshot.View().Options().ChattyDiagnostics || mod.Action == source.Open || mod.Action == source.Close { + s.mustPublishDiagnostics(uri) + } + } + } + go func() { s.diagnoseSnapshots(snapshots, onDisk) release()