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

Closed
mpvl opened this Issue Aug 27, 2015 · 8 comments

Comments

Projects
None yet
5 participants
Member

mpvl commented Aug 27, 2015

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.

@mikioh mikioh added the Proposal label Aug 27, 2015

Contributor

rsc commented Aug 27, 2015

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.

Member

mpvl commented Aug 28, 2015

Thanks for the much clearer reformulation Russ.

Member

mpvl commented Aug 28, 2015

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.

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

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

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

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

@adg adg added Proposal and removed Proposal labels Sep 25, 2015

@rsc rsc added this to the Proposal milestone Oct 24, 2015

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

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

mpvl added a commit that referenced this issue Oct 26, 2015

reflect: adjust access to unexported embedded structs
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>

mpvl added a commit that referenced this issue Oct 26, 2015

encoding/json: check for exported fields in embedded structs
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>

mpvl added a commit that referenced this issue Oct 26, 2015

encoding/xml: check for exported fields in embedded structs
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 mpvl closed this in afe9837 Oct 26, 2015

Member

mpvl commented Oct 26, 2015

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.

ugorji added a commit to ugorji/go that referenced this issue Oct 26, 2015

codec: json: inline skipWhitespace, handle exported anonymous members…
…, 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

sqs added a commit to sourcegraph/go-flags that referenced this issue Dec 22, 2015

Parse unexported embedded option struct fields in Go 1.6 (consistent …
…w/pre-Go 1.6)

Pre-Go 1.6, the newly added TestEmbeddedUnexported test passes. In Go
1.6beta1, it fails:

```
$ go test
--- FAIL: TestEmbedded (0.00s)
        assert_test.go:92: Unexpected parse error: unknown flag `v'
```

This commit makes the behavior consistent across Go versions.

Related: golang/go#12367, specifically the
comments about how "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."

sqs added a commit to sourcegraph/go-flags that referenced this issue Dec 22, 2015

Parse unexported embedded option struct fields in Go 1.6 (consistent …
…w/pre-Go 1.6)

Pre-Go 1.6, the newly added TestEmbeddedUnexported test passes. In Go
1.6beta1, it fails:

```
$ go test
--- FAIL: TestEmbedded (0.00s)
        assert_test.go:92: Unexpected parse error: unknown flag `v'
```

This commit makes the behavior consistent across Go versions.

Related: golang/go#12367, specifically the
comments about how "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."

Rican7 added a commit to Rican7/go-querystring that referenced this issue Feb 18, 2016

Now checking if a field is not anonymous before
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

taylorchu added a commit to go-ndn/tlv that referenced this issue Feb 19, 2016

fix embedded struct fields
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

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

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

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

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

rjeczalik added a commit to rjeczalik/structs that referenced this issue Sep 11, 2016

@gopherbot gopherbot locked and limited conversation to collaborators Oct 26, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.