gopls/internal/settings: add missing deep cloning in Options.Clone

As noted in a TODO, it appeared that settings.Clone was failing to deep
clone several map or slice fields. A test revealed that ten (!) fields
were not deeply cloned.

Fix this by:
1. Removing pointers and interfaces from settings.Options, by making
   ClientInfo a non-pointer, and by making LinksInHover a proper enum.
2. Adding a deepclone package that implements deep cloning using
   reflection. By avoiding supporting pointers and interfaces, this
   package doesn't need to worry about recursive data structures.

Change-Id: Ic89916f7cad51d8e60ed0a8a095758acd1c09a2d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/606816
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>

gopls/internal/cache/snapshot.go

@@ -964,7 +964,7 @@ func (s *Snapshot) watchSubdirs() bool {
 		// requirements that client names do not change. We should update the VS
 		// Code extension to set a default value of "subdirWatchPatterns" to "on",
 		// so that this workaround is only temporary.
-		if s.Options().ClientInfo != nil && s.Options().ClientInfo.Name == "Visual Studio Code" {
+		if s.Options().ClientInfo.Name == "Visual Studio Code" {
 			return true
 		}
 		return false

gopls/internal/clonetest/clonetest.go

@@ -0,0 +1,152 @@
+// Copyright 2024 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 clonetest provides utility functions for testing Clone operations.
+//
+// The [NonZero] helper may be used to construct a type in which fields are
+// recursively set to a non-zero value. This value can then be cloned, and the
+// [ZeroOut] helper can set values stored in the clone to zero, recursively.
+// Doing so should not mutate the original.
+package clonetest
+
+import (
+	"fmt"
+	"reflect"
+)
+
+// NonZero returns a T set to some appropriate nonzero value:
+//   - Values of basic type are set to an arbitrary non-zero value.
+//   - Struct fields are set to a non-zero value.
+//   - Array indices are set to a non-zero value.
+//   - Pointers point to a non-zero value.
+//   - Maps and slices are given a non-zero element.
+//   - Chan, Func, Interface, UnsafePointer are all unsupported.
+//
+// NonZero breaks cycles by returning a zero value for recursive types.
+func NonZero[T any]() T {
+	var x T
+	t := reflect.TypeOf(x)
+	if t == nil {
+		panic("untyped nil")
+	}
+	v := nonZeroValue(t, nil)
+	return v.Interface().(T)
+}
+
+// nonZeroValue returns a non-zero, addressable value of the given type.
+func nonZeroValue(t reflect.Type, seen []reflect.Type) reflect.Value {
+	for _, t2 := range seen {
+		if t == t2 {
+			// Cycle: return the zero value.
+			return reflect.Zero(t)
+		}
+	}
+	seen = append(seen, t)
+	v := reflect.New(t).Elem()
+	switch t.Kind() {
+	case reflect.Bool:
+		v.SetBool(true)
+
+	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
+		v.SetInt(1)
+
+	case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
+		v.SetUint(1)
+
+	case reflect.Float32, reflect.Float64:
+		v.SetFloat(1)
+
+	case reflect.Complex64, reflect.Complex128:
+		v.SetComplex(1)
+
+	case reflect.Array:
+		for i := 0; i < v.Len(); i++ {
+			v.Index(i).Set(nonZeroValue(t.Elem(), seen))
+		}
+
+	case reflect.Map:
+		v2 := reflect.MakeMap(t)
+		v2.SetMapIndex(nonZeroValue(t.Key(), seen), nonZeroValue(t.Elem(), seen))
+		v.Set(v2)
+
+	case reflect.Pointer:
+		v2 := nonZeroValue(t.Elem(), seen)
+		v.Set(v2.Addr())
+
+	case reflect.Slice:
+		v2 := reflect.Append(v, nonZeroValue(t.Elem(), seen))
+		v.Set(v2)
+
+	case reflect.String:
+		v.SetString(".")
+
+	case reflect.Struct:
+		for i := 0; i < v.NumField(); i++ {
+			v.Field(i).Set(nonZeroValue(t.Field(i).Type, seen))
+		}
+
+	default: // Chan, Func, Interface, UnsafePointer
+		panic(fmt.Sprintf("reflect kind %v not supported", t.Kind()))
+	}
+	return v
+}
+
+// ZeroOut recursively sets values contained in t to zero.
+// Values of king Chan, Func, Interface, UnsafePointer are all unsupported.
+//
+// No attempt is made to handle cyclic values.
+func ZeroOut[T any](t *T) {
+	v := reflect.ValueOf(t).Elem()
+	zeroOutValue(v)
+}
+
+func zeroOutValue(v reflect.Value) {
+	if v.IsZero() {
+		return // nothing to do; this also handles untyped nil values
+	}
+
+	switch v.Kind() {
+	case reflect.Bool,
+		reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64,
+		reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr,
+		reflect.Float32, reflect.Float64,
+		reflect.Complex64, reflect.Complex128,
+		reflect.String:
+
+		v.Set(reflect.Zero(v.Type()))
+
+	case reflect.Array:
+		for i := 0; i < v.Len(); i++ {
+			zeroOutValue(v.Index(i))
+		}
+
+	case reflect.Map:
+		iter := v.MapRange()
+		for iter.Next() {
+			mv := iter.Value()
+			if mv.CanAddr() {
+				zeroOutValue(mv)
+			} else {
+				mv = reflect.New(mv.Type()).Elem()
+			}
+			v.SetMapIndex(iter.Key(), mv)
+		}
+
+	case reflect.Pointer:
+		zeroOutValue(v.Elem())
+
+	case reflect.Slice:
+		for i := 0; i < v.Len(); i++ {
+			zeroOutValue(v.Index(i))
+		}
+
+	case reflect.Struct:
+		for i := 0; i < v.NumField(); i++ {
+			zeroOutValue(v.Field(i))
+		}
+
+	default:
+		panic(fmt.Sprintf("reflect kind %v not supported", v.Kind()))
+	}
+}

gopls/internal/clonetest/clonetest_test.go

@@ -0,0 +1,74 @@
+// Copyright 2024 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 clonetest_test
+
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"golang.org/x/tools/gopls/internal/clonetest"
+)
+
+func Test(t *testing.T) {
+	doTest(t, true, false)
+	type B bool
+	doTest(t, B(true), false)
+	doTest(t, 1, 0)
+	doTest(t, int(1), 0)
+	doTest(t, int8(1), 0)
+	doTest(t, int16(1), 0)
+	doTest(t, int32(1), 0)
+	doTest(t, int64(1), 0)
+	doTest(t, uint(1), 0)
+	doTest(t, uint8(1), 0)
+	doTest(t, uint16(1), 0)
+	doTest(t, uint32(1), 0)
+	doTest(t, uint64(1), 0)
+	doTest(t, uintptr(1), 0)
+	doTest(t, float32(1), 0)
+	doTest(t, float64(1), 0)
+	doTest(t, complex64(1), 0)
+	doTest(t, complex128(1), 0)
+	doTest(t, [3]int{1, 1, 1}, [3]int{0, 0, 0})
+	doTest(t, ".", "")
+	m1, m2 := map[string]int{".": 1}, map[string]int{".": 0}
+	doTest(t, m1, m2)
+	doTest(t, &m1, &m2)
+	doTest(t, []int{1}, []int{0})
+	i, j := 1, 0
+	doTest(t, &i, &j)
+	k, l := &i, &j
+	doTest(t, &k, &l)
+
+	s1, s2 := []int{1}, []int{0}
+	doTest(t, &s1, &s2)
+
+	type S struct {
+		Field int
+	}
+	doTest(t, S{1}, S{0})
+
+	doTest(t, []*S{{1}}, []*S{{0}})
+
+	// An arbitrary recursive type.
+	type LinkedList[T any] struct {
+		V    T
+		Next *LinkedList[T]
+	}
+	doTest(t, &LinkedList[int]{V: 1}, &LinkedList[int]{V: 0})
+}
+
+// doTest checks that the result of NonZero matches the nonzero argument, and
+// that zeroing out that result matches the zero argument.
+func doTest[T any](t *testing.T, nonzero, zero T) {
+	got := clonetest.NonZero[T]()
+	if diff := cmp.Diff(nonzero, got); diff != "" {
+		t.Fatalf("NonZero() returned unexpected diff (-want +got):\n%s", diff)
+	}
+	clonetest.ZeroOut(&got)
+	if diff := cmp.Diff(zero, got); diff != "" {
+		t.Errorf("ZeroOut() returned unexpected diff (-want +got):\n%s", diff)
+	}
+}

gopls/internal/golang/hover.go

@@ -1279,7 +1279,7 @@ func StdSymbolOf(obj types.Object) *stdlib.Symbol {
 
 // If pkgURL is non-nil, it should be used to generate doc links.
 func formatLink(h *hoverJSON, options *settings.Options, pkgURL func(path PackagePath, fragment string) protocol.URI) string {
-	if options.LinksInHover == false || h.LinkPath == "" {
+	if options.LinksInHover == settings.LinksInHover_None || h.LinkPath == "" {
 		return ""
 	}
 	var url protocol.URI

gopls/internal/server/hover.go

@@ -12,6 +12,7 @@ import (
 	"golang.org/x/tools/gopls/internal/label"
 	"golang.org/x/tools/gopls/internal/mod"
 	"golang.org/x/tools/gopls/internal/protocol"
+	"golang.org/x/tools/gopls/internal/settings"
 	"golang.org/x/tools/gopls/internal/telemetry"
 	"golang.org/x/tools/gopls/internal/template"
 	"golang.org/x/tools/gopls/internal/work"
@@ -38,7 +39,7 @@ func (s *server) Hover(ctx context.Context, params *protocol.HoverParams) (_ *pr
 		return mod.Hover(ctx, snapshot, fh, params.Position)
 	case file.Go:
 		var pkgURL func(path golang.PackagePath, fragment string) protocol.URI
-		if snapshot.Options().LinksInHover == "gopls" {
+		if snapshot.Options().LinksInHover == settings.LinksInHover_Gopls {
 			web, err := s.getWeb()
 			if err != nil {
 				event.Error(ctx, "failed to start web server", err)

gopls/internal/settings/default.go

@@ -89,7 +89,7 @@ func DefaultOptions(overrides ...func(*Options)) *Options {
 					DocumentationOptions: DocumentationOptions{
 						HoverKind:    FullDocumentation,
 						LinkTarget:   "pkg.go.dev",
-						LinksInHover: true,
+						LinksInHover: LinksInHover_LinkTarget,
 					},
 					NavigationOptions: NavigationOptions{
 						ImportShortcut: BothShortcuts,

gopls/internal/settings/settings.go

@@ -13,8 +13,8 @@ import (
 
 	"golang.org/x/tools/gopls/internal/file"
 	"golang.org/x/tools/gopls/internal/protocol"
+	"golang.org/x/tools/gopls/internal/util/frob"
 	"golang.org/x/tools/gopls/internal/util/maps"
-	"golang.org/x/tools/gopls/internal/util/slices"
 )
 
 type Annotation string
@@ -36,7 +36,8 @@ const (
 // Options holds various configuration that affects Gopls execution, organized
 // by the nature or origin of the settings.
 //
-// Options must be comparable with reflect.DeepEqual.
+// Options must be comparable with reflect.DeepEqual, and serializable with
+// [frob.Codec].
 //
 // This type defines both the logic of LSP-supplied option parsing
 // (see [SetOptions]), and the public documentation of options in
@@ -58,7 +59,7 @@ type Options struct {
 //
 // ClientOptions must be comparable with reflect.DeepEqual.
 type ClientOptions struct {
-	ClientInfo                                 *protocol.ClientInfo
+	ClientInfo                                 protocol.ClientInfo
 	InsertTextFormat                           protocol.InsertTextFormat
 	InsertReplaceSupported                     bool
 	ConfigurationSupported                     bool
@@ -371,7 +372,29 @@ type DocumentationOptions struct {
 //
 // Note: this type has special logic in loadEnums in generate.go.
 // Be sure to reflect enum and doc changes there!
-type LinksInHoverEnum any
+type LinksInHoverEnum int
+
+const (
+	LinksInHover_None LinksInHoverEnum = iota
+	LinksInHover_LinkTarget
+	LinksInHover_Gopls
+)
+
+// MarshalJSON implements the json.Marshaler interface, so that the default
+// values are formatted correctly in documentation. (See [Options.setOne] for
+// the flexible custom unmarshalling behavior).
+func (l LinksInHoverEnum) MarshalJSON() ([]byte, error) {
+	switch l {
+	case LinksInHover_None:
+		return []byte("false"), nil
+	case LinksInHover_LinkTarget:
+		return []byte("true"), nil
+	case LinksInHover_Gopls:
+		return []byte(`"gopls"`), nil
+	default:
+		return nil, fmt.Errorf("invalid LinksInHover value %d", l)
+	}
+}
 
 // Note: FormattingOptions must be comparable with reflect.DeepEqual.
 type FormattingOptions struct {
@@ -798,7 +821,20 @@ func (o *Options) Set(value any) (errors []error) {
 	case map[string]any:
 		seen := make(map[string]struct{})
 		for name, value := range value {
-			if err := o.set(name, value, seen); err != nil {
+			// Use only the last segment of a dotted name such as
+			// ui.navigation.symbolMatcher. The other segments
+			// are discarded, even without validation (!).
+			// (They are supported to enable hierarchical names
+			// in the VS Code graphical configuration UI.)
+			split := strings.Split(name, ".")
+			name = split[len(split)-1]
+
+			if _, ok := seen[name]; ok {
+				errors = append(errors, fmt.Errorf("duplicate value for %s", name))
+			}
+			seen[name] = struct{}{}
+
+			if err := o.setOne(name, value); err != nil {
 				err := fmt.Errorf("setting option %q: %w", name, err)
 				errors = append(errors, err)
 			}
@@ -809,8 +845,10 @@ func (o *Options) Set(value any) (errors []error) {
 	return errors
 }
 
-func (o *Options) ForClientCapabilities(clientName *protocol.ClientInfo, caps protocol.ClientCapabilities) {
-	o.ClientInfo = clientName
+func (o *Options) ForClientCapabilities(clientInfo *protocol.ClientInfo, caps protocol.ClientCapabilities) {
+	if clientInfo != nil {
+		o.ClientInfo = *clientInfo
+	}
 	if caps.Workspace.WorkspaceEdit != nil {
 		o.SupportedResourceOperations = caps.Workspace.WorkspaceEdit.ResourceOperations
 	}
@@ -860,24 +898,13 @@ func (o *Options) ForClientCapabilities(clientName *protocol.ClientInfo, caps pr
 	}
 }
 
-func (o *Options) Clone() *Options {
-	// TODO(rfindley): has this function gone stale? It appears that there are
-	// settings that are incorrectly cloned here (such as TemplateExtensions).
-	result := &Options{
-		ClientOptions:   o.ClientOptions,
-		InternalOptions: o.InternalOptions,
-		ServerOptions:   o.ServerOptions,
-		UserOptions:     o.UserOptions,
-	}
-	// Fully clone any slice or map fields. Only UserOptions can be modified.
-	result.Analyses = maps.Clone(o.Analyses)
-	result.Codelenses = maps.Clone(o.Codelenses)
-	result.SetEnvSlice(o.EnvSlice())
-	result.BuildFlags = slices.Clone(o.BuildFlags)
-	result.DirectoryFilters = slices.Clone(o.DirectoryFilters)
-	result.StandaloneTags = slices.Clone(o.StandaloneTags)
+var codec = frob.CodecFor[*Options]()
 
-	return result
+func (o *Options) Clone() *Options {
+	data := codec.Encode(o)
+	var clone *Options
+	codec.Decode(data, &clone)
+	return clone
 }
 
 // validateDirectoryFilter validates if the filter string
@@ -904,23 +931,10 @@ func validateDirectoryFilter(ifilter string) (string, error) {
 	return strings.TrimRight(filepath.FromSlash(filter), "/"), nil
 }
 
-// set updates a field of o based on the name and value.
+// setOne updates a field of o based on the name and value.
 // It returns an error if the value was invalid or duplicate.
 // It is the caller's responsibility to augment the error with 'name'.
-func (o *Options) set(name string, value any, seen map[string]struct{}) error {
-	// Use only the last segment of a dotted name such as
-	// ui.navigation.symbolMatcher. The other segments
-	// are discarded, even without validation (!).
-	// (They are supported to enable hierarchical names
-	// in the VS Code graphical configuration UI.)
-	split := strings.Split(name, ".")
-	name = split[len(split)-1]
-
-	if _, ok := seen[name]; ok {
-		return fmt.Errorf("duplicate value")
-	}
-	seen[name] = struct{}{}
-
+func (o *Options) setOne(name string, value any) error {
 	switch name {
 	case "env":
 		env, ok := value.(map[string]any)
@@ -1005,8 +1019,12 @@ func (o *Options) set(name string, value any, seen map[string]struct{}) error {
 
 	case "linksInHover":
 		switch value {
-		case false, true, "gopls":
-			o.LinksInHover = value
+		case false:
+			o.LinksInHover = LinksInHover_None
+		case true:
+			o.LinksInHover = LinksInHover_LinkTarget
+		case "gopls":
+			o.LinksInHover = LinksInHover_Gopls
 		default:
 			return fmt.Errorf(`invalid value %s; expect false, true, or "gopls"`,
 				value)
@@ -1334,7 +1352,6 @@ func setAnnotationMap(dest *map[Annotation]bool, value any) error {
 			default:
 				return err
 			}
-			continue
 		}
 		m[a] = enabled
 	}

gopls/internal/settings/settings_test.go

@@ -2,12 +2,16 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-package settings
+package settings_test
 
 import (
 	"reflect"
 	"testing"
 	"time"
+
+	"github.com/google/go-cmp/cmp"
+	"golang.org/x/tools/gopls/internal/clonetest"
+	. "golang.org/x/tools/gopls/internal/settings"
 )
 
 func TestDefaultsEquivalence(t *testing.T) {
@@ -18,7 +22,7 @@ func TestDefaultsEquivalence(t *testing.T) {
 	}
 }
 
-func TestSetOption(t *testing.T) {
+func TestOptions_Set(t *testing.T) {
 	type testCase struct {
 		name      string
 		value     any
@@ -206,7 +210,7 @@ func TestSetOption(t *testing.T) {
 
 	for _, test := range tests {
 		var opts Options
-		err := opts.set(test.name, test.value, make(map[string]struct{}))
+		err := opts.Set(map[string]any{test.name: test.value})
 		if err != nil {
 			if !test.wantError {
 				t.Errorf("Options.set(%q, %v) failed: %v",
@@ -225,3 +229,23 @@ func TestSetOption(t *testing.T) {
 		}
 	}
 }
+
+func TestOptions_Clone(t *testing.T) {
+	// Test that the Options.Clone actually performs a deep clone of the Options
+	// struct.
+
+	golden := clonetest.NonZero[*Options]()
+	opts := clonetest.NonZero[*Options]()
+	opts2 := opts.Clone()
+
+	// The clone should be equivalent to the original.
+	if diff := cmp.Diff(golden, opts2); diff != "" {
+		t.Errorf("Clone() does not match original (-want +got):\n%s", diff)
+	}
+
+	// Mutating the clone should not mutate the original.
+	clonetest.ZeroOut(opts2)
+	if diff := cmp.Diff(golden, opts); diff != "" {
+		t.Errorf("Mutating clone mutated the original (-want +got):\n%s", diff)
+	}
+}