-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
bevy_reflect: Function reflection #13152
bevy_reflect: Function reflection #13152
Conversation
888544b
to
7af7456
Compare
Hm, I'm debating on whether or not I should rename |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few comments but overall this looks good to me!
The example at the end is fantastic, and overall the new functionality is impressive
} | ||
} | ||
|
||
impl #impl_generics #bevy_reflect::func::args::FromArg for &'static #type_path #ty_generics #where_reflect_clause { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has to be &'static
because the output's 'from_arg
could be any lifetime?
Could we have an impl for any lifetime longer than 'from_arg
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'from_arg
will take the lifetime of the argument, so it could be any lifetime, including 'static
. The &'static
is just because we don't actually care about the lifetime of Self
. I think you could even do impl<'a> FromArg for &'a Foo
, but again we don't care about that lifetime, just the one for Item
.
/// | ||
/// [`Function`]: crate::func::Function | ||
#[derive(Default, Debug)] | ||
pub struct ArgList<'a>(Vec<Arg<'a>>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is why all the arguments need to have the same lifetime? Maybe it would be worth mentioning in a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty much. In one iteration I had originally used an enum in an effort to avoid Vec
allocations:
enum ArgList<'a> {
Arg0,
Arg1(Arg<'a>),
Arg2(Arg<'a>, Arg<'a>),
Arg3(Arg<'a>, Arg<'a>, Arg<'a>),
// ...
Variadic(Vec<Arg<'a>>)
}
If we did that, we potentially could maintain some degree of lifetimes by just adding a bunch of lifetimes to ArgList
:
enum ArgList<'a, 'b, 'c, 'default: 'a + 'b + 'c> {
Arg0,
Arg1(Arg<'a>),
Arg2(Arg<'a>, Arg<'b>),
Arg3(Arg<'a>, Arg<'b>, Arg<'c>),
// ...
Variadic(Vec<Arg<'default>>)
}
The output would still only be able to tie itself to the first argument's lifetime, but doing this would mean that we shouldn't need to shrink lifetimes down (most of the time).
So that might be something to explore.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the enum ArgList
might work; and it wouldn't even require the Variadic
variant no?
Every new arg added would update the ArgList
from ArgN
to ArgN+1
.
Might be something to explore, but I think it's also ok to merge the functionality as is and explore this in a future PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the enum
ArgList
might work; and it wouldn't even require theVariadic
variant no?
The Variadic
would be needed so users could supply more arguments than we have variants for. While IntoFunction
supports a limited number of arguments, users are still able to construct their Function
manually, with as many arguments as they want.
Might be something to explore, but I think it's also ok to merge the functionality as is and explore this in a future PR
Yeah that might be a good idea. Any breakage would be minimal and really only apply to cases where the lifetime of ArgList
is explicitly set by a user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah definitely going to save this for a followup PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, please leave this to followup.
a33b961
to
5406c74
Compare
1be1ad3
to
6a536e8
Compare
Removed the `args` parameter, opting fully into the `with_***` pattern
into -> into_function
Co-authored-by: Periwink <charlesbour@gmail.com>
6a536e8
to
a70dd8a
Compare
// The `Return` value can be pattern matched or unwrapped to get the underlying reflection data. | ||
// For the sake of brevity, we'll just unwrap it here. | ||
let result: Box<dyn Reflect> = result.unwrap_owned(); | ||
assert_eq!(result.take::<i32>().unwrap(), 4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment here about what take
does would be very useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose to briefly acknowledge it in a comment since it's less a function reflection thing and more of a general reflection thing. Let me know if I should still call it out specifically though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fundamentally on board with both the design and functionality here, and the implementation is solid.
Docs are good, but I've left some suggestions on where we can strengthen them. Let me know when you're done with that and I'll give you my stamp of approval.
3c0350f
to
b8ed0a8
Compare
Closures no longer need to be `'static` as we now track the lifetime of the wrapped function
This takes after similar concepts in bevy_ecs
Updated the PR description with the new |
Two updates:
|
Talked about this on Discord with @NthTensor, but I think I may explore an |
9ab4702
to
91f249f
Compare
# Objective We're able to reflect types sooooooo... why not functions? The goal of this PR is to make functions callable within a dynamic context, where type information is not readily available at compile time. For example, if we have a function: ```rust fn add(left: i32, right: i32) -> i32 { left + right } ``` And two `Reflect` values we've already validated are `i32` types: ```rust let left: Box<dyn Reflect> = Box::new(2_i32); let right: Box<dyn Reflect> = Box::new(2_i32); ``` We should be able to call `add` with these values: ```rust // ????? let result: Box<dyn Reflect> = add.call_dynamic(left, right); ``` And ideally this wouldn't just work for functions, but methods and closures too! Right now, users have two options: 1. Manually parse the reflected data and call the function themselves 2. Rely on registered type data to handle the conversions for them For a small function like `add`, this isn't too bad. But what about for more complex functions? What about for many functions? At worst, this process is error-prone. At best, it's simply tedious. And this is assuming we know the function at compile time. What if we want to accept a function dynamically and call it with our own arguments? It would be much nicer if `bevy_reflect` could alleviate some of the problems here. ## Solution Added function reflection! This adds a `DynamicFunction` type to wrap a function dynamically. This can be called with an `ArgList`, which is a dynamic list of `Reflect`-containing `Arg` arguments. It returns a `FunctionResult` which indicates whether or not the function call succeeded, returning a `Reflect`-containing `Return` type if it did succeed. Many functions can be converted into this `DynamicFunction` type thanks to the `IntoFunction` trait. Taking our previous `add` example, this might look something like (explicit types added for readability): ```rust fn add(left: i32, right: i32) -> i32 { left + right } let mut function: DynamicFunction = add.into_function(); let args: ArgList = ArgList::new().push_owned(2_i32).push_owned(2_i32); let result: Return = function.call(args).unwrap(); let value: Box<dyn Reflect> = result.unwrap_owned(); assert_eq!(value.take::<i32>().unwrap(), 4); ``` And it also works on closures: ```rust let add = |left: i32, right: i32| left + right; let mut function: DynamicFunction = add.into_function(); let args: ArgList = ArgList::new().push_owned(2_i32).push_owned(2_i32); let result: Return = function.call(args).unwrap(); let value: Box<dyn Reflect> = result.unwrap_owned(); assert_eq!(value.take::<i32>().unwrap(), 4); ``` As well as methods: ```rust #[derive(Reflect)] struct Foo(i32); impl Foo { fn add(&mut self, value: i32) { self.0 += value; } } let mut foo = Foo(2); let mut function: DynamicFunction = Foo::add.into_function(); let args: ArgList = ArgList::new().push_mut(&mut foo).push_owned(2_i32); function.call(args).unwrap(); assert_eq!(foo.0, 4); ``` ### Limitations While this does cover many functions, it is far from a perfect system and has quite a few limitations. Here are a few of the limitations when using `IntoFunction`: 1. The lifetime of the return value is only tied to the lifetime of the first argument (useful for methods). This means you can't have a function like `(a: i32, b: &i32) -> &i32` without creating the `DynamicFunction` manually. 2. Only 15 arguments are currently supported. If the first argument is a (mutable) reference, this number increases to 16. 3. Manual implementations of `Reflect` will need to implement the new `FromArg`, `GetOwnership`, and `IntoReturn` traits in order to be used as arguments/return types. And some limitations of `DynamicFunction` itself: 1. All arguments share the same lifetime, or rather, they will shrink to the shortest lifetime. 2. Closures that capture their environment may need to have their `DynamicFunction` dropped before accessing those variables again (there is a `DynamicFunction::call_once` to make this a bit easier) 3. All arguments and return types must implement `Reflect`. While not a big surprise coming from `bevy_reflect`, this implementation could actually still work by swapping `Reflect` out with `Any`. Of course, that makes working with the arguments and return values a bit harder. 4. Generic functions are not supported (unless they have been manually monomorphized) And general, reflection gotchas: 1. `&str` does not implement `Reflect`. Rather, `&'static str` implements `Reflect` (the same is true for `&Path` and similar types). This means that `&'static str` is considered an "owned" value for the sake of generating arguments. Additionally, arguments and return types containing `&str` will assume it's `&'static str`, which is almost never the desired behavior. In these cases, the only solution (I believe) is to use `&String` instead. ### Followup Work This PR is the first of two PRs I intend to work on. The second PR will aim to integrate this new function reflection system into the existing reflection traits and `TypeInfo`. The goal would be to register and call a reflected type's methods dynamically. I chose not to do that in this PR since the diff is already quite large. I also want the discussion for both PRs to be focused on their own implementation. Another followup I'd like to do is investigate allowing common container types as a return type, such as `Option<&[mut] T>` and `Result<&[mut] T, E>`. This would allow even more functions to opt into this system. I chose to not include it in this one, though, for the same reasoning as previously mentioned. ### Alternatives One alternative I had considered was adding a macro to convert any function into a reflection-based counterpart. The idea would be that a struct that wraps the function would be created and users could specify which arguments and return values should be `Reflect`. It could then be called via a new `Function` trait. I think that could still work, but it will be a fair bit more involved, requiring some slightly more complex parsing. And it of course is a bit more work for the user, since they need to create the type via macro invocation. It also makes registering these functions onto a type a bit more complicated (depending on how it's implemented). For now, I think this is a fairly simple, yet powerful solution that provides the least amount of friction for users. --- ## Showcase Bevy now adds support for storing and calling functions dynamically using reflection! ```rust // 1. Take a standard Rust function fn add(left: i32, right: i32) -> i32 { left + right } // 2. Convert it into a type-erased `DynamicFunction` using the `IntoFunction` trait let mut function: DynamicFunction = add.into_function(); // 3. Define your arguments from reflected values let args: ArgList = ArgList::new().push_owned(2_i32).push_owned(2_i32); // 4. Call the function with your arguments let result: Return = function.call(args).unwrap(); // 5. Extract the return value let value: Box<dyn Reflect> = result.unwrap_owned(); assert_eq!(value.take::<i32>().unwrap(), 4); ``` ## Changelog #### TL;DR - Added support for function reflection - Added a new `Function Reflection` example: https://github.com/bevyengine/bevy/blob/ba727898f2adff817838fc4cdb49871bbce37356/examples/reflection/function_reflection.rs#L1-L157 #### Details Added the following items: - `ArgError` enum - `ArgId` enum - `ArgInfo` struct - `ArgList` struct - `Arg` enum - `DynamicFunction` struct - `FromArg` trait (derived with `derive(Reflect)`) - `FunctionError` enum - `FunctionInfo` struct - `FunctionResult` alias - `GetOwnership` trait (derived with `derive(Reflect)`) - `IntoFunction` trait (with blanket implementation) - `IntoReturn` trait (derived with `derive(Reflect)`) - `Ownership` enum - `ReturnInfo` struct - `Return` enum --------- Co-authored-by: Periwink <charlesbour@gmail.com>
# Objective We're able to reflect types sooooooo... why not functions? The goal of this PR is to make functions callable within a dynamic context, where type information is not readily available at compile time. For example, if we have a function: ```rust fn add(left: i32, right: i32) -> i32 { left + right } ``` And two `Reflect` values we've already validated are `i32` types: ```rust let left: Box<dyn Reflect> = Box::new(2_i32); let right: Box<dyn Reflect> = Box::new(2_i32); ``` We should be able to call `add` with these values: ```rust // ????? let result: Box<dyn Reflect> = add.call_dynamic(left, right); ``` And ideally this wouldn't just work for functions, but methods and closures too! Right now, users have two options: 1. Manually parse the reflected data and call the function themselves 2. Rely on registered type data to handle the conversions for them For a small function like `add`, this isn't too bad. But what about for more complex functions? What about for many functions? At worst, this process is error-prone. At best, it's simply tedious. And this is assuming we know the function at compile time. What if we want to accept a function dynamically and call it with our own arguments? It would be much nicer if `bevy_reflect` could alleviate some of the problems here. ## Solution Added function reflection! This adds a `DynamicFunction` type to wrap a function dynamically. This can be called with an `ArgList`, which is a dynamic list of `Reflect`-containing `Arg` arguments. It returns a `FunctionResult` which indicates whether or not the function call succeeded, returning a `Reflect`-containing `Return` type if it did succeed. Many functions can be converted into this `DynamicFunction` type thanks to the `IntoFunction` trait. Taking our previous `add` example, this might look something like (explicit types added for readability): ```rust fn add(left: i32, right: i32) -> i32 { left + right } let mut function: DynamicFunction = add.into_function(); let args: ArgList = ArgList::new().push_owned(2_i32).push_owned(2_i32); let result: Return = function.call(args).unwrap(); let value: Box<dyn Reflect> = result.unwrap_owned(); assert_eq!(value.take::<i32>().unwrap(), 4); ``` And it also works on closures: ```rust let add = |left: i32, right: i32| left + right; let mut function: DynamicFunction = add.into_function(); let args: ArgList = ArgList::new().push_owned(2_i32).push_owned(2_i32); let result: Return = function.call(args).unwrap(); let value: Box<dyn Reflect> = result.unwrap_owned(); assert_eq!(value.take::<i32>().unwrap(), 4); ``` As well as methods: ```rust #[derive(Reflect)] struct Foo(i32); impl Foo { fn add(&mut self, value: i32) { self.0 += value; } } let mut foo = Foo(2); let mut function: DynamicFunction = Foo::add.into_function(); let args: ArgList = ArgList::new().push_mut(&mut foo).push_owned(2_i32); function.call(args).unwrap(); assert_eq!(foo.0, 4); ``` ### Limitations While this does cover many functions, it is far from a perfect system and has quite a few limitations. Here are a few of the limitations when using `IntoFunction`: 1. The lifetime of the return value is only tied to the lifetime of the first argument (useful for methods). This means you can't have a function like `(a: i32, b: &i32) -> &i32` without creating the `DynamicFunction` manually. 2. Only 15 arguments are currently supported. If the first argument is a (mutable) reference, this number increases to 16. 3. Manual implementations of `Reflect` will need to implement the new `FromArg`, `GetOwnership`, and `IntoReturn` traits in order to be used as arguments/return types. And some limitations of `DynamicFunction` itself: 1. All arguments share the same lifetime, or rather, they will shrink to the shortest lifetime. 2. Closures that capture their environment may need to have their `DynamicFunction` dropped before accessing those variables again (there is a `DynamicFunction::call_once` to make this a bit easier) 3. All arguments and return types must implement `Reflect`. While not a big surprise coming from `bevy_reflect`, this implementation could actually still work by swapping `Reflect` out with `Any`. Of course, that makes working with the arguments and return values a bit harder. 4. Generic functions are not supported (unless they have been manually monomorphized) And general, reflection gotchas: 1. `&str` does not implement `Reflect`. Rather, `&'static str` implements `Reflect` (the same is true for `&Path` and similar types). This means that `&'static str` is considered an "owned" value for the sake of generating arguments. Additionally, arguments and return types containing `&str` will assume it's `&'static str`, which is almost never the desired behavior. In these cases, the only solution (I believe) is to use `&String` instead. ### Followup Work This PR is the first of two PRs I intend to work on. The second PR will aim to integrate this new function reflection system into the existing reflection traits and `TypeInfo`. The goal would be to register and call a reflected type's methods dynamically. I chose not to do that in this PR since the diff is already quite large. I also want the discussion for both PRs to be focused on their own implementation. Another followup I'd like to do is investigate allowing common container types as a return type, such as `Option<&[mut] T>` and `Result<&[mut] T, E>`. This would allow even more functions to opt into this system. I chose to not include it in this one, though, for the same reasoning as previously mentioned. ### Alternatives One alternative I had considered was adding a macro to convert any function into a reflection-based counterpart. The idea would be that a struct that wraps the function would be created and users could specify which arguments and return values should be `Reflect`. It could then be called via a new `Function` trait. I think that could still work, but it will be a fair bit more involved, requiring some slightly more complex parsing. And it of course is a bit more work for the user, since they need to create the type via macro invocation. It also makes registering these functions onto a type a bit more complicated (depending on how it's implemented). For now, I think this is a fairly simple, yet powerful solution that provides the least amount of friction for users. --- ## Showcase Bevy now adds support for storing and calling functions dynamically using reflection! ```rust // 1. Take a standard Rust function fn add(left: i32, right: i32) -> i32 { left + right } // 2. Convert it into a type-erased `DynamicFunction` using the `IntoFunction` trait let mut function: DynamicFunction = add.into_function(); // 3. Define your arguments from reflected values let args: ArgList = ArgList::new().push_owned(2_i32).push_owned(2_i32); // 4. Call the function with your arguments let result: Return = function.call(args).unwrap(); // 5. Extract the return value let value: Box<dyn Reflect> = result.unwrap_owned(); assert_eq!(value.take::<i32>().unwrap(), 4); ``` ## Changelog #### TL;DR - Added support for function reflection - Added a new `Function Reflection` example: https://github.com/bevyengine/bevy/blob/ba727898f2adff817838fc4cdb49871bbce37356/examples/reflection/function_reflection.rs#L1-L157 #### Details Added the following items: - `ArgError` enum - `ArgId` enum - `ArgInfo` struct - `ArgList` struct - `Arg` enum - `DynamicFunction` struct - `FromArg` trait (derived with `derive(Reflect)`) - `FunctionError` enum - `FunctionInfo` struct - `FunctionResult` alias - `GetOwnership` trait (derived with `derive(Reflect)`) - `IntoFunction` trait (with blanket implementation) - `IntoReturn` trait (derived with `derive(Reflect)`) - `Ownership` enum - `ReturnInfo` struct - `Return` enum --------- Co-authored-by: Periwink <charlesbour@gmail.com>
# Objective Looks like I accidentally disabled the reflection compile fail tests in #13152. These should be re-enabled. ## Solution Re-enable reflection compile fail tests. ## Testing CI should pass. You can also test locally by navigating to `crates/bevy_reflect/compile_fail/` and running: ``` cargo test --target-dir ../../../target ```
Objective
We're able to reflect types sooooooo... why not functions?
The goal of this PR is to make functions callable within a dynamic context, where type information is not readily available at compile time.
For example, if we have a function:
And two
Reflect
values we've already validated arei32
types:We should be able to call
add
with these values:And ideally this wouldn't just work for functions, but methods and closures too!
Right now, users have two options:
For a small function like
add
, this isn't too bad. But what about for more complex functions? What about for many functions?At worst, this process is error-prone. At best, it's simply tedious.
And this is assuming we know the function at compile time. What if we want to accept a function dynamically and call it with our own arguments?
It would be much nicer if
bevy_reflect
could alleviate some of the problems here.Solution
Added function reflection!
This adds a
DynamicFunction
type to wrap a function dynamically. This can be called with anArgList
, which is a dynamic list ofReflect
-containingArg
arguments. It returns aFunctionResult
which indicates whether or not the function call succeeded, returning aReflect
-containingReturn
type if it did succeed.Many functions can be converted into this
DynamicFunction
type thanks to theIntoFunction
trait.Taking our previous
add
example, this might look something like (explicit types added for readability):And it also works on closures:
As well as methods:
Limitations
While this does cover many functions, it is far from a perfect system and has quite a few limitations. Here are a few of the limitations when using
IntoFunction
:(a: i32, b: &i32) -> &i32
without creating theDynamicFunction
manually.Reflect
will need to implement the newFromArg
,GetOwnership
, andIntoReturn
traits in order to be used as arguments/return types.And some limitations of
DynamicFunction
itself:DynamicFunction
dropped before accessing those variables again (there is aDynamicFunction::call_once
to make this a bit easier)Reflect
. While not a big surprise coming frombevy_reflect
, this implementation could actually still work by swappingReflect
out withAny
. Of course, that makes working with the arguments and return values a bit harder.And general, reflection gotchas:
&str
does not implementReflect
. Rather,&'static str
implementsReflect
(the same is true for&Path
and similar types). This means that&'static str
is considered an "owned" value for the sake of generating arguments. Additionally, arguments and return types containing&str
will assume it's&'static str
, which is almost never the desired behavior. In these cases, the only solution (I believe) is to use&String
instead.Followup Work
This PR is the first of two PRs I intend to work on. The second PR will aim to integrate this new function reflection system into the existing reflection traits and
TypeInfo
. The goal would be to register and call a reflected type's methods dynamically.I chose not to do that in this PR since the diff is already quite large. I also want the discussion for both PRs to be focused on their own implementation.
Another followup I'd like to do is investigate allowing common container types as a return type, such as
Option<&[mut] T>
andResult<&[mut] T, E>
. This would allow even more functions to opt into this system. I chose to not include it in this one, though, for the same reasoning as previously mentioned.Alternatives
One alternative I had considered was adding a macro to convert any function into a reflection-based counterpart. The idea would be that a struct that wraps the function would be created and users could specify which arguments and return values should be
Reflect
. It could then be called via a newFunction
trait.I think that could still work, but it will be a fair bit more involved, requiring some slightly more complex parsing. And it of course is a bit more work for the user, since they need to create the type via macro invocation.
It also makes registering these functions onto a type a bit more complicated (depending on how it's implemented).
For now, I think this is a fairly simple, yet powerful solution that provides the least amount of friction for users.
Showcase
Bevy now adds support for storing and calling functions dynamically using reflection!
Changelog
TL;DR
Function Reflection
example:bevy/examples/reflection/function_reflection.rs
Lines 1 to 157 in ba72789
Details
Added the following items:
ArgError
enumArgId
enumArgInfo
structArgList
structArg
enumDynamicFunction
structFromArg
trait (derived withderive(Reflect)
)FunctionError
enumFunctionInfo
structFunctionResult
aliasGetOwnership
trait (derived withderive(Reflect)
)IntoFunction
trait (with blanket implementation)IntoReturn
trait (derived withderive(Reflect)
)Ownership
enumReturnInfo
structReturn
enum