reflect: bad test cases in TestCallPanic?

[mdempsky]

In TestCallPanic, there are these two related test cases that are expected to execute without panicking:

type t0 interface { W() }
type T struct { t0 }

x := T{...}
ValueOf(x).Field(0).Method(0).Call(nil)        // 1
ValueOf(x).Field(0).Elem().Method(0).Call(nil) // 2

The first case makes sense to me: it's calling the method W that has been promoted to T via embedding t0.

However, the second case seems iffy to me: after the Elem() call, we're accessing the dynamic value and Method is indexing its complete method set, so we're able to call methods that wouldn't be accessible otherwise.

For example, this program currently runs ok, though I think it should fail: https://play.golang.org/p/QHa7dTEIzJ

Related to #22031. Easily fixable at the same time, assuming others agree that this is an issue.

[gopherbot]

Change https://golang.org/cl/66331 mentions this issue: reflect: fix mutability of non-exported embedded fields

[ianlancetaylor]

I think we've worked our way into a confusing and unfortunate state. reflect.Type.Method indexes into exported methods. reflect.Value.Method seems to index into all methods.

CC @crawshaw

[mdempsky]

If that's the case, that's confusing and unfortunate indeed, but I believe to be a separate issue than what I'm reporting here.

[crawshaw]

Looking at your play example, I think that program should run. The method F is exported. If you return the type it is declared on as an interface{}, you can, without reflect, cast that to an interface{ F() } and call the method.

The same logic follows for your inline example. The type t0 could be returned as an interface{}, and via a new interface{ W() } declared in the calling package, we can access W.

I need to think harder about Ian's comment, which looks unrelated. Without digging, I think neither reflect.Type.Method nor reflect.Value.Method should enumerate unexported methods.

[mdempsky]

@crawshaw Without reflection or package-local access, you can't access the embedded field w to assert its value to interface { F() }.

[crawshaw]

Got it. But I still don't follow why your play example should fail. If I have RO access to w, why can't I read it, assert to interface { F() } and call F?

[mdempsky]

The reflect API appears to consider calls to be write operations. I suppose because they can't be proven safe (e.g., maybe it's an internal helper function that isn't safe for arbitrary user calls).

Here's a corresponding example except without embedding that also panics because Call was used on a value obtained via unexported fields: https://play.golang.org/p/zUbHj0-_YF

[crawshaw]

Oh I see, I mistakenly thought we were calling the method on a copy of the value in the unexported field. Yes, I agree with your change.

[mdempsky]

@ianlancetaylor Following up on your earlier comment, was there other evidence that Type.Method and Value.Method are behaving inconsistently? (I just took a quick look at the code, and I don't think that to be the case.)

[ianlancetaylor]

When I run the following program, printing Type.Method(0) shows ΦExported , but calling Value.Method(0) calls the unexported method. It's weird that the indexes would be different for types and values.

package main

import (
	"fmt"
	"reflect"
)

type UnExportedFirst int

func (i UnExportedFirst) ΦExported()  {}
func (i UnExportedFirst) unexported() { panic("unexported") }

func main() {
	v := reflect.ValueOf(UnExportedFirst(0))
	fmt.Println(v.Type().Method(0))
	v.Method(0).Call(nil)
}