Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reflect: permit unexported fields in StructOf #25401

Open
dotaheor opened this issue May 15, 2018 · 24 comments
Open

reflect: permit unexported fields in StructOf #25401

dotaheor opened this issue May 15, 2018 · 24 comments
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dotaheor
Copy link

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.

@ianlancetaylor ianlancetaylor added Documentation NeedsFix The path to resolution is known, but the work has not been done. labels May 15, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone May 15, 2018
@ALTree ALTree changed the title reflect: StructOf panics on unexported fileds. reflect: StructOf panics on unexported fields May 29, 2018
@gopherbot
Copy link

Change https://golang.org/cl/115015 mentions this issue: reflect: document that StructOf panics on unexported fields

@dotaheor
Copy link
Author

dotaheor commented May 30, 2018

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 ...

@purpleidea
Copy link

Can we get this fixed so it is allowed, or learn why it is not? (I think it should be.)

@ianlancetaylor ianlancetaylor changed the title reflect: StructOf panics on unexported fields reflect: permit unexported fields in StructOf Jan 4, 2019
@ianlancetaylor
Copy link
Contributor

Reopening to permit this case.

@purpleidea
Copy link

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.

@purpleidea
Copy link

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!

@ianlancetaylor
Copy link
Contributor

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.

@purpleidea
Copy link

@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.

@ianlancetaylor
Copy link
Contributor

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.

@dotaheor
Copy link
Author

dotaheor commented Feb 3, 2020

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

@ianlancetaylor
Copy link
Contributor

@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

@dotaheor
Copy link
Author

dotaheor commented Feb 4, 2020

@ianlancetaylor It looks your output is for tip. So will this be fixed in Go 1.15?

@ianlancetaylor
Copy link
Contributor

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.

@dotaheor
Copy link
Author

dotaheor commented Feb 5, 2020

I mean it looks like a bug if the go1.14beta1 output is the same as go1.14.

@ianlancetaylor
Copy link
Contributor

Can you tell us precisely what you expect to see from this program?

@dotaheor
Copy link
Author

dotaheor commented Feb 5, 2020

Nothing, I just need to verify whether or not there is a bug in go1.14beta1.

@ianlancetaylor
Copy link
Contributor

I don't know if you are expecting any answer from me.

@dotaheor
Copy link
Author

dotaheor commented Feb 5, 2020

Do I still need to file a separated issue for this?

@ianlancetaylor
Copy link
Contributor

I'm sorry, I have no idea what "this" means, since you haven't answered any of my questions.

@dotaheor
Copy link
Author

dotaheor commented Feb 5, 2020

"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.

@ianlancetaylor
Copy link
Contributor

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).

@dotaheor
Copy link
Author

dotaheor commented Feb 5, 2020

OK, thanks for the confirmations.

@dotaheor
Copy link
Author

dotaheor commented Feb 6, 2020

Related: #36191

@ianlancetaylor Thanks for fixing this problem.

I think this issue can be closed now.

@dotaheor dotaheor closed this as completed Feb 6, 2020
@ianlancetaylor
Copy link
Contributor

As mentioned above, we were keeping this issue open specifically to add support for embedded methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants