gopls/internal/lsp: add OnSave diagnostics

Gopls publishes diagnostics as soon as it observes issues, even
while the user is in the middle of typing resulting in transient
errors. Some users find this behavior rather distracting.
'diagnosticsDelay' may help avoid wasted work, but time-based
decision does not always match users' expectation on when they want
to see diagnostics updates.

Historically, vscode-go offered two additional ways to diagnose code.

* On save
* Manual trigger

They were implemented by running the go compiler and vet/lint tools
outside gopls. Now we are working to move all code analysis logic
into the language server (golang/vscode-go#2799). We need replacement
for these features (golang/vscode-go#50).

This change introduces a new gopls setting 'diagnosticsTrigger'.
The default is 'Edit'. The additional option is 'Save', which
is implemented by preventing file modification events from triggering
diagnosis. This helps migrating users of the legacy "Build/Vet On Save"
mode. For users of the manual trigger mode, we can consider to add the
"Manual" mode and expose a custom LSP command that triggers
diagnosis when we see the demand.

Alternatives I explored:

* Pull Model Diagnostics - LSP 3.17 introduced client-initiated diagnostics
supporta The idea fits well with 'on save' or 'manual trigger'
features, but I am afraid this requires non-trivial amount of work in
gopls side.

  https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_pullDiagnostics

Moreover, the state of client side implementations is
unclear. For example, VS Code does not seem to support all features
listed in the DiagnosticClientCapability yet. The interaction between
DocumentDiagnostics and WorkspaceDiagnostics, and how mature the vscode
implementation is unclear to me at this moment.

* Emulate from Client-side - I attempted to buffer diagnostics reports
in the LSP client middleware layer, and make them visible only when
files are saved. That adds a non-trivial amount of TS/JS code on the
extension side which defeats the purpose of our deprecation work.
Moreover, that causes the diagnostics diff to be computed in one more
place (in addition to VSCode side and Gopls side), and adds
complexities. I also think some users in other editors want this
feature.

For golang/vscode-go#50

Change-Id: If07d3446bee7bed90851ad2272d82d163ae586cd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/534861
Reviewed-by: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: hyangah

gopls/doc/settings.md

@@ -354,6 +354,20 @@ This option must be set to a valid duration string, for example `"250ms"`.
 
 Default: `"1s"`.
 
+##### **diagnosticsTrigger** *enum*
+
+**This setting is experimental and may be deleted.**
+
+diagnosticsTrigger controls when to run diagnostics.
+
+Must be one of:
+
+* `"Edit"`: Trigger diagnostics on file edit and save. (default)
+* `"Save"`: Trigger diagnostics only on file save. Events like initial workspace load
+or configuration change will still trigger diagnostics.
+
+Default: `"Edit"`.
+
 ##### **analysisProgressReporting** *bool*
 
 analysisProgressReporting controls whether gopls sends progress

gopls/internal/lsp/diagnostics.go

@@ -155,9 +155,12 @@ func computeDiagnosticHash(diags ...*source.Diagnostic) string {
 	return fmt.Sprintf("%x", h.Sum(nil))
 }
 
-func (s *Server) diagnoseSnapshots(snapshots map[source.Snapshot][]span.URI, onDisk bool) {
+func (s *Server) diagnoseSnapshots(snapshots map[source.Snapshot][]span.URI, onDisk bool, cause ModificationSource) {
 	var diagnosticWG sync.WaitGroup
 	for snapshot, uris := range snapshots {
+		if snapshot.Options().DiagnosticsTrigger == source.DiagnosticsOnSave && cause == FromDidChange {
+			continue // user requested to update the diagnostics only on save. do not diagnose yet.
+		}
 		diagnosticWG.Add(1)
 		go func(snapshot source.Snapshot, uris []span.URI) {
 			defer diagnosticWG.Done()

gopls/internal/lsp/source/api_json.go

@@ -137,6 +137,7 @@ func DefaultOptions(overrides ...func(*Options)) *Options {
 						},
 						Vulncheck:                 ModeVulncheckOff,
 						DiagnosticsDelay:          1 * time.Second,
+						DiagnosticsTrigger:        DiagnosticsOnEdit,
 						AnalysisProgressReporting: true,
 					},
 					InlayHintOptions: InlayHintOptions{},
@@ -468,6 +469,9 @@ type DiagnosticOptions struct {
 	// This option must be set to a valid duration string, for example `"250ms"`.
 	DiagnosticsDelay time.Duration `status:"advanced"`
 
+	// DiagnosticsTrigger controls when to run diagnostics.
+	DiagnosticsTrigger DiagnosticsTrigger `status:"experimental"`
+
 	// AnalysisProgressReporting controls whether gopls sends progress
 	// notifications when construction of its index of analysis facts is taking a
 	// long time. Cancelling these notifications will cancel the indexing task,
@@ -810,6 +814,17 @@ const (
 	// TODO: VulncheckRequire, VulncheckCallgraph
 )
 
+type DiagnosticsTrigger string
+
+const (
+	// Trigger diagnostics on file edit and save. (default)
+	DiagnosticsOnEdit DiagnosticsTrigger = "Edit"
+	// Trigger diagnostics only on file save. Events like initial workspace load
+	// or configuration change will still trigger diagnostics.
+	DiagnosticsOnSave DiagnosticsTrigger = "Save"
+	// TODO: support "Manual"?
+)
+
 type OptionResults []OptionResult
 
 type OptionResult struct {
@@ -1244,6 +1259,14 @@ func (o *Options) set(name string, value interface{}, seen map[string]struct{})
 	case "diagnosticsDelay":
 		result.setDuration(&o.DiagnosticsDelay)
 
+	case "diagnosticsTrigger":
+		if s, ok := result.asOneOf(
+			string(DiagnosticsOnEdit),
+			string(DiagnosticsOnSave),
+		); ok {
+			o.DiagnosticsTrigger = DiagnosticsTrigger(s)
+		}
+
 	case "analysisProgressReporting":
 		result.setBool(&o.AnalysisProgressReporting)
 

gopls/internal/lsp/source/options.go

@@ -289,7 +289,7 @@ func (s *Server) didModifyFiles(ctx context.Context, modifications []source.File
 
 	wg.Add(1)
 	go func() {
-		s.diagnoseSnapshots(snapshots, onDisk)
+		s.diagnoseSnapshots(snapshots, onDisk, cause)
 		release()
 		wg.Done()
 	}()

gopls/internal/lsp/text_synchronization.go

@@ -2112,3 +2112,44 @@ func (B) New() {}
 		)
 	})
 }
+
+func TestDiagnosticsOnlyOnSaveFile(t *testing.T) {
+	const onlyMod = `
+-- go.mod --
+module mod.com
+
+go 1.12
+-- main.go --
+package main
+
+func main() {
+	Foo()
+}
+-- foo.go --
+package main
+
+func Foo() {}
+`
+	WithOptions(
+		Settings{
+			"diagnosticsTrigger": "Save",
+		},
+	).Run(t, onlyMod, func(t *testing.T, env *Env) {
+		env.OpenFile("foo.go")
+		env.RegexpReplace("foo.go", "(Foo)", "Bar") // Makes reference to Foo undefined/undeclared.
+		env.AfterChange(NoDiagnostics())            // No diagnostics update until file save.
+
+		env.SaveBuffer("foo.go")
+		// Compiler's error message about undeclared names vary depending on the version,
+		// but must be explicit about the problematic name.
+		env.AfterChange(Diagnostics(env.AtRegexp("main.go", "Foo"), WithMessage("Foo")))
+
+		env.OpenFile("main.go")
+		env.RegexpReplace("main.go", "(Foo)", "Bar")
+		// No diagnostics update until file save. That results in outdated diagnostic.
+		env.AfterChange(Diagnostics(env.AtRegexp("main.go", "Bar"), WithMessage("Foo")))
+
+		env.SaveBuffer("main.go")
+		env.AfterChange(NoDiagnostics())
+	})
+}