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: no way to tell whether a method is value method or not #20995

Closed
reusee opened this issue Jul 13, 2017 · 8 comments
Closed

reflect: no way to tell whether a method is value method or not #20995

reusee opened this issue Jul 13, 2017 · 8 comments

Comments

@reusee
Copy link

reusee commented Jul 13, 2017

What version of Go are you using (go version)?

go version devel +75f1de8329 Wed Jul 12 23:39:39 2017 +0000 linux/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/reus/go"
GORACE=""
GOROOT="/home/reus/gotip"
GOTOOLDIR="/home/reus/gotip/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build623176799=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

package main

import (
	crand "crypto/rand"
	"encoding/binary"
	"math/rand"
	"reflect"
)

type Foo struct{}

func (f Foo) Check() {}

type Bar struct{}

func (b *Bar) Check() {}

func main() {
	var foo *Foo
	var bar *Bar

	var v reflect.Value
	if rand.Intn(2) == 0 {
		v = reflect.ValueOf(foo)
	} else {
		v = reflect.ValueOf(bar)
	}

	method := v.MethodByName("Check")
	// want to skip Call if v is nil pointer and method is value method
        if !(methodIsValueMethod && vIsNilPointer) {
	        method.Call([]reflect.Value{})
        }

}

func init() {
	var seed int64
	binary.Read(crand.Reader, binary.LittleEndian, &seed)
	rand.Seed(seed)
}

What did you expect to see?

a proper way to skip Call to avoid panic

What did you see instead?

panic randomly

panic: value method main.Foo.Check called using nil *Foo pointer

@dsnet
Copy link
Member

dsnet commented Jul 13, 2017

reflect matches exactly what happens when you write this naturally:

type Foo struct{}
func (f Foo) Check() {}

func main() {
	var foo *Foo
	foo.Check()
}

This panics:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x467f52]

Thus, I would expect reflect to also panic.

@reusee
Copy link
Author

reusee commented Jul 13, 2017

@dsnet I know it will panic but what I want to do is get the original receiver type of the method to determine whether to invoke Call.
The panic says it's "value method", so I wonder whether this information could be exposed to reflection.

@dsnet
Copy link
Member

dsnet commented Jul 13, 2017

Here's an example of how to check for the Check method on a value receiver.

type Foo struct{}

func (f Foo) Check() {}

func main() {
	var foo *Foo
	v := reflect.ValueOf(foo)

	method, _ := v.Type().Elem().MethodByName("Check")
	fmt.Println(method.Type)
}

If you try to do the same on Bar, it will not find the Check method on the value receiver.

@dsnet
Copy link
Member

dsnet commented Jul 13, 2017

I'm going to mark this as resolved. The API in reflect allows you to do what you're asking for. It's not the nicest, but the reflect API is already pretty bloated that I don't think we would add new API surface just for this use-case.

@dsnet dsnet closed this as completed Jul 13, 2017
@reusee
Copy link
Author

reusee commented Jul 14, 2017

@dsnet
A more realistic demo:

package main

import (
	"context"
	"fmt"
	"reflect"
)

type State string

func (s State) Check() error {
	if s != "on" && s != "off" {
		return fmt.Errorf("bad state")
	}
	fmt.Printf("state ok\n")
	return nil
}

func check(ctx context.Context, o interface{}) error {
	v := reflect.ValueOf(o)
	t := v.Type()
	if t.Kind() != reflect.Struct {
		panic("should be struct")
	}
	for i, l := 0, v.NumField(); i < l; i++ {
		field := v.Field(i)
		fieldType := field.Type()

		method := field.MethodByName("Check")
		if !method.IsValid() {
			continue
		}

		methodIsValueMethod := false // TODO how?
		if fieldType.Kind() == reflect.Ptr && field.IsNil() && methodIsValueMethod {
			continue
		}

		switch method := method.Interface().(type) {
		case func() error:
			if err := method(); err != nil {
				return err
			}
		case func(ctx context.Context) error:
			if err := method(ctx); err != nil {
				return err
			}
		}

	}
	return nil
}

func main() {
	ctx := context.Background()

	// ok, call Check on State field
	check(ctx, struct {
		State State
	}{
		State: "on",
	})

	// ok, call Check on State field
	on := State("on")
	check(ctx, struct {
		State *State
	}{
		State: &on,
	})

	// panic, should avoid calling Check
	check(ctx, struct {
		State *State
	}{
		State: nil,
	})
}

Your approach does not help in above situation, MethodByName will not return nil for fieldType or fieldType.Elem().

I can workaround this by wrap method calling in defer/recover and check panic message against "value method ... called using nil ... pointer", but it's obviously too hacky.

@dsnet
Copy link
Member

dsnet commented Jul 14, 2017

You can replace:

methodIsValueMethod := false // TODO how?
if fieldType.Kind() == reflect.Ptr && field.IsNil() && methodIsValueMethod {
	continue
}

with:

if fieldType.Kind() == reflect.Ptr && field.IsNil() {
	// Method defined on a value receiver.
	if _, ok := fieldType.Elem().MethodByName("Check"); ok {
		continue // Ignore since value can't be retrieved from nil pointer
	}
}

@reusee
Copy link
Author

reusee commented Jul 14, 2017

@dsnet got it. tricky but works.

@dsnet
Copy link
Member

dsnet commented Jul 14, 2017

welcome to reflect ;)

@golang golang locked and limited conversation to collaborators Jul 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants