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.
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 @@
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 @@
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 @@
// 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 @@
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 @@
"io"
"math"
"math/rand"
+ "path"
"reflect"
"regexp"
"sort"
@@ -1223,6 +1224,31 @@
}, {
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),
y: createStructJ(1),
opts: []cmp.Option{
cmp.AllowUnexported(ts.ParentStructJ{}, ts.PublicStruct{}, privateStruct),
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 @@
}
// 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 @@
//
// 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
+}
-func (visibleStructs) filter(_ *state, _, _ reflect.Value, _ reflect.Type) applicableOption {
+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 (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
@@ -45,6 +45,10 @@
args: []interface{}{ts.StructA{}, &ts.StructB{}, ts.StructA{}},
wantPanic: "invalid struct type",
}, {
+ label: "AllowUnexportedWithinModule",
+ fnc: AllowUnexportedWithinModule,
+ args: []interface{}{""},
+ }, {
label: "Comparer",
fnc: Comparer,
args: []interface{}{5},
