Skip to content

Commit

Permalink
gopls/internal/settings: add missing deep cloning in Options.Clone
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
findleyr authored and gopherbot committed Sep 6, 2024
1 parent ce7eed4 commit 9f9b7e3
Show file tree
Hide file tree
Showing 8 changed files with 317 additions and 49 deletions.
2 changes: 1 addition & 1 deletion gopls/internal/cache/snapshot.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
152 changes: 152 additions & 0 deletions gopls/internal/clonetest/clonetest.go
Original file line number Diff line number Diff line change
@@ -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()))
}
}
74 changes: 74 additions & 0 deletions gopls/internal/clonetest/clonetest_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
2 changes: 1 addition & 1 deletion gopls/internal/golang/hover.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion gopls/internal/server/hover.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion gopls/internal/settings/default.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading

0 comments on commit 9f9b7e3

Please sign in to comment.