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: StructOf doesn't generate wrapper methods for embedded fields #15924

Open
adonovan opened this issue Jun 1, 2016 · 13 comments

Comments

@adonovan
Copy link

commented Jun 1, 2016

% go version
go version devel +bbd1dcd Wed Jun 1 21:32:46 2016 +0000 linux/amd64
% cat ~/a.go
package main

import (
        "bytes"
        "io"
        "reflect"
        "sync"
)

func main() {
        t := reflect.StructOf([]reflect.StructField{
                {Type: reflect.TypeOf(sync.Mutex{}), Anonymous: true},
                {Type: reflect.TypeOf(bytes.Buffer{}), Anonymous: true},
        })
        x := reflect.New(t).Interface()
        _ = x.(io.Writer)
}

% go run ~/a.go
panic: interface conversion: *truct { sync.Mutex; bytes.Buffer } is not io.Writer: missing method Write

I expected that x would have generated wrapper methods for Mutex.Lock, etc.

Interestingly, adding this declaration inside func main (not at package level):

var _ interface{} = new(struct {
      sync.Mutex
      bytes.Buffer
})

causes the program to succeed. Presumably it causes the compiler to generate the correct type information, which reflect.StructOf then finds.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2016

This is a known deficiency, but it should be documented (or we should unexport StructOf until it is fixed).

CC @sbinet

@ianlancetaylor ianlancetaylor added this to the Go1.7 milestone Jun 2, 2016

@sbinet

This comment has been minimized.

Copy link
Member

commented Jun 2, 2016

let me send a CL that documents this temporary (ie: go-1.7) limitation.

@gopherbot

This comment has been minimized.

Copy link

commented Jun 2, 2016

CL https://golang.org/cl/23681 mentions this issue.

mk0x9 pushed a commit to mk0x9/go that referenced this issue Jun 2, 2016
reflect: document StructOf embedded fields limitation
This CL documents that StructOf currently does not generate wrapper
methods for embedded fields.

Updates golang#15924

Change-Id: I932011b1491d68767709559f515f699c04ce70d4
Reviewed-on: https://go-review.googlesource.com/23681
Reviewed-by: David Crawshaw <crawshaw@golang.org>

@ianlancetaylor ianlancetaylor modified the milestones: Go1.8, Go1.7 Jun 2, 2016

@crawshaw

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2016

cc me. I had a prototype of this that I didn't send out in the 1.7 cycle, hopefully I can get to it in 1.8.

@quentinmit quentinmit added the NeedsFix label Oct 10, 2016

@rsc rsc modified the milestones: Go1.8Maybe, Go1.8 Oct 13, 2016

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2016

Too late.

@rsc rsc modified the milestones: Go1.9, Go1.8Maybe Oct 20, 2016

@crawshaw crawshaw modified the milestones: Go1.10, Go1.9 May 5, 2017

@crawshaw

This comment has been minimized.

Copy link
Contributor

commented May 5, 2017

Requires creating methods at run time, which is discussed in #16522.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 28, 2017

Dup with more repros in #20824

@gopherbot

This comment has been minimized.

Copy link

commented Jun 28, 2017

CL https://golang.org/cl/47035 mentions this issue.

gopherbot pushed a commit that referenced this issue Jul 15, 2017
reflect: make StructOf panic for methods that don't work
When StructOf is used with an anonymous field that has methods, and
that anonymous field is not the first field, the methods we generate
are incorrect because they do not offset to the field as required.
If we encounter that case, panic rather than doing the wrong thing.

Fixes #20824
Updates #15924

Change-Id: I3b0901ddbc6d58af5f7e84660b5e3085a431035d
Reviewed-on: https://go-review.googlesource.com/47035
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 29, 2017

@mdempsky

This comment has been minimized.

Copy link
Member

commented Mar 15, 2018

FYI, as of CL 100846, methods are now sorted with exported names before non-exported names, and there's an extra field to track the number of exported methods.

The current code works because it only allows exported methods from one type, so they're already in the same sort order, and we don't need to worry about counting how many exported methods there are. If either of those constraints change, we'll need to change the code (e.g., to sort methods and/or count the exported methods). I've left TODOs to mark where I expect this needs to be done.

Also, one other comment: currently, StructOf only allows promoting methods from the first field, but it could be generalized slightly to allowing promoting methods of any field with offset==0. This would allow methods from multiple zero-width types to be promoted.

@gopherbot gopherbot added this to the Unplanned milestone May 23, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Jun 27, 2018

Change https://golang.org/cl/121316 mentions this issue: reflect: prevent additional StructOf embedded method cases

