gopls/internal/lsp: merge notifications about deprecated settings

VS Code suppresses notifications if we send too many, but we don't want
users to miss warnings about deprecated settings. Merge them all into a
single message body.

Also fix a race in a test that added in the preceding CL: the old go
warnings may race with the initial workspace load.

For golang/vscode-go#2487

Change-Id: I69d61a17e0e6e888fa04fa1edce864e28a8d650e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/440180
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>

gopls/internal/lsp/general.go

@@ -13,6 +13,8 @@ import (
 	"os"
 	"path"
 	"path/filepath"
+	"sort"
+	"strings"
 	"sync"
 
 	"golang.org/x/tools/gopls/internal/lsp/debug"
@@ -459,28 +461,46 @@ func (s *Server) eventuallyShowMessage(ctx context.Context, msg *protocol.ShowMe
 }
 
 func (s *Server) handleOptionResults(ctx context.Context, results source.OptionResults) error {
+	var warnings, errors []string
 	for _, result := range results {
-		var msg *protocol.ShowMessageParams
 		switch result.Error.(type) {
 		case nil:
 			// nothing to do
 		case *source.SoftError:
-			msg = &protocol.ShowMessageParams{
-				Type:    protocol.Warning,
-				Message: result.Error.Error(),
-			}
+			warnings = append(warnings, result.Error.Error())
 		default:
-			msg = &protocol.ShowMessageParams{
-				Type:    protocol.Error,
-				Message: result.Error.Error(),
-			}
-		}
-		if msg != nil {
-			if err := s.eventuallyShowMessage(ctx, msg); err != nil {
-				return err
-			}
+			errors = append(errors, result.Error.Error())
 		}
 	}
+
+	// Sort messages, but put errors first.
+	//
+	// Having stable content for the message allows clients to de-duplicate. This
+	// matters because we may send duplicate warnings for clients that support
+	// dynamic configuration: one for the initial settings, and then more for the
+	// individual view settings.
+	var msgs []string
+	msgType := protocol.Warning
+	if len(errors) > 0 {
+		msgType = protocol.Error
+		sort.Strings(errors)
+		msgs = append(msgs, errors...)
+	}
+	if len(warnings) > 0 {
+		sort.Strings(warnings)
+		msgs = append(msgs, warnings...)
+	}
+
+	if len(msgs) > 0 {
+		// Settings
+		combined := "Invalid settings: " + strings.Join(msgs, "; ")
+		params := &protocol.ShowMessageParams{
+			Type:    msgType,
+			Message: combined,
+		}
+		return s.eventuallyShowMessage(ctx, params)
+	}
+
 	return nil
 }
 

gopls/internal/lsp/source/options.go

@@ -1052,10 +1052,10 @@ func (o *Options) set(name string, value interface{}, seen map[string]struct{})
 		result.setBool(&o.ExperimentalPostfixCompletions)
 
 	case "experimentalWorkspaceModule":
-		const msg = "The experimentalWorkspaceModule feature has been replaced by go workspaces, " +
-			"and will be removed in a future version of gopls (https://go.dev/issue/55331). " +
-			"Please see https://github.com/golang/tools/blob/master/gopls/doc/workspace.md " +
-			"for information on setting up multi-module workspaces using go.work files."
+		const msg = "experimentalWorkspaceModule has been replaced by go workspaces, " +
+			"and will be removed in a future version of gopls (https://go.dev/issue/55331) -- " +
+			"see https://github.com/golang/tools/blob/master/gopls/doc/workspace.md " +
+			"for information on setting up multi-module workspaces using go.work files"
 		result.softErrorf(msg)
 		result.setBool(&o.ExperimentalWorkspaceModule)
 
@@ -1084,8 +1084,8 @@ func (o *Options) set(name string, value interface{}, seen map[string]struct{})
 		result.setDuration(&o.DiagnosticsDelay)
 
 	case "experimentalWatchedFileDelay":
-		const msg = "The experimentalWatchedFileDelay setting is deprecated, and will " +
-			"be removed in a future version of gopls (https://go.dev/issue/55332)."
+		const msg = "experimentalWatchedFileDelay is deprecated, and will " +
+			"be removed in a future version of gopls (https://go.dev/issue/55332)"
 		result.softErrorf(msg)
 		result.setDuration(&o.ExperimentalWatchedFileDelay)
 
@@ -1099,8 +1099,8 @@ func (o *Options) set(name string, value interface{}, seen map[string]struct{})
 		result.setBool(&o.AllowImplicitNetworkAccess)
 
 	case "experimentalUseInvalidMetadata":
-		const msg = "The experimentalUseInvalidMetadata setting is deprecated, and will be removed" +
-			"in a future version of gopls (https://go.dev/issue/55333)."
+		const msg = "experimentalUseInvalidMetadata is deprecated, and will be removed " +
+			"in a future version of gopls (https://go.dev/issue/55333)"
 		result.softErrorf(msg)
 		result.setBool(&o.ExperimentalUseInvalidMetadata)
 

gopls/internal/regtest/workspace/workspace_test.go

@@ -1267,10 +1267,9 @@ func TestOldGoNotification_UnsupportedVersion(t *testing.T) {
 
 	Run(t, "", func(t *testing.T, env *Env) {
 		env.Await(
-			OnceMet(
-				InitialWorkspaceLoad,
-				ShownMessage("Please upgrade"),
-			),
+			// Note: cannot use OnceMet(InitialWorkspaceLoad, ...) here, as the
+			// upgrade message may race with the IWL.
+			ShownMessage("Please upgrade"),
 		)
 	})
 }
@@ -1293,10 +1292,9 @@ func TestOldGoNotification_Fake(t *testing.T) {
 
 	Run(t, "", func(t *testing.T, env *Env) {
 		env.Await(
-			OnceMet(
-				InitialWorkspaceLoad,
-				ShownMessage("Please upgrade"),
-			),
+			// Note: cannot use OnceMet(InitialWorkspaceLoad, ...) here, as the
+			// upgrade message may race with the IWL.
+			ShownMessage("Please upgrade"),
 		)
 	})
 }