Allow passing Python functions to Go for invocation.#189
Allow passing Python functions to Go for invocation.#189nairb774 wants to merge 1 commit intogoogle:masterfrom
Conversation
trotterdylan
left a comment
trotterdylan
left a comment
There was a problem hiding this comment.
This is really cool. I like the general approach. Just a few suggestions.
runtime/native.go
Outdated
| continue | ||
|
|
||
| case reflect.Func: | ||
| if fn, ok := val.Interface().(func(f *Frame, args Args, kwargs KWArgs) (*Object, *BaseException)); ok { |
There was a problem hiding this comment.
You could store reflect.TypeOf(func(*Frame, Args, KWArgs) (*Object, *BaseException) {}) and compare directly before doing val.Interface(). I have a sense that'd be more efficient if this is the uncommon case.
There was a problem hiding this comment.
The common case should be that when value.Kind()==Func this interface assertion should pass (at least today). If other built in types were returning other func like values from their Native slot, then this might need to be adjusted.
runtime/native.go
Outdated
| } | ||
|
|
||
| func nativeToPyFuncBridge(fn Func, target reflect.Type) reflect.Value { | ||
| firstInIsFrame := target.NumIn() > 0 && target.In(0) == reflect.TypeOf((*Frame)(nil)) |
There was a problem hiding this comment.
Might want to cache reflect.TypeOf((*Frame)(nil)) as a global somewhere.
runtime/native.go
Outdated
|
|
||
| case reflect.Func: | ||
| if fn, ok := val.Interface().(func(f *Frame, args Args, kwargs KWArgs) (*Object, *BaseException)); ok { | ||
| val = nativeToPyFuncBridge(Func(fn), expectedRType) |
There was a problem hiding this comment.
I think Func(..) is redundant.
There was a problem hiding this comment.
Kind of, the function type of nativeToPyFuncBridge was wrong. This was left from a bit of debugging/re-writing I did earlier.
runtime/native.go
Outdated
| var raised *BaseException | ||
| pyArgs[i], raised = WrapNative(frame, arg) | ||
| if raised != nil { | ||
| panic(fmt.Sprintf("WrapNative(%v)=%v", arg, raised)) // TODO: Figure this out... |
There was a problem hiding this comment.
I don't have any great suggestions here. Given that we can't assume anything about the nature or the context of the call, panic is probably the only general option available to us. In the specific case where the last return value is *BaseException, we could return the raised exception, but that's going to have limited applicability.
The value panicked could just be the raised exception, that way at least there's some opportunity to catch it and recover.
runtime/native.go
Outdated
| return []reflect.Value{v} | ||
| } | ||
|
|
||
| // TODO: Support other iterables? |
There was a problem hiding this comment.
This should be pretty easy to accomplish with seqForEach() or similar.
nairb774
force-pushed
the
nativecallback
branch
from
f87ed68 to
627e648
Compare
|
This still needs some tests. I'll start working on those now that this looks like it is in a better place, but feel free to review the code in the implementation. |
nairb774
force-pushed
the
nativecallback
branch
from
627e648 to
496bd31
Compare
trotterdylan
left a comment
trotterdylan
left a comment
There was a problem hiding this comment.
Just one small suggestion in addition to your plan to add tests.
Also, something occurred to me: if *BaseException implemented the error interface, then we could also return an exception when the target function returns an error as its last result. I don't think we should do this now because it could definitely be surprising. But it's nice to have in our back pockets if it turns out to be useful and it feels natural.
runtime/native.go
Outdated
| } | ||
| } | ||
|
|
||
| var baseExceptionReflectType = reflect.TypeOf((*BaseException)(nil)) |
There was a problem hiding this comment.
Group these globals at the top of the file please.
nairb774
force-pushed
the
nativecallback
branch
from
496bd31 to
dc044ec
Compare
nairb774
force-pushed
the
nativecallback
branch
from
dc044ec to
c81a76b
Compare
|
PTAL. |
trotterdylan
left a comment
trotterdylan
left a comment
There was a problem hiding this comment.
Thanks for working on this, it's looking really good. Just one high level question/thought and one minor nit.
| continue | ||
|
|
||
| case reflect.Func: | ||
| if fn, ok := val.Interface().(func(*Frame, Args, KWArgs) (*Object, *BaseException)); ok { |
There was a problem hiding this comment.
This strikes me as not quite right because it only (currently) really works for function objects which return their Call method (IIUC). I wonder if we should be checking whether expectedRType.Kind() == reflect.Func upfront and if so, checking whether o.__call__ exists. If so then produce a bridge for it, otherwise, fall back to ToNative.
| } | ||
| } | ||
|
|
||
| func TestNativveToPyFuncBridge(t *testing.T) { |
There was a problem hiding this comment.
s/TestNativve/TestNative/
2 participants
Big WIP - This code is gnarly and rather than continuing to sit on it, thoughts on how to handle all the nasty
panicsbelow? There has to be better ways. Regardless, this opens up a lot more interoperability with the Go ecosystem - just a matter of how to handle error cases.