Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Permit use of IgnoreFields with unexported fields #203

Merged
merged 1 commit into from
May 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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