matsadler / magnus

Ruby bindings for Rust. Write Ruby extension gems in Rust, or call Ruby from Rust.
https://docs.rs/magnus/latest/magnus/
MIT License
682 stars 35 forks source link

`try_convert` compiles for `Vec<(T,)>`, but not `Vec<T>` #119

Closed ankane closed 1 month ago

ankane commented 1 month ago

Hi @matsadler, hope you're doing well.

I ran into some unexpected behavior with try_convert (but there may be a reason for it).

use magnus::{Error, TryConvert, Value};

struct MyType {}

impl TryConvert for MyType {
    fn try_convert(_: Value) -> Result<Self, Error> {
        Ok(MyType {})
    }
}

fn hello(v: Value) {
    // works
    Vec::<(MyType,)>::try_convert(v).unwrap();

    // does not work
    Vec::<MyType>::try_convert(v).unwrap();
}

The non-tuple version fails to compile with:

Vec::<MyType>::try_convert(v).unwrap();
               ^^^^^^^^^^^ function or associated item cannot be called on `Vec<MyType>` due to unsatisfied trait bounds
...
note: the trait `TryConvertOwned` must be implemented
matsadler commented 1 month ago

It's not safe to put Ruby's Value type on the heap, so Vec<T> only implements TryConvert where T implements TryConvertOwned. And TryConvertOwned is just a promise that your type doesn't contain a Value.

A unsafe impl TryConvertOwned for MyType {} should fix your example (as far as getting Vec::<MyType>::try_convert(v) working).

Vec::<(MyType,)> should encounter the same error, but I messed up the type bounds for tuples. I'll fix that.

ankane commented 1 month ago

MyType should be fine to put on the heap, right? (since it shouldn't contain a value?)

ankane commented 1 month ago

Ah, I think I see what you're saying. The code should use unsafe impl TryConvertOwned for MyType {} if it won't contain a value.

matsadler commented 1 month ago

The code should use unsafe impl TryConvertOwned for MyType {} if it won't contain a value.

Yep, that's it. TryConvertOwned is just a marker trait that requires TryConvert for the actual implementation. Similar to how PartialEq has the equality implementation, and Eq is just a promise on top of that that the type is fully equivalent.

ankane commented 1 month ago

Great, thanks @matsadler!