Skip to content

Commit 6c80e02

Browse files
committed
Support AllowUnexportedWithin
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.AllowUnexportedWithin("")) This "feature" is intentionally undocumented (but a natural side-effect of the expressed behavior of PackagePrefix) since it considered to be bad practice.
1 parent 77b690b commit 6c80e02

File tree

4 files changed

+122
-28
lines changed

4 files changed

+122
-28
lines changed

cmp/compare.go

+10-10
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,8 @@ type state struct {
116116
dynChecker dynChecker
117117

118118
// These fields, once set by processOption, will not change.
119-
exporters map[reflect.Type]bool // Set of structs with unexported field visibility
120-
opts Options // List of all fundamental and filter options
119+
exporters fieldExporter // Set of structs with unexported field visibility
120+
opts Options // List of all fundamental and filter options
121121
}
122122

123123
func newState(opts []Option) *state {
@@ -143,12 +143,12 @@ func (s *state) processOption(opt Option) {
143143
panic(fmt.Sprintf("cannot use an unfiltered option: %v", opt))
144144
}
145145
s.opts = append(s.opts, opt)
146-
case visibleStructs:
147-
if s.exporters == nil {
148-
s.exporters = make(map[reflect.Type]bool)
146+
case fieldExporter:
147+
for t := range opt.typs {
148+
s.exporters.insertType(t)
149149
}
150-
for t := range opt {
151-
s.exporters[t] = true
150+
for p := range opt.pkgs {
151+
s.exporters.insertPrefix(p)
152152
}
153153
case reporter:
154154
if s.reporter != nil {
@@ -379,8 +379,8 @@ func detectRaces(c chan<- reflect.Value, f reflect.Value, vs ...reflect.Value) {
379379
// Otherwise, it returns the input value as is.
380380
func sanitizeValue(v reflect.Value, t reflect.Type) reflect.Value {
381381
// TODO(dsnet): Workaround for reflect bug (https://golang.org/issue/22143).
382-
// The upstream fix landed in Go1.10, so we can remove this when drop support
383-
// for Go1.9 and below.
382+
// The upstream fix landed in Go1.10, so we can remove this when dropping
383+
// support for Go1.9 and below.
384384
if v.Kind() == reflect.Interface && v.IsNil() && v.Type() != t {
385385
return reflect.New(t).Elem()
386386
}
@@ -413,7 +413,7 @@ func (s *state) compareStruct(vx, vy reflect.Value, t reflect.Type) {
413413
vax = makeAddressable(vx)
414414
vay = makeAddressable(vy)
415415
}
416-
step.force = s.exporters[t]
416+
step.force = s.exporters.mayExport(t, t.Field(i))
417417
step.pvx = vax
418418
step.pvy = vay
419419
step.field = t.Field(i)

cmp/compare_test.go

+26
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"io"
1313
"math"
1414
"math/rand"
15+
"path"
1516
"reflect"
1617
"regexp"
1718
"sort"
@@ -1220,6 +1221,31 @@ func embeddedTests() []test {
12201221
opts: []cmp.Option{
12211222
cmp.AllowUnexported(ts.ParentStructJ{}, ts.PublicStruct{}, privateStruct),
12221223
},
1224+
}, {
1225+
label: label + "ParentStructJ",
1226+
x: createStructJ(0),
1227+
y: createStructJ(0),
1228+
opts: []cmp.Option{
1229+
cmp.AllowUnexportedWithin(""),
1230+
},
1231+
}, {
1232+
label: label + "ParentStructJ",
1233+
x: createStructJ(0),
1234+
y: createStructJ(0),
1235+
opts: []cmp.Option{
1236+
cmp.AllowUnexportedWithin("wrong/package/prefix"),
1237+
},
1238+
wantPanic: "cannot handle unexported field",
1239+
}, {
1240+
label: label + "ParentStructJ",
1241+
x: createStructJ(0),
1242+
y: createStructJ(0),
1243+
opts: []cmp.Option{
1244+
cmp.AllowUnexportedWithin(func() string {
1245+
type X struct{}
1246+
return path.Dir(reflect.TypeOf(X{}).PkgPath())
1247+
}()),
1248+
},
12231249
}, {
12241250
label: label + "ParentStructJ",
12251251
x: createStructJ(0),

cmp/options.go

+82-18
Original file line numberDiff line numberDiff line change
@@ -338,17 +338,15 @@ func (cm comparer) String() string {
338338
}
339339

340340
// AllowUnexported returns an Option that forcibly allows operations on
341-
// unexported fields in certain structs, which are specified by passing in a
342-
// value of each struct type.
341+
// unexported fields in certain structs. Struct types with permitted visibility
342+
// are specified by passing in a value of the struct type.
343343
//
344-
// Users of this option must understand that comparing on unexported fields
345-
// from external packages is not safe since changes in the internal
346-
// implementation of some external package may cause the result of Equal
347-
// to unexpectedly change. However, it may be valid to use this option on types
348-
// defined in an internal package where the semantic meaning of an unexported
349-
// field is in the control of the user.
344+
// Comparing unexported fields from packages that are not owned by the user
345+
// is unsafe since changes in the internal implementation may cause the result
346+
// of Equal to unexpectedly change. This option should only be used on types
347+
// where the semantic meaning of unexported fields is in full control of the user.
350348
//
351-
// For some cases, a custom Comparer should be used instead that defines
349+
// For most cases, a custom Comparer should be used instead that defines
352350
// equality as a function of the public API of a type rather than the underlying
353351
// unexported implementation.
354352
//
@@ -363,24 +361,90 @@ func (cm comparer) String() string {
363361
//
364362
// In other cases, the cmpopts.IgnoreUnexported option can be used to ignore
365363
// all unexported fields on specified struct types.
366-
func AllowUnexported(types ...interface{}) Option {
364+
func AllowUnexported(typs ...interface{}) Option {
367365
if !supportAllowUnexported {
368366
panic("AllowUnexported is not supported on purego builds, Google App Engine Standard, or GopherJS")
369367
}
370-
m := make(map[reflect.Type]bool)
371-
for _, typ := range types {
372-
t := reflect.TypeOf(typ)
368+
var x fieldExporter
369+
for _, v := range typs {
370+
t := reflect.TypeOf(v)
373371
if t.Kind() != reflect.Struct {
374-
panic(fmt.Sprintf("invalid struct type: %T", typ))
372+
panic(fmt.Sprintf("invalid struct type: %T", v))
375373
}
376-
m[t] = true
374+
x.insertType(t)
377375
}
378-
return visibleStructs(m)
376+
return x
379377
}
380378

381-
type visibleStructs map[reflect.Type]bool
379+
// AllowUnexportedWithin returns an Option that forcibly allows
380+
// operations on unexported fields in certain structs.
381+
// See AllowUnexported for proper guidance on comparing unexported fields.
382+
//
383+
// Unexported visibility is permitted for any struct type declared within a
384+
// package where the pkgPrefix is path prefix match of the full package path.
385+
// A path prefix match is defined as a string prefix match where the next
386+
// character is either the first character, a forward slash,
387+
// or the end of the string.
388+
//
389+
// For example, the package path "example.com/foo/bar" is matched by:
390+
// • "example.com/foo/bar"
391+
// • "example.com/foo"
392+
// • "example.com"
393+
// and is not matched by:
394+
// • "example.com/foo/ba"
395+
// • "example.com/fizz"
396+
// • "example.org"
397+
func AllowUnexportedWithin(pkgPrefix string) Option {
398+
if !supportAllowUnexported {
399+
panic("AllowUnexported is not supported on purego builds, Google App Engine Standard, or GopherJS")
400+
}
401+
var x fieldExporter
402+
x.insertPrefix(pkgPrefix)
403+
return x
404+
}
405+
406+
type fieldExporter struct {
407+
typs map[reflect.Type]struct{}
408+
pkgs map[string]struct{}
409+
}
410+
411+
func (x *fieldExporter) insertType(t reflect.Type) {
412+
if x.typs == nil {
413+
x.typs = make(map[reflect.Type]struct{})
414+
}
415+
x.typs[t] = struct{}{}
416+
}
417+
418+
func (x *fieldExporter) insertPrefix(p string) {
419+
if x.pkgs == nil {
420+
x.pkgs = make(map[string]struct{})
421+
}
422+
x.pkgs[p] = struct{}{}
423+
}
424+
425+
func (x fieldExporter) mayExport(t reflect.Type, sf reflect.StructField) bool {
426+
// TODO(dsnet): Workaround for reflect bug (https://golang.org/issue/21122).
427+
// The upstream fix landed in Go1.10, so we can remove this when dropping
428+
// support for Go1.9 and below.
429+
if len(x.pkgs) > 0 && sf.PkgPath == "" {
430+
return true // Liberally allow exporting since we lack path information
431+
}
432+
433+
if _, ok := x.typs[t]; ok {
434+
return true
435+
}
436+
for pkgPrefix := range x.pkgs {
437+
if !strings.HasPrefix(sf.PkgPath, string(pkgPrefix)) {
438+
continue
439+
}
440+
if len(sf.PkgPath) == len(pkgPrefix) || sf.PkgPath[len(pkgPrefix)] == '/' || len(pkgPrefix) == 0 {
441+
return true
442+
}
443+
}
444+
return false
445+
}
382446

383-
func (visibleStructs) filter(_ *state, _, _ reflect.Value, _ reflect.Type) applicableOption {
447+
func (fieldExporter) filter(_ *state, _, _ reflect.Value, _ reflect.Type) applicableOption {
384448
panic("not implemented")
385449
}
386450

cmp/options_test.go

+4
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,10 @@ func TestOptionPanic(t *testing.T) {
4444
fnc: AllowUnexported,
4545
args: []interface{}{ts.StructA{}, &ts.StructB{}, ts.StructA{}},
4646
wantPanic: "invalid struct type",
47+
}, {
48+
label: "AllowUnexportedWithin",
49+
fnc: AllowUnexportedWithin,
50+
args: []interface{}{""},
4751
}, {
4852
label: "Comparer",
4953
fnc: Comparer,

0 commit comments

Comments
 (0)