ralfbiedert / interoptopus

The polyglot bindings generator for your library (C#, C, Python, …) 🐙
302 stars 27 forks source link

Allowed generic wrapper structs to have callback generics. #97

Closed Earthmark closed 5 months ago

Earthmark commented 5 months ago

This came up when trying to add a contextual pointer to C# callbacks, but it became painful to add the same code to every callback. The type signatures for this kind of callback appeared correct, but the generated code was invalid (it was of the form <wrapper_struct>fn(<arguments>) -> <return> instead of appending the struct names.

This fixes that, the name of the callback name is appended to the wrapper struct automatically. This is still an issue with un-named callbacks, as they will likely still generate the expanded form name.

To support this FnPointer was extended to include an optional name that refers to the delegate.

How this breaks the build

This currently breaks python, I believe due to how python is interpreted. In the example code these are the orders of code sections causing the issue:

"""Block of generated types"""
class SliceMutu8(ctypes.Structure):

class DelegateCallbackMyCallbackContextual(ctypes.Structure):

    # These fields represent the underlying C data layout
    _fields_ = [
        ("callback", callbacks.fn_pconst__u32),
        ("context", ctypes.c_void_p),
    ]

"""Block of generated callback types"""
class callbacks:
    """Helpers to define callbacks."""
    fn_SliceMutu8 = ctypes.CFUNCTYPE(None, SliceMutu8)
    fn_pconst__u32 = ctypes.CFUNCTYPE(None, ctypes.c_void_p, ctypes.c_uint32)

Currently SliceMutu8 and DelegateCallbackMyCallbackContextual appear to be generated in the same section, so they have a 'dependency' between each other.

It seems like there isn't a good approach to reordering the classes, it feels to me like any possible order may result in a circular dependency. I'm not sure how this is solved today with circular dependencies between struct types, but if that same solution can be used here that'd probably be best. Please advise on how this can be resolved.

ralfbiedert commented 5 months ago

I haven't fully thought this through, but shooting from the hip it appears handling how _fields_ in the structs are generated needs to be changed. Specifically this here writes the fields as references to callbacks:

https://github.com/ralfbiedert/interoptopus/blob/e0c1a9f3adaa2392f940e06e52211495f786e632/backends/cpython/src/writer.rs#L124

which in turn ends up at this place that hardcodes callbacks:

https://github.com/ralfbiedert/interoptopus/blob/e0c1a9f3adaa2392f940e06e52211495f786e632/backends/cpython/src/converter.rs#L111

One or both of these places might have to be reworked, so that struct definitions inline them instead, turning

    _fields_ = [
        ("callback", callbacks.fn_pconst__u32),
        ("context", ctypes.c_void_p),
    ]

into

    _fields_ = [
        ("callback", ctypes.CFUNCTYPE(None, ctypes.c_void_p, ctypes.c_uint32)),
        ("context", ctypes.c_void_p),
    ]
Earthmark commented 5 months ago

Would that change be breaking? I normally have experience from the c# side, where that would be a different delegate type and would require a constructor.

Looking at how the field is assigned, it does look like it would be ok, but I'd just like to verify that.

I guess that's the point of the ci, if the changes to the field require any other changes to test files then a different approach is needed.

ralfbiedert commented 5 months ago

Would that change be breaking?

TBH I'm not entirely sure myself, but my guess is 'no' as long as callbacks are still preserved as well. I think the ctypes annotation is more 'structural', in the sense that it would still accept a Python value even if its type annotation refers to a different ctypes instance.

ralfbiedert commented 5 months ago

Thanks!