reflect: non-exported anonymous field handling #7522
Labels
Milestone
Comments
In my opinion it would not be correct to revert the change. The change was to the type reflection information, and I believe it was correct (though it should be clearly documented in the 1.3 changes, which I think is not yet the case). What I'm wondering is whether we can make encoding/json continue to work as it did before. |
Either the reflection logic is now incorrect, or the compiler is. This tiny program
shows the divergence more clearly:
https://gist.github.com/niemeyer/ed26a8784b0f0b5da733
That used to work fine, and it doesn't anymore.
This is breaking existing packages and applications, breaking the Go1 promise, and
making the compiler and the reflect package disagree.
Can we please consider that more carefully, perhaps not in Go 1.3?
|
I lie, sorry. This is broken for unrelated reasons. This is the correct one:
https://gist.github.com/niemeyer/c2ca054af6b6f8dfe4dc
|
6c0339d94123 introduces a similar breakage in XML, which in turn breaks a bunch of other
stuff, like go.text/cldr. Also, it introduces what I think is a bug in reflect.
I don't think 6c0339d94123 is correct, in that it may fix things, but also breaks
things. From the Go spec on embedding:
A field or method f of an anonymous field in a struct x is called promoted if x.f is a
legal selector that denotes that field or method f.
Promoted fields act like ordinary fields of a struct except that they cannot be used as
field names in composite literals of the struct.
The behavior in Go is consistent with this, even when it applies to exported fields of
unexported structs:
a.C = "foo" works, as per the definition, and a{C: "foo"} does not, also per the
definition.
If we allow exported fields of unexported structs to be promoted by this mechanism, it
should be consistent with reflect's behavior in my opinion.
However, since 6c0339d94123 a variable that is visible and accessible by virtue of the
"promotion rule" cannot be set despite the same rule.
I would suggest reverting the change until this is fixed. It results in more breakage
than that it fixes, it seems to me.
|
CL https://golang.org/cl/78500044 references this issue. |
CL https://golang.org/cl/85580046 mentions this issue. |
CL https://golang.org/cl/85580046 mentions this issue. |
|
This issue was closed by revision c48db9a. Status changed to Fixed. |
This issue was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
A breaking change was recently introduced with 6c0339d94123, related to issues 7247 and 7363: http://golang.org/issue/7247 http://golang.org/issue/7363 http://golang.org/change/6c0339d94123 This issue was discovered because goyaml considered the PkgPath for deciding whether to marshal something or not, and the tests ensuring this works were breaking in gccgo. So the gc suite was fixed to behave like gccgo, and now goyaml is breaking on both. I can easily fix goyaml and other packages I'm responsible for to do something else, but I'm concerned that no debate took place about how this is not only breaking the Go1 promise, but it's knowingly breaking packages in the wild, and even the standard library itself. This program: http://play.golang.org/p/kGOVanS2RA Produces {"C": "v"} on Go 1.2 and {} on tip. Based on that, I suggest we: 1) Revert the change 2) Add a test to ensure this json case does not regress 3) Clarify the spec, given that two implementations divergedThe text was updated successfully, but these errors were encountered: