gopls/internal/lsp: delay workspace diagnostics and analysis
Analysis runs its own type-checking, which we don't want to compete with
normal type-checking of active packages. Delay analysis along with
workspace-wide diagnostics, which seems like a reasonable trade-off.
Also, increase the diagnostics delay to 1s. We produce type-checking
errors for actively edited packages immediately, and the user will
probably not notice an additional delay to other diagnostics.
This change exposed a "bug" in the initial workspace load benchmark,
which opened a file, and therefore had multiple parallel diagnostics
passes running simultaneously. Remove the file opening.
Also: update the marker tests to set a short diagnostics delay, as
otherwise each test incurs this additional delay.
Change-Id: I886ef89f440a9ebae8e4e6d0d076e47c40b5cd04
Reviewed-on: https://go-review.googlesource.com/c/tools/+/476255
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
diff --git a/gopls/internal/lsp/diagnostics.go b/gopls/internal/lsp/diagnostics.go
index 1cac729..72f920c 100644
--- a/gopls/internal/lsp/diagnostics.go
+++ b/gopls/internal/lsp/diagnostics.go
@@ -124,7 +124,7 @@
func (s *Server) diagnoseDetached(snapshot source.Snapshot) {
ctx := snapshot.BackgroundContext()
ctx = xcontext.Detach(ctx)
- s.diagnose(ctx, snapshot, false)
+ s.diagnose(ctx, snapshot, analyzeOpenPackages)
s.publishDiagnostics(ctx, true, snapshot)
}
@@ -164,14 +164,14 @@
// TODO(rfindley): it would be cleaner to simply put the diagnostic
// debouncer on the view, and remove the "key" argument to debouncing.
if ok := <-s.diagDebouncer.debounce(snapshot.View().Name(), snapshot.SequenceID(), time.After(delay)); ok {
- s.diagnose(ctx, snapshot, false)
+ s.diagnose(ctx, snapshot, analyzeOpenPackages)
s.publishDiagnostics(ctx, true, snapshot)
}
return
}
// Ignore possible workspace configuration warnings in the normal flow.
- s.diagnose(ctx, snapshot, false)
+ s.diagnose(ctx, snapshot, analyzeOpenPackages)
s.publishDiagnostics(ctx, true, snapshot)
}
@@ -219,12 +219,12 @@
}
}
}
- s.diagnosePkgs(ctx, snapshot, toDiagnose, false)
+ s.diagnosePkgs(ctx, snapshot, toDiagnose, analyzeNothing)
}
// diagnose is a helper function for running diagnostics with a given context.
// Do not call it directly. forceAnalysis is only true for testing purposes.
-func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, forceAnalysis bool) {
+func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, analyze analysisMode) {
ctx, done := event.Start(ctx, "Server.diagnose", source.SnapshotLabels(snapshot)...)
defer done()
@@ -335,7 +335,7 @@
}
toDiagnose = append(toDiagnose, m)
}
- s.diagnosePkgs(ctx, snapshot, toDiagnose, forceAnalysis)
+ s.diagnosePkgs(ctx, snapshot, toDiagnose, analyze)
// Orphaned files.
// Confirm that every opened file belongs to a package (if any exist in
@@ -352,23 +352,36 @@
}
}
-func (s *Server) diagnosePkgs(ctx context.Context, snapshot source.Snapshot, mds []*source.Metadata, alwaysAnalyze bool) {
+// analysisMode parameterizes analysis behavior of a call to diagnosePkgs.
+type analysisMode int
+
+const (
+ analyzeNothing analysisMode = iota // don't run any analysis
+ analyzeOpenPackages // run analysis on packages with open files
+ analyzeEverything // run analysis on all packages
+)
+
+func (s *Server) diagnosePkgs(ctx context.Context, snapshot source.Snapshot, mds []*source.Metadata, analysis analysisMode) {
ctx, done := event.Start(ctx, "Server.diagnosePkgs", source.SnapshotLabels(snapshot)...)
defer done()
var needIDs []source.PackageID // exclude e.g. testdata packages
needAnalysis := make(map[source.PackageID]*source.Metadata)
+
for _, m := range mds {
- enableDiagnostics := false
- includeAnalysis := alwaysAnalyze // only run analyses for packages with open files
+ var hasNonIgnored, hasOpenFile bool
for _, uri := range m.CompiledGoFiles {
- enableDiagnostics = enableDiagnostics || !snapshot.IgnoredFile(uri)
- includeAnalysis = includeAnalysis || snapshot.IsOpen(uri)
+ if !snapshot.IgnoredFile(uri) {
+ hasNonIgnored = true
+ }
+ if snapshot.IsOpen(uri) {
+ hasOpenFile = true
+ }
}
- if enableDiagnostics {
+ if hasNonIgnored {
needIDs = append(needIDs, m.ID)
- }
- if includeAnalysis && enableDiagnostics {
- needAnalysis[m.ID] = m
+ if analysis == analyzeEverything || analysis == analyzeOpenPackages && hasOpenFile {
+ needAnalysis[m.ID] = m
+ }
}
}
diff --git a/gopls/internal/lsp/lsp_test.go b/gopls/internal/lsp/lsp_test.go
index 4b19d29..dac7407 100644
--- a/gopls/internal/lsp/lsp_test.go
+++ b/gopls/internal/lsp/lsp_test.go
@@ -1265,7 +1265,7 @@
defer release()
// Always run diagnostics with analysis.
- r.server.diagnose(r.ctx, snapshot, true)
+ r.server.diagnose(r.ctx, snapshot, analyzeEverything)
for uri, reports := range r.server.diagnostics {
for _, report := range reports.reports {
for _, d := range report.diags {
diff --git a/gopls/internal/lsp/regtest/marker.go b/gopls/internal/lsp/regtest/marker.go
index c0ae449..fb7454d 100644
--- a/gopls/internal/lsp/regtest/marker.go
+++ b/gopls/internal/lsp/regtest/marker.go
@@ -293,6 +293,12 @@
Settings: test.settings,
Env: test.env,
}
+ if _, ok := config.Settings["diagnosticsDelay"]; !ok {
+ if config.Settings == nil {
+ config.Settings = make(map[string]interface{})
+ }
+ config.Settings["diagnosticsDelay"] = "10ms"
+ }
run := &markerTestRun{
test: test,
env: newEnv(t, cache, test.files, config),
diff --git a/gopls/internal/lsp/source/api_json.go b/gopls/internal/lsp/source/api_json.go
old mode 100755
new mode 100644
index e655eb0..74d8d71
--- a/gopls/internal/lsp/source/api_json.go
+++ b/gopls/internal/lsp/source/api_json.go
@@ -509,7 +509,7 @@
Name: "diagnosticsDelay",
Type: "time.Duration",
Doc: "diagnosticsDelay controls the amount of time that gopls waits\nafter the most recent file modification before computing deep diagnostics.\nSimple diagnostics (parsing and type-checking) are always run immediately\non recently modified packages.\n\nThis option must be set to a valid duration string, for example `\"250ms\"`.\n",
- Default: "\"250ms\"",
+ Default: "\"1s\"",
Status: "advanced",
Hierarchy: "ui.diagnostic",
},
diff --git a/gopls/internal/lsp/source/options.go b/gopls/internal/lsp/source/options.go
index a4ae51a..e4cfb6d5 100644
--- a/gopls/internal/lsp/source/options.go
+++ b/gopls/internal/lsp/source/options.go
@@ -124,7 +124,7 @@
},
UIOptions: UIOptions{
DiagnosticOptions: DiagnosticOptions{
- DiagnosticsDelay: 250 * time.Millisecond,
+ DiagnosticsDelay: 1 * time.Second,
Annotations: map[Annotation]bool{
Bounds: true,
Escape: true,
diff --git a/gopls/internal/regtest/bench/iwl_test.go b/gopls/internal/regtest/bench/iwl_test.go
index cdfb363..cf3fd6b 100644
--- a/gopls/internal/regtest/bench/iwl_test.go
+++ b/gopls/internal/regtest/bench/iwl_test.go
@@ -53,11 +53,8 @@
defer env.Close()
b.StartTimer()
- // Open an arbitrary file to ensure that gopls starts working.
- //
- // In the future, this may matter if gopls doesn't eagerly construct
- // the workspace.
- env.OpenFile(file)
+ // Note: in the future, we may need to open a file in order to cause gopls to
+ // start loading. the workspace.
env.Await(InitialWorkspaceLoad)
// TODO(rfindley): remove this guard once the released gopls version supports
