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

RHash.lookup for bool value #95

Closed uvlad7 closed 12 months ago

uvlad7 commented 12 months ago

So, I have an option with default value true. As bool can be converted from any Value, I assumed nil is converted into Some(false), but it's converted into None:

3.0.0 :001 > puts Xml2Json::Json.build("<root><a>1</a><b>2</b></root>")
{"root":{"a":["1"],"b":["2"]}}
 => nil 
3.0.0 :002 > puts Xml2Json::Json.build("<root><a>1</a><b>2</b></root>", explicit_array: false)
{"root":{"a":"1","b":"2"}}
 => nil 
3.0.0 :003 > puts Xml2Json::Json.build("<root><a>1</a><b>2</b></root>", explicit_array: true)
{"root":{"a":["1"],"b":["2"]}}
 => nil 
3.0.0 :004 > puts Xml2Json::Json.build("<root><a>1</a><b>2</b></root>", explicit_array: 1)
{"root":{"a":["1"],"b":["2"]}}
 => nil 
3.0.0 :005 > puts Xml2Json::Json.build("<root><a>1</a><b>2</b></root>", explicit_array: nil)
{"root":{"a":["1"],"b":["2"]}}
 => nil

I assume it's because of how TryConvert for Option<T> is implemented. Not sure if scan_args can help here, it's not implemented for such a big number of kwargs.

So, is there any way to work with opts like it would be in Ruby?

def build(opts = {})
# or def build(**opts)
  ...
  if opts[:explicit_root]
    ...
  end
end

I see that aref is better here because it takes default value/proc into account, and that's exactly how #[] is implemented, but it doesn't solve conversion problem. And fetch is not an option because I need to differentiate conversion errors and key-not-found cases. So, the only option - applicable for any type - here is to use get and try_convert separately, right? But get doesn't care about defaults, and aref::<_, Option<Value>> is also not, uh, an option - it converts nil to None

uvlad7 commented 12 months ago

And the same problem appears with Option<Value>. It's absolutely not expected when you use a raw Value that nil becomes None.

matsadler commented 12 months ago

RHash::lookup binds to the rb_hash_lookup function from Ruby's C api. This function returns nil when there is no value for a key, so it is is impossible to distinguish between a missing key and a key with the value nil using this function, regardless of anything Magnus does.

RHash::aref (rb_hash_aref under the hood) suffers from a very similar problem, and while RHash::fetch (rb_hash_fetch) can distinguish missing keys, the API may feel a little awkward to use from Rust:

let ruby = unsafe { magnus::embed::init() };

let hash = ruby.hash_new();
// hash.aset("foo", "bar");
// hash.aset("foo", 1);

match hash.fetch::<_, String>("foo") {
    Ok(v) => println!("got {v}"),
    Err(e) if e.is_kind_of(ruby.exception_key_error()) => println!("missing key"),
    Err(e) => println!("error: {e}"),
}

For this reason RHash::get is provided, which more closely matches Rust's std::collections::HashMap::get. This is implemented with rb_hash_lookup2, which doesn't explicitly provide a way to distinguish between a missing and nil key, but can be used to implement something to detect that case.

RHash::get doesn't implement type automatic type conversion like the other functions to avoid having Option<Result<T> as the return type (that could be Option<Result<Option<T>>> with type conversion), and to avoid confusion around nil converting to Option::None.

Following on from the example above you'd probably want to use that something like:

use magnus::TryConvert;
RString::try_convert(hash.get("foo").unwrap_or_else(|| ruby.into_value("default")))?

Probably not helpful for you right now as it will take a while to get to a release, but I think there's a good use case for exposing rb_hash_lookup2 in Magnus, so I've done that, as that would easily let you match the behaviour of keyword args with defaults. I'd not considered that use case before.


The conversion from a Ruby object to Option<T> just checks if the object is nil, if it is it returns None, if it isn't it invokes the conversion for T and returns whatever that is wrapped in Some. If a function is returning T: TryConvert it will invoke this conversion, if it's returning Option<T> it won't.

If you want no conversion done at all, set the return value to Value, not Option<Value>.

I'm pretty sure this is the least confusing way for it to work.


scan_args tops out at 9 arguments of each category (required, optional, etc) because of a limitation of the underlying Ruby api. get_kwargs doesn't suffer the same limitation from Ruby, but to bind it sensibly to Rust it does end up that you have to limit the number of keyword args to something, and by picking 9 I can reuse some code from scan_args. I can't really see a good argument for changing it when I have to pick a limit, and whatever number I pick will always be not enough for someone. Hopefully Rust will get variadic tuples at some point and I can remove the limit.

uvlad7 commented 12 months ago

Ok, thank you for the detailed answer. BTW, are there any docs about these C API methods? aref is straightforward, it's just #[], but the others aren't. Even Ruby fetch doesn't use rb_hash_fetch, but st_lookup. I haven't found any docs.

it will take a while to get to a release

I didn't even manage to use '0.6.2', had to make it compile with ruby 2.6.

matsadler commented 12 months ago

The only docs for Ruby's C API are the comments in the C header files.

had to make it compile with ruby 2.6.

ah, sorry about that. 2.6 is a hassle to support in magnus, and as it 2.6 is no longer supported by the maintainers of Ruby I decided to drop support.

uvlad7 commented 12 months ago

I decided to drop support

Yeah, I should probably do the same. Or create some feature for old Rubies which depends on old magnus.