code review 78500044: reflect: fixed bug introduced with change 6c0339d94123.

reflect: fixed bug introduced with change 66510044.

A struct may contain embedded unexported structs which, in turn, contains
exported fields. According to the Go spec, fields that are promoted from an
embedded struct are treated as ordinary fields. So exported fields in such
a struct should be exported in the embedding struct as well. Go handles
this correctly. The reflect package should and did handle this analoguously.
The aforementioned change broke this. The changes in this CL to the reflect
package fix this again. CanSet will still be false for a anonymous field
with an unexported struct, but it will be true for values of exported fields
within such a struct.

This change also changed the semantics of PkgPath. Several packages relied
on PkgPath != "" to mean that a field no longer needs to be searched for
unexported fields. This is no longer the case. This CL includes fixes for
the xml and json package. I didn't see similar cases in core, but I suspect
there are others out there.

Fixes Issue 7522.

[mpvl, 2014-03-21 11:56:00]

Hello rsc@golang.org, iant@golang.org, gri@golang.org, cmang@golang.org (cc:
golang-codereviews@googlegroups.com, r@golang.org),

I'd like you to review this change to
https://code.google.com/p/go

[mpvl, 2014-03-21 12:01:41]

Hello rsc@golang.org, iant@golang.org, gri@golang.org, cmang@golang.org (cc:
golang-codereviews@googlegroups.com, r@golang.org),

Please take another look.

[rsc, 2014-03-25 00:52:50]

thanks. will look more closely tomorrow.

[mpvl, 2014-03-25 09:56:01]

Hi Russ,

Thanks for looking in to this. There are quite a few nuances to this
chance, so I deliberated a bit more on it below.

Note that this change still doesn't fully fix the inconsistencies. There
were several inconsistencies before 66510044. This CL  fixed some and
introduced others, mostly making the swing from being too loose to being
too restrictive. Normally I would say, if consistency is not possible now,
take the more restrictive option and open it up over time. The problem now
is that there are legitimate reasons to use exported fields in unexported
structs and CL 66510044  changes the semantics beyond fixing bugs and will
break the compatibility promise.

The intention of my change is to allow exported variables in unexported
structs to be promoted and visible in reflect, consistent with the
semantics in Go. Using exported variables in unexported structs in general
is useful if one, for example, wants to parse XML without exporting a
struct.  Embedding can be useful in defining a set of commonly used fields
at the top level of several such structs. If a user is using unexported
structs for parsing XML, he will want such embedded structs to also be
unexported. One can debate whether these semantics are correct or if
another interpretation might be better, but it is a useful construct, it is
according to spec, it is how Go itself works, and people are using it.
Breaking this would violate the compatibility promise in my opinion.

My change fixes this but reintroduces some of the older inconsistencies.
 Most notably, consider:

package p
type a struct { Foo int }
type B struct { a }

package main
var b B
var v = relfect.ValueOf(&b).Elem()

The following should hold:
1) b.Foo is accessible      // truth
2) v.FieldByName("Foo").CanSet()           // works in Go 1.2, broken in
tip (reading works though!!), fixed with my CL
3) v.FieldByName("a").CanSet() == false  // broken in Go 1.2, fixed in tip,
remains fixed with my CL
4) v.Field(0).Field(0).CanSet() == false    // broken in Go 1.2,  fixed in
tip, broken again with my CL (by choice)

Note that it is Case 4 that I'm breaking again. The problem is that there
is currently no way to find the promoted fields in reflect API, short of
descending into the hierarchy and getting fields. A user of the API will
therefore obtain values of promoted fields in a number of ways, including
using the index obtained from StructFields, using Field() on the fly, or
building up an index on the fly, or using the name of the field to index
FieldByName.  3 out of 4 of these methods rely on the
Field()/FieldByIndex() method working in Case 4 mentioned above.  Moreover,
looking at the code, it seems that these methods are considerably faster
than using FieldByName. So I suspect, and believe to be the case, that most
implementations doing such a thing will not use the FieldByName method. By
fixing case 4, one is likely to break a lot of code.

