From 9f9b7e39b519bd4bdbfdc4e13a61bd88821a0fad Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Mon, 19 Aug 2024 19:30:51 +0000 Subject: [PATCH] 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 Reviewed-by: Alan Donovan Auto-Submit: Robert Findley --- gopls/internal/cache/snapshot.go | 2 +- gopls/internal/clonetest/clonetest.go | 152 +++++++++++++++++++++ gopls/internal/clonetest/clonetest_test.go | 74 ++++++++++ gopls/internal/golang/hover.go | 2 +- gopls/internal/server/hover.go | 3 +- gopls/internal/settings/default.go | 2 +- gopls/internal/settings/settings.go | 101 ++++++++------ gopls/internal/settings/settings_test.go | 30 +++- 8 files changed, 317 insertions(+), 49 deletions(-) create mode 100644 gopls/internal/clonetest/clonetest.go create mode 100644 gopls/internal/clonetest/clonetest_test.go diff --git a/gopls/internal/cache/snapshot.go b/gopls/internal/cache/snapshot.go index d575ae63b61..9014817bdff 100644 --- a/gopls/internal/cache/snapshot.go +++ b/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 diff --git a/gopls/internal/clonetest/clonetest.go b/gopls/internal/clonetest/clonetest.go new file mode 100644 index 00000000000..3542476ae09 --- /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 00000000000..bbb803f2447 --- /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 2edb8a99d34..b315b7383d4 100644 --- a/gopls/internal/golang/hover.go +++ b/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 diff --git a/gopls/internal/server/hover.go b/gopls/internal/server/hover.go index 1470210c32e..80c35c09565 100644 --- a/gopls/internal/server/hover.go +++ b/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) diff --git a/gopls/internal/settings/default.go b/gopls/internal/settings/default.go index 7b14d2a5d79..9641613cd5d 100644 --- a/gopls/internal/settings/default.go +++ b/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, diff --git a/gopls/internal/settings/settings.go b/gopls/internal/settings/settings.go index 935eb103980..2cd504b2555 100644 --- a/gopls/internal/settings/settings.go +++ b/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 } diff --git a/gopls/internal/settings/settings_test.go b/gopls/internal/settings/settings_test.go index aa566d8f0b4..e2375222639 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 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) + } +}