Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a helper for asserting kwargs shape and extracting params #2091

Open
lopopolo opened this issue Aug 21, 2022 · 6 comments
Open

Add a helper for asserting kwargs shape and extracting params #2091

lopopolo opened this issue Aug 21, 2022 · 6 comments
Labels
A-vm Area: Interpreter VM implementations. B-mruby Backend: Implementation of artichoke-core using mruby.

Comments

@lopopolo
Copy link
Member

Agreed per the conversation we had on Discord. This looks like we should extract this to the convert module.

String::new wants the same functionality with its capacity: kwarg:

[3.1.2] > String.new(capacity: 10)
=> ""
[3.1.2] > String.new(capa: 10)
(irb):4:in `initialize': unknown keyword: :capa (ArgumentError)
        from (irb):4:in `new'
        from (irb):4:in `<main>'
        from /usr/local/var/rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/irb-1.4.1/exe/irb:11:in `<top (required)>'
        from /usr/local/var/rbenv/versions/3.1.2/bin/irb:25:in `load'
        from /usr/local/var/rbenv/versions/3.1.2/bin/irb:25:in `<main>'
[3.1.2] > String.new(capa: 10)
^C
[3.1.2] > String.new(capacity: 10, nine: 10)
(irb):5:in `initialize': unknown keyword: :nine (ArgumentError)
        from (irb):5:in `new'
        from (irb):5:in `<main>'
        from /usr/local/var/rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/irb-1.4.1/exe/irb:11:in `<top (required)>'
        from /usr/local/var/rbenv/versions/3.1.2/bin/irb:25:in `load'
        from /usr/local/var/rbenv/versions/3.1.2/bin/irb:25:in `<main>'

Originally posted by @lopopolo in #1956 (comment)

@lopopolo lopopolo added A-vm Area: Interpreter VM implementations. B-mruby Backend: Implementation of artichoke-core using mruby. labels Aug 21, 2022
@lopopolo
Copy link
Member Author

Use this new converter function in impl TryConvertMut<Value, Option<Offset>> for Artichoke in extn::core::time::offset.

@lopopolo
Copy link
Member Author

This ask is related to #2092. We should probably add a converter function specific to Symbol kwargs and return the right error with value.inspect in the error message.

@b-n
Copy link
Member

b-n commented Sep 28, 2022

I've been playing around a little bit with irb and some of these kwargs in things such as Time and String.

As per the description, there is validation on the kwargs. What I find interesting is this though:

[3.1.2] > String.new("hello", 1)
(irb):2:in `initialize': wrong number of arguments (given 2, expected 0..1) (ArgumentError)
	from (irb):2:in `new'                           
	from (irb):2:in `<main>'                        
	from /home/ben/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/irb-1.4.1/exe/irb:11:in `<top (required)>'
	from /home/ben/.rbenv/versions/3.1.2/bin/irb:25:in `load'
	from /home/ben/.rbenv/versions/3.1.2/bin/irb:25:in `<main>'

Specifically, it doesn't look like kwargs should count to the number of arguments, it's essentially an extra "options hash".

This lead me down some rabbit holes, and found some fun things in the vendored mruby source:

☝️ The things surrounded in ? are a guess, I didn't go too deep, just left a surface level investigation.

And then to do a sanity check, it seems that kwargs should always be last in the arguments:

[3.1.2] > Time.at(0, 100, in: "Z")
=> 1970-01-01 00:00:00.0001 UTC
[3.1.2] > Time.at(0, in: "Z", 100)
/home/ben/.rbenv/versions/3.1.2/lib/ruby/3.1.0/irb/workspace.rb:119:in `eval': (irb):20: syntax error, unexpected ')', expecting => (SyntaxError)
Time.at(0, in: "Z", 100)                                       
                       ^                                       
	from /home/ben/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/irb-1.4.1/exe/irb:11:in `<top (required)>'
	from /home/ben/.rbenv/versions/3.1.2/bin/irb:25:in `load'
	from /home/ben/.rbenv/versions/3.1.2/bin/irb:25:in `<main>'

And I see that right now, artichoke doesn't have any extraction methods for pulling out kwargs:

I'm also not sure how/where ::sys::mrb_get_args is being pulled in (although I see it is an extern call), however, my assumption here is that we're delegating out to mruby c source when calling mrg_get_args - which means that in theory we have "support" for it from mruby side, we just need some plumbing in the marcos, argspec, etc. Is that a valid analysis?

If so, how do you feel about this psuedo-ish code for extracting the kwargs by name:

unsafe extern "C" fn string_initialize(mrb: *mut sys::mrb_state, slf: sys::mrb_value) -> sys::mrb_value {
    let s = mrb_get_args!(mrb, optional = 1);
    // validation of the kwargs
    let kwargs = mrb_get_args!(mrb, kwargs = "capacity,encoding"); // Note: I need to research how this is meant to work
    unwrap_interpreter!(mrb, to => guard);
    let value = Value::from(slf);
    let s = s.map(Value::from);
    // types included for brevity (wouldn't expect these to be needed)
    let capacity: Option<Value> = kwargs.get("capacity");.
    let encoding: Option<Value> = kwargs.get("encoding");
    let result = trampoline::initialize(&mut guard, value, capacity, encoding, s);
    match result {
        Ok(value) => value.inner(),
        Err(exception) => error::raise(guard, exception),
    }
}

@b-n
Copy link
Member

b-n commented Sep 28, 2022

Ah, I stumbled upon the actual kwargs structure that mruby uses: https://github.com/artichoke/artichoke/blob/trunk/artichoke-backend/vendor/mruby/include/mruby.h#L948-L1001

The most interesting part (their examples are very nice too):

struct mrb_kwargs
{
  uint32_t num;                 /* number of keyword arguments */
  uint32_t required;            /* number of required keyword arguments */
  const mrb_sym *table;         /* C array of symbols for keyword names */
  mrb_value *values;            /* keyword argument values */
  mrb_value *rest;              /* keyword rest (dict) */
};

Which leans me more towards this kind of macro:

let kwargs: OptionalKwargs = mrb_get_args!(mrb, required = false, ["capacity","encoding"]);
let capacity: Option<Value> = kwargs.get("capacity");
let rest: Option<Value> = kwargs.rest(); // Should unbox to Hash

// I believe `mrb_get_args` will enforce the argument to exist, so it feels "safe" to not wrap in Option
let kwargs: RequiredKwargs = mrb_get_args!(mrb, required = true, ["capacity","encoding"]);
let capacity: Value = kwargs.get("capacity");
let encoding: Value = kwargs.get("encoding");
//let rest: Value = kwargs.rest(); Assumption this would not work, since required forces checking of all args

☝️ Could just be Kwargs, always return Option<Value> and do .expect('mrb_get_args should have returned a value') sort of stuff instead of having a OptionalKwargs vs. RequiredKwargs structs.

@lopopolo
Copy link
Member Author

Quick note here, mrb_get_args is defined in mruby/src/class.c.

@lopopolo
Copy link
Member Author

lopopolo commented Sep 28, 2022

As a tip, grepping for ^mrb_function_name should find you the definition due to the way the C code is formatted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-vm Area: Interpreter VM implementations. B-mruby Backend: Implementation of artichoke-core using mruby.
Development

No branches or pull requests

2 participants