mlua-rs / mlua

High level Lua 5.4/5.3/5.2/5.1 (including LuaJIT) and Roblox Luau bindings to Rust with async/await support
Other
1.54k stars 132 forks source link

Allow composition of FromLuaMulti #365

Open Caellian opened 6 months ago

Caellian commented 6 months ago

I'm bringing a suggestion from my rlua PR (and piccolo) that proposes adding the ability to compose argument parsers. Motivation for this is simplifying function overloading from bound lua methods. There's a lot of reasons why function overloading can be useful, but the way mlua handles arguments (inherited from rlua) makes is very verbose to correctly handle it.

The idea is to allow dependents to declare a struct representing a parameter pack (e.g. Color, Position) and provide an interface to parse that parameter pack as a part of function argument list.

A simplified example is providing a function that binds to some drawing API in order to provide means of drawing a line. Here the arguments would be start position, end position and a color. In order to provide a nicer interface, the function could take parameters in following forms:

As visible from those 5 examples, number of combinations of different arguments can be huge for just "3" arguments and it's not practical to handle all the cases in a single method body, especially given that this is just a single function and larger bindings can have several dozen of those.

What I think is the best (DRYest) approach here, is adding a trait that allows returning converted T and a usize telling the outer scope how many arguments were consumed i.e. how many arguments the outer iterator should advance by.

I implemented this functionality 2/3 times for rlua already while I was discussing the API with the maintainer, so I'm not particularity eager to do it again myself. I'll probably write a trait that handles the conversion in a way I find best suitable for my use case and link it here, but feel free to modify my original PR to fit the mlua API.

khvzak commented 6 months ago

What I think is the best (DRYest) approach here, is adding a trait that allows returning converted T and a usize telling the outer scope how many arguments were consumed i.e. how many arguments the outer iterator should advance by.

Wondering, how this would work in a case when we have a backward dependency and need to go back and start converting arguments again? Let's say we have a simple interface do_something(a, b), where rules of parsing a are depends on b, so a can represent few different possible types, but only b know how exactly a should be represented.

khvzak commented 6 months ago

There are some other possible ambiguities as well. mlua currently tells what argument (number) is wrong in a function call, eg. for a function do_something(a: string, b: bool) if call with ("abc", nil) you will get a nice error message that argument#2 is wrong (expected bool, provided nil). rlua does not do this but it's quite convenient feedback for users. I'm not sure yet how this should work for overloaded functions. Let's say for functions:

draw_line({x, y}, _: string)
draw_line({x, y}, _: table)

calling with draw_line({1,2}, nil) does not give a right answer what argument was really expected, string or table.

Caellian commented 6 months ago

how this would work in a case when we have a backward dependency

They would need to be grouped and parsed together. If they're adjacent, they can be packed into a composed ABArgPack{A, B} where I guess A would be an enum or something.

I have a similar case with my skia bindings where FilterMode and MipmapMode are two lua strings but have overlapping values so they need to be parsed together like this, because both are optional, and the conversion has to fail due to ambiguity in case only a single string is provided.

Manual parsing would be simpler only in the case where there's a bunch of arguments between a and b, but I think the case where dependent arguments are spread apart is far less common than them being next to each other.

Caellian commented 6 months ago

mlua currently tells what argument (number) is wrong in a function call [...] I'm not sure yet how this should work for overloaded functions.

My current WIP trait looks like this:

pub trait FromArgPack<'lua>: Sized {
    fn convert(args: &mut Vec<Value<'lua>>, lua: &'lua Lua) -> Result<Self, mlua::Error>;
}

This allows the implementor to return Error::BadArgument for the appropriate argument if it doesn't match the expected type/value. I guess convert should also take the starting index where FromArgPack was initially applied in order to compute absolute argument position correctly.

As for the error message, I think something along the lines of "expected Nth argument to be a string or table" would work fine. Languages that support function overloading provide the same level of information (as well as listing different signatures). In some cases a message like "expected a table, or inlined RGB or RGBA values" might be more appropriate. That's up to the implementer though, for your backwards dependency example a more appropriate message would be "because b was X, expected a to be Y" and returning the pos of a argument.

I would argue that an error returned from FromArgPack implementation potentially has more semantics to work with in order to provide meaningful error messages, while with FromLuaMulti it's more likely the developer will copy-paste inconsistent/generic error messages because they'd have to write them in all places they're using different argument groups.

For reference, here is an example of errors I previously returned. Adding a position is basically just counting how many arguments have been previously processed. Outer scope can fill in any missing information (e.g. BadArgument::to).

Caellian commented 6 months ago

As I said earlier, here is an untested implementation that builds over existing types. It would be more convenient of course if UserDataMethods didn't require an extension trait, but the linked code showcases what this code would look like as part of mlua codebase. It's not 100% there yet and some things are missing, but that's it for the most part.

Caellian commented 6 months ago

I finished migration to mlua and here is the code related to argument conversion. I ended up taking MultiValue, writing my own conversion logic and added a macro to generate UserData implementation from regular implementation blocks.

The macro is also responsible for passing function and argument names from rust into error messages.

I grant permission to this project (mlua) and its contributors to copy parts (clunky/mlua-skia) of my code (as needed), into the mlua project, under the more permissive MIT license.

I might create a PR once I'm done with exams, I think mlua would benefit from a lot of linked code.