This program failed even before CL 228792:

package main

import "reflect"

type foo struct {}
func (foo) X(x ...int) { println("ok") }

var h = reflect.Type.Method

func main() {
	v := reflect.ValueOf(foo{})
	m := h(v.Type(), 0)
	m.Func.CallSlice([]reflect.Value{v, reflect.ValueOf([]int(nil))})
}

I think as a short-term workaround, we should revert CL 228792, and also add a check for reflect.Value.CallSlice, and add these as regress tests.

Longer term, we should figure out how to make the reflection-use analysis more precise without compromising soundness.

Observation: Changing

var h = reflect.Type.Method

to

var h = (interface { Method(int) reflect.Method }).Method

causes the test cases to pass, even with CL 228792.

It looks like what's happening is the ReflectMethod check doesn't work within package reflect itself. While compiling a package, localpkg.Path will be "", not it's actual package path (e.g., "reflect").

Changing s.Pkg.Path == "reflect" to s.Pkg.Path == "reflect" || s.Pkg == localpkg && myimportpath == "reflect" seems like it might work.

CC @cherrymui

This program failed even before CL 228792:

@mdempsky, thanks! Nice! That was the program I was trying to write that prompted my golang-dev post.

In playground form: https://play.golang.org/p/8L3UVmShhQf (showing that a Call anywhere else makes it work fine in Go 1.14)

Changing s.Pkg.Path == "reflect" to s.Pkg.Path == "reflect" || s.Pkg == localpkg && myimportpath == "reflect" seems like it might work.

@mdempsky How about using Type.LongString():

diff --git a/src/cmd/compile/internal/gc/walk.go b/src/cmd/compile/internal/gc/walk.go
index 06910450ff..7b9dee746a 100644
--- a/src/cmd/compile/internal/gc/walk.go
+++ b/src/cmd/compile/internal/gc/walk.go
@@ -3657,8 +3657,8 @@ func usemethod(n *Node) {
        }

        // Note: Don't rely on res0.Type.String() since its formatting depends on multiple factors
-       //       (including global variables such as numImports - was issue #19028).
-       if s := res0.Type.Sym; s != nil && s.Name == "Method" && s.Pkg != nil && s.Pkg.Path == "reflect" {
+       //       (including global variables such as numImports - was issue #19028)
+       if s := res0.Type.Sym; s != nil && res0.Type.LongString() == "reflect.Method" {
                Curfn.Func.SetReflectMethod(true)
        }
 }

Yeah, actually I was thinking about this the other day. It looks like the compiler only marks direct call to reflect.Type.Method, but not call by function pointers. And reflect.Value.Call doesn't really matter, as you could get a method and do other things with it without actually call it, and the method should be live in this case as well. I'm thinking there could be a different way of doing it.

Change https://golang.org/cl/228879 mentions this issue: cmd/link: handle indrect call of reflect.Type.Method in deadcode pass

@cuonglm I wouldn't think that would fix the test cases. Does it?

@cherrymui I don't think the linker should be involved in checking for interface wrapper methods. There can be many wrappers semantically equivalent to reflect.Type.Method (as a method expression) in different packages, and only the compiler can detect them all correctly.

@mdempsky it does, I have a patch in my local, do you want to take a look?

@mdempsky it does, I have a patch in my local, do you want to take a look?

Ah, sorry, I haven’t tested with the CallSlice case.

@mdempsky it does, I have a patch in my local, do you want to take a look?

Ah, sorry, I haven’t tested with the CallSlice case.

It does pass.

@cuonglm Oh, right, LongString uses package name, not package path.

No, I don't think that's a desirable change. It's not likely to cause problems in practice, but that can lead to false positives. We always want to identify standard library packages by path, not name.

Yeah, I think you're probably right. There could be more method wrappers, like

type I interface { Method(int) reflect.Method }
I.Method

It seems those wrapper functions actually do have REFLECTMETHOD set. Only reflect.Type.Method itself. Maybe we just didn't check the package name correctly, then.

@cherrymui Yeah, that's what the change I mentioned in #38515 (comment) was meant to address. With that change, reflect.Type.Method gets marked REFLECTMETHOD.

Yeah, thanks! (I somehow missed that, sorry.) Do you want to send a CL? Or I can do that.

(With that, the linker change actually makes it work. But it is clearly not the best way to do it.)

@cherrymui No problem, and either way works for me. I probably won't get to my desktop to create a CL until later today. Happy if you beat me to it. :)

Okay, I can do that. Thanks!

Just wanted to mention that, as @rsc pointed out in the email thread, Call or CallSlice is not important here. You could obtain a method and do things with it, without using Call or CallSlice. If the method is not retained by the linker, bad things will happen. E.g. https://play.golang.org/p/rSIx14FBMNs

@cherrymui Thanks for pointing out that example.

Yeah, I don't think the linker should need to check for Call or CallSlice. I only suggested restoring those checks as a short-term workaround.

I think if we just fix the compiler's usemethod check so that reflect.Type.Method is marked REFLECTMETHOD, we should be in a good state. Do you agree with that assessment?

Edit: I do think the linker should / needs to continue checking for reflect.Value.Method.

Yeah, I think so. Thanks.

Change https://golang.org/cl/228880 mentions this issue: cmd/compile: when marking REFLECTMETHOD, check for reflect package itself