Skip to content

Conversation

Rican7
Copy link
Contributor

@Rican7 Rican7 commented Feb 18, 2016

This fixes an incompatibility introduced in Go 1.6.

This PR updates the reflective struct walking in the reflectValue() method to check if a field is not anonymous before skipping it, making sure that we don't skip validly accessible (exported) embedded values in an unexported field.

This new checking logic is the new recommended strategy for reflective struct walking as of the Go 1.6 release.

Related/References:

skipping it when walking over values in an struct, to make sure that we
don't skip validly accessible (exported) embedded values in an
unexported field.

This new checking logic is the new recommended strategy for reflective
struct walking as of the Go 1.6 release.

Related/References:

- Go 1.6 reflect release note: https://golang.org/doc/go1.6#reflect
- Issue: https://golang.org/issue/12367
    (golang/go#12367)
- CL: https://golang.org/cl/14085
    (https://go-review.googlesource.com/#/c/14085/)
- Commit: golang/go@afe9837
@willnorris
Copy link
Collaborator

this test doesn't seem to properly exercise this. I installed go1.6 and run the test without the change to encode.go and it still passes. I'll read through the linked issue more closely to better understand what's going on, but wanted to see if you could reproduce.

@Rican7
Copy link
Contributor Author

Rican7 commented Feb 18, 2016

Strange, it's working for me:

2-18-2016-2-55-12-am-de34

@willnorris
Copy link
Collaborator

oops, you're right. I was testing the wrong thing. Looks great!

@Rican7
Copy link
Contributor Author

Rican7 commented Feb 18, 2016

Ha, no problem. I've done the same before. 😜

@willnorris willnorris merged commit 6bb77fe into google:master Feb 18, 2016
@Rican7
Copy link
Contributor Author

Rican7 commented Feb 18, 2016

Thanks for the merge! 😃

@Rican7 Rican7 deleted the bugfix/go-1.6-reflect-embedded-unexported-field-access branch February 18, 2016 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants