reflect: permit unexported fields in StructOf
dotaheor opened this issue · 24 comments
Please answer these questions before submitting your issue. Thanks!
What version of Go are you using (go version)?
go version go1.10.2 linux/amd64
Does this issue reproduce with the latest release?
yes
What did you do?
package main
import "fmt"
import "reflect"
func main() {
type T = struct {
x int
}
fmt.Println(reflect.TypeOf(T{})) // struct { x int }
var n int
tn := reflect.TypeOf(n)
// panic: reflect.StructOf: field "x" is unexported but missing PkgPath
tt := reflect.StructOf([]reflect.StructField{
{Name: "x", Type: tn, Anonymous: false},
})
fmt.Println(tt)
}
What did you expect to see?
not panic.
What did you see instead?
panic.
If it really should panic, it should be documented.
Change https://golang.org/cl/115015 mentions this issue: reflect: document that StructOf panics on unexported fields
It looks the modified docs is still not accurate.
StructOf currently does not generate wrapper methods for embedded fields ...
In fact, for gc, if the number of methods is no more than 32 for the new created struct type, the wrapper methods are generated without problems. #25402
package main
import "fmt"
import "reflect"
type T int
func (t T) M() (int, string) {
return 123, "hello"
}
func main() {
var t T
tt := reflect.StructOf([]reflect.StructField{
{Name: "T", Anonymous: true, Type: reflect.TypeOf(t)},
})
vtt := reflect.New(tt).Elem()
fmt.Println(tt.Method(0).Func.Call([]reflect.Value{vtt})) // [<int Value> hello]
fmt.Println(vtt.Method(0).Call([]reflect.Value{})) // [<int Value> hello]
}
Maybe, the following description is better
StructOf currently does not guarantee to generate wrapper methods for embedded fields ...
Can we get this fixed so it is allowed, or learn why it is not? (I think it should be.)
Reopening to permit this case.
Thanks @ianlancetaylor ! I have some good examples of code that would be vastly simplified if this were allowed... In https://github.com/purpleidea/mgmt/ ... grep for StructOf. Some recent unmerged patches need to be changed to work around this limitation.
Hiya, I'd love to be able to push this forward in someway, any recommendations on how I could help? Without it, I'd need to seriously re-architecture some of my code which would be significantly less elegant. Thank you!
One issue to address is the requirements on unexported fields in the StructField values passed to StructOf. Should we require that the caller both use an unexported name and also set the PkgPath field? I guess so. But what should the PkgPath field be? It doesn't seem ideal to let package p1 create a struct with unexported fields that appears to be from package p2. Maybe it doesn't matter.
A complexity is recreating the data structure that the compiler creates for an unexported name, but that should be doable.
@ianlancetaylor Thanks Ian! I feel like if you're using this particular feature, you're implementing a language on top of golang or similar, and you might not necessarily care about PkgPath, although I'm not sure what the internal implications of it are.
Anything I can do to help here? I can golang, but I don't know the internals of your compiler very well.
An unexported field requires a PkgPath. In effect the PkgPath is part of the name of the field. The language specifies that an unexported name from one package is never the same as an unexported name from a different package, even if the names are the same identifier. This is implemented using the PkgPath.
Working on this issue doesn't require touching the compiler at all. Everything can be done in the reflect package. Look closely at how the reflect package handles the name type, including the pkgPath and isExported methods. The name field of structField will have to be set such that those methods do the right thing, one way or another.
Bug or not?
package main
import (
"fmt"
"reflect"
)
func main() {
type T = struct {
x int
}
t0 := reflect.TypeOf(T{})
var n int
tn := reflect.TypeOf(n)
t1 := reflect.StructOf([]reflect.StructField{
{Name: "x", Type: tn, Anonymous: false, PkgPath: t0.Field(0).PkgPath},
})
t2 := reflect.StructOf([]reflect.StructField{
{Name: "x", Type: tn, Anonymous: false, PkgPath: "net"},
})
fmt.Println(t0 == t1) // false
fmt.Println(t1 == t2) // true
fmt.Println(t0.Field(0)) // {x main int 0 [0] false}
fmt.Println(t1.Field(0)) // {x int 0 [0] false}
fmt.Println(t2.Field(0)) // {x int 0 [0] false}
fmt.Println(t0.Field(0).PkgPath) // main
fmt.Println(t1.Field(0).PkgPath) // (blank)
fmt.Println(t2.Field(0).PkgPath) // (blank)
}
$ go version
go version go1.14beta1 linux/amd64
@dotaheor I do not see the output mentioned in the comments. I see
true
false
{x main int 0 [0] false}
{x main int 0 [0] false}
{x net int 0 [0] false}
main
main
net
@ianlancetaylor It looks your output is for tip. So will this be fixed in Go 1.15?
My output is for tip which will become 1.14.
I'm sorry, I don't understand what is broken. You said above "Bug or not?" and then showed a program that does not produce the output that the comments suggest. I don't know whether there is a bug or where it may be. Can you tell us precisely what you expect to see and precisely what you do see? Thanks.
I mean it looks like a bug if the go1.14beta1 output is the same as go1.14.
Can you tell us precisely what you expect to see from this program?
Nothing, I just need to verify whether or not there is a bug in go1.14beta1.
I don't know if you are expecting any answer from me.
Do I still need to file a separated issue for this?
I'm sorry, I have no idea what "this" means, since you haven't answered any of my questions.
"This" means the program in #25401 (comment)
It looks you have implied that the output from go1.14beta1 is not correct, whereas the output from tip is right. It is great if the tip code will be merged into Go 1.14. And if this is true, I think it is not necessary to file a new issue.
We have not yet made the Go 1.14 release branch (though I think we are making it today). Everything that is on tip today will be in Go 1.14. That is what I tried to say above at #25401 (comment).
OK, thanks for the confirmations.
Related: #36191
@ianlancetaylor Thanks for fixing this problem.
I think this issue can be closed now.
As mentioned above, we were keeping this issue open specifically to add support for embedded methods.