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

Jump to bottom

reflect: Value.Method does not do the same nil checks as the language #32021

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

reflect: Value.Method does not do the same nil checks as the language #32021

dotaheor opened this issue May 14, 2019 · 13 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@dotaheor
Copy link

dotaheor commented May 14, 2019 •
edited

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.

Update: similarly for interface dynamic values
@cuonglm
Copy link
Member

cuonglm commented May 14, 2019

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

The program works as expected.

@ianlancetaylor
Copy link
Contributor

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 14, 2019
@ianlancetaylor ianlancetaylor added this to the Go1.14 milestone May 14, 2019
@dotaheor
Copy link
Author

dotaheor commented May 14, 2019 •
edited

@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
Copy link
Author

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
Copy link
Author

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

@dotaheor
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

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

@dotaheor
Copy link
Author

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

@griesemer, how do you think?

@dotaheor
Copy link
Author

dotaheor commented May 14, 2019 •
edited

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
Copy link
Author

dotaheor commented May 14, 2019 •
edited

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 mentioned this issue May 14, 2019
Open
@dotaheor dotaheor changed the title reflect: check for nil pointer when returning promoted value method reflect: inconsistent with direct code May 14, 2019
@ianlancetaylor
Copy link
Contributor

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
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@dotaheor
Copy link
Author

Similarly for interface dynamic values:

package main

import (
	"fmt"
	"reflect"
	"io"
)

func main() {
	var e error = io.EOF
	f := e.Error
	g := reflect.ValueOf(&e).Elem().MethodByName("Error").Interface().(func() string)
	e = io.ErrUnexpectedEOF
	fmt.Println(f()) // EOF
	fmt.Println(g()) // unexpected EOF
}

@dotaheor dotaheor mentioned this issue Jun 16, 2020
Closed
@go101 go101 mentioned this issue Aug 24, 2021
Closed
This was referenced Mar 30, 2022
Open
Open
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Go Compiler / Runtime
Status: Triage Backlog
Milestone
Backlog
Development

No branches or pull requests
5 participants
@rsc @ianlancetaylor @cuonglm @gopherbot @dotaheor