Ideally, Case 4 would be fixed and the API would give an alternative to
finding promoted fields .  A nice start would be some way to get the set of
all fields (including promoted) as generated internally by the scan in
FieldByName.  This would simplify a lot of implementations, I reckon, as
users would not have to manually compute promoted fields, which is somewhat
cumbersome. The trivial implementation would just return a slice of Field
names. This approach seems to have a performance issue, though.  An
alternative is to return []ints and handle the promotion detection in
FieldByIndex, but this is not trivially done without impacting performance
and it still breaks implementations using Field. Another alternative is to
return some handle for each field or return a type for fetching fields,
with its own NumField and Field methods. Either way, an approach by which
the user can tell which fields were promoted would be preferable, IMO,
which would probably make the solution of returning the field names less
optimal.

It seems to me, though, that we would not want to introduce such an API
extension at this point in the release cycle. Even if an alternative is
offered, we would probably want some grace period updating libraries to
properly use the alternative before locking down Case 4. Fixing Case 4 now
does far more harm than good, in my opinion.

An alternative to my CL, of course, is to revert CL 66510044 until some
better solution is found. I understand this will leave an inconsistency
between gccgo and gc, though, which seems highly undesirable. I suspect
leaving CL 66510044 will break more than just json and xml, outside of
core, but it is fully justifiable if one fixes this divergence. I expect
that the fixes required (after my CL) will be trivial, as is the case for
xml and json.  I have not given much thought, btw, to the alternative of
fixing gcc, instead of gc. This would have less impact on code, I suspect,
but would be less correct as well, so I didn't consider it.

Notes:
1) Note that with any introduction of an alternative in combination with
fixing Case 4, the flagEmbed introduced in my CL could be removed again.

2) <less_important_drivel>
There is another case where the embedding unexported structs is "useful":
using embedded unexported structs within exported structs has the effect of
hiding the fields in the documentation.  This can be handy (and I use this
in go.text/cldr), but I would classify this as an abusive reliance on
semantics, especially because this is not how Go itself works. I am aware
of taking the risk that these fields will appear in the documentation some
day. Equally, the current semantics in tip opens op the possibility of
other abusive use cases. Most notably, exported fields within an unexported
embedded struct are now read-only in reflect. So to some extend one can
simulate const-ness with this. Leaving it as is, it will only be a matter
of time before people will use this as a pattern. Again an abusive reliance
on semantics, for the same reason, imo.
</less_important_drivel>

Marcel


On Tue, Mar 25, 2014 at 1:52 AM, Russ Cox <rsc@golang.org> wrote:

> thanks. will look more closely tomorrow.
>
>

[rsc, 2014-04-08 15:41:05]

I support the intent of this change but I think the code is not quite right. In
particular I think the flagRO should never be cleared. It gets set for a variety
of things, and clearing it unconditionally is sure to lead to bugs. Here is one
that I think it leads to.

If you have logically x.foo1.foo2.Z where foo1 is not an embedded field but foo2
is, then the .foo1 will set flagRO, the .foo2 will set flagRO|flagEmbed, and
then the .Z will clear flagRO because flagEmbed was set.

There needs to be some record in the bits that "this is RO only because of
embedded fields". Perhaps not setting flagEmbed if flagRO was already in effect
is enough.

Here is a test case; please add it to the CL. With the current logic I get a
failure-to-panic on .namedT1.t0.W in the second block.

