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

spec: document nil checks for value methods #32035

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

spec: document nil checks for value methods #32035

dotaheor opened this issue May 14, 2019 · 9 comments
Assignees
Milestone

Comments

@dotaheor
Copy link

@dotaheor dotaheor 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"
)


type S struct {}

func (S) M(n int) {
	fmt.Println(n)
}

type T struct {
	*S
}

type I interface {
	M(n int)
}


func foo() {
	var v = &T{}
	var i I = v
	f := i.M // ok
	v.S = &S{}
	f(111) // 111
}

func bar() {
	var v = &T{}
	f := v.M // panic
	v.S = &S{}
	f(111)
}

func main() {
	foo()
	bar()
}

What did you expect to see?

Same behavior

What did you see instead?

Different behaviors.

@dotaheor
Copy link
Author

@dotaheor dotaheor commented May 14, 2019

This is another bug I found in playing with this one #32021, which is closed. But I think there is also an inconsistency between reflection and direct code.

@dotaheor

This comment has been hidden.

@dotaheor

This comment has been hidden.

@FiloSottile FiloSottile added this to the Unplanned milestone May 14, 2019
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented May 14, 2019

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 14, 2019

In the case f := v.M we check for a nil pointer at the point where we create the method value. We do this because checking for the nil pointer when the method value is invoked leads to a confusing backtrace. In the case f := i.M we check that i is not nil, but we do not check that the value itself contains a nil pointer.

So this is working as expected, in order to avoid a nil dereference in a generated wrapper function, but, you're right, it's not completely consistent.

@ianlancetaylor ianlancetaylor changed the title runtime: inconsistency between evaluating interface method and promoted method cmd/compile: inconsistency between evaluating interface method and promoted method May 14, 2019
@beoran
Copy link

@beoran beoran commented May 15, 2019

I would say this inconsistency needs to be documented better since it is surprising people who use Go. Please make this a documentation issue.

@dotaheor
Copy link
Author

@dotaheor dotaheor commented May 15, 2019

In my opinion, the inconsistency in #32021 is more serious than the one is the current issue. I think, if it is possible, reflection code implementation and direct code implementation should be consistent with each other.

Personally, I prefer the current reflection implementation way.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 15, 2019

If we document something, it would be a change to the language spec saying that you can not create a method value of a value method of a nil pointer, and you can not create a method value of a promoted method of an embedded pointer if the pointer is nil. To put it another way, all nil checks are done when the method value is created, not when it is invoked.

For interface types, you can not create a value method if the interface value is nil. However, we do not check anything about the value stored in the interface value, and I'm not sure that we can.

@ianlancetaylor ianlancetaylor changed the title cmd/compile: inconsistency between evaluating interface method and promoted method spec: document nil checks for value methods May 15, 2019
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.