Implement nested composition of FromLuaMulti

[Caellian]

This PR adds ability to compose FromLuaMulti values which makes it convenient to add functions which consume arguments of different kinds.

This places an additional requirement on user implementing FromLuaMulti to mutate consumed argument with the number of arguments that were consumed from provided remaining arguments.

I'm not too happy with complexity added by this feature, but it significantly simplifies my workflow in a case where I have to consume arguments in form (&[(f32, f32)], bool) and the tuple is supposed to be flattened. The main place that's affected by this additional argument is impl_tuples! trait.

• I decided to add it as a &mut usize argument instead of return type as I think that's more convenient for implementers, as it allows gradual mutation of the usage counter instead of having to provide their own. It also provides argument location (didn't test that though).

Usage

pub struct FromLuaPoint {
  x: f32,
  y: f32,
}

impl<'lua> FromLuaMulti<'lua> for FromLuaPoint {
  fn from_lua_multi(
        values: LuaMultiValue<'lua>,
        lua: LuaContext<'lua>,
        consumed: &mut usize,
    ) -> LuaResult<Self> {
      let mut values = value.into_iter();
      match values.next() {
        LuaValue::Number(x) => {
          // try access y or error...
          *consumed += 2;
          return Ok(FromLuaPoint{ x: x as f32, y: y as f32 })
        }
        LuaValue::Table(table) => {
          // process table...
          *consumed += 1;
          return Ok(FromLuaPoint{ x: table_x, y: table_y })
        }
        _ => {
          // return error when missing arguments, bad arguments, or some other issue...
          return Err(LuaError);
        }
      }
    }
}

methods.add_method("addPoint", |_, this, (point, color): (FromLuaPoint, FromLuaColor)| {
  // do something with point and color parsed from LuaMultiValue
  println!("Added point: {}, {}", point.x, point.y);
});

UserData:addPoint(x, y, color_str)
UserData:addPoint(x, y, r, g, b, a)
UserData:addPoint({x, y}, r, g, b, a)
UserData:addPoint({x = x, y = y}, r, g, b, a)

Expanded argument semantics

I'll express these as regexes where each Lua value is represented by a different letter. Groups (bc) represent Rust types which implement FromLuaMulti directly, rest if flat FromLua Rust types (a & d)

• FromLua: Stays the same, blanket implementation of FromLuaMulti now updates the consumed by 1 if conversion succeeds or returns an error otherwise.
• Option<FromLuaMulti>: works like a(bc)?d regex, error is still on d if bc isn't captured and d can't be converted.
• Variadic<FromLuaMulti>: works like a(bc)*?d regex, error is on d if bc isn't (i.e. is partially) captured and d can't be converted.
  • previously couldn't be followed by any arguments.
• Vec<FromLuaMulti>: works like a{(bc)*}d regex, error is on d.
  • same as Variadic<FromLuaMulti> only bc list has to be wrapped in a Table.

Breaking changes

FromLuaMulti::from_lua_multi takes an additional consumed: &mut usize argument.

Memory

Currently, every FromLuaMulti is provided with a clone of LuaMultiValue. Given that the size of Value is usize and max supported number of arguments is 16, this means that in worst case processing $n$ arguments could temporarily do $(n - 1) \times 120B$ additional allocations on heap (due to LuaMultiValue being backed by Vec). That's a very degenerate case where a user specifies 16 Option<Inconstructible> types in add_method, in most cases it will do far less ($\sim32\text{B}$). That should fast path on most allocators due to size, but clone should probably be avoided if possible, or Cow used internally by LuaMultiValue (didn't feel comfortable changing that).

[jugglerchris]

Hi,
Thanks for the merge request!

I wonder if this is maybe a bit too much extra complexity for what seems like is an unusual case, plus I worry that there's a lot of scope for confusion when some type consumes more (or fewer) values than the user expected. In my experience it can sometimes be tricky debugging issues where the automatic argument conversions fail (your function ends up not being called), and this feels like they'd have more ways to go wrong.

Do you gain much from this modified trait being in rlua, compared to having it in your application? Your function could just take a MultiValue<'lua>, and your types can have a construct_from_multi(&mut MultiValue<'lua>, &mut consumed) which you call for both (for example) types?

[Caellian]

Do you gain much from this modified trait being in rlua, compared to having it in your application?

I register 50+ methods which all take Point types and want to allow passing it as a Table from Lua so that they can be passed around/generated easily. Forcing separate x & y arguments would require unpacking in Lua code, but I think this provides best experience from Lua code while at the same time making the binding code much cleaner.

A simple case here would add like ~5 more lines because I'd have to match against the tailing f32 manually after calling construct_from_multi.

