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 · 9 comments

Comments

@dotaheor
Copy link

commented May 15, 2018

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

This comment has been minimized.

Copy link

commented May 29, 2018

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

@gopherbot gopherbot closed this in 377567e May 29, 2018

@dotaheor

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link

commented Jan 3, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

Reopening to permit this case.

@purpleidea

This comment has been minimized.

Copy link

commented Jan 4, 2019

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

This comment has been minimized.

Copy link

commented Aug 24, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2019

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

This comment has been minimized.

Copy link

commented Aug 25, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.