From a823945b37dd76caebe4af9b00aa801b7cac4526 Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Mon, 25 Feb 2019 13:54:08 -0800 Subject: [PATCH] Support AllowUnexportedWithinModule For sufficiently large codebases with many struct types defined, it is untenable to specify a large list of all types in the repository that AllowUnexported permits visibility for. In Go, we observe that code with a shared owner often lives in the same repository, and all sub-packages within a repository often share a common package path prefix. Thus, permit expressing visibility allowances in terms of package path prefixes instead of individual types. While not explicitly documented, this feature allows the universal comparing of unexported fields on all types: cmp.Equal(x, y, cmp.AllowUnexportedWithinModule("")) This "feature" is intentionally undocumented (but a natural side-effect of the expressed behavior of PackagePrefix) since it considered to be bad practice. --- cmp/compare.go | 20 ++++----- cmp/compare_test.go | 26 ++++++++++++ cmp/options.go | 100 ++++++++++++++++++++++++++++++++++++-------- cmp/options_test.go | 4 ++ 4 files changed, 122 insertions(+), 28 deletions(-) diff --git a/cmp/compare.go b/cmp/compare.go index a7bcaab..cd4fc3c 100644 --- a/cmp/compare.go +++ b/cmp/compare.go @@ -116,8 +116,8 @@ type state struct { dynChecker dynChecker // These fields, once set by processOption, will not change. - exporters map[reflect.Type]bool // Set of structs with unexported field visibility - opts Options // List of all fundamental and filter options + exporters fieldExporter // Set of structs with unexported field visibility + opts Options // List of all fundamental and filter options } func newState(opts []Option) *state { @@ -143,12 +143,12 @@ func (s *state) processOption(opt Option) { panic(fmt.Sprintf("cannot use an unfiltered option: %v", opt)) } s.opts = append(s.opts, opt) - case visibleStructs: - if s.exporters == nil { - s.exporters = make(map[reflect.Type]bool) + case fieldExporter: + for t := range opt.typs { + s.exporters.insertType(t) } - for t := range opt { - s.exporters[t] = true + for p := range opt.pkgs { + s.exporters.insertPrefix(p) } case reporter: if s.reporter != nil { @@ -379,8 +379,8 @@ func detectRaces(c chan<- reflect.Value, f reflect.Value, vs ...reflect.Value) { // Otherwise, it returns the input value as is. func sanitizeValue(v reflect.Value, t reflect.Type) reflect.Value { // TODO(dsnet): Workaround for reflect bug (https://golang.org/issue/22143). - // The upstream fix landed in Go1.10, so we can remove this when drop support - // for Go1.9 and below. + // The upstream fix landed in Go1.10, so we can remove this when dropping + // support for Go1.9 and below. if v.Kind() == reflect.Interface && v.IsNil() && v.Type() != t { return reflect.New(t).Elem() } @@ -413,7 +413,7 @@ func (s *state) compareStruct(vx, vy reflect.Value, t reflect.Type) { vax = makeAddressable(vx) vay = makeAddressable(vy) } - step.force = s.exporters[t] + step.force = s.exporters.mayExport(t, t.Field(i)) step.pvx = vax step.pvy = vay step.field = t.Field(i) diff --git a/cmp/compare_test.go b/cmp/compare_test.go index c98b088..5916e91 100644 --- a/cmp/compare_test.go +++ b/cmp/compare_test.go @@ -12,6 +12,7 @@ import ( "io" "math" "math/rand" + "path" "reflect" "regexp" "sort" @@ -1220,6 +1221,31 @@ func embeddedTests() []test { opts: []cmp.Option{ cmp.AllowUnexported(ts.ParentStructJ{}, ts.PublicStruct{}, privateStruct), }, + }, { + label: label + "ParentStructJ", + x: createStructJ(0), + y: createStructJ(0), + opts: []cmp.Option{ + cmp.AllowUnexportedWithinModule(""), + }, + }, { + label: label + "ParentStructJ", + x: createStructJ(0), + y: createStructJ(0), + opts: []cmp.Option{ + cmp.AllowUnexportedWithinModule("wrong/package/prefix"), + }, + wantPanic: "cannot handle unexported field", + }, { + label: label + "ParentStructJ", + x: createStructJ(0), + y: createStructJ(0), + opts: []cmp.Option{ + cmp.AllowUnexportedWithinModule(func() string { + type X struct{} + return path.Dir(reflect.TypeOf(X{}).PkgPath()) + }()), + }, }, { label: label + "ParentStructJ", x: createStructJ(0), diff --git a/cmp/options.go b/cmp/options.go index a9306b4..552cd44 100644 --- a/cmp/options.go +++ b/cmp/options.go @@ -338,17 +338,15 @@ func (cm comparer) String() string { } // AllowUnexported returns an Option that forcibly allows operations on -// unexported fields in certain structs, which are specified by passing in a -// value of each struct type. +// unexported fields in certain structs. Struct types with permitted visibility +// are specified by passing in a value of the struct type. // -// Users of this option must understand that comparing on unexported fields -// from external packages is not safe since changes in the internal -// implementation of some external package may cause the result of Equal -// to unexpectedly change. However, it may be valid to use this option on types -// defined in an internal package where the semantic meaning of an unexported -// field is in the control of the user. +// Comparing unexported fields from packages that are not owned by the user +// is unsafe since changes in the internal implementation may cause the result +// of Equal to unexpectedly change. This option should only be used on types +// where the semantic meaning of unexported fields is in full control of the user. // -// For some cases, a custom Comparer should be used instead that defines +// For most cases, a custom Comparer should be used instead that defines // equality as a function of the public API of a type rather than the underlying // unexported implementation. // @@ -363,24 +361,90 @@ func (cm comparer) String() string { // // In other cases, the cmpopts.IgnoreUnexported option can be used to ignore // all unexported fields on specified struct types. -func AllowUnexported(types ...interface{}) Option { +func AllowUnexported(typs ...interface{}) Option { if !supportAllowUnexported { panic("AllowUnexported is not supported on purego builds, Google App Engine Standard, or GopherJS") } - m := make(map[reflect.Type]bool) - for _, typ := range types { - t := reflect.TypeOf(typ) + var x fieldExporter + for _, v := range typs { + t := reflect.TypeOf(v) if t.Kind() != reflect.Struct { - panic(fmt.Sprintf("invalid struct type: %T", typ)) + panic(fmt.Sprintf("invalid struct type: %T", v)) } - m[t] = true + x.insertType(t) } - return visibleStructs(m) + return x } -type visibleStructs map[reflect.Type]bool +// AllowUnexportedWithinModule returns an Option that forcibly allows +// operations on unexported fields in certain structs. +// See AllowUnexported for proper guidance on comparing unexported fields. +// +// Unexported visibility is permitted for any struct type declared within a +// module where the pkgPrefix is a path prefix match of the full package path. +// A path prefix match is defined as a string prefix match where the next +// character is either the first character, a forward slash, +// or the end of the string. +// +// For example, the package path "example.com/foo/bar" is matched by: +// • "example.com/foo/bar" +// • "example.com/foo" +// • "example.com" +// and is not matched by: +// • "example.com/foo/ba" +// • "example.com/fizz" +// • "example.org" +func AllowUnexportedWithinModule(pkgPrefix string) Option { + if !supportAllowUnexported { + panic("AllowUnexported is not supported on purego builds, Google App Engine Standard, or GopherJS") + } + var x fieldExporter + x.insertPrefix(pkgPrefix) + return x +} + +type fieldExporter struct { + typs map[reflect.Type]struct{} + pkgs map[string]struct{} +} + +func (x *fieldExporter) insertType(t reflect.Type) { + if x.typs == nil { + x.typs = make(map[reflect.Type]struct{}) + } + x.typs[t] = struct{}{} +} + +func (x *fieldExporter) insertPrefix(p string) { + if x.pkgs == nil { + x.pkgs = make(map[string]struct{}) + } + x.pkgs[p] = struct{}{} +} + +func (x fieldExporter) mayExport(t reflect.Type, sf reflect.StructField) bool { + // TODO(dsnet): Workaround for reflect bug (https://golang.org/issue/21122). + // The upstream fix landed in Go1.10, so we can remove this when dropping + // support for Go1.9 and below. + if len(x.pkgs) > 0 && sf.PkgPath == "" { + return true // Liberally allow exporting since we lack path information + } + + if _, ok := x.typs[t]; ok { + return true + } + for pkgPrefix := range x.pkgs { + if !strings.HasPrefix(sf.PkgPath, string(pkgPrefix)) { + continue + } + if len(sf.PkgPath) == len(pkgPrefix) || sf.PkgPath[len(pkgPrefix)] == '/' || len(pkgPrefix) == 0 { + return true + } + } + return false +} -func (visibleStructs) filter(_ *state, _, _ reflect.Value, _ reflect.Type) applicableOption { +func (fieldExporter) filter(_ *state, _, _ reflect.Value, _ reflect.Type) applicableOption { panic("not implemented") } diff --git a/cmp/options_test.go b/cmp/options_test.go index f5d2b4a..7064086 100644 --- a/cmp/options_test.go +++ b/cmp/options_test.go @@ -44,6 +44,10 @@ func TestOptionPanic(t *testing.T) { fnc: AllowUnexported, args: []interface{}{ts.StructA{}, &ts.StructB{}, ts.StructA{}}, wantPanic: "invalid struct type", + }, { + label: "AllowUnexportedWithinModule", + fnc: AllowUnexportedWithinModule, + args: []interface{}{""}, }, { label: "Comparer", fnc: Comparer,