I don't really see how to fix this safely: ensuring that the Addr can not be used in some way to set the unexported fields. If you think that your changes are safe, do you want to send a pull request? Thanks.

@ianlancetaylor I thought after your response, I think it is safe for reasons:

• Result of Value.Addr() is not CanAddr and hence not CanSet, so the field itself can't be modified later, this does not interfere with flagRO.
• Addr().Elem() get flagRO same as origin value, so it should behave same as before in CanSet.
• Value.Field() inherits flagStickyRO but not flagEmbedRO from wrapped value, so
  • if the value itself is flagEmbedRO, Value.Field()'s flagRO depends only on that field but not its wrapped value;
  • if the value itself is flagStickyRO, all its fields are unexported.

I have update commits with more test cases in trial fix branch to (kezhuw/go@306e9c9 kezhuw/go@a16093d), I plan to send a pull request later after job (at least 10 hours late) if above sounds good to you.

See Value.Field().

Change https://golang.org/cl/183937 mentions this issue: Fix lost flagEmbedRO in reflect.Value.Addr().Elem()

I'm sympathetic to the rationale that if v.CanAddr(), then v.Addr().Elem() should be the same as just v alone. After all, if x is an addressable Go expression, then *&x is the same as x.

In general, package reflect exposes the same API as you could use through a package's exported interface. An issue arises when fields are promoted from a non-exported embedded field. For example:

type t struct { X int }
type U struct { t }
var V U

An external package can access V.X, which is short-hand for V.t.X (though the latter isn't actually in external packages).

This is problematic for package reflect because reflect doesn't provide a way to directly access promoted fields. They have to be accessed field-by-field, and the behavior differed between cmd/compile (née gc) and gccgo. It was decided in #12367 (included in Go 1.6) to reconcile this by special casing accesses like v.Field(0).Field(0) if the result is an exported field. That is, to special case V.t.X, since package reflect doesn't support V.X directly. We also special case dereferences, because of pointer embedding. (This was further fixed in #22031, released in Go 1.10.)

This is where the intuition about *& falls down in my opinion: is that intuition important enough to further special case expressions (*&V.t).X? I don't think it's unreasonable, but I'm also not convinced.

@kezhuw You mentioned finding this when upgrading from Go 1.5 to Go 1.11. Do you mean this broke some code you had written? Can you share the code?

@mdempsky You can see the last two commits in https://github.com/kezhuw/toml/commits/9d1a8af4e1bcb6c1efa7523c915d9570b4a40e48. I have push another branch https://github.com/kezhuw/toml/commits/go-issues-32772 to revert the fix and preserve only one test case, you can go test . to see the panic stack.

@mdempsky Here is build matrix https://travis-ci.org/github/kezhuw/toml/builds/676804186 after reverting the fix.

@kezhuw Thanks. Knowing this broke real world code rather than just a theoretical concern is useful information.

@ianlancetaylor What do you think? Package reflect today special cases V.t.X as the way to access promoted field V.X. Should it also special case (*&V.t).X (when V.t is addressable)?

I can't think of any negative consequences to this aside from theoretical purity.

Thanks for the analysis. I'm OK with the change if it doesn't introduce any holes in the type system, which it sounds like it doesn't.