Allow passing Python functions to Go for invocation. #189
Conversation
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
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. |
| 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.
| @@ -422,6 +422,122 @@ func TestMaybeConvertValue(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestNativveToPyFuncBridge(t *testing.T) { | |||
There was a problem hiding this comment.
s/TestNativve/TestNative/
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.