gopherbot pushed a commit that referenced this issue Jun 27, 2018
reflect: prevent additional StructOf embedded method cases
The current implementation does not generate wrappers for methods of
embedded non-interface types. We can only skip the wrapper if
kindDirectIface of the generated struct type matches kindDirectIface
of the embedded type. Panic if that is not the case.

It would be better to actually generate wrappers, but that can be done
later.

Updates #15924
Fixes #24782

Change-Id: I01f5c76d9a07f44e1b04861bfe9f9916a04e65ca
Reviewed-on: https://go-review.googlesource.com/121316
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@cosmos72

This comment has been minimized.

Copy link

commented Dec 26, 2018

while implementing reflect.NamedOf, I noticed that reflect.StructOf creates wrapper methods for embedded interfaces, and I was surprised that declaring methods at runtime would simply require calling reflect.MakeFunc (see https://github.com/golang/go/blob/master/src/reflect/type.go#L2386).

Alas, it seems not.

The following example crashes with SIGSEGV on both my local Go 1.11.4 installation and on playground https://play.golang.org/p/jny1YKZsHlp

package main

import (
	"fmt"
	"reflect"
)

type MyInt int

func (n MyInt) String() string {
	return fmt.Sprint("MyInt(value = ", int(n), ")")
}

func main() {
	embed()
	embed_reflect()
}

func embed() {
	type S = struct {
		fmt.Stringer
	}
	s := S{
		MyInt(3),
	}
	fmt.Println(s.Stringer.String()) // works, prints 3

	var i fmt.Stringer = s
	fmt.Println(i.String()) // works, prints 3
}

func embed_reflect() {
	tstringer := reflect.TypeOf((*fmt.Stringer)(nil)).Elem()

	t := reflect.StructOf(
		[]reflect.StructField{
			reflect.StructField{
				Name:      tstringer.Name(),
				Type:      tstringer,
				Anonymous: true,
			},
		},
	)
	v := reflect.New(t).Elem()
	v.Field(0).Set(reflect.ValueOf(MyInt(4)))

        i := v.Field(0).Interface().(fmt.Stringer)
	fmt.Println(i.String()) // works, prints 4

	i = v.Interface().(fmt.Stringer)
	fmt.Println(i.String()) // crashes
}

on my local Go 1.11.4, the output is:

MyInt(value = 3)
MyInt(value = 3)
MyInt(value = 4)
unexpected fault address 0xc00000a080
fatal error: fault
[signal SIGSEGV: segmentation violation code=0x2 addr=0xc00000a080 pc=0xc00000a080]

goroutine 1 [running]:
runtime.throw(0x4eebf7, 0x5)
	/usr/local/go/src/runtime/panic.go:608 +0x72 fp=0xc000055e00 sp=0xc000055dd0 pc=0x4288a2
runtime.sigpanic()
	/usr/local/go/src/runtime/signal_unix.go:397 +0x275 fp=0xc000055e50 sp=0xc000055e00 pc=0x43b7f5
main.embed_reflect()
	/home/max/go/src/github.com/cosmos72/gomacro/_example/namedof.go:51 +0x386 fp=0xc000055f88 sp=0xc000055e50 pc=0x4b5486
main.main()
	/home/max/go/src/github.com/cosmos72/gomacro/_example/namedof.go:16 +0x25 fp=0xc000055f98 sp=0xc000055f88 pc=0x4b4f65
runtime.main()
	/usr/local/go/src/runtime/proc.go:201 +0x207 fp=0xc000055fe0 sp=0xc000055f98 pc=0x42a217
runtime.goexit()
	/usr/local/go/src/runtime/asm_amd64.s:1333 +0x1 fp=0xc000055fe8 sp=0xc000055fe0 pc=0x452a31

on playground https://play.golang.org/p/jny1YKZsHlp the output is instead:

MyInt(value = 3)
MyInt(value = 3)
MyInt(value = 4)
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0xffffffff addr=0x0 pc=0x40c0e0]

goroutine 1 [running]:
main.embed_reflect()
	/tmp/sandbox101416977/main.go:51 +0x400
main.main()
	/tmp/sandbox101416977/main.go:16 +0x40

I would expect that either reflect.StructOf fails (due to unsupported interface embedding) or that its returned type does not implement fmt.Stringer - I was surely not expecting that the method call crashes at runtime.

@mihaiav

This comment has been minimized.

Copy link

commented Jan 18, 2019

83092a4#diff-cdbb65f700222dd514ff85e972c88be2R2470 broke my program after I've updated the Go version.

I didn't use any of the the struct methods. The struct was built only to create dynamic data structures matching a specific format when encoded with various encoders such encoding/json.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

@mihaiav I'm sorry that happened but given the choice between "incorrectly claim we did what the program requested" and "give an error" I think it's better to give an error.

You should be able to fix your program by making a regular field rather than an embedded field.

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