func TestSetPanic(t *testing.T) {
	type t0 struct {
		W int
	}
	type t1 struct {
		Y int
		t0
	}
	type T2 struct {
		Z int
		namedT0 t0
	}
	type T struct {
		X int
		t1
		T2
		NamedT1 t1
		NamedT2 T2
		namedT1 t1
		namedT2 T2
	}
	ok := func(f func()) {f()}
	bad := shouldPanic
	clear := func(v Value) { v.Set(Zero(v.Type())) }

	// not addressable
	v := ValueOf(T{})
	bad(func() { clear(v.Field(0)) }) // .X
	bad(func() { clear(v.Field(1)) }) // .t1
	bad(func() { clear(v.Field(1).Field(0)) }) // .t1.Y
	bad(func() { clear(v.Field(1).Field(1)) }) // .t1.t0
	bad(func() { clear(v.Field(1).Field(1).Field(0)) }) // .t1.t0.W
	bad(func() { clear(v.Field(2)) }) // .T2
	bad(func() { clear(v.Field(2).Field(0)) }) // .T2.Z
	bad(func() { clear(v.Field(2).Field(1)) }) // .T2.namedT0
	bad(func() { clear(v.Field(2).Field(1).Field(0)) }) // .T2.namedT0.W
	bad(func() { clear(v.Field(3)) }) // .NamedT1
	bad(func() { clear(v.Field(3).Field(0)) }) // .NamedT1.Y
	bad(func() { clear(v.Field(3).Field(1)) }) // .NamedT1.t0
	bad(func() { clear(v.Field(3).Field(1).Field(0)) }) // .NamedT1.t0.W
	bad(func() { clear(v.Field(4)) }) // .NamedT2
	bad(func() { clear(v.Field(4).Field(0)) }) // .NamedT2.Z
	bad(func() { clear(v.Field(4).Field(1)) }) // .NamedT2.namedT0
	bad(func() { clear(v.Field(4).Field(1).Field(0)) }) // .NamedT2.namedT0.W
	bad(func() { clear(v.Field(5)) }) // .namedT1
	bad(func() { clear(v.Field(5).Field(0)) }) // .namedT1.Y
	bad(func() { clear(v.Field(5).Field(1)) }) // .namedT1.t0
	bad(func() { clear(v.Field(5).Field(1).Field(0)) }) // .namedT1.t0.W
	bad(func() { clear(v.Field(6)) }) // .namedT2
	bad(func() { clear(v.Field(6).Field(0)) }) // .namedT2.Z
	bad(func() { clear(v.Field(6).Field(1)) }) // .namedT2.namedT0
	bad(func() { clear(v.Field(6).Field(1).Field(0)) }) // .namedT2.namedT0.W

	// addressable
	v = ValueOf(&T{}).Elem()
	ok(func() { clear(v.Field(0)) }) // .X
	bad(func() { clear(v.Field(1)) }) // .t1
	ok(func() { clear(v.Field(1).Field(0)) }) // .t1.Y
	bad(func() { clear(v.Field(1).Field(1)) }) // .t1.t0
	ok(func() { clear(v.Field(1).Field(1).Field(0)) }) // .t1.t0.W
	ok(func() { clear(v.Field(2)) }) // .T2
	ok(func() { clear(v.Field(2).Field(0)) }) // .T2.Z
	bad(func() { clear(v.Field(2).Field(1)) }) // .T2.namedT0
	bad(func() { clear(v.Field(2).Field(1).Field(0)) }) // .T2.namedT0.W
	ok(func() { clear(v.Field(3)) }) // .NamedT1
	ok(func() { clear(v.Field(3).Field(0)) }) // .NamedT1.Y
	bad(func() { clear(v.Field(3).Field(1)) }) // .NamedT1.t0
	ok(func() { clear(v.Field(3).Field(1).Field(0)) }) // .NamedT1.t0.W
	ok(func() { clear(v.Field(4)) }) // .NamedT2
	ok(func() { clear(v.Field(4).Field(0)) }) // .NamedT2.Z
	bad(func() { clear(v.Field(4).Field(1)) }) // .NamedT2.namedT0
	bad(func() { clear(v.Field(4).Field(1).Field(0)) }) // .NamedT2.namedT0.W
	bad(func() { clear(v.Field(5)) }) // .namedT1
	bad(func() { clear(v.Field(5).Field(0)) }) // .namedT1.Y
	bad(func() { clear(v.Field(5).Field(1)) }) // .namedT1.t0
	bad(func() { clear(v.Field(5).Field(1).Field(0)) }) // .namedT1.t0.W
	bad(func() { clear(v.Field(6)) }) // .namedT2
	bad(func() { clear(v.Field(6).Field(0)) }) // .namedT2.Z
	bad(func() { clear(v.Field(6).Field(1)) }) // .namedT2.namedT0
	bad(func() { clear(v.Field(6).Field(1).Field(0)) }) // .namedT2.namedT0.W
}

