search

d-unsed / ruru

Native Ruby extensions written in Rust
MIT License
834 stars 40 forks source link

eval functions with panic safe implementation #79

Open danielpclark opened 6 years ago

danielpclark commented 6 years ago

This requires a version bump in ruby-sys for the new eval methods and then the Cargo.toml here can be set to that version.

The code works as is now but I have a few questions. Before those questions I'll explain that I've chosen to use rb_eval_string_protect for VM::eval since it allows us to get a standard Result<AnyObject, c_int> back and we won't need to worry about it crashing our program. The rb_eval_string I added as an unsafe VM::eval_str because any exceptions raised crash the program. So until we have exceptions implemented I figured this would be the safer Rust way to do it.

My questions are whether we should have VM::eval_str at all since I've marked it as unsafe. Should the VM::eval be enough and I remove the other method since we don't have exception handling? If so I would suppose the binding for that should go for now, or maybe #[allow(dead_code)]?

Of course once we have exceptions implemented then I would expect the VM::eval method to switch from the rb_eval_string_protect to rb_eval_string.

Thinking out loud: But thinking about the way Rust works I believe that any time an exception is, or can be, raised then methods should return a Result<AnyObject, Error> and it would be nice for Result to implement the try_convert_to directly on it. Errors can still be of type AnyObject but return types in general would make more sense as a Result and just re-implement casting to the Result types Ok and Err. Those are my thoughts on that.

TODO

danielpclark commented 6 years ago

After looking into Ruby's Exception handling code I believe rb_eval_string_protect is the right choice. With that if it comes back as an Err() we can then use the rb_errinfo() -> Value to retrieve the instance of the Exception class.

danielpclark commented 6 years ago

Thinking more about this I want to update this feature after https://github.com/steveklabnik/ruby-sys/pull/28 is merged. I want to update eval's error path to return an AnyObject of the exception class raised.

danielpclark commented 6 years ago

Alright the Rust eval method for the Ruby API will return a Result<AnyObject, AnyObject> allowing us to work with exceptions perfectly between Ruby and Rust. An example is provided.