gopls: strict LSP compliance for empty slices

There are a number of places in the LSP where []T is
required and nil is unacceptable.  This change attempts
to ensure that gopls generates the correct data.

The change would like to use  a generic function,
but instead uses 4 functions, one for each of string, TextEdit,
DiagnosticTag, and CompletionItemTag.

There are also many places that call for []T, but nil is
acceptable, as the json tag include 'omitempty'. These
don't need any changes.

This change corrects the existing code,
but changes in either gopls or the LSP could introduce new
instances of the bug.

On the other hand, this bug has been present for a long time,
and was only now found, by a strict LSP client that didn't
use semantic tokens.

Fixes: golang/go#59479

Change-Id: I00f30e6b50eaf7768bc6c39859c64dc350181f20
Reviewed-on: https://go-review.googlesource.com/c/tools/+/483216
gopls-CI: kokoro <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Peter Weinberger <[email protected]>

Author: pjweinbgo

gopls/internal/lsp/cmd/suggested_fix.go

@@ -80,6 +80,10 @@ func (s *suggestedFix) Run(ctx context.Context, args ...string) error {
 	if err != nil {
 		return err
 	}
+	if file.diagnostics == nil {
+		// LSP requires a slice, not a nil.
+		file.diagnostics = []protocol.Diagnostic{}
+	}
 	p := protocol.CodeActionParams{
 		TextDocument: protocol.TextDocumentIdentifier{
 			URI: protocol.URIFromSpanURI(uri),

gopls/internal/lsp/code_action.go

@@ -405,7 +405,7 @@ func documentChanges(fh source.FileHandle, edits []protocol.TextEdit) []protocol
 						URI: protocol.URIFromSpanURI(fh.URI()),
 					},
 				},
-				Edits: edits,
+				Edits: nonNilSliceTextEdit(edits),
 			},
 		},
 	}