https://codereview.appspot.com/78500044/diff/60001/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

https://codereview.appspot.com/78500044/diff/60001/src/pkg/reflect/value.go#n...
src/pkg/reflect/value.go:81: //  - flagEmbed: val is obtained from an anonymous
field
fix spacing here (compare to previous line)

https://codereview.appspot.com/78500044/diff/60001/src/pkg/reflect/value.go#n...
src/pkg/reflect/value.go:875: } else if (v.flag & flagEmbed) == flagEmbed {
elsewhere this is written v.flag&flagEmbed != 0, such as on line 886
the ( ) are unnecessary regardless - this isn't C, thank goodness.

[mpvl, 2014-04-10 22:21:32]

Good point.  Will work on a solution and add more thorough tests as well.
 There are a lot more edge cases to be checked, really.


On Tue, Apr 8, 2014 at 5:41 PM, <rsc@golang.org> wrote:

> I support the intent of this change but I think the code is not quite
> right. In particular I think the flagRO should never be cleared. It gets
> set for a variety of things, and clearing it unconditionally is sure to
> lead to bugs. Here is one that I think it leads to.
>
> If you have logically x.foo1.foo2.Z where foo1 is not an embedded field
> but foo2 is, then the .foo1 will set flagRO, the .foo2 will set
> flagRO|flagEmbed, and then the .Z will clear flagRO because flagEmbed
> was set.
>
> There needs to be some record in the bits that "this is RO only because
> of embedded fields". Perhaps not setting flagEmbed if flagRO was already
> in effect is enough.
>
> Here is a test case; please add it to the CL. With the current logic I
> get a failure-to-panic on .namedT1.t0.W in the second block.
>
> func TestSetPanic(t *testing.T) {
>         type t0 struct {
>                 W int
>         }
>         type t1 struct {
>                 Y int
>                 t0
>         }
>         type T2 struct {
>                 Z int
>                 namedT0 t0
>         }
>         type T struct {
>                 X int
>                 t1
>                 T2
>                 NamedT1 t1
>                 NamedT2 T2
>                 namedT1 t1
>                 namedT2 T2
>         }
>         ok := func(f func()) {f()}
>         bad := shouldPanic
>         clear := func(v Value) { v.Set(Zero(v.Type())) }
>
>         // not addressable
>         v := ValueOf(T{})
>         bad(func() { clear(v.Field(0)) }) // .X
>         bad(func() { clear(v.Field(1)) }) // .t1
>         bad(func() { clear(v.Field(1).Field(0)) }) // .t1.Y
>         bad(func() { clear(v.Field(1).Field(1)) }) // .t1.t0
>         bad(func() { clear(v.Field(1).Field(1).Field(0)) }) // .t1.t0.W
>         bad(func() { clear(v.Field(2)) }) // .T2
>         bad(func() { clear(v.Field(2).Field(0)) }) // .T2.Z
>         bad(func() { clear(v.Field(2).Field(1)) }) // .T2.namedT0
>         bad(func() { clear(v.Field(2).Field(1).Field(0)) }) //
> .T2.namedT0.W
>         bad(func() { clear(v.Field(3)) }) // .NamedT1
>         bad(func() { clear(v.Field(3).Field(0)) }) // .NamedT1.Y
>         bad(func() { clear(v.Field(3).Field(1)) }) // .NamedT1.t0
>         bad(func() { clear(v.Field(3).Field(1).Field(0)) }) //
> .NamedT1.t0.W
>         bad(func() { clear(v.Field(4)) }) // .NamedT2
>         bad(func() { clear(v.Field(4).Field(0)) }) // .NamedT2.Z
>         bad(func() { clear(v.Field(4).Field(1)) }) // .NamedT2.namedT0
>         bad(func() { clear(v.Field(4).Field(1).Field(0)) }) //
> .NamedT2.namedT0.W
>         bad(func() { clear(v.Field(5)) }) // .namedT1
>         bad(func() { clear(v.Field(5).Field(0)) }) // .namedT1.Y
>         bad(func() { clear(v.Field(5).Field(1)) }) // .namedT1.t0
>         bad(func() { clear(v.Field(5).Field(1).Field(0)) }) //
> .namedT1.t0.W
>         bad(func() { clear(v.Field(6)) }) // .namedT2
>         bad(func() { clear(v.Field(6).Field(0)) }) // .namedT2.Z
>         bad(func() { clear(v.Field(6).Field(1)) }) // .namedT2.namedT0
>         bad(func() { clear(v.Field(6).Field(1).Field(0)) }) //
> .namedT2.namedT0.W
>
>         // addressable
>         v = ValueOf(&T{}).Elem()
>         ok(func() { clear(v.Field(0)) }) // .X
>         bad(func() { clear(v.Field(1)) }) // .t1
>         ok(func() { clear(v.Field(1).Field(0)) }) // .t1.Y
>         bad(func() { clear(v.Field(1).Field(1)) }) // .t1.t0
>         ok(func() { clear(v.Field(1).Field(1).Field(0)) }) // .t1.t0.W
>         ok(func() { clear(v.Field(2)) }) // .T2
>         ok(func() { clear(v.Field(2).Field(0)) }) // .T2.Z
>         bad(func() { clear(v.Field(2).Field(1)) }) // .T2.namedT0
>         bad(func() { clear(v.Field(2).Field(1).Field(0)) }) //
> .T2.namedT0.W
>         ok(func() { clear(v.Field(3)) }) // .NamedT1
>         ok(func() { clear(v.Field(3).Field(0)) }) // .NamedT1.Y
>         bad(func() { clear(v.Field(3).Field(1)) }) // .NamedT1.t0
>         ok(func() { clear(v.Field(3).Field(1).Field(0)) }) //
> .NamedT1.t0.W
>         ok(func() { clear(v.Field(4)) }) // .NamedT2
>         ok(func() { clear(v.Field(4).Field(0)) }) // .NamedT2.Z
>         bad(func() { clear(v.Field(4).Field(1)) }) // .NamedT2.namedT0
>         bad(func() { clear(v.Field(4).Field(1).Field(0)) }) //
> .NamedT2.namedT0.W
>         bad(func() { clear(v.Field(5)) }) // .namedT1
>         bad(func() { clear(v.Field(5).Field(0)) }) // .namedT1.Y
>         bad(func() { clear(v.Field(5).Field(1)) }) // .namedT1.t0
>         bad(func() { clear(v.Field(5).Field(1).Field(0)) }) //
> .namedT1.t0.W
>         bad(func() { clear(v.Field(6)) }) // .namedT2
>         bad(func() { clear(v.Field(6).Field(0)) }) // .namedT2.Z
>         bad(func() { clear(v.Field(6).Field(1)) }) // .namedT2.namedT0
>         bad(func() { clear(v.Field(6).Field(1).Field(0)) }) //
> .namedT2.namedT0.W
> }
>
>
> https://codereview.appspot.com/78500044/diff/60001/src/
> pkg/reflect/value.go
> File src/pkg/reflect/value.go (right):
>
> https://codereview.appspot.com/78500044/diff/60001/src/
> pkg/reflect/value.go#newcode81
> src/pkg/reflect/value.go:81: //  - flagEmbed: val is obtained from an
> anonymous field
> fix spacing here (compare to previous line)
>
> https://codereview.appspot.com/78500044/diff/60001/src/
> pkg/reflect/value.go#newcode875
> src/pkg/reflect/value.go:875: } else if (v.flag & flagEmbed) ==
> flagEmbed {
> elsewhere this is written v.flag&flagEmbed != 0, such as on line 886
> the ( ) are unnecessary regardless - this isn't C, thank goodness.
>
> https://codereview.appspot.com/78500044/
>

[rsc, 2014-04-11 02:55:11]

We need this by Monday for the first beta, ideally. If you'd like me to
take over the CL let me know. I think the fix might be as simple as
defining that flagEmbed is only set when flagRO is also set and means
"embedded unexported fields are the only reason flagRO is set".

[mpvl, 2014-04-11 03:09:19]

I did make some progress today. I added some tests to cover a bunch of edge
cases that were not covered before. I wanted to go over it another time
tomorrow and create an alternative that preserves the flagRO.

However, looking at the code, I could really only detect one meaning of
flagRO, albeit a slight different one from what the name suggests, namely:
this value obtained using an unexported identifier.  If this is the correct
interpretation, I wonder if it is really right to preserve it. What are the
various interpretations according to you?

If the meaning of flagRO is what it seems to me, then an promoted exported
field of an embedded unexported struct is effectively not RO, and the same
rules for deriving flags will apply as if it is a top-level value.  In
other words, preserving flagRO would not give any useful information in
this case. At the same time, preserving it would seem to make the code a
bit more complicated.

Anyway, I've uploaded the latest version. I want to take another look
tomorrow and I should be able to finish this up before the weekend.



On Fri, Apr 11, 2014 at 4:55 AM, Russ Cox <rsc@golang.org> wrote:

> We need this by Monday for the first beta, ideally. If you'd like me to
> take over the CL let me know. I think the fix might be as simple as
> defining that flagEmbed is only set when flagRO is also set and means
> "embedded unexported fields are the only reason flagRO is set".
>
>
>
>

[mpvl, 2014-04-11 03:09:21]

Would like to go over the code a bit more tomorrow, but you might want to review
this early given the time constraints.

https://codereview.appspot.com/78500044/diff/60001/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

https://codereview.appspot.com/78500044/diff/60001/src/pkg/reflect/value.go#n...
src/pkg/reflect/value.go:81: //  - flagEmbed: val is obtained from an anonymous
field
On 2014/04/08 15:41:05, rsc wrote:
> fix spacing here (compare to previous line)

Done.

https://codereview.appspot.com/78500044/diff/60001/src/pkg/reflect/value.go#n...
src/pkg/reflect/value.go:875: } else if (v.flag & flagEmbed) == flagEmbed {
:) Done.

On 2014/04/08 15:41:05, rsc wrote:
> elsewhere this is written v.flag&flagEmbed != 0, such as on line 886
> the ( ) are unnecessary regardless - this isn't C, thank goodness.

[rsc, 2014-04-11 04:00:04]

I've been on the fence, but now I'm convinced. We can't have this ready in time.
Let's roll back cmang's change instead for Go 1.3.

[rsc, 2014-04-11 04:07:25]

The rollback is https://codereview.appspot.com/85580046/.

[rsc, 2014-04-17 02:52:58]

R=close

Let's pick this up in 1.4. (hg mail will reopen then.)

[mpvl, 2014-04-17 22:10:48]

Sounds good.


On Wed, Apr 16, 2014 at 7:52 PM, <rsc@golang.org> wrote:

> R=close
>
> Let's pick this up in 1.4. (hg mail will reopen then.)
>
>
> https://codereview.appspot.com/78500044/
>