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,