@@ -437,7 +437,7 @@ func codeActionsMatchingDiagnostics(ctx context.Context, snapshot source.Snapsho
 func codeActionsForDiagnostic(ctx context.Context, snapshot source.Snapshot, sd *source.Diagnostic, pd *protocol.Diagnostic) ([]protocol.CodeAction, error) {
 	var actions []protocol.CodeAction
 	for _, fix := range sd.SuggestedFixes {
-		var changes []protocol.DocumentChanges
+		changes := []protocol.DocumentChanges{} // must be a slice
 		for uri, edits := range fix.Edits {
 			fh, err := snapshot.ReadFile(ctx, uri)
 			if err != nil {

gopls/internal/lsp/command.go

@@ -159,7 +159,7 @@ func (c *commandHandler) ApplyFix(ctx context.Context, args command.ApplyFixArgs
 		if err != nil {
 			return err
 		}
-		var changes []protocol.DocumentChanges
+		changes := []protocol.DocumentChanges{} // must be a slice
 		for _, edit := range edits {
 			edit := edit
 			changes = append(changes, protocol.DocumentChanges{
@@ -376,7 +376,7 @@ func (c *commandHandler) RemoveDependency(ctx context.Context, args command.Remo
 									URI: protocol.URIFromSpanURI(deps.fh.URI()),
 								},
 							},
-							Edits: edits,
+							Edits: nonNilSliceTextEdit(edits),
 						},
 					},
 				},
@@ -591,7 +591,7 @@ func (s *Server) runGoModUpdateCommands(ctx context.Context, snapshot source.Sna
 	if len(changes) == 0 {
 		return nil
 	}
-	var documentChanges []protocol.DocumentChanges
+	documentChanges := []protocol.DocumentChanges{} // must be a slice
 	for _, change := range changes {
 		change := change
 		documentChanges = append(documentChanges, protocol.DocumentChanges{

gopls/internal/lsp/completion.go

@@ -134,7 +134,7 @@ func toProtocolCompletionItems(candidates []completion.CompletionItem, rng proto
 
 			Preselect:     i == 0,
 			Documentation: doc,
-			Tags:          candidate.Tags,
+			Tags:          nonNilSliceCompletionItemTag(candidate.Tags),
 			Deprecated:    candidate.Deprecated,
 		}
 		items = append(items, item)

gopls/internal/lsp/completion_test.go

@@ -118,6 +118,7 @@ func expected(t *testing.T, test tests.Completion, items tests.CompletionItems)
 			Label:  item.Label,
 			Kind:   item.Kind,
 			Detail: item.Detail,
+			Tags:   []protocol.CompletionItemTag{}, // must be a slice
 			Documentation: &protocol.Or_CompletionItem_documentation{
 				Value: item.Documentation,
 			},

gopls/internal/lsp/diagnostics.go

@@ -793,7 +793,7 @@ func toProtocolDiagnostics(diagnostics []*source.Diagnostic) []protocol.Diagnost
 			Range:              diag.Range,
 			Severity:           diag.Severity,
 			Source:             string(diag.Source),
-			Tags:               diag.Tags,
+			Tags:               emptySliceDiagnosticTag(diag.Tags),
 			RelatedInformation: diag.Related,
 		}
 		if diag.Code != "" {

gopls/internal/lsp/general.go

@@ -152,7 +152,7 @@ See https://github.com/golang/go/issues/45732 for more information.`,
 			DocumentSymbolProvider:     &protocol.Or_ServerCapabilities_documentSymbolProvider{Value: true},
 			WorkspaceSymbolProvider:    &protocol.Or_ServerCapabilities_workspaceSymbolProvider{Value: true},
 			ExecuteCommandProvider: &protocol.ExecuteCommandOptions{
-				Commands: options.SupportedCommands,
+				Commands: nonNilSliceString(options.SupportedCommands),
 			},
 			FoldingRangeProvider:      &protocol.Or_ServerCapabilities_foldingRangeProvider{Value: true},
 			HoverProvider:             &protocol.Or_ServerCapabilities_hoverProvider{Value: true},
@@ -166,8 +166,8 @@ See https://github.com/golang/go/issues/45732 for more information.`,
 				Range: &protocol.Or_SemanticTokensOptions_range{Value: true},
 				Full:  &protocol.Or_SemanticTokensOptions_full{Value: true},
 				Legend: protocol.SemanticTokensLegend{
-					TokenTypes:     s.session.Options().SemanticTypes,
-					TokenModifiers: s.session.Options().SemanticMods,
+					TokenTypes:     nonNilSliceString(s.session.Options().SemanticTypes),
+					TokenModifiers: nonNilSliceString(s.session.Options().SemanticMods),
 				},
 			},
 			SignatureHelpProvider: &protocol.SignatureHelpOptions{
@@ -452,7 +452,7 @@ func (s *Server) registerWatchedDirectoriesLocked(ctx context.Context, patterns
 	for k := range s.watchedGlobPatterns {
 		delete(s.watchedGlobPatterns, k)
 	}
-	var watchers []protocol.FileSystemWatcher
+	watchers := []protocol.FileSystemWatcher{} // must be a slice
 	val := protocol.WatchChange | protocol.WatchDelete | protocol.WatchCreate
 	for pattern := range patterns {
 		watchers = append(watchers, protocol.FileSystemWatcher{
@@ -629,3 +629,31 @@ func (s *Server) exit(ctx context.Context) error {
 	// close naturally if needed after the connection is closed.
 	return nil
 }
+
+// TODO: when we can assume go1.18, replace with generic
+// (after retiring support for go1.17)
+func nonNilSliceString(x []string) []string {
+	if x == nil {
+		return []string{}
+	}
+	return x
+}
+func nonNilSliceTextEdit(x []protocol.TextEdit) []protocol.TextEdit {
+	if x == nil {
+		return []protocol.TextEdit{}
+	}
+
+	return x
+}
+func nonNilSliceCompletionItemTag(x []protocol.CompletionItemTag) []protocol.CompletionItemTag {
+	if x == nil {
+		return []protocol.CompletionItemTag{}
+	}
+	return x
+}
+func emptySliceDiagnosticTag(x []protocol.DiagnosticTag) []protocol.DiagnosticTag {
+	if x == nil {
+		return []protocol.DiagnosticTag{}
+	}
+	return x
+}

gopls/internal/lsp/lsprpc/lsprpc_test.go

@@ -6,6 +6,7 @@ package lsprpc
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"regexp"
 	"strings"
@@ -343,3 +344,30 @@ func TestListenParsing(t *testing.T) {
 		}
 	}
 }
+
+// For #59479, verify that empty slices are serialized as [].
+func TestEmptySlices(t *testing.T) {
+	// The LSP would prefer that empty slices be sent as [] rather than null.
+	const bad = `{"a":null}`
+	const good = `{"a":[]}`
+	var x struct {
+		A []string `json:"a"`
+	}
+	buf, _ := json.Marshal(x)
+	if string(buf) != bad {
+		// uninitialized is ezpected to give null
+		t.Errorf("unexpectedly got %s, want %s", buf, bad)
+	}
+	x.A = make([]string, 0)
+	buf, _ = json.Marshal(x)
+	if string(buf) != good {
+		// expect []
+		t.Errorf("unexpectedly got %s, want %s", buf, good)
+	}
+	x.A = []string{}
+	buf, _ = json.Marshal(x)
+	if string(buf) != good {
+		// expect []
+		t.Errorf("unexpectedly got %s, want %s", buf, good)
+	}
+}

gopls/internal/lsp/rename.go

@@ -31,7 +31,7 @@ func (s *Server) rename(ctx context.Context, params *protocol.RenameParams) (*pr
 		return nil, err
 	}
 
-	var docChanges []protocol.DocumentChanges
+	docChanges := []protocol.DocumentChanges{} // must be a slice
 	for uri, e := range edits {
 		fh, err := snapshot.ReadFile(ctx, uri)
 		if err != nil {

gopls/internal/lsp/source/format.go

@@ -336,7 +336,7 @@ func protocolEditsFromSource(src []byte, edits []diff.Edit) ([]protocol.TextEdit
 	return result, nil
 }
 
-// ToProtocolEdits converts diff.Edits to LSP TextEdits.
+// ToProtocolEdits converts diff.Edits to a non-nil slice of LSP TextEdits.
 // See https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textEditArray
 func ToProtocolEdits(m *protocol.Mapper, edits []diff.Edit) ([]protocol.TextEdit, error) {
 	// LSP doesn't require TextEditArray to be sorted: