reflect: non-exported anonymous field handling

[niemeyer]

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 diverged

[niemeyer]

Comment 1:

dev thread: https://groups.google.com/d/msg/golang-dev/1mW0fmtPSVU/p6cyiV1kAj4J

[niemeyer]

Comment 2:

Labels changed: added release-go1.3.

[ianlancetaylor]

Comment 3:

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.

[niemeyer]

Comment 4:

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?

[niemeyer]

Comment 5:

I lie, sorry. This is broken for unrelated reasons. This is the correct one:
    https://gist.github.com/niemeyer/c2ca054af6b6f8dfe4dc

[mpvl]

Comment 6:

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.

[gopherbot]

Comment 7:

CL https://golang.org/cl/78500044 references this issue.

[gopherbot]

Comment 8:

CL https://golang.org/cl/85580046 mentions this issue.

[gopherbot]

Comment 9:

CL https://golang.org/cl/85580046 mentions this issue.

[rsc]

Comment 10:

This issue was closed by revision c48db9a.

Status changed to Fixed.