It also makes it very straightforward to implement functions with Variadic length that have tailing arguments (here).

• In never version that Variadic is a Vec to closer match the Skia API. Same logic applies though, only the outermost value must be an array Table.

In some cases I have 5 arguments all of which would have to be manually type checked. In some cases, 1-line expressions would have to be turned into 7 lines of code, so without this feature, bindings code would be about 50% larger.

I also sometimes need to optionally capture either 2 (x & y) arguments or none.

This change makes it very convenient to configure the number of consumed arguments in method signature instead of body which I find nicer, but same functionality could be achieved with proposed construct_from_multi.

Implementation of FromLuaMulti stays mostly the same except for having to update the counter.

• I'll have to add that to the FromLuaMulti doc comment.

I could provide index of failed top-level argument Type (and corresponding Lua argument index) in the error message if you find it necessary, but from what I experimented with 1-level of nesting, the error messages produced from LuaPoint type seemed enough for me to debug issues while it had bugs.

• It would be nice though if error were to say: "value X doesn't continue either Y nor is valid for Z: Z inner message"
  • I'd have to add and check for a new error type that'd be returned by Option and Variadic, but it's doable.

Note that while I have written over 2000 LoC using this feature, I haven't tested it much yet.

[Caellian]

• CI failing reminds me to add tests for these features.

[jugglerchris]

Sorry for the delay between replies!

I can see that this is a very useful feature for some use cases! To recap, my current concerns are:

• Extra complexity and chance of errors, for example:
  • The consumed count is another thing to have to get right (and very slightly more annoying to implement for existing cases)
  • There are more ways to have confusing behaviour when the FromMulti is not last, e.g. values not being consumed by the expected parameter,
• It's a breaking change. That in itself is fine, we can bump the version number, but I'm concerned that having to update all the users' FromLuaMulti impls to add the right counts makes it a fairly annoying update.

So I'd like to make sure we can balance the problems with the benefits.

Some thoughts I've had (but not had the time to investigate properly):

1. Can this be done by adding another wrapper type that implements the original FromLuaMulti, but that delegates to your modified version, without changing the original? Something like:

pub struct FlexibleFromLuaMulti<T>(T);

... generated from macro for different sized tuples
impl<A:NewFromLuaMulti, B:NewFromLuaMulti> FromLuaMulti for FlexibleFromLuaMulti<(A, B)> {
    fn from_lua_multi(...) {
        let mut consumed = 0usize;
        // delegate to NewFromLuaMulti::from_lua_multi(..., &mut consumed)
    }
}

// using it
methods.add_method("addPoint", |_, this, FlexibleFromLuaMulti((point, color)): FlexibleFromLuaMulti<(FromLuaPoint, FromLuaColor)>| {
  // do something with point and color parsed from LuaMultiValue
  println!("Added point: {}, {}", point.x, point.y);
});

I guess a shorter name would probably be needed!

2. As a different option, is there a way to do this by adding the method to FromLuaMulti which lets simpler implementations (i.e. most of them) keep their current implementation? I'm thinking of adding the new method with the count parameter, but with a default implementation which calls the old version and increments the count by the number of values (i.e. consuming all of them).

The (probably fatal, but maybe there's a creative solution) issue I can see here is that I don't think there's a way to do this without either forcing some users to implement both methods, or having an impl which implements neither which compiles but fails at runtime.

I'm still thinking about this - I'd appreciate your thoughts.

[Caellian]

Right, a solution that's backwards compatible did cross my mind at some point but I didn't have time to implement it and half-way forgot about it, but it was something like:

pub trait FromArgumentPack<'lua> {
  // current FromLuaMulti::from_lua_multi
}

impl<'lua, M: FromLuaMulti<'lua>> FromArgumentPack for M {
  // pass to multi without counter
}
// `impl for FromLua` is done though `impl FromLuaMulti for FromLua`

// use `FromArgumentPack` instead of `FromLuaMulti` in `impl_tuple`

So it's similar to adding a function to FromLuaMulti but I think it allows dependents to use FromLuaMulti as they were.

I think that would:

• keep the old behavior and API untouched,
  • besides now allowing Variadic in the middle,
• isolate the complexity from FromLuaMulti,
• avoid having to wrap arguments in another type, i.e. look cleaner,
• avoid having to clone MultiValue for each try,
  • would require specialization for FromLua though, so probably will clone
• provide impl_tuple more info and make the macro itself smaller (less generated code).

