ocaml-interop integration #48

zshipko · 2020-12-18T23:30:47Z

zshipko · 2021-02-04T16:35:08Z

@tizoc I'm back to looking at this, it seems pretty good right now, so I hope to release this branch as long as you don't see any issues with it.

tizoc · 2021-02-04T17:13:07Z

@zshipko ok, I will take a look at it soon. Right now I am playing with an experimental fast global-roots implementation, if it works well it could replace the need for local roots which would help us simplify the API quite a bit.

tizoc · 2021-02-05T19:41:35Z

derive/src/derive.rs

@@ -53,7 +53,7 @@ fn variant_attrs(attrs: &[syn::Attribute]) -> Attrs {
        })
 }

-pub fn tovalue_derive(mut s: synstructure::Structure) -> proc_macro::TokenStream {
+pub fn intovalue_derive(mut s: synstructure::Structure) -> proc_macro::TokenStream {


@zshipko question, why did you go from to -> into? (I'm curious about this because I went the opposite direction in ocaml-interop, by making into_rust -> to_rust because I think that in rust the into assumes that self is consumed.

Ah this is something I probably want to change back. In this case the trait is actually consuming self, but I don't think that is really the best idea here.

Yes, the thing is that Value is Copy right? I think I had it like that for the same reason, but since the original value is still available I decided to switch to the to_ prefix. I am not really sure about what the convention is in cases like this, but my reasoning is that (in addition to what I said on the previous sentence) since Value is more like a reference than the actual value to_ made more sense.

Now I remember that IntoValue consumes self to make Custom values better integrated. It seems like consuming self is actually kind of nice because it prevents the value from being accessed after being converted to OCaml.

I will need to think about the various trade-offs a little more...

Yes, the thing is that Value is Copy right? I think I had it like that for the same reason, but since the original value is still available I decided to switch to the to_ prefix. I am not really sure about what the convention is in cases like this, but my reasoning is that (in addition to what I said on the previous sentence) since Value is more like a reference than the actual value to_ made more sense.

The main impetus behind the name change was that clippy was complaining about it being named To* and not taking a reference. But I think your explanation makes more sense.

tizoc · 2021-02-05T20:31:50Z

src/conv.rs

-        unsafe impl ToValue for $t {
-            fn to_value(self) -> $crate::Value {
-                $crate::Value::int(self as crate::Int)
+        unsafe impl IntoValue for $t {


One mismatch between ocaml-interop and ocaml-rs is:

FromOCaml<OCamlT> for RustT vs FromValue for RustT

ToOCaml<OCamlT> for RustT vs ToValue for RustT

The idea for the API I went with in ocaml-interop is that one value could support multiple conversions, but maybe it is not necessary to do it this way and it would be simpler to just use intermediary types. This is something I am still not decided on, not sure if you have any thoughts on this.

I think having the option to support multiple conversions is nice, but I left this in because I don't see the harm in providing an option (and the 1-to-1 type conversion can be simpler in some cases). But since ocaml-interop values can convert to and from ocaml-rs values it should be possible to use OCaml<T> arguments and return types too.

tizoc · 2021-02-05T21:13:10Z

src/conv.rs

@@ -252,7 +228,7 @@ unsafe impl ToValue for &str {
    }
 }

-unsafe impl FromValue for &mut str {
+unsafe impl<'a> FromValue for &mut str {


Why is this <'a> needed here?

tizoc · 2021-02-05T21:16:23Z

src/macros.rs

-macro_rules! body {
-    ($(($($param:expr),*))? $code:block) => {
-        $crate::sys::caml_body!($(($($param.0),*))? $code);
+            $crate::interop::ocaml_frame!($gc, ($($param),*), {


note that ocaml_frame! only makes sense when there is at least one root, when there are no roots there is no need to open a frame. So this macro should use + instead of *.

tizoc · 2021-02-05T21:30:03Z

src/value.rs

+        let mut frame = rt.open_frame();
+        let gc = frame.initialize(&[]);
+        let mut x = unsafe { crate::interop::internal::OCamlRawRoot::reserve(gc) };
+        unsafe { core::mem::transmute(x.keep(OCaml::<T>::from_value(value))) }


👀 this doesn't look very safe. I don't think it makes sense to convert a Value into an OCamlRef because Value doesn't have any information about the runtime handle like OCaml<'a, T> does. So producing an OCamlRef from a Value is not really safe. The other thing is that the frame you create here gets dropped as soon as this method returns, so it is not really doing much.

I would just remove this one.

tizoc · 2021-02-05T21:30:53Z

src/value.rs

+    fn from_value(value: Value) -> OCaml<'a, T> {
+        let rt = unsafe { &mut Runtime::recover_handle() };
+        let x: OCaml<T> = unsafe { OCaml::new(rt, value.0) };
+        unsafe { core::mem::transmute(x) }


Same as the OCamlRef conversion. I think that by default you can assume that for any case where you are using Runtime::recover_handle(), the conversion you are making is not safe.

tizoc · 2021-02-05T21:39:38Z

I don't have anything else to say for now, maybe I should give it another pass tomorrow because I may have missed something. But I think it will be during a possibly eventual adaptation of Tezedge code to use ocaml-derive instead of the macros ocaml-interop provides that I will start noticing more things, and then we can figure out the necessary API changes to support whatever turns out to be missing (if anything).

zshipko · 2021-02-05T22:22:49Z

Thanks! I appreciate the review and I will take a look at some of those rough edges - I forgot about that transmute

Since you have an actual use-case, I am happy to help turn ocaml-rs into whatever would be safest/most usable. I'm not really attached to Value and don't have any future plans for improvements right now, so once ocaml-derive is ported to use ocaml-interop directly it doesn't make sense to keep that interface around anymore.

At that point, it seems like it could be best to let ocaml-interop take over the ocaml crate name, since I would definitely prefer not to continue to maintain the less safe API when I'm not actually using it.

tizoc · 2021-02-05T22:48:40Z

Thanks! I appreciate the review and I will take a look at some of those rough edges - I forgot about that transmute

Since you have an actual use-case, I am happy to help turn ocaml-rs into whatever would be safest/most usable. This initially started as an experiment for me and I'm not really attached to Value and don't have any future plans for improvements right now, so once ocaml-derive is ported to use ocaml-interop directly it doesn't make sense to keep that interface around anymore.

At that point, it seems like it could be best to let ocaml-interop take over the ocaml crate name, since I would prefer not to continue to maintain the less safe API when I'm not actually using it.

Well, my intention has always been to eventually merge everything under the ocaml namespace, and not to have two separate libraries that circularly depend on each other!

The reason I haven't suggested that yet is that I still consider ocaml-interop to be experimental and with an evolving design. It is getting closer to stabilizing, but it is not there yet. Thats why I have added an unstability notice to the README in the latest releases.

I'm still undecided on some things, like complicating the traits to support multi-way conversion, and also I'm waiting to see how things like fast-global-roots go to see if I can simplify the API further (by getting rid of the need to use frames and local roots).

Also, for now the focus has been only on the Rust->OCaml side because that is what has been needed the most in Tezedge until now, but there are things that could be done to make things better for OCaml->Rust calls (both Tezos having Rust dependencies now + Tezedge also going to require that soon are real use-cases I have for that now).

zshipko · 2021-02-05T23:16:49Z

Alright cool, that makes sense! I'm mostly just pointing out that the whole Value interface can be removed soon, since it pretty much only exists right now to facilitate minimal changes to the derive package.

It does sound like releasing this PR, once I clean things up, would be a good intermediate step. But in the long run we are kind of waiting on the ocaml-interop/ocaml-derive integration to continue combining everything. I have some time to start thinking about the derive changes, but need to take a look at the ocaml-interop codebase again because things are changing so fast!

Also, the current Custom value implementation should be possible to port over to ocaml-interop directly and there may be a few other items like that too. While we're waiting for things to stabilize on your end I will try to come up with solutions for some of these smaller integration points.

zshipko · 2021-02-18T19:12:04Z

Alright @tizoc, I have removed those conversions between Value and ocaml_interop::OCaml that require transmute - I replaced FromValue for OCaml<T> with a function called Value::interop that does the conversion instead. This means OCaml values can't currently be used as ocaml::func parameters, but I am still happy with the level of integration between the two crates as an intermediary step.

If you don't see any more issues, I will make a new release for these changes in the next few days!

tizoc · 2021-02-18T19:38:27Z

derive/src/derive.rs

            })
        } else if attrs.unboxed {
            if variant.bindings().len() > 1 {
                panic!("ocaml cannot unboxed record with multiple fields")
            }
-            variant.each(|field| quote!(#field.to_value()))
+            variant.each(|field| quote!(unsafe {#field.into_value(gc) }))


@zshipko is this call really unsafe? the fn into_value(...) declarations that show up on this diff don't have the unsafe qualifier.

Thanks, that doesn't need to be in an unsafe block

tizoc · 2021-02-18T19:47:13Z

src/value.rs

-                return None;
-            }
+    /// Convert from `Value` into `interop::OCaml<T>`
+    pub fn interop<'a, T>(&self, rt: &'a Runtime) -> OCaml<'a, T> {


This one probably needs to be marked unsafe, because as it is it allows you to convert into any T, which is not always valid.

…tion from Rust in tests

zshipko force-pushed the interop2 branch from 99f9a28 to dd89d38 Compare December 19, 2020 00:34

zshipko marked this pull request as ready for review January 20, 2021 16:45

zshipko mentioned this pull request Jan 20, 2021

Investigate how to increase compatibility with ocaml-rs to ease integration tizoc/ocaml-interop#13

Open

tizoc reviewed Feb 5, 2021

View reviewed changes

zshipko force-pushed the interop2 branch from dd55305 to 7921d01 Compare February 18, 2021 17:22

zshipko added 14 commits February 18, 2021 11:07

Use ocaml-interop for low-level operations

7b77771

WIP

077e0a5

Add ability to rename gc handle

1678907

Mark Value methods as unsafe

3e4d49c

Remove ocaml-sys macros

e52a231

WIP

8d59b1e

Remove Rooted

05157f3

Create rooted values from parameters in ocaml::body

8b5c9e8

Use ocaml-interop git repo

468f57f

clippy

6b44c41

ci

31c53ab

clippy

692e411

Fix attr

2d25bc0

Remove call to Gc.minor to ensure allocation issues have been fixed

aadc97d

zshipko added 8 commits February 18, 2021 11:07

cleanup

4f81aed

Start adding more OCaml values in tests

caff018

Updates for latest ocaml-interop

bc69178

as_i64 -> to_i64

d3a8d68

Switch to ocaml-interop release

b750c46

ToValue -> IntoValue

ac59e68

Use immutable ref for runtime handle in into_value

548276c

Versions, cleanup derive

4fc48dd

zshipko force-pushed the interop2 branch from ac6c17f to 4fc48dd Compare February 18, 2021 19:09

zshipko force-pushed the interop2 branch 2 times, most recently from c5216a9 to f508c34 Compare February 18, 2021 19:20

clippy

56d55c0

tizoc reviewed Feb 18, 2021

View reviewed changes

zshipko force-pushed the interop2 branch from f508c34 to 56d55c0 Compare February 18, 2021 19:39

Calls to into_value should not be marked as unsafe

d8f7af4

tizoc reviewed Feb 18, 2021

View reviewed changes

zshipko added 4 commits February 18, 2021 14:34

Mark Value::interop as unsafe and remove unnecessary transmute

a538444

Fix test

dcf0330

Add a note about ocaml-interop, use interop::ocaml to call OCaml func…

802a9f9

…tion from Rust in tests

Fix clippy

e9fb849

zshipko merged commit cac6a2c into master Feb 26, 2021

zshipko deleted the interop2 branch February 26, 2021 21:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ocaml-interop integration #48

ocaml-interop integration #48

zshipko commented Dec 18, 2020 •

edited

Loading

zshipko commented Feb 4, 2021

tizoc commented Feb 4, 2021

tizoc Feb 5, 2021 •

edited

Loading

zshipko Feb 5, 2021

tizoc Feb 5, 2021

zshipko Feb 5, 2021

zshipko Feb 5, 2021 •

edited

Loading

tizoc Feb 5, 2021

zshipko Feb 5, 2021

tizoc Feb 5, 2021

tizoc Feb 5, 2021

tizoc Feb 5, 2021

tizoc Feb 5, 2021

tizoc commented Feb 5, 2021

zshipko commented Feb 5, 2021 •

edited

Loading

tizoc commented Feb 5, 2021

zshipko commented Feb 5, 2021

zshipko commented Feb 18, 2021 •

edited

Loading

tizoc Feb 18, 2021

zshipko Feb 18, 2021

tizoc Feb 18, 2021

ocaml-interop integration #48

ocaml-interop integration #48

Conversation

zshipko commented Dec 18, 2020 • edited Loading

zshipko commented Feb 4, 2021

tizoc commented Feb 4, 2021

tizoc Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zshipko Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tizoc commented Feb 5, 2021

zshipko commented Feb 5, 2021 • edited Loading

tizoc commented Feb 5, 2021

zshipko commented Feb 5, 2021

zshipko commented Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zshipko commented Dec 18, 2020 •

edited

Loading

tizoc Feb 5, 2021 •

edited

Loading

zshipko Feb 5, 2021 •

edited

Loading

zshipko commented Feb 5, 2021 •

edited

Loading

zshipko commented Feb 18, 2021 •

edited

Loading