From 86f42ea87bf8e701ec784a1aa2106fa6a796af14 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Sun, 5 Apr 2026 22:56:53 +0000 Subject: [PATCH] cmd/cloner, cmd/viewer: handle named map/slice types with Clone/View methods The cloner and viewer code generators didn't handle named types with basic underlying types (map/slice) that have their own Clone or View methods. For example, a type like: type Map map[string]any func (m Map) Clone() Map { ... } func (m Map) View() MapView { ... } When used as a struct field, the cloner would descend into the underlying map[string]any and fail because it can't clone the any (interface{}) value type. Similarly, the viewer would try to create a MapFnOf view and fail. Fix the cloner to check for a Clone method on the named type before falling through to the underlying type handling. Fix the viewer to check for a View method on named map/slice types, so the type author can provide a purpose-built safe view that doesn't leak raw any values. Named map/slice types without a View method fall through to normal handling, which correctly rejects types like map[string]any as unsupported. Updates tailscale/corp#39502 (needed by tailscale/corp#39594) Change-Id: Iaef0192a221e02b4b8e409c99ef8398090327744 Signed-off-by: Brad Fitzpatrick --- cmd/cloner/cloner.go | 6 ++ cmd/cloner/cloner_test.go | 28 +++++ cmd/cloner/clonerex/clonerex.go | 23 +++- cmd/cloner/clonerex/clonerex_clone.go | 28 ++++- cmd/viewer/tests/tests.go | 60 ++++++++++- cmd/viewer/tests/tests_clone.go | 34 ++++++ cmd/viewer/tests/tests_view.go | 150 +++++++++++++++++++++++++- cmd/viewer/viewer.go | 16 +++ cmd/viewer/viewer_test.go | 98 +++++++++++++++++ net/dns/dns_clone.go | 3 +- 10 files changed, 440 insertions(+), 6 deletions(-) diff --git a/cmd/cloner/cloner.go b/cmd/cloner/cloner.go index 9a5ff3de2..ab4a7b22f 100644 --- a/cmd/cloner/cloner.go +++ b/cmd/cloner/cloner.go @@ -129,6 +129,12 @@ func gen(buf *bytes.Buffer, it *codegen.ImportTracker, typ *types.Named) { } continue } + // Named types with basic underlying types (map/slice) that + // have their own Clone method should use it directly. + if methodResultType(ft, "Clone") != nil { + writef("dst.%s = src.%s.Clone()", fname, fname) + continue + } } switch ft := ft.Underlying().(type) { case *types.Slice: diff --git a/cmd/cloner/cloner_test.go b/cmd/cloner/cloner_test.go index b06f5c4fa..c0a946480 100644 --- a/cmd/cloner/cloner_test.go +++ b/cmd/cloner/cloner_test.go @@ -154,6 +154,34 @@ func TestMapWithPointers(t *testing.T) { } } +func TestNamedMapContainer(t *testing.T) { + orig := &clonerex.NamedMapContainer{ + Attrs: clonerex.NamedMap{ + "str": "hello", + "num": int64(42), + "bool": true, + }, + } + + cloned := orig.Clone() + if !reflect.DeepEqual(orig, cloned) { + t.Errorf("Clone() = %v, want %v", cloned, orig) + } + + // Mutate the cloned map to verify no aliasing. + cloned.Attrs["str"] = "modified" + if orig.Attrs["str"] == "modified" { + t.Errorf("Clone() aliased memory in Attrs: original was modified") + } + + // Verify nil handling. + nilContainer := &clonerex.NamedMapContainer{} + nilClone := nilContainer.Clone() + if !reflect.DeepEqual(nilContainer, nilClone) { + t.Errorf("Clone() of nil Attrs = %v, want %v", nilClone, nilContainer) + } +} + func TestDeeplyNestedMap(t *testing.T) { num := 123 orig := &clonerex.DeeplyNestedMap{ diff --git a/cmd/cloner/clonerex/clonerex.go b/cmd/cloner/clonerex/clonerex.go index 1007d0c6b..d17dbefc5 100644 --- a/cmd/cloner/clonerex/clonerex.go +++ b/cmd/cloner/clonerex/clonerex.go @@ -1,7 +1,7 @@ // Copyright (c) Tailscale Inc & contributors // SPDX-License-Identifier: BSD-3-Clause -//go:generate go run tailscale.com/cmd/cloner -clonefunc=true -type SliceContainer,InterfaceContainer,MapWithPointers,DeeplyNestedMap +//go:generate go run tailscale.com/cmd/cloner -clonefunc=true -type SliceContainer,InterfaceContainer,MapWithPointers,DeeplyNestedMap,NamedMapContainer // Package clonerex is an example package for the cloner tool. package clonerex @@ -39,6 +39,27 @@ type MapWithPointers struct { CloneInterface map[string]Cloneable } +// NamedMap is a named map type with its own Clone method. +// This tests that the cloner uses the type's Clone method +// rather than trying to descend into the map's value type. +type NamedMap map[string]any + +func (m NamedMap) Clone() NamedMap { + if m == nil { + return nil + } + m2 := make(NamedMap, len(m)) + for k, v := range m { + m2[k] = v + } + return m2 +} + +// NamedMapContainer has a field whose type is a named map with a Clone method. +type NamedMapContainer struct { + Attrs NamedMap +} + // DeeplyNestedMap tests arbitrary depth of map nesting (3+ levels) type DeeplyNestedMap struct { ThreeLevels map[string]map[string]map[string]int diff --git a/cmd/cloner/clonerex/clonerex_clone.go b/cmd/cloner/clonerex/clonerex_clone.go index ad7765381..7d94688a3 100644 --- a/cmd/cloner/clonerex/clonerex_clone.go +++ b/cmd/cloner/clonerex/clonerex_clone.go @@ -159,9 +159,26 @@ func (src *DeeplyNestedMap) Clone() *DeeplyNestedMap { FourLevels map[string]map[string]map[string]map[string]*SliceContainer }{}) +// Clone makes a deep copy of NamedMapContainer. +// The result aliases no memory with the original. +func (src *NamedMapContainer) Clone() *NamedMapContainer { + if src == nil { + return nil + } + dst := new(NamedMapContainer) + *dst = *src + dst.Attrs = src.Attrs.Clone() + return dst +} + +// A compilation failure here means this code must be regenerated, with the command at the top of this file. +var _NamedMapContainerCloneNeedsRegeneration = NamedMapContainer(struct { + Attrs NamedMap +}{}) + // Clone duplicates src into dst and reports whether it succeeded. // To succeed, must be of types <*T, *T> or <*T, **T>, -// where T is one of SliceContainer,InterfaceContainer,MapWithPointers,DeeplyNestedMap. +// where T is one of SliceContainer,InterfaceContainer,MapWithPointers,DeeplyNestedMap,NamedMapContainer. func Clone(dst, src any) bool { switch src := src.(type) { case *SliceContainer: @@ -200,6 +217,15 @@ func Clone(dst, src any) bool { *dst = src.Clone() return true } + case *NamedMapContainer: + switch dst := dst.(type) { + case *NamedMapContainer: + *dst = *src.Clone() + return true + case **NamedMapContainer: + *dst = src.Clone() + return true + } } return false } diff --git a/cmd/viewer/tests/tests.go b/cmd/viewer/tests/tests.go index 5f6218ad3..060ac9d8e 100644 --- a/cmd/viewer/tests/tests.go +++ b/cmd/viewer/tests/tests.go @@ -12,7 +12,7 @@ "tailscale.com/types/views" ) -//go:generate go run tailscale.com/cmd/viewer --type=StructWithPtrs,StructWithoutPtrs,Map,StructWithSlices,OnlyGetClone,StructWithEmbedded,GenericIntStruct,GenericNoPtrsStruct,GenericCloneableStruct,StructWithContainers,StructWithTypeAliasFields,GenericTypeAliasStruct,StructWithMapOfViews --clone-only-type=OnlyGetClone +//go:generate go run tailscale.com/cmd/viewer --type=StructWithPtrs,StructWithoutPtrs,Map,StructWithSlices,OnlyGetClone,StructWithEmbedded,GenericIntStruct,GenericNoPtrsStruct,GenericCloneableStruct,StructWithContainers,StructWithTypeAliasFields,GenericTypeAliasStruct,StructWithMapOfViews,StructWithNamedMap,StructWithNamedSlice --clone-only-type=OnlyGetClone type StructWithoutPtrs struct { Int int @@ -241,3 +241,61 @@ type GenericTypeAliasStruct[T integer, T2 views.ViewCloner[T2, V2], V2 views.Str type StructWithMapOfViews struct { MapOfViews map[string]StructWithoutPtrsView } + +// NamedMap is a named map type with its own Clone and View methods. +// This tests that the viewer calls View() on named map types rather +// than trying to generate a view of the underlying map[string]any. +type NamedMap map[string]any + +func (m NamedMap) Clone() NamedMap { + if m == nil { + return nil + } + m2 := make(NamedMap, len(m)) + for k, v := range m { + m2[k] = v + } + return m2 +} + +// NamedMapView is a read-only view of NamedMap. +type NamedMapView struct { + ж NamedMap +} + +func (m NamedMap) View() NamedMapView { return NamedMapView{m} } + +func (v NamedMapView) Get(k string) (any, bool) { val, ok := v.ж[k]; return val, ok } +func (v NamedMapView) Len() int { return len(v.ж) } + +type StructWithNamedMap struct { + Attrs NamedMap +} + +// NamedSlice is a named slice type with its own Clone and View methods. +// This tests that the viewer calls View() on named slice types rather +// than trying to generate a view of the underlying []any. +type NamedSlice []any + +func (s NamedSlice) Clone() NamedSlice { + if s == nil { + return nil + } + s2 := make(NamedSlice, len(s)) + copy(s2, s) + return s2 +} + +// NamedSliceView is a read-only view of NamedSlice. +type NamedSliceView struct { + ж NamedSlice +} + +func (s NamedSlice) View() NamedSliceView { return NamedSliceView{s} } + +func (v NamedSliceView) At(i int) any { return v.ж[i] } +func (v NamedSliceView) Len() int { return len(v.ж) } + +type StructWithNamedSlice struct { + Items NamedSlice +} diff --git a/cmd/viewer/tests/tests_clone.go b/cmd/viewer/tests/tests_clone.go index 545b9546b..08cae87e2 100644 --- a/cmd/viewer/tests/tests_clone.go +++ b/cmd/viewer/tests/tests_clone.go @@ -563,3 +563,37 @@ func (src *StructWithMapOfViews) Clone() *StructWithMapOfViews { var _StructWithMapOfViewsCloneNeedsRegeneration = StructWithMapOfViews(struct { MapOfViews map[string]StructWithoutPtrsView }{}) + +// Clone makes a deep copy of StructWithNamedMap. +// The result aliases no memory with the original. +func (src *StructWithNamedMap) Clone() *StructWithNamedMap { + if src == nil { + return nil + } + dst := new(StructWithNamedMap) + *dst = *src + dst.Attrs = src.Attrs.Clone() + return dst +} + +// A compilation failure here means this code must be regenerated, with the command at the top of this file. +var _StructWithNamedMapCloneNeedsRegeneration = StructWithNamedMap(struct { + Attrs NamedMap +}{}) + +// Clone makes a deep copy of StructWithNamedSlice. +// The result aliases no memory with the original. +func (src *StructWithNamedSlice) Clone() *StructWithNamedSlice { + if src == nil { + return nil + } + dst := new(StructWithNamedSlice) + *dst = *src + dst.Items = src.Items.Clone() + return dst +} + +// A compilation failure here means this code must be regenerated, with the command at the top of this file. +var _StructWithNamedSliceCloneNeedsRegeneration = StructWithNamedSlice(struct { + Items NamedSlice +}{}) diff --git a/cmd/viewer/tests/tests_view.go b/cmd/viewer/tests/tests_view.go index fe073446e..29be2d78b 100644 --- a/cmd/viewer/tests/tests_view.go +++ b/cmd/viewer/tests/tests_view.go @@ -16,7 +16,7 @@ "tailscale.com/types/views" ) -//go:generate go run tailscale.com/cmd/cloner -clonefunc=false -type=StructWithPtrs,StructWithoutPtrs,Map,StructWithSlices,OnlyGetClone,StructWithEmbedded,GenericIntStruct,GenericNoPtrsStruct,GenericCloneableStruct,StructWithContainers,StructWithTypeAliasFields,GenericTypeAliasStruct,StructWithMapOfViews +//go:generate go run tailscale.com/cmd/cloner -clonefunc=false -type=StructWithPtrs,StructWithoutPtrs,Map,StructWithSlices,OnlyGetClone,StructWithEmbedded,GenericIntStruct,GenericNoPtrsStruct,GenericCloneableStruct,StructWithContainers,StructWithTypeAliasFields,GenericTypeAliasStruct,StructWithMapOfViews,StructWithNamedMap,StructWithNamedSlice // View returns a read-only view of StructWithPtrs. func (p *StructWithPtrs) View() StructWithPtrsView { @@ -1129,3 +1129,151 @@ func (v StructWithMapOfViewsView) MapOfViews() views.Map[string, StructWithoutPt var _StructWithMapOfViewsViewNeedsRegeneration = StructWithMapOfViews(struct { MapOfViews map[string]StructWithoutPtrsView }{}) + +// View returns a read-only view of StructWithNamedMap. +func (p *StructWithNamedMap) View() StructWithNamedMapView { + return StructWithNamedMapView{ж: p} +} + +// StructWithNamedMapView provides a read-only view over StructWithNamedMap. +// +// Its methods should only be called if `Valid()` returns true. +type StructWithNamedMapView struct { + // ж is the underlying mutable value, named with a hard-to-type + // character that looks pointy like a pointer. + // It is named distinctively to make you think of how dangerous it is to escape + // to callers. You must not let callers be able to mutate it. + ж *StructWithNamedMap +} + +// Valid reports whether v's underlying value is non-nil. +func (v StructWithNamedMapView) Valid() bool { return v.ж != nil } + +// AsStruct returns a clone of the underlying value which aliases no memory with +// the original. +func (v StructWithNamedMapView) AsStruct() *StructWithNamedMap { + if v.ж == nil { + return nil + } + return v.ж.Clone() +} + +// MarshalJSON implements [jsonv1.Marshaler]. +func (v StructWithNamedMapView) MarshalJSON() ([]byte, error) { + return jsonv1.Marshal(v.ж) +} + +// MarshalJSONTo implements [jsonv2.MarshalerTo]. +func (v StructWithNamedMapView) MarshalJSONTo(enc *jsontext.Encoder) error { + return jsonv2.MarshalEncode(enc, v.ж) +} + +// UnmarshalJSON implements [jsonv1.Unmarshaler]. +func (v *StructWithNamedMapView) UnmarshalJSON(b []byte) error { + if v.ж != nil { + return errors.New("already initialized") + } + if len(b) == 0 { + return nil + } + var x StructWithNamedMap + if err := jsonv1.Unmarshal(b, &x); err != nil { + return err + } + v.ж = &x + return nil +} + +// UnmarshalJSONFrom implements [jsonv2.UnmarshalerFrom]. +func (v *StructWithNamedMapView) UnmarshalJSONFrom(dec *jsontext.Decoder) error { + if v.ж != nil { + return errors.New("already initialized") + } + var x StructWithNamedMap + if err := jsonv2.UnmarshalDecode(dec, &x); err != nil { + return err + } + v.ж = &x + return nil +} + +func (v StructWithNamedMapView) Attrs() NamedMapView { return v.ж.Attrs.View() } + +// A compilation failure here means this code must be regenerated, with the command at the top of this file. +var _StructWithNamedMapViewNeedsRegeneration = StructWithNamedMap(struct { + Attrs NamedMap +}{}) + +// View returns a read-only view of StructWithNamedSlice. +func (p *StructWithNamedSlice) View() StructWithNamedSliceView { + return StructWithNamedSliceView{ж: p} +} + +// StructWithNamedSliceView provides a read-only view over StructWithNamedSlice. +// +// Its methods should only be called if `Valid()` returns true. +type StructWithNamedSliceView struct { + // ж is the underlying mutable value, named with a hard-to-type + // character that looks pointy like a pointer. + // It is named distinctively to make you think of how dangerous it is to escape + // to callers. You must not let callers be able to mutate it. + ж *StructWithNamedSlice +} + +// Valid reports whether v's underlying value is non-nil. +func (v StructWithNamedSliceView) Valid() bool { return v.ж != nil } + +// AsStruct returns a clone of the underlying value which aliases no memory with +// the original. +func (v StructWithNamedSliceView) AsStruct() *StructWithNamedSlice { + if v.ж == nil { + return nil + } + return v.ж.Clone() +} + +// MarshalJSON implements [jsonv1.Marshaler]. +func (v StructWithNamedSliceView) MarshalJSON() ([]byte, error) { + return jsonv1.Marshal(v.ж) +} + +// MarshalJSONTo implements [jsonv2.MarshalerTo]. +func (v StructWithNamedSliceView) MarshalJSONTo(enc *jsontext.Encoder) error { + return jsonv2.MarshalEncode(enc, v.ж) +} + +// UnmarshalJSON implements [jsonv1.Unmarshaler]. +func (v *StructWithNamedSliceView) UnmarshalJSON(b []byte) error { + if v.ж != nil { + return errors.New("already initialized") + } + if len(b) == 0 { + return nil + } + var x StructWithNamedSlice + if err := jsonv1.Unmarshal(b, &x); err != nil { + return err + } + v.ж = &x + return nil +} + +// UnmarshalJSONFrom implements [jsonv2.UnmarshalerFrom]. +func (v *StructWithNamedSliceView) UnmarshalJSONFrom(dec *jsontext.Decoder) error { + if v.ж != nil { + return errors.New("already initialized") + } + var x StructWithNamedSlice + if err := jsonv2.UnmarshalDecode(dec, &x); err != nil { + return err + } + v.ж = &x + return nil +} + +func (v StructWithNamedSliceView) Items() NamedSliceView { return v.ж.Items.View() } + +// A compilation failure here means this code must be regenerated, with the command at the top of this file. +var _StructWithNamedSliceViewNeedsRegeneration = StructWithNamedSlice(struct { + Items NamedSlice +}{}) diff --git a/cmd/viewer/viewer.go b/cmd/viewer/viewer.go index 1c04dbb2b..21d417878 100644 --- a/cmd/viewer/viewer.go +++ b/cmd/viewer/viewer.go @@ -282,6 +282,22 @@ func genView(buf *bytes.Buffer, it *codegen.ImportTracker, typ *types.Named, fie writeTemplateWithComment("valueField", fname) continue } + // Named map/slice types whose element type is opaque (e.g. any) + // can't be safely wrapped in views.Map/views.Slice because the + // accessor would leak the raw element. If the type provides its + // own View() method the author can return a purpose-built safe + // view; use it. Otherwise fall through to the normal handling, + // which will reject the type as unsupported. + if named, _ := codegen.NamedTypeOf(fieldType); named != nil { + switch fieldType.Underlying().(type) { + case *types.Map, *types.Slice: + if viewType := viewTypeForValueType(fieldType); viewType != nil { + args.FieldViewName = it.QualifiedName(viewType) + writeTemplateWithComment("viewField", fname) + continue + } + } + } switch underlying := fieldType.Underlying().(type) { case *types.Slice: slice := underlying diff --git a/cmd/viewer/viewer_test.go b/cmd/viewer/viewer_test.go index 8bd18d480..e53f1d3c5 100644 --- a/cmd/viewer/viewer_test.go +++ b/cmd/viewer/viewer_test.go @@ -10,11 +10,109 @@ "go/parser" "go/token" "go/types" + "strings" "testing" "tailscale.com/util/codegen" ) +// TestNamedMapWithView tests that a named map type with a user-supplied +// View() method causes the generated view accessor to call .View() and +// return the user-defined view type. Without the View() method the +// generator should reject the field as unsupported. +func TestNamedMapWithView(t *testing.T) { + const src = ` +package test + +// AttrMap is a named map whose values are opaque (any). +// It provides its own Clone and View methods. +type AttrMap map[string]any + +func (m AttrMap) Clone() AttrMap { + m2 := make(AttrMap, len(m)) + for k, v := range m { m2[k] = v } + return m2 +} + +// AttrMapView is a hand-written read-only view of AttrMap. +type AttrMapView struct{ m AttrMap } + +func (m AttrMap) View() AttrMapView { return AttrMapView{m} } + +// Container holds an AttrMap field. +type Container struct { + Attrs AttrMap +} +` + output := genViewOutput(t, src, "Container") + + // The generated accessor must call .View() and return the + // user-defined AttrMapView, not views.Map or the raw AttrMap. + const want = "func (v ContainerView) Attrs() AttrMapView { return v.ж.Attrs.View() }" + if !strings.Contains(output, want) { + t.Errorf("generated output missing expected accessor\nwant: %s\ngot:\n%s", want, output) + } +} + +// TestNamedMapWithoutView tests that a named map[string]any WITHOUT a +// View() method does NOT generate an accessor that calls .View(). +func TestNamedMapWithoutView(t *testing.T) { + const src = ` +package test + +type AttrMap map[string]any + +func (m AttrMap) Clone() AttrMap { + m2 := make(AttrMap, len(m)) + for k, v := range m { m2[k] = v } + return m2 +} + +type Container struct { + Attrs AttrMap +} +` + output := genViewOutput(t, src, "Container") + + // Must not generate an accessor that calls .Attrs.View(), + // since AttrMap doesn't have a View() method. + if strings.Contains(output, "Attrs.View()") { + t.Errorf("generated code calls .Attrs.View() but AttrMap has no View method:\n%s", output) + } + // Must not return AttrMapView (which doesn't exist). + if strings.Contains(output, "AttrMapView") { + t.Errorf("generated code references AttrMapView but it doesn't exist:\n%s", output) + } +} + +// genViewOutput parses src, runs genView on the named type, and returns +// the generated Go source. +func genViewOutput(t *testing.T, src string, typeName string) string { + t.Helper() + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "test.go", src, 0) + if err != nil { + t.Fatal(err) + } + conf := types.Config{} + pkg, err := conf.Check("test", fset, []*ast.File{f}, nil) + if err != nil { + t.Fatal(err) + } + obj := pkg.Scope().Lookup(typeName) + if obj == nil { + t.Fatalf("type %q not found", typeName) + } + named, ok := obj.(*types.TypeName).Type().(*types.Named) + if !ok { + t.Fatalf("%q is not a named type", typeName) + } + var buf bytes.Buffer + tracker := codegen.NewImportTracker(pkg) + genView(&buf, tracker, named, nil) + return buf.String() +} + func TestViewerImports(t *testing.T) { tests := []struct { name string diff --git a/net/dns/dns_clone.go b/net/dns/dns_clone.go index 291f96ec2..32765ceb4 100644 --- a/net/dns/dns_clone.go +++ b/net/dns/dns_clone.go @@ -6,7 +6,6 @@ package dns import ( - "maps" "net/netip" "tailscale.com/types/dnstype" @@ -45,7 +44,7 @@ func (src *Config) Clone() *Config { dst.Hosts[k] = append([]netip.Addr{}, src.Hosts[k]...) } } - dst.SubdomainHosts = maps.Clone(src.SubdomainHosts) + dst.SubdomainHosts = src.SubdomainHosts.Clone() return dst }