bevy_reflect: Improve `DynamicFunction` ergonomics
Objective
Many functions can be converted to DynamicFunction using IntoFunction. Unfortunately, we are limited by Rust itself and the implementations are far from exhaustive. For example, we can't convert functions with more than 16 arguments. Additionally, we can't handle returns with lifetimes not tied to the lifetime of the first argument.
In such cases, users will have to create their DynamicFunction manually.
Let's take the following function:
fn get(index: usize, list: &Vec<String>) -> &String {
&list[index]
}
This function cannot be converted to a DynamicFunction via IntoFunction due to the lifetime of the return value being tied to the second argument. Therefore, we need to construct the DynamicFunction manually:
DynamicFunction::new(
|mut args, info| {
let list = args
.pop()
.unwrap()
.take_ref::<Vec<String>>(&info.args()[1])?;
let index = args.pop().unwrap().take_owned::<usize>(&info.args()[0])?;
Ok(Return::Ref(get(index, list)))
},
FunctionInfo::new()
.with_name("get")
.with_args(vec![
ArgInfo::new::<usize>(0).with_name("index"),
ArgInfo::new::<&Vec<String>>(1).with_name("list"),
])
.with_return_info(ReturnInfo::new::<&String>()),
);
While still a small and straightforward snippet, there's a decent amount going on here. There's a lot of room for improvements when it comes to ergonomics and readability.
The goal of this PR is to address those issues.
Solution
Improve the ergonomics and readability of manually created DynamicFunctions.
Some of the major changes:
- Removed the need for
&ArgInfowhen reifying arguments (i.e. the&info.args()[1]calls) - Added additional
popmethods onArgListto handle both popping and casting - Added
takemethods onArgListfor taking the arguments out in order - Removed the need for
&FunctionInfoin the internal closure (Change 1 made it no longer necessary) - Added methods to automatically handle generating
ArgInfoandReturnInfo
With all these changes in place, we get something a lot nicer to both write and look at:
DynamicFunction::new(
|mut args| {
let index = args.take::<usize>()?;
let list = args.take::<&Vec<String>>()?;
Ok(Return::Ref(get(index, list)))
},
FunctionInfo::new()
.with_name("get")
.with_arg::<usize>("index")
.with_arg::<&Vec<String>>("list")
.with_return::<&String>(),
);
Alternatively, to rely on type inference for taking arguments, you could do:
DynamicFunction::new(
|mut args| {
let index = args.take_owned()?;
let list = args.take_ref()?;
Ok(Return::Ref(get(index, list)))
},
FunctionInfo::new()
.with_name("get")
.with_arg::<usize>("index")
.with_arg::<&Vec<String>>("list")
.with_return::<&String>(),
);
Testing
You can test locally by running:
cargo test --package bevy_reflect
Changelog
- Removed
&ArgInfoargument fromFromArg::from_argtrait method - Removed
&ArgInfoargument fromArg::take_***methods - Added
ArgValue Argis now a struct containing anArgValueand an argumentindexArg::take_***methods now requireTis alsoTypePath- Added
Arg::new,Arg::index,Arg::value,Arg::take_value, andArg::takemethods - Replaced
ArgIdinArgErrorwith just the argumentindex - Added
ArgError::EmptyArgList - Renamed
ArgList::pushtoArgList::push_arg - Added
ArgList::pop_arg,ArgList::pop_owned,ArgList::pop_ref, andArgList::pop_mut - Added
ArgList::take_arg,ArgList::take_owned,ArgList::take_ref,ArgList::take_mut, andArgList::take ArgList::popis now generic- Renamed
FunctionError::InvalidArgCounttoFunctionError::ArgCountMismatch - The closure given to
DynamicFunction::newno longer has a&FunctionInfoargument - Added
FunctionInfo::with_arg - Added
FunctionInfo::with_return
Internal Migration Guide
[!important] Function reflection was introduced as part of the 0.15 dev cycle. This migration guide was written for developers relying on
mainduring this cycle, and is not a breaking change coming from 0.14.
- The
FromArg::from_argtrait method and theArg::take_***methods no longer take a&ArgInfoargument. - What used to be
Argis nowArgValue.Argis now a struct which contains anArgValue. Arg::take_***methods now requireTis alsoTypePath- Instances of
id: ArgIdinArgErrorhave been replaced withindex: usize ArgList::pushis nowArgList::push_arg. It also takes the newArgValuetype.ArgList::pophas becomeArgList::pop_argand now returnsArgValue.Arg::popnow takes a generic type and downcasts to that type. It's recommended to useArgList::takeand friends instead since they allow removing the arguments from the list in the order they were pushed (rather than reverse order).FunctionError::InvalidArgCountis nowFunctionError::ArgCountMismatch- The closure given to
DynamicFunction::newno longer has a&FunctionInfoargument. This argument can be removed.
Clippy's not happy with my choice of naming in ArgList::next.
Should we rename? If so, to what?
So far on Discord, we've come up with the following alternatives:
taketake_nexttake_firstpull(I think this one is my favorite)pop_front(and probably renamepoptopop_back)
Keep in mind that we also need _arg, _owned, _ref, and _mut variants of whatever method name we choose.
We might also want to consider whether we want to keep ArgList::pop or remove it altogether.
I just went with ArgList::take for now. Let me know if there are any other opinions on this!