tizoc / ocaml-interop

OCaml<->Rust FFI with an emphasis on safety.
MIT License
134 stars 21 forks source link

Add an OCaml<DynBox> type for passing Rust values through OCaml #24

Closed g2p closed 3 years ago

g2p commented 3 years ago

There's also a few commits that help with testing:

g2p commented 3 years ago

Note: I broke down the commits like this so I could share some parts with other PRs (like the one for bigarrays).

That said, while it's fairly granular for reviewing, I can also rebase it to make a big PR if you don't want merge conflicts (mostly imports and tests).

tizoc commented 3 years ago

@g2p thank you! I can't go through this one today, but on the surface it looks good. I will do a more thorough review once I'm free from other stuff

g2p commented 3 years ago

@tizoc Hello! I hope you can find time for reviewing these

tizoc commented 3 years ago

@g2p yes, and sorry! I have not forgotten, the main reason I haven't yet is that I'm waiting to integrate BoxRoots first, which will impact everything in ocaml-interop (and anything new that comes). I planned on doing that weeks ago but some urgent things got in the way, but will be taking care of it very soon.

tizoc commented 3 years ago

One thing that I would change here is the OCamlBox name, because BoxRoot already exists, and could be confusing.

The other thing I was thinking: how far is this from a full custom blocks implementation? Thinking of https://github.com/zshipko/ocaml-rs/blob/master/src/custom.rs

I think it has a good API. Would it make more sense to have that ported?

g2p commented 3 years ago

I've rebased this (and the rest) after the introduction of BoxRoots.

Re the name: yeah, it was meant to reference Rust's Box as a generic way of moving Rust data into allocated memory. Not sure what to use instead. OCaml<DynBox<T>> would also work and be fairly accurate, does that still risk confusion? OCaml<RustBox<T>> maybe, to contrast with BoxRoots containing OCaml data? OCaml<GenWrapper<T>>?

Re generic custom blocks: the rest of the vtable is for serialisation and generic comparisons. Mapping the whole generic functionality of the C API is meaningful for ocaml-sys, this OCamlBox (or whatever the new name is) is just one kind of custom block with one vtable function, but one that works well for wrapping arbitrary data.

I could see it being specialized to skip the dyn on Rust's side, and maybe put the inner T data directly in the OCaml block (assuming Unpin and proper alignment), but I don't see something as generic as the C API as adding value over the existing low-level abstraction.

tizoc commented 3 years ago

OCaml<DynBox<T>> would also work and be fairly accurate, does that still risk confusion?

No, I think that one is quite good (DynBox, RustValue, etc, not sure what is best, but I like the overall concept).

I'm actually doing something very similar already in code that uses custom blocks and ocaml-interop, just have not made it part of ocaml-interop because I am not entirely sure about what the API should look like and the code I have now is very ugly and written in haste (you can see it here).

Re generic custom blocks: the rest of the vtable is for serialisation and generic comparisons. Mapping the whole generic functionality of the C API is meaningful for ocaml-sys, this OCamlBox (or whatever the new name is) is just one kind of custom block with one vtable function, but one that works well for wrapping arbitrary data.

Well, for now none of the other things are used, but I don't think support for that is useless. Doesn't mean that it has to be done now, but it would be good to keep it in mind and leave space for implementing it in the future.

g2p commented 3 years ago

Well, for now none of the other things are used, but I don't think support for that is useless. Doesn't mean that it has to be done now, but it would be good to keep it in mind and leave space for implementing it in the future.

Okay, agreed. For example, a macro that would register user-defined custom types by TypeId so that we could add more functionality if necessary, and cast back safely.

It doesn't necessarily need to be in this PR, assuming OCaml<DynBox> is sufficiently useful on its own.

g2p commented 3 years ago

(Pushed again with the new name)

tizoc commented 3 years ago

Closing this one, will be merged in #31.