danielpclark / rutie

“The Tie Between Ruby and Rust.”
MIT License
940 stars 62 forks source link

Rutie Float type requires an exact Float when called from Ruby #99

Closed bbugh closed 5 years ago

bbugh commented 5 years ago

Hi 👋. Thanks for building Rutie, I am glad to see that there's another option for Ruby/Rust integration that is actively developed.

I'm experimenting with offloading some of our high density math to Rust. I made a repository for benchmarking and API experimentation with the various ways to integrate Rust and Ruby. I'm currently adding rutie to the benchmark, mostly for API comparison.

I noticed that Rutie inherited the same problematic ruby Float behavior from ruru that the other APIs (rust/helix, rust/ffi, c, etc.) don't share.

Issue

// This will cause a panic if passed a non float like `2123`
fn double_me(input: Float) -> Float {
    Float::new(input.unwrap().to_f64() * 2.0)
}

rutie and ruru require Ruby to pass in an exact Float value 0.0, rather than allowing integers passed as floats 0 like Ruby itself. If a non-exact float is passed, this will panic.

# works fine when called with an exact float
def call_double_me
  RutieExample.double_me(100.0)
end

# Rutie PANIC when called with a non-exact float
# Other libraries work fine
def call_double_me
  RutieExample.double_me(100)
end

thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: #<TypeError: Error converting to Float>', src/libcore/result.rs:999:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
fatal runtime error: failed to initiate panic, error 5

Expectation

Given that a valid ruby Integer is always a valid ruby Float, and a valid rust iX is always a valid fX, this seems like it would be optimal for rutie to handle this easily without a panic attack.

The Ruby C API (using rb_num2dbl/NUM2DBL) correctly handles this cast. It looks like the rutie source code is calling extern rb_num2dbl, so I'm assuming the issue is happening Rust-side. Is it missing traits?

I'm happy to PR with a bit of guidance on how to approach it. I'm guessing it's something like this integer PR.

danielpclark commented 5 years ago

First of all thanks for your interest and feedback. ^_^

I noticed that Rutie inherited the same problematic ruby Float behavior from ruru that the other APIs (rust/helix, rust/ffi, c, etc.) don't share.

The behavior you are describing is by design. The soundness of identifying what a Ruby object is when it is given to Rust is very important.

The unwrap() you're using in input.unwrap().to_f64() is not the right use of the object passed in from Ruby which is a Result<rutie::Float, rutie::AnyException> object since you're most likely defining in your method macro that you expect a Float for input. If you wanted something that may be other than a float object you should set the input type as AnyObject and unwrap() will give you Result<rutie::AnyObject, rutie::AnyException> which should always work in our case to be the AnyObject type.

In Rutie we're not trying to make a C like interface to Ruby and behave like the Ruby C API. What we're doing is trying to match Ruby objects and behavior as best we can in a 1 to 1 design.

The behavior you pointed out in rb_num2dbl/NUM2DBL is a C API implementation of implicit conversion for types to float. In the Ruby ecosystem many types like for converting to String have implicit to_str or explicit to_s. Float does not have an implicit method name used in Ruby but instead uses Float() as the way to do this same implicit conversion you're talking about with rb_num2dbl.

So, as of this writing, the PR you've submitted for verifying the Float type and adding the types Bignum and Rational to it is against the soundness of Rutie's current design. Instead the way this should be implemented is to create a method on Float in Rutie named implicit_to_f(obj: AnyObject) which will do the Float() behavior that the Ruby side does. And in your own code you would write your Rutie code to take in an AnyObject for values passed in from Ruby, rather than a Float, and pass that in to Float.implicit_to_f in Rutie.

bbugh commented 5 years ago

Hi @danielpclark, thanks for the explanation! 🎉 That helps me understand more about designing our interop API. It sounds like it would simplify things for the pub_* functions to be a translation layer that behaves more like Ruby accepting duck types, which are then static casted, which are then passed to an internal Rust function. I can get down with that.

Your described implicit_to_f makes sense, but I'm somewhat uncertain about handling the boundaries of the error. I would expect it to be a Result type typically, but I don't know how errors fall through from Rust to Rutie and on.

// option 1, if this is invalid, rust panics? Or an error is caught elsewhere?
pub fn implicit_to_f(obj: AnyObject) -> Self {
    Float::new(float::num_to_float(obj.value()));
}

// Option 2: Or it's a result, and the caller should handle the error?
// Should it be a Ruby AnyException or a Rust<_, &str> or something?
pub fn implicit_to_f(obj: AnyObject) -> Result<Self, AnyException> {
    if ty == ValueType::Float || ty == ValueType::Fixnum || ty == ValueType::Bignum || ty == ValueType::Rational { 
        Ok(convert)
    } else {
        Err(something)
    }
}

If I understand correctly option 1 would be used like this:

let pretend_ruby_argument = AnyObject::from(Float::new(46.0 as f64).value());

// usage
let value = Float::implicit_to_f(pretend_ruby_argument);
assert_eq!(value.to_f64(), 46.0 as f64);
bbugh commented 5 years ago

We do a lot of complex real-time financial calculations that are starting to buckle under Ruby's interpreted performance. C was fine for writing games but I'd rather have something with built-in safety for mission-critical financial stuff. I think your comment explains a bit more about how we'll use BigDecimal (with an AnyObject that we check that it's some kind of number, which we then convert to a rust_decimal, then send the result back as an AnyObject). Thanks for carrying the torch with Rutie so we have a viable option for incremental replacement as we optimize. 🙏

danielpclark commented 5 years ago

I will have Rutie version 0.7.0 released tonight with a Float::implicit_to_f method on it.

You can use it to get a Result<Float, AnyException>. My advice is not to use unwrap() on a Result here because it could be an exception type. You can use match or if let and you can even use map or map_err if you plan to do some more type casting in Rust. If you want to re-raise the exception in Ruby use map_err(|e| VM::raise_ex(e)) and you can add an unwrap() on this use case… you can see a nice example of this where you get a clean Ruby object out of it here: https://github.com/danielpclark/faster_path/blob/32b0225293d8eacf7c0ecf690459282873f8094e/src/lib.rs#L103-L106

Your pub_ method might look somewhat like.

// With AnyObject as input type declared in macro
// giving us `Result<AnyObject, AnyException>`
let any_object = input.map_err(|e| VM::raise_ex(e)).unwrap();
Float::implicit_to_f(any_object).map_err(|e| VM::raise_ex(e)).unwrap().to_f64()

This way if the type is invalid it raises the exception back on the Ruby side. Otherwise it uses a valid Float & f64 on the Rust side.

danielpclark commented 5 years ago

If for some reason you decide you want to implement your own type casting version directly from Ruby then the way to handle catching exceptions is with rb_protect. You can search this code base for uses of our own protect to see how it's done.

If you'd like a less Ruby object way to do it in Rutie you can implement a low level wrapper for rb_num2dbl in src/util.rs. This is not an official Rutie API and can have some efficient type conversions here. You'll still want to use Ruby's rb_protect in some way to keep your code safe in Rust.

bbugh commented 5 years ago

Wonderful! My hero. Thank you for the extra explanations as well, it's very helpful. I really appreciate your quick turnaround.