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: Value.Method does not do the same nil checks as the language #32021

Open
dotaheor opened this issue May 14, 2019 · 12 comments

Comments

@dotaheor
Copy link

commented May 14, 2019

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

go version go1.12 linux/amd64

What did you do?

package main

import (
	"fmt"
	"reflect"
)

type S struct {
	x int
}
func (S) M(int) {}


type T struct {
	*S
}

func main() {
	var v = T{}
	
	t := reflect.TypeOf(v)
	fmt.Println(t, "has", t.NumMethod(), "methods:")
	for i := 0; i < t.NumMethod(); i++ {
		fmt.Print("  method#", i, ": ", t.Method(i).Type, "\n")
	}
	fmt.Println(reflect.ValueOf(v).Method(0)) // ok
	_ = v.M // panic
}

What did you expect to see?

Run okay.

What did you see instead?

panic.

@cuonglm

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

In var v = T{}, the *S embedded in T is nil, causing panic.

The program works as expected.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

Yes, this is a value method, but there is no value. When using Method(0), you will get a panic if you try to call the method. If anything we should change reflect.Value.Method to check for this case and panic then, rather than returning a reflect.Value that can not be used.

@ianlancetaylor ianlancetaylor changed the title runtime: panic when accessing promoted methods reflect: check for nil pointer when returning promoted value method May 14, 2019

@ianlancetaylor ianlancetaylor added this to the Go1.14 milestone May 14, 2019

@dotaheor

This comment has been minimized.

Copy link
Author

commented May 14, 2019

@cuonglm there is an inconsistency between refection and direct code.

@ianlancetaylor I think it in another way. The program prints

main.T has 1 methods:
  method#0: func(main.T, int)
...

which indicates type T has type method with the first parameter type as T.
It looks compilers declare an implicit method for both type T and *T:

func (t T) M(int) {
    t.S.M()
}

func (t *T) M(int) {
    (*t).M()
}

So I think the gc compiler does some unnecessary thing when evaluating v.M.

@dotaheor

This comment has been minimized.

Copy link
Author

commented May 14, 2019

It looks gccgo implements it correctly. The following program prints two same values if it is compiled with gc, but two different values if it is compiled with gccgo.

package main

import (
	"fmt"
)

type S struct {
	x int
}
func (S) M(int) {}


type T struct {
	*S
}

func main() {
	var v = T{&S{}}
	fmt.Println(v.M)
	fmt.Println(v.S.M)
}
@dotaheor

This comment has been minimized.

Copy link
Author

commented May 14, 2019

But gccgo also makes the program in the first comment panic.

@dotaheor

This comment has been minimized.

Copy link
Author

commented May 14, 2019

A short one which indicates T and *T both has a M method.

package main

type S struct {
	x int
}
func (S) M(int) {}


type T struct {
	*S
}

func main() {
	var v = T{}

	_ = T.M    // ok
	_ = (*T).M // ok
	_ = v.M    // panic
}
@dotaheor

This comment has been minimized.

Copy link
Author

commented May 14, 2019

An example which proves the current reflection implementation is correct.

package main

import (
	"fmt"
	"reflect"
)

type S struct {
	x int
}
func (s S) M(n int) {
	fmt.Println(s.x, n)
}


type T struct {
	*S
}

func main() {
	var v = &T{}
	
	f := reflect.ValueOf(v).Elem().Method(0)
	v.S = &S{x: 123}
	f.Call([]reflect.Value{reflect.ValueOf(789)})
}
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

Thanks for that final example. I'm willing to believe that there is nothing to do here.

@dotaheor

This comment has been minimized.

Copy link
Author

commented May 14, 2019

@ianlancetaylor, so do you think type T and its values haven't a M method?

@griesemer, how do you think?

@dotaheor

This comment has been minimized.

Copy link
Author

commented May 14, 2019

I think it is no doubt that there is an inconsistency between refection and direct code, whether or not type T and its values haven't a M method.

package main

import (
	"fmt"
	"reflect"
)

type S struct {
	x int
}
func (s S) M(n int) {
	fmt.Println(s.x, n)
}


type T struct {
	*S
}

func main() {
	{
		var v = &T{}
		
		f := reflect.ValueOf(v).Elem().Method(0)
		v.S = &S{x: 123}
		f.Call([]reflect.Value{reflect.ValueOf(789)}) // 123 789
	}
	
	// If the above reflection code works,
	// the following direct one should also.
	// However, it is not.

	{
		var v = &T{}
		f := v.M // panic
		v.S = &S{x: 123}
		f(789)
	}
}
@dotaheor

This comment has been minimized.

Copy link
Author

commented May 14, 2019

I think, whether or not type T and its values haven't a M method, the outputs of the reflection code and direct code should be always consistent. But they are not.

package main

import (
	"fmt"
	"reflect"
)

type S struct {
	x int
}
func (s S) M(n int) {
	fmt.Println(s.x, n)
}


type T struct {
	*S
}

func main() {
	{
		var v = &T{&S{123}}
		f := v.M
		f(789) // 123 789
		v.S = nil
		f(789) // 123 789
	}
	
	{
		var v = &T{&S{123}}
		f := reflect.ValueOf(v).Elem().Method(0)
		f.Call([]reflect.Value{reflect.ValueOf(789)}) // 123 789
		v.S = nil
		f.Call([]reflect.Value{reflect.ValueOf(789)}) // panic
	}
}

@dotaheor dotaheor changed the title reflect: check for nil pointer when returning promoted value method reflect: inconsistent with direct code May 14, 2019

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

Reopening issue.

@ianlancetaylor ianlancetaylor changed the title reflect: inconsistent with direct code reflect: `Method` does not do the same nil checks as the language May 15, 2019

@bradfitz bradfitz changed the title reflect: `Method` does not do the same nil checks as the language reflect: Value.Method does not do the same nil checks as the language May 15, 2019

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