reflect: resolve incompatibility between gc and gccgo for unexported embedded structs #12367

mpvl · 2015-08-27T20:51:59Z

Current Situation

gc: unexported embedded structs are settable
gccgo: unexported embedded structs are not settable

Both are not correct w.r.t. the way Go works: an unexported embedded struct should not be settable as a whole, however, exported fields and methods of unexported embedded structs may be promoted and be visible.

Goal

Make gc and gccgo compatible again with a workable solution.
address issues 7363 and 11007.

Solution

Make unexported embedded structs unsettable, but allow the reflect library to access exported fields and methods of such structs. Such fields could also be modified.

Rationale: this prevents blanket changes to values in structs that are generally not settable, while allowing access to fields that might be promoted.

The approach might still allow access to fields that are not accessible in the language, namely exported fields and methods that are not promoted because they are masked by or conflict with fields or methods of the same name. However, access to such fields is also necessary. Packages like encoding/xml and encoding/json allow users to map fields to alternative names using tags effectively breaking ties and making fields visible again. Disallowing access to blocked fields now would be too much of a disruptive change and break the Go 1 compatibility promise.

Impact

This will break packages like xml and json, but only minimally. The typical check for whether a field is exported, f.PkgPath != "", would have to be amended to f.PkgPath != "" && !f.Anonymous.

Alternatives

adopt gccgo behavior

Unacceptable: it would be too disruptive to packages like xml and json and would break documented behavior, breaking the Go 1 compatibility promise. It would mean reflect cannot give access to fields that are visible in Go itself.

adopt gc behavior

Less problematic, but it would make it very easy to access structs that are indicated as hidden. Users could bypass initialization code etc, raising various security issues. The proposed solution does not eliminate this, but it does considerably mitigate it by limiting access to fields that are deemed exported and settable if the embedded type were accessed directly.

package reflect provides a different API for accessing promoted fields and methods.

This will be a huge change while still not addressing what to do with the remaining old API and its incompatibility.
It would not be possible for xml and json to implement their documented behavior.

Provide a higher-level API for packages like xml and json for mapping structured data to struct fields

This would potentially give greater consistency and would make it easier for developers to write package like json and xml. However, it would be a huge change and similarly does not address the concerns of what to do with the current API. Like the current proposal, it would still allow access to non-promoted fields and methods. It could be implemented on top of the proposed solution.

rsc · 2015-08-27T23:15:37Z

I think the statement here can be made much more precise. Here is my understanding.

Today, given:

type T1 struct {
    t2
}

type t2 struct {
    X int
    u int
}

var t1 T1
v1 := reflect.ValueOf(&t1).Elem()
v2 := v1.Field(0)
X := v2.Field(0)
u := v2.Field(1)

In the gc toolchain,

v1.CanSet() == true
v1.Type().Field(0).PkgPath == nil // nil usually indicates exported name
v2.CanSet() == true // usually unexported cannot be set
X.CanSet() == true
u.CanSet() == false

In the gccgo toolchain,

v1.CanSet() == true
v1.Type().Field(0).PkgPath != nil // unexported name "t2"
v2.CanSet() == false
X.CanSet() == false // uses of encoding/json and encoding/xml expect true here
u.CanSet() == false

The fundamental problem here is that a Go program's source code can refer to t1.X (implicitly t1.t2.X) directly, while the reflect API cannot. The reflect API requires stepping into each field explicitly. The gc implementation allows reflection to set t1.X by recording the field t2 as exported (wrong): that makes it possible to set t1.t2.X (arguably right) but also all of t1.t2 (wrong). The gccgo implementation records t2 as unexported (right), which disallows setting t1.t2.X (arguably wrong) and also disallows setting t1.t2 (right).

The proposal is that both implementations record t2 as unexported, and then change reflect to record whether a value like v2 is read-only only because it is an unexported embedded field. In that case, accessing an exported element within would clear the read-only bit. The effect would be that t2 is record as unexported (right), t1.t2.X is settable (arguably right), t1.t2 is not settable (right), and of course t1.t2.u is also not settable (right).

That is, under this proposal:

v1.CanSet() == true
v1.Type().Field(0).PkgPath != nil // unexported name "t2"
v2.CanSet() == false
X.CanSet() == true
u.CanSet() == false

That satisfies encoding/xml and encoding/json, disallows most of what is incorrectly allowed in the gc toolchain today, and allows what is arguably incorrectly disallowed in the gccgo toolchain today.

Both encoding/json and encoding/xml assume today that if f.PkgPath != nil then the field is unexported and must be ignored. That test needs to be revised to f.PkgPath != nil && !f.Anonymous, so that they do try to walk into the embedded fields to look for unexported fields contained within. Any similar external packages would need the same ~1 line change. The new condition is correct both before and after the proposed change, so it could be made by any affected packages independent of the eventual Go 1.6 release.

mpvl · 2015-08-28T06:19:26Z

Thanks for the much clearer reformulation Russ.

mpvl · 2015-08-28T08:29:43Z

Preparations for json and xml package:
https://go-review.googlesource.com/#/c/14011/
https://go-review.googlesource.com/#/c/14012/

See the actual change:
https://go-review.googlesource.com/#/c/14010/

Note that the tests added in the json and xml CLs will currently break for gccgo.

Packages just checking for f.PkgPath to determine if a field is exported should implement a fix similar to those in 14011 and 14012 now to prevent breaking when CL 14010 is checked in.

gopherbot · 2015-08-28T09:00:53Z

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

gopherbot · 2015-08-28T09:00:57Z

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

gopherbot · 2015-08-28T09:01:19Z

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

gopherbot · 2015-08-31T21:00:37Z

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

This CL changes reflect to allow access to exported fields and methods in unexported embedded structs for gccgo and after gc has been adjusted to disallow access to embedded unexported structs. Adresses #12367, #7363, #11007, and #7247. Change-Id: If80536eab35abcd25300d8ddc2d27d5c42d7e78e Reviewed-on: https://go-review.googlesource.com/14010 Reviewed-by: Russ Cox <rsc@golang.org>

Addresses issue #12367. Must be checked in before CL 14010. Change-Id: I7233c3a62d4f55d0ac7e8a87df5fc4ee7beb7207 Reviewed-on: https://go-review.googlesource.com/14011 Reviewed-by: Russ Cox <rsc@golang.org>

Addresses issue #12367. Must be checked in before CL 14010. Change-Id: I4523a1de112ed02371504e27882659bce8028a9f Reviewed-on: https://go-review.googlesource.com/14012 Reviewed-by: Russ Cox <rsc@golang.org>

mpvl · 2015-10-26T14:00:57Z

Code that assumes

f.PkgPath != nil

means a field is unexported and must be ignored must now be revised to check for

f.PkgPath != nil && !f.Anonymous

for it to walk into the embedded structs to look for exported fields contained within.

…, docs - json: inline skipWhitespace: this may give some slight performance improvement by eliding the function call on the hot-path. - handle exported members of unexported anonymous fields See golang/go#12367 for more info - document that WriteExt, ConvertExt may take a pointer value. To avoid potentially expensive copying, we may pass in the address to a struct or array

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

code that assumes `f.PkgPath != nil` means a field is unexported and must be ignored must now be revised to check for `f.PkgPath != nil && !f.Anonymous` for it to walk into the embedded structs to look for exported fields contained within. golang/go#12367

See https://golang.org/doc/go1.6#reflect and golang/go#12367

mikioh added the Proposal label Aug 27, 2015

mwhudson mentioned this issue Aug 28, 2015

cmd/compile: empty PkgPath for embedded, unexported struct field #7247

Closed

adg added Proposal and removed Proposal labels Sep 25, 2015

rsc added this to the Proposal milestone Oct 24, 2015