Preventing Variadic in the middle doesn't seem like a good choice to me because it was a hard error so far and if people do try it after merge I think error if the next argument is invalid makes sense.
Avoiding it altogether is possible by adding a marker trait LastArgument to Variadic and reverting some of impl_tuple changes, but it would add complexity while gating against a use case that might be wanted by dependents anyway (and can't currently occur). One would basically have to re-implement Variadic2 that's allowed in the middle to get the same functionality that's otherwise blocked by this trait.

I don't think the "added confusion when FromMultiValue isn't last" is a valid concern as that isn't currently possible, nor would happen after merge if dependents don't do so explicitly themselves. That is, they need to either place a Variadic argument in the middle, or their own types that implement FromLuaMulti.

• In the former case they want to greedily consume variable number of values in the middle.
• In latter case they do actually want to consume multiple values in the middle and can return their own error messages.

That also leads to another point - consumed doesn't really need to be updated for the last argument. So after merge the change that needs to be performed in most cases is just adding the &mut consumed argument. That would duplicate arguments if dependents were to now move the last argument into the middle of the argument list, but it doesn't cause any issues by default. Release notes can specify "if you want FromLuaMulti in the middle, consumed must be updated, there's an example on how to do it in Variadic", just like the docs do.

My FromArgumentPack trait idea is basically just a compatibility wrapper. FromLuaMulti exists afaict to allow the last argument to be a regular FromLua or Variadic (or something similar). Given the change that's being suggested, adding another trait to the mix seems superfluous. If this was merged and rlua was used the same as before, dependents wouldn't observe any difference in behavior.

[jugglerchris]

Hi,
That idea sounds really good to me.

I didn't have time to implement it and half-way forgot about it

Do you think it's still a lot of work to get to that from where you are now? I may find time to pitch in a bit.

[Caellian]

Do you think it's still a lot of work to get to that from where you are now? I may find time to pitch in a bit.

Shouldn't be a lot of work, just have to replace FromLuaMulti with a differently named trait in most cases. I'm finishing up my Skia bindings atm, once I'm finished with that I'll get to it - so this or the following weekend.

[Caellian]

Note: #288 (comment) now shows an example where FromLuaMulti might be needed with this change in user-space code. This would've previously been FromLua, meaning this change might also affect some generic code, so it's not just impl blocks as I previously assumed.

[Caellian]

I tried doing it with the wrapper trait.

The issue is the language lacks negative trait bounds so this solution can't work:

• If implemented only over FromLua it works fine for those types, but it can't support FromLuaMulti in that case.
• If I try implementing it only over FromLuaMulti then regular FromLua types eat all the remaining arguments.
• Implementation over both is not possible without specialization because the type checker can't decide which implementation to use in case both traits are implemented (which is the case for all FromLua).

I tried replacing FromLuaMulti while still keeping it around (current state of the linked branch), but that breaks existing code.

• This isn't well tested for From, there's a test for ToLuaMulti with Points that broke though.
  • I wrapped both at the start to get better type checking coverage so caught that by accident.

That also answers the question:

Can this be done by adding another wrapper type that implements the original FromLuaMulti, but that delegates to your modified version, without changing the original?

• Adding a struct would only allow direct implementation for specific implementations as FromLuaMulti would still eagerly consume all arguments.

Anything reading from FromLuaMulti can't really tell whether the underlying data required a single or several (and how many) Lua values.

I have a couple of ideas for some other approaches so I'll try those out as well.

[Caellian]

I think adding a separate method like this is probably the cleanest solution that would cause least breakage in dependent code. It also makes the diff a lot more concise.

The only issue I can see is someone implementing FromLuaMulti without overriding either method which would cause infinite recursion at runtime, but I think it's obvious that something has to be specified (and it's noted in the doc).

[Caellian]

The benefit of having those methods call each other is that dependents only need to implement one, depending on whether they need to handle arguments non-exhaustively.

[jugglerchris]

I like having the option of implementing either, but I don't like the failure mode if you don't. :-(

[Caellian]

I think this is the best solution possible atm. Alternatives are:

• depending on nightly,
• having to manually implement both methods, or
• the first solution which requires possibly a lot of updates to existing code base.

[Caellian]

@jugglerchris I think this is the final solution, just let me know whether you think there would be any benefit to passing the MultiValue by reference. I'm not sure because I noticed LuaRef has some custom logic for reference counting and I'm assuming it's not a huge cost to clone them but can't really tell without getting more familiar with inner workings of the library.

[jugglerchris]

I like having the option of implementing either, but I don't like the failure mode if you don't. :-(

[jugglerchris]

Cloning the values each time seems like a problem - it means means converting the list is now O(n^2).

I'm thinking aloud (you've looked at this a lot more than I have recently), but it feels like passing &mut values instead of by-value would solve that and also make the consumed count unnecessary as from_counted_multi could just drop_front(...) (or maybe consume(n)) instead.
The downside is that the function could do other weird things like replace it with a different list.
Perhaps a narrower wrapper MultiValueView or something which can only consume items. What do you think?

[Caellian]

The main drawback with that approach is that implementations have to manually juggle values (revert them when testing fails). Fallible could do a clone and revert it on error but it complicates the implementation for most other structures.

I tried adding a wrapper as well at some point but it required adding lifetimes to all the traits/functions that deal with FromLuaMulti conversion which felt like it complicates code too much. Though I do think an iterator with "revert last argument" function and "list skipped types" function would be ideal.

[jugglerchris]

I agree that adding extra lifetimes to traits would be awkward, and make updates from previous versions difficult.
But I think the quadratic runtime for long (even non-multi-value) argument lists is also a problem.
That quadratic issue is only because multiple arguments can have a crack a (nearly) the same list, and wouldn't be a problem with the old "take all" version. Maybe the new counted method can take a different type which does behave as an iterator. Perhaps with a "peek next" rather than "revert pop"?

[jugglerchris]

What is this increment for? I would expect the T::from_counted_multi() would already have done this.

[Caellian]

Right, that's an error. Didn't test this latest version as much.

[jugglerchris]

Rather than incrementing consumed, should it just start at 0 and the implementer just writes the number consumed, rather than incrementing a value they're not interested in?

[Caellian]

That could also work but it makes the code more complicated as consumed would have to be constructed from scratch before each call. In case of nested arguments that are composed out of several others, that's a lot more room for error.

[Caellian]

While reading your review, I thought about maybe removing the added method and changing the existing from_lua_multi to:

fn from_lua_multi(values: &mut MultiValue<'lua>, lua: Context<'lua>) -> Result<Self>

which is a mix of one of your suggestions and the initial version.

It's good because:

• it's the simplest solution that handles counting automatically,
• avoids the infinite recursion issue when trait is incorrectly implemented,

but the downsides are:

• it's a breaking change,
• requires manually reverting values in some (infrequent) cases.

I think we've covered all the different ways this could be implemented currently, so it's up to you to pick which stable tradeoffs you find acceptable. I was thinking about adding a feature for specialization but it's not needed if a breaking change is made and maintaining the additional code would be more complicated.

I think this last suggestion to change from_lua_multi to take a &mut MultiValue is the cleanest and avoids most pitfalls. I would also accompany this with changing from_lua to take a reference as well for consistency.

[jugglerchris]

Sorry again for slow replies!

While reading your review, I thought about maybe removing the added method and changing the existing from_lua_multi to:

fn from_lua_multi(values: &mut MultiValue<'lua>, lua: Context<'lua>) -> Result<Self>

which is a mix of one of your suggestions and the initial version.

It's good because:

* it's the simplest solution that handles counting automatically,

* avoids the infinite recursion issue when trait is incorrectly implemented,

but the downsides are:

* it's a breaking change,

* requires manually reverting values in some (infrequent) cases.

I think changing the &mut is a reasonable breaking change. The contents of the implemented methods would mostly not change - occasionally there might need to be a small change in places where the MultiValue was moved.

I think this last suggestion to change from_lua_multi to take a &mut MultiValue is the cleanest and avoids most pitfalls. I would also accompany this with changing from_lua to take a reference as well for consistency.

I don't see any reason to change from_lua though - Value is already really a kind of reference, and I don't see a benefit to breaking existing impls here.

[Caellian]

I added some small utility functions to MultiValue as well so it's easier to pop and revert values from implementations.

[jugglerchris]

This is looking great, thanks! I just noticed one out of date comment, otherwise I think this is good to merge. Thanks for your patience!

[Caellian]

Give me a few days to update my binding code and maybe add some tests. There have been a few minor semantic changes I did since I last properly tested the entire modified surface area so I'd just like to make sure stuff like Variadic and Vec do what's expected.
I'd also like to update the original PR message because there's a few tiny bits that are outdated.

[Caellian]

@jugglerchris Tested it, it seems to work so it's green flag from me as well.

I added a From<*> implementation for all Value::* variants so that MultiValue::push_front can now take any Into<Value> which makes it less verbose to use.

The implementation with a counter reference was a bit simpler to implement (less code), but I see how it could've been confusing for people.

Sorry it took me this much time, I had a lot of colloquiums and seminars at the uni I had to prepare for all at once and work.

[khvzak]

Thanks for the PR!
Currently rlua is merging with mlua (crates.io), see #294 for the proposal and PR.
The v0.20 will be the last rlua release with re-export of mlua types + compat trait.

I would recommend to swith your project to mlua and open the issue/discussion about this PR in the mlua repo to discuss possible options. mlua uses a modified version of FromLuaMulti trait to utilize Lua stack without MultiValue layer where possible to achieve high performance and zero costs.