reflect: regression in Value.Addr().Elem() roundtrip

[dsnet]

Consider the following:

type child struct{ Field string }
type parent struct{ child }

v := reflect.ValueOf(&parent{}).Elem().Field(0) // v is a child value
fmt.Println(v.Field(0).CanSet())                // Is child.Field settable? Yes.
v = v.Addr().Elem()                             // Perform roundtrip: v = *(&v)
fmt.Println(v.Field(0).CanSet())                // Is child.Field still settable? Maybe?

On go1.9, this printed:

true
true

On go1.10, this printed:

true
false

I find it very surprising that a round-trip reference/derefence would cause the exported Field to no longer be settable. It's not clear to me whether this is intended behavior or not.

\cc @mdempsky @ianlancetaylor

[dsnet]

Git bisect blames https://golang.org/cl/66331.

The CL description seems to indicate that this is intended behavior:

However, the way this was implemented allowed other operations to be
interspersed between the Field calls, which could grant inappropriate
access.

If this is intended, then for #24152 we'll need to fix the json package to not assume that a roundtrip Vaue.Addr().Elem() is side-effect free (which the json.decodeState.indirect method currently assumes).

[mdempsky]

I would argue that reflect is working correctly in Go 1.10, and that json will need to fix its Addr/Elem roundtrip.

Rationale: Given

p := &parent{}
v := reflect.ValueOf(p)

it's legal to write

(*p).Field

which maps to the reflect operations v.Elem().Field(0).Field(0) in your example.

However, it's not legal to write

(*&(*p).child).Field

because child is not exported. Thus reflect should not allow v.Elem().Field(0).Addr().Elem().Field(0).

[dsnet]

I argue that:

// case 1
(*p).Field = "hello"
v.Elem().FieldByName("Field").SetString("hello")

// case 2
(*p).child.Field = "hello"
v.Elem().FieldByName("child").FieldByName("Field").SetString("hello")

// case 3
(*&(*p).child).Field = "hello"
v.Elem().FieldByName("child").Addr().Elem().FieldByName("Field").SetString("hello")

Case 1 is clearly fine. Case 2 is permitted (but violates normal visibility rules). If case 2 is allowed, I don't see why case 3 is forbidden. You could say that we should forbid case 2, but I surmise that would break a lot of existing Go code.

[mdempsky]

I think case 1 is clearly okay.

Case 2 is tricky though, and shouldn't be used as a precedent to justify case 3.

Earlier, we were talking about Value.Field, not Value.FieldByName. Field doesn't support directly accessing promoted fields. This means v.Elem().Field(0).Field(0) needs to be allowed because it's the only way to represent p.Field (which is valid), even though it also represents p.child.Field (which should be invalid).

It seems FieldByName is acting consistent with Field, even though it doesn't need to: since we can directly allow v.FieldByName("Field"), there's not a strong reason to need to allow v.FieldByName("child").FieldByName("Field"), IMO.

Maybe we need to continue allowing it for backwards compatibility, or so that FieldByName parallels Field. In either case, I don't think it's a good argument for allowing case 3.

[dsnet]

Bleh, the whole situation is funky.

Back-to-back use of Field also allows other things not normally allowed:

type child1 struct{ Conflict string }
type child2 struct{ Conflict string }
type parent struct {
	child1
	child2
}

p := new(parent)
reflect.ValueOf(p).Elem().Field(0).Field(0).SetString("hello")
reflect.ValueOf(p).Elem().Field(1).Field(0).SetString("goodbye")
fmt.Println(p) // &{{hello} {goodbye}}

Here, the existence of a conflict on Conflict makes it such that it would normally be impossible to access this field. However, it is possible with the back-to-back Field calls.

[mdempsky]

Yeah, it's kind of a mess. :( I'd argue that should be disallowed too, but I'm not sure how to do that efficiently.

[gopherbot]

Change https://golang.org/cl/97796 mentions this issue: encoding/json: avoid assuming side-effect free reflect.Value.Addr().Elem()

[ianlancetaylor]

The problem described by this issue is working as intended.

[gopherbot]

Change https://golang.org/cl/102784 mentions this issue: [release-branch.go1.10] encoding/json: avoid assuming side-effect free reflect.Value.Addr().Elem()