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: cannot call *T methods on addressable Values of type T #24184

Open
mdempsky opened this issue Feb 28, 2018 · 5 comments
Open

reflect: cannot call *T methods on addressable Values of type T #24184

mdempsky opened this issue Feb 28, 2018 · 5 comments
Labels
NeedsInvestigation
Milestone

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Feb 28, 2018

I would expect this program to work:

package main

import "reflect"

type T int
func (*T) M() {}

func main() {
        var t T
        v := reflect.ValueOf(&t).Elem()
        v.MethodByName("M").Call(nil)
}

Currently, it produces panic: reflect: call of reflect.Value.Call on zero Value panic, because M is not in the method set of T (only *T).

This seems overly strict to me. The Go spec allows calling t.M() where t is an addressable value of type T; it's just implicitly executed as (&t).M(). I would expect package reflect to handle this implicit dereference, but it does not.

For comparison, the spec also allows an implicit dereference to call value-receiver methods on pointer types, and package reflect does perform this implicit dereference.

/cc @ianlancetaylor @dsnet

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Feb 28, 2018

This appears to be the root cause of #24153:

// If v is a named type and is addressable,
// start with its address, so that if the type has pointer methods,
// we find them.
if v.Kind() != reflect.Ptr && v.Type().Name() != "" && v.CanAddr() {
	v = v.Addr()
}

The code needs to explicitly indirect values to get their full method set, and then needs to undo that operation, which then conflicts with the fix implemented for #22031.

@dsnet
Copy link
Member

@dsnet dsnet commented Feb 28, 2018

I have wanted reflect to perform this implicitly. However, I have a suspicion that some things will break that were depending on the previous behavior.

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Feb 28, 2018

@dsnet Yeah, I'm somewhat nervous about breakage too. @ianlancetaylor argued though that types generally don't have both value and pointer methods, so perhaps in practice this wouldn't actually come up very often.

One way forward would be to prepare a CL that changes package reflect to include the implicit indirection, and see how much (if anything) breaks.

@dsnet
Copy link
Member

@dsnet dsnet commented Feb 28, 2018

If you craft the CL, I can run it through regression testing to measure the damage (if any).

As for https://golang.org/cl/97796, even though fixing this issue would avoid the need for CL/97796, should we still move forward with as it fixes the Go1.10 regression? We certainly won't be able to cherry-pick this issue into the 1.10 branch.

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Feb 28, 2018

@dsnet Yeah, I think CL 97796 is still worth separately pursuing until we make a decision here.

@andybons andybons added this to the Unplanned milestone Mar 13, 2018
@andybons andybons added the NeedsInvestigation label Mar 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation
Projects
None yet
Development

No branches or pull requests

3 participants