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

Allow composition of FromLuaMulti #365

Open
Caellian opened this issue Feb 6, 2024 · 6 comments
Open

Allow composition of FromLuaMulti #365

Caellian opened this issue Feb 6, 2024 · 6 comments

Comments

@Caellian
Copy link

Caellian commented Feb 6, 2024

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:

  • draw_line({x, y}, {x, y}, r, g, b)
  • draw_line(start_x, start_y, end_x, end_y, r, g, b)
  • draw_line(start_x, start_y, end_x, end_y, r, g, b, a)
  • draw_line(start_x, start_y, end_x, end_y, h, s, l, a)
  • draw_line(start_x, start_y, length, direction, r, g, b, a)

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
Copy link
Member

khvzak commented Feb 6, 2024

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
Copy link
Member

khvzak commented Feb 6, 2024

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
Copy link
Author

Caellian commented Feb 7, 2024

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
Copy link
Author

Caellian commented Feb 7, 2024

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
Copy link
Author

Caellian commented Feb 7, 2024

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
Copy link
Author

Caellian commented Feb 23, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants