code review 85580046: undo CL 66510044 / 6c0339d94123

undo CL 66510044 / 6c0339d94123

Broke other things - see issue 7522.

Fixes issue 7522.
Reopens issue 7363.

««« original CL description
cmd/gc: make embedded, unexported fields read-only.

Fixes issue 7363.

LGTM=gri
R=gri, rsc, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/66510044
»»»

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

Hello golang-codereviews@googlegroups.com (cc: iant, mpvl),

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

[r, 2014-04-11 04:19:12]

LGTM

[mpvl, 2014-04-11 17:12:18]

LGTM.

Probably a good idea.

Fixing the divergence between the two compilers cannot be done without some
pain.  If we can wait fixing the divergence between the two compilers and
given that my fix was (knowingly) still not fixing the issue, it is better
to come up with a complete solution and make the pain more worthwhile.

I think the following is reasonable plan:

Step 1:
- Augment the reflect API to allow for easy identification of all fields,
including promoted ones, without the need to access unexported fields.
- Verify that the API works well for the xml and json packages, as well as
a bunch of public ones.

Step 2:
- Publicly announce the upcoming change in semantics of PkgPath, the new
API, and its implications for xml and json.

Step 3:
- Add test cases to xml and json packages to cover the multitude of edge
cases.
- Update xml and json packages to use the augmented API.
- Note: depending on the details of the various implementations, this may
result in a slight API change, as conflicting fields in embedded unexported
structs cannot be accessed by the new alternative API.
- Give people a chance to update their code as well in an analogous manner.

Step 4:
- If all is well and dandy, modify the PkgPath semantics as in CL 66510044.
- A successful implementation of the previous steps should minimize the
effect of this change.
- No harm is done if it is decided that CL 66510044 should not be applied
at this point; the augmented reflect API will be useful either way.

An advantage of introducing a new API for this purpose is that the existing
reflect code can roughly stay as is, thereby minimizing the possibility of
introducing other errors. That is, once a value becomes "read-only", it
will stay as such.  The only way to get to promoted fields, other than
explicitly following the path, is to use the new API.

Another advantage is that packages using reflect can be expected to be more
consistent. For example, the json package now typically drops conflicting
fields of embedded unexported structs, as expected. However, it basis a
conflict on the "json name", not the Go accessibility. So rename one of the
fields with a json tag, and the field becomes accessible. This seems wrong
(Go itself does not allow access here) and would be prevented with the
proposed change. It is really hard to get this right, so it makes sense to
me that this is handled by the reflect API itself.

This advantage is also a disadvantage, as it is another API breakage,
albeit a pretty esoteric one and eliminating IMO buggy behavior.

A disadvantage of this approach overall is that it could be considered a
considerable breakage of the no API changes promise. However, it solves a
series of inconsistencies if done well. As a bonus dealing with reflect in
packages like xml, json and yaml should become noticeably easier.



On Fri, Apr 11, 2014 at 6:02 AM, <rsc@golang.org> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com (cc: iant, mpvl),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> undo CL 66510044 / 6c0339d94123
>
> Broke other things - see issue 7522.
>
> Fixes issue 7522.
> Reopens issue 7363.
>
> ««« original CL description
> cmd/gc: make embedded, unexported fields read-only.
>
> Fixes issue 7363.
>
> LGTM=gri
> R=gri, rsc, bradfitz
> CC=golang-codereviews
> https://codereview.appspot.com/66510044
> »»»
>
> Please review this at https://codereview.appspot.com/85580046/
>
> Affected files (+1, -28 lines):
>   M src/cmd/gc/reflect.c
>   R test/fixedbugs/issue7363.go
>
>
> Index: src/cmd/gc/reflect.c
> ===================================================================
> --- a/src/cmd/gc/reflect.c
> +++ b/src/cmd/gc/reflect.c
> @@ -1138,8 +1138,7 @@
>                                         ot = dgopkgpath(s, ot,
> t1->sym->pkg);
>                         } else {
>                                 ot = dgostringptr(s, ot, nil);
> -                               if(t1->type->sym != S &&
> -                                  (t1->type->sym->pkg == builtinpkg ||
> !exportname(t1->type->sym->name)))
> +                               if(t1->type->sym != S &&
> t1->type->sym->pkg == builtinpkg)
>                                         ot = dgopkgpath(s, ot, localpkg);
>                                 else
>                                         ot = dgostringptr(s, ot, nil);
> Index: test/fixedbugs/issue7363.go
> ===================================================================
> deleted file mode 100644
> --- a/test/fixedbugs/issue7363.go
> +++ /dev/null
> @@ -1,26 +0,0 @@
> -// run
> -
> -// Copyright 2014 The Go Authors.  All rights reserved.
> -// Use of this source code is governed by a BSD-style
> -// license that can be found in the LICENSE file.
> -
> -// issue 7363: CanSet must return false for unexported embedded struct
> fields.
> -
> -package main
> -
> -import "reflect"
> -
> -type a struct {
> -}
> -
> -type B struct {
> -       a
> -}
> -
> -func main() {
> -       b := &B{}
> -       v := reflect.ValueOf(b).Elem().Field(0)
> -       if v.CanSet() {
> -               panic("B.a is an unexported embedded struct field")
> -       }
> -}
>
>
>

[rsc, 2014-04-14 13:48:16]

*** Submitted as https://code.google.com/p/go/source/detail?r=77578375f623 ***

undo CL 66510044 / 6c0339d94123

Broke other things - see issue 7522.

Fixes issue 7522.
Reopens issue 7363.

««« original CL description
cmd/gc: make embedded, unexported fields read-only.

Fixes issue 7363.

LGTM=gri
R=gri, rsc, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/66510044
»»»

LGTM=r, mpvl
R=golang-codereviews, r
CC=golang-codereviews, iant, mpvl
https://codereview.appspot.com/85580046