Skip to content

Commit

Permalink
Permit use of IgnoreFields with unexported fields (#203)
Browse files Browse the repository at this point in the history
Modify IgnoreFields to permit the specification of unexported fields.
To avoid a bug with reflect.Type.FieldByName, we disallow forwarding
on unexported fields. See https://golang.org/issue/4876 for details.

Fixes #196
  • Loading branch information
dsnet committed May 20, 2020
1 parent aa7c82a commit d08c604
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 9 deletions.
17 changes: 11 additions & 6 deletions cmp/cmpopts/struct_filter.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,14 +160,19 @@ func canonicalName(t reflect.Type, sel string) ([]string, error) {

// Find the canonical name for this current field name.
// If the field exists in an embedded struct, then it will be expanded.
sf, _ := t.FieldByName(name)
if !isExported(name) {
// Disallow unexported fields:
// * To discourage people from actually touching unexported fields
// * FieldByName is buggy (https://golang.org/issue/4876)
return []string{name}, fmt.Errorf("name must be exported")
// Avoid using reflect.Type.FieldByName for unexported fields due to
// buggy behavior with regard to embeddeding and unexported fields.
// See https://golang.org/issue/4876 for details.
sf = reflect.StructField{}
for i := 0; i < t.NumField() && sf.Name == ""; i++ {
if t.Field(i).Name == name {
sf = t.Field(i)
}
}
}
sf, ok := t.FieldByName(name)
if !ok {
if sf.Name == "" {
return []string{name}, fmt.Errorf("does not exist")
}
var ss []string
Expand Down
49 changes: 46 additions & 3 deletions cmp/cmpopts/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -771,6 +771,39 @@ func TestOptions(t *testing.T) {
opts: []cmp.Option{IgnoreFields(Bar3{}, "Bar1", "Bravo", "Delta", "Alpha")},
wantEqual: false,
reason: "not equal because highest-level field is not ignored: Foo3",
}, {
label: "IgnoreFields",
x: ParentStruct{
privateStruct: &privateStruct{private: 1},
PublicStruct: &PublicStruct{private: 2},
private: 3,
},
y: ParentStruct{
privateStruct: &privateStruct{private: 10},
PublicStruct: &PublicStruct{private: 20},
private: 30,
},
opts: []cmp.Option{cmp.AllowUnexported(ParentStruct{}, PublicStruct{}, privateStruct{})},
wantEqual: false,
reason: "not equal because unexported fields mismatch",
}, {
label: "IgnoreFields",
x: ParentStruct{
privateStruct: &privateStruct{private: 1},
PublicStruct: &PublicStruct{private: 2},
private: 3,
},
y: ParentStruct{
privateStruct: &privateStruct{private: 10},
PublicStruct: &PublicStruct{private: 20},
private: 30,
},
opts: []cmp.Option{
cmp.AllowUnexported(ParentStruct{}, PublicStruct{}, privateStruct{}),
IgnoreFields(ParentStruct{}, "PublicStruct.private", "privateStruct.private", "private"),
},
wantEqual: true,
reason: "equal because mismatching unexported fields are ignored",
}, {
label: "IgnoreTypes",
x: []interface{}{5, "same"},
Expand Down Expand Up @@ -1192,12 +1225,22 @@ func TestPanic(t *testing.T) {
args: args(&Foo1{}, "Alpha"),
wantPanic: "must be a struct",
reason: "the type must be a struct (not pointer to a struct)",
}, {
label: "IgnoreFields",
fnc: IgnoreFields,
args: args(struct{ privateStruct }{}, "privateStruct"),
reason: "privateStruct field permitted since it is the default name of the embedded type",
}, {
label: "IgnoreFields",
fnc: IgnoreFields,
args: args(struct{ privateStruct }{}, "Public"),
reason: "Public field permitted since it is a forwarded field that is exported",
}, {
label: "IgnoreFields",
fnc: IgnoreFields,
args: args(Foo1{}, "unexported"),
wantPanic: "name must be exported",
reason: "unexported fields must not be specified",
args: args(struct{ privateStruct }{}, "private"),
wantPanic: "does not exist",
reason: "private field not permitted since it is a forwarded field that is unexported",
}, {
label: "IgnoreTypes",
fnc: IgnoreTypes,
Expand Down

0 comments on commit d08c604

Please sign in to comment.