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>
diff --git a/gopls/internal/cache/snapshot.go b/gopls/internal/cache/snapshot.go
index d575ae6..9014817 100644
--- a/gopls/internal/cache/snapshot.go
+++ b/gopls/internal/cache/snapshot.go
@@ -964,7 +964,7 @@
// 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
diff --git a/gopls/internal/clonetest/clonetest.go b/gopls/internal/clonetest/clonetest.go
new file mode 100644
index 0000000..3542476
--- /dev/null
+++ b/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()))
+ }
+}
diff --git a/gopls/internal/clonetest/clonetest_test.go b/gopls/internal/clonetest/clonetest_test.go
new file mode 100644
index 0000000..bbb803f
--- /dev/null
+++ b/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)
+ }
+}
diff --git a/gopls/internal/golang/hover.go b/gopls/internal/golang/hover.go
index 2edb8a9..b315b73 100644
--- a/gopls/internal/golang/hover.go
+++ b/gopls/internal/golang/hover.go
@@ -1279,7 +1279,7 @@
// 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
diff --git a/gopls/internal/server/hover.go b/gopls/internal/server/hover.go
index 1470210..80c35c0 100644
--- a/gopls/internal/server/hover.go
+++ b/gopls/internal/server/hover.go
@@ -12,6 +12,7 @@
"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 @@
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)
diff --git a/gopls/internal/settings/default.go b/gopls/internal/settings/default.go
index 7b14d2a..9641613 100644
--- a/gopls/internal/settings/default.go
+++ b/gopls/internal/settings/default.go
@@ -89,7 +89,7 @@
DocumentationOptions: DocumentationOptions{
HoverKind: FullDocumentation,
LinkTarget: "pkg.go.dev",
- LinksInHover: true,
+ LinksInHover: LinksInHover_LinkTarget,
},
NavigationOptions: NavigationOptions{
ImportShortcut: BothShortcuts,
diff --git a/gopls/internal/settings/settings.go b/gopls/internal/settings/settings.go
index 935eb10..2cd504b 100644
--- a/gopls/internal/settings/settings.go
+++ b/gopls/internal/settings/settings.go
@@ -13,8 +13,8 @@
"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 @@
// 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 @@
//
// 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 @@
//
// 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 @@
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 @@
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) 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 @@
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 @@
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 @@
default:
return err
}
- continue
}
m[a] = enabled
}
diff --git a/gopls/internal/settings/settings_test.go b/gopls/internal/settings/settings_test.go
index aa566d8..e237522 100644
--- a/gopls/internal/settings/settings_test.go
+++ b/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 TestSetOption(t *testing.T) {
+func TestOptions_Set(t *testing.T) {
type testCase struct {
name string
value any
@@ -206,7 +210,7 @@
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 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)
+ }
+}