rsc changed the title ~~proposal: workable solution to reflect incompatibility between gc and gccgo~~ reflect: resolve incompatibility between gc and gccgo for unexported embedded structs Oct 24, 2015

rsc modified the milestones: Go1.6Early, Proposal Oct 24, 2015

rsc added the Proposal-Accepted label Oct 24, 2015

mpvl closed this as completed in afe9837 Oct 26, 2015

xStrom mentioned this issue Dec 18, 2015

Correctly handle embedded unexported structs in Go 1.6. maddyblue/goon#54

Merged

sqs mentioned this issue Dec 22, 2015

Parse unexported embedded option struct fields in Go 1.6 (consistent w/pre-Go 1.6) jessevdk/go-flags#156

Merged

Rican7 mentioned this issue Feb 18, 2016

Bugfix - Go 1.6 reflect embedded unexported field access google/go-querystring#10

Merged

tatatodd mentioned this issue Mar 3, 2016

Ensure go1.6 reflect changes are properly handled vanadium/issues#1218

Open

groob added a commit to groob/plist that referenced this issue Mar 4, 2016

fix embeded structs for 1.6 golang/go#12367

b61497a

daboyuka added a commit to pendo-io/martini-contrib-binding that referenced this issue Mar 24, 2016

Fixed reliance on incorrect Go <=1.5 reflection behavior

38c7ae7

See https://golang.org/doc/go1.6#reflect and golang/go#12367

daboyuka added a commit to pendo-io/martini-contrib-binding that referenced this issue Mar 24, 2016

Fixed reliance on incorrect Go <=1.5 reflection behavior

045e4ff

See https://golang.org/doc/go1.6#reflect and golang/go#12367

daboyuka added a commit to pendo-io/martini-contrib-binding that referenced this issue Mar 24, 2016

Fixed reliance on incorrect Go <=1.5 reflection behavior

618903c

See https://golang.org/doc/go1.6#reflect and golang/go#12367

andrewkroh mentioned this issue Mar 29, 2016

Export embedded struct used in config reflection elastic/beats#1256

Merged

jgoelitz mentioned this issue Apr 8, 2016

Added support for Go 1.6. nimajalali/go-force#29

Merged

This was referenced Sep 9, 2016

golang 1.7 issue fatih/structs#72

Closed

-help panicking with Go 1.7 koding/multiconfig#50

Closed

rjeczalik mentioned this issue Sep 11, 2016

update IsExported check for go1.6 refelct change fatih/structs#73

Closed

golang locked and limited conversation to collaborators Oct 26, 2016

gopherbot added the FrozenDueToAge label Oct 26, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reflect: resolve incompatibility between gc and gccgo for unexported embedded structs #12367

reflect: resolve incompatibility between gc and gccgo for unexported embedded structs #12367

mpvl commented Aug 27, 2015

rsc commented Aug 27, 2015

mpvl commented Aug 28, 2015

mpvl commented Aug 28, 2015

gopherbot commented Aug 28, 2015

gopherbot commented Aug 28, 2015

gopherbot commented Aug 28, 2015

gopherbot commented Aug 31, 2015

mpvl commented Oct 26, 2015

reflect: resolve incompatibility between gc and gccgo for unexported embedded structs #12367

reflect: resolve incompatibility between gc and gccgo for unexported embedded structs #12367

Comments

mpvl commented Aug 27, 2015

Current Situation

Goal

Solution

Impact

Alternatives

adopt gccgo behavior

adopt gc behavior

package reflect provides a different API for accessing promoted fields and methods.

Provide a higher-level API for packages like xml and json for mapping structured data to struct fields

rsc commented Aug 27, 2015

mpvl commented Aug 28, 2015

mpvl commented Aug 28, 2015

gopherbot commented Aug 28, 2015

gopherbot commented Aug 28, 2015

gopherbot commented Aug 28, 2015

gopherbot commented Aug 31, 2015

mpvl commented Oct 26, 2015