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

proposal: reflect: add Value.CanCall method #39545

Closed
sgrodriguez opened this issue Jun 12, 2020 · 12 comments
Closed

proposal: reflect: add Value.CanCall method #39545

sgrodriguez opened this issue Jun 12, 2020 · 12 comments

Comments

@sgrodriguez
Copy link

Introduction

If you want to call a method with a slice of Value's as input of the method the inner function of call runs diferents checks if one of them fails ie reflect: Call with too few input arguments it panic.

Why not have a method like:

func (v Value) CanCall(in []Value) bool {
  
}

So we avoid panic as we have with CanInteface, CanAddr.

This is the actual checks that call make:

func (v Value) call(op string, in []Value) []Value {
	// Get function pointer, type.
	t := (*funcType)(unsafe.Pointer(v.typ))
	var (
		fn       unsafe.Pointer
		rcvr     Value
		rcvrtype *rtype
	)
	if v.flag&flagMethod != 0 {
		rcvr = v
		rcvrtype, t, fn = methodReceiver(op, v, int(v.flag)>>flagMethodShift)
	} else if v.flag&flagIndir != 0 {
		fn = *(*unsafe.Pointer)(v.ptr)
	} else {
		fn = v.ptr
	}

	if fn == nil {
		panic("reflect.Value.Call: call of nil function")
	}

	isSlice := op == "CallSlice"
	n := t.NumIn()
	if isSlice {
		if !t.IsVariadic() {
			panic("reflect: CallSlice of non-variadic function")
		}
		if len(in) < n {
			panic("reflect: CallSlice with too few input arguments")
		}
		if len(in) > n {
			panic("reflect: CallSlice with too many input arguments")
		}
	} else {
		if t.IsVariadic() {
			n--
		}
		if len(in) < n {
			panic("reflect: Call with too few input arguments")
		}
		if !t.IsVariadic() && len(in) > n {
			panic("reflect: Call with too many input arguments")
		}
	}
	for _, x := range in {
		if x.Kind() == Invalid {
			panic("reflect: " + op + " using zero Value argument")
		}
	}
	for i := 0; i < n; i++ {
		if xt, targ := in[i].Type(), t.In(i); !xt.AssignableTo(targ) {
			panic("reflect: " + op + " using " + xt.String() + " as type " + targ.String())
		}
	}

IMHO will refactor some checks in order to reuse in CanCall.

@ianlancetaylor ianlancetaylor changed the title reflection: add value method to check if its callable proposal: reflect: add Value.CanCall method Jun 12, 2020
@gopherbot gopherbot added this to the Proposal milestone Jun 12, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jun 12, 2020
@ianlancetaylor
Copy link
Contributor

When does this come up in practice?

@sgrodriguez
Copy link
Author

sgrodriguez commented Jun 12, 2020

@ianlancetaylor I was working in something like this:

...
method := reflect.ValueOf(str).MethodByName(methodName)
	if !method.IsValid() {
		return nil, errors.New("structMethod Invalid Fn Arg")
	}
	res := method.Call(inputs)
...

Calling a struct method by his name in order to do some generic stuff for a particular problem. The way to avoid the panic is to code all the checks again and I dont think its the fn responsibility.

@dsnet
Copy link
Member

dsnet commented Jun 12, 2020

The implementation of cmp could probably benefit from something like this. It's not complicated code, but we have a separate helper package to identify whether a function signature matches what we expect. The alternative of manually checking the signatures is not complicated. However, if variadic arguments are involved, getting the signature check correct is typically difficult for those not accustomed to using Go reflection.

@sgrodriguez
Copy link
Author

@dsnet That is what I am trying to avoid, because everyone who wants to implement a secure call is going to have to add these validations, which do not have to do with the purpose of what is being programmed or import an external package that does it.

@rsc
Copy link
Contributor

rsc commented Jun 17, 2020

Is there benefit from being able to ask CanCall without actually doing the Call?
If doing the call itself is OK, the code could recover from the panic, which would avoid type-checking twice in the (presumably common) case where there's no mismatch.

Also, Call panics with a much more useful message than the bool that CanCall as proposed would return. It could be CheckCall returning an error, of course, but it's still duplicate work if you're just going to Call next anyway.

What's the context here where CanCall without Call is an important operation?

@rsc rsc moved this from Incoming to Active in Proposals (old) Jun 17, 2020
@rsc
Copy link
Contributor

rsc commented Jun 24, 2020

@sgrodriguez, did you see my comment from last week? We are having trouble understanding a context where CanCall is not immediately followed by Call. If Call will happen anyway, it is safer and cheaper to have one function and recover the panic.

@sgrodriguez
Copy link
Author

@rsc Sorry for not answering, the pandemic is hitting hard here (100 days of mandatory quarantine and counting) and I was off for a few days.

Let me explain a little bit the context of this issue. I'm currently developing a library that needs something like this. It has a function that receives an struct and some variables. This function needs to execute a method exported by the struct received with the variables as parameters. This is implemented using reflection.

When calling the method it could panic, so I thought two ways of dealing with it.

  • Check wether it's possible to do the call, by doing the same checks that the reflect Call method implements.
  • Recover from the panic.

I think that the first option is not completly correct because it couples the entire solution with reflect implementation. So whenever the implementation of Call change, my library would break.
And the second option is not to friendly, I understand that the use of recovers is not so common and I only see in testing most of the time.

@ianlancetaylor
Copy link
Contributor

Recovering from a panic is well defined. If you are always going to use Value.Call, and recovering the panic will work just as well as adding new API, then I think that seems preferable to adding new API that will almost never be used.

@rsc
Copy link
Contributor

rsc commented Jul 8, 2020

Just to echo what Ian said, recovering from defined panics is a fine thing to do.

Based on the discussion above, this seems like a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Jul 8, 2020
@sgrodriguez
Copy link
Author

sgrodriguez commented Jul 13, 2020

I agree. if the recover is well define makes no sense to change reflect call.

@rsc
Copy link
Contributor

rsc commented Jul 15, 2020

No change in consensus, so declined.

@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Jul 15, 2020
@rsc rsc closed this as completed Oct 29, 2020
@rsc
Copy link
Contributor

rsc commented Oct 29, 2020

(Forgot to close this back in July, sorry.)

@golang golang locked and limited conversation to collaborators Oct 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

5 participants