bevy_reflect: Improve DynamicFunction ergonomics (#14201)

# 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:

```rust
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:

```rust
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
`DynamicFunction`s.

Some of the major changes:
1. Removed the need for `&ArgInfo` when reifying arguments (i.e. the
`&info.args()[1]` calls)
2. Added additional `pop` methods on `ArgList` to handle both popping
and casting
3. Added `take` methods on `ArgList` for taking the arguments out in
order
4. Removed the need for `&FunctionInfo` in the internal closure (Change
1 made it no longer necessary)
5. Added methods to automatically handle generating `ArgInfo` and
`ReturnInfo`

With all these changes in place, we get something a lot nicer to both
write and look at:

```rust
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:

```rust
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 `&ArgInfo` argument from `FromArg::from_arg` trait method
- Removed `&ArgInfo` argument from `Arg::take_***` methods
- Added `ArgValue`
- `Arg` is now a struct containing an `ArgValue` and an argument `index`
- `Arg::take_***` methods now require `T` is also `TypePath`
- Added `Arg::new`, `Arg::index`, `Arg::value`, `Arg::take_value`, and
`Arg::take` methods
- Replaced `ArgId` in `ArgError` with just the argument `index`
- Added `ArgError::EmptyArgList`
- Renamed `ArgList::push` to `ArgList::push_arg`
- Added `ArgList::pop_arg`, `ArgList::pop_owned`, `ArgList::pop_ref`,
and `ArgList::pop_mut`
- Added `ArgList::take_arg`, `ArgList::take_owned`, `ArgList::take_ref`,
`ArgList::take_mut`, and `ArgList::take`
- `ArgList::pop` is now generic
- Renamed `FunctionError::InvalidArgCount` to
`FunctionError::ArgCountMismatch`
- The closure given to `DynamicFunction::new` no longer has a
`&FunctionInfo` argument
- 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 `main` during this
cycle, and is not a breaking change coming from 0.14.

* The `FromArg::from_arg` trait method and the `Arg::take_***` methods
no longer take a `&ArgInfo` argument.
* What used to be `Arg` is now `ArgValue`. `Arg` is now a struct which
contains an `ArgValue`.
* `Arg::take_***` methods now require `T` is also `TypePath`
* Instances of `id: ArgId` in `ArgError` have been replaced with `index:
usize`
* `ArgList::push` is now `ArgList::push_arg`. It also takes the new
`ArgValue` type.
* `ArgList::pop` has become `ArgList::pop_arg` and now returns
`ArgValue`. `Arg::pop` now takes a generic type and downcasts to that
type. It's recommended to use `ArgList::take` and friends instead since
they allow removing the arguments from the list in the order they were
pushed (rather than reverse order).
* `FunctionError::InvalidArgCount` is now
`FunctionError::ArgCountMismatch`
* The closure given to `DynamicFunction::new` no longer has a
`&FunctionInfo` argument. This argument can be removed.

Author: MrGVSV

crates/bevy_reflect/derive/src/impls/func/from_arg.rs

@@ -15,32 +15,23 @@ pub(crate) fn impl_from_arg(
 
     quote! {
         impl #impl_generics #bevy_reflect::func::args::FromArg for #type_path #ty_generics #where_reflect_clause {
-            type Item<'from_arg> = #type_path #ty_generics;
-            fn from_arg<'from_arg>(
-                arg: #bevy_reflect::func::args::Arg<'from_arg>,
-                info: &#bevy_reflect::func::args::ArgInfo,
-            ) -> #FQResult<Self::Item<'from_arg>, #bevy_reflect::func::args::ArgError> {
-                arg.take_owned(info)
+            type This<'from_arg> = #type_path #ty_generics;
+            fn from_arg(arg: #bevy_reflect::func::args::Arg) -> #FQResult<Self::This<'_>, #bevy_reflect::func::args::ArgError> {
+                arg.take_owned()
             }
         }
 
         impl #impl_generics #bevy_reflect::func::args::FromArg for &'static #type_path #ty_generics #where_reflect_clause {
-            type Item<'from_arg> = &'from_arg #type_path #ty_generics;
-            fn from_arg<'from_arg>(
-                arg: #bevy_reflect::func::args::Arg<'from_arg>,
-                info: &#bevy_reflect::func::args::ArgInfo,
-            ) -> #FQResult<Self::Item<'from_arg>, #bevy_reflect::func::args::ArgError> {
-                arg.take_ref(info)
+            type This<'from_arg> = &'from_arg #type_path #ty_generics;
+            fn from_arg(arg: #bevy_reflect::func::args::Arg) -> #FQResult<Self::This<'_>, #bevy_reflect::func::args::ArgError> {
+                arg.take_ref()
             }
         }
 
         impl #impl_generics #bevy_reflect::func::args::FromArg for &'static mut #type_path #ty_generics #where_reflect_clause {
-            type Item<'from_arg> = &'from_arg mut #type_path #ty_generics;
-            fn from_arg<'from_arg>(
-                arg: #bevy_reflect::func::args::Arg<'from_arg>,
-                info: &#bevy_reflect::func::args::ArgInfo,
-            ) -> #FQResult<Self::Item<'from_arg>, #bevy_reflect::func::args::ArgError> {
-                arg.take_mut(info)
+            type This<'from_arg> = &'from_arg mut #type_path #ty_generics;
+            fn from_arg(arg: #bevy_reflect::func::args::Arg) -> #FQResult<Self::This<'_>, #bevy_reflect::func::args::ArgError> {
+                arg.take_mut()
             }
         }
     }

crates/bevy_reflect/src/func/args/arg.rs

@@ -1,81 +1,200 @@
-use crate::func::args::{ArgError, ArgInfo, Ownership};
-use crate::Reflect;
+use crate::func::args::{ArgError, FromArg, Ownership};
+use crate::{Reflect, TypePath};
+use std::ops::Deref;
 
-/// Represents an argument that can be passed to a [`DynamicFunction`] or [`DynamicClosure`].
+/// Represents an argument that can be passed to a [`DynamicFunction`], [`DynamicClosure`],
+/// or [`DynamicClosureMut`].
 ///
 /// [`DynamicFunction`]: crate::func::DynamicFunction
 /// [`DynamicClosure`]: crate::func::DynamicClosure
+/// [`DynamicClosureMut`]: crate::func::DynamicClosureMut
 #[derive(Debug)]
-pub enum Arg<'a> {
-    Owned(Box<dyn Reflect>),
-    Ref(&'a dyn Reflect),
-    Mut(&'a mut dyn Reflect),
+pub struct Arg<'a> {
+    index: usize,
+    value: ArgValue<'a>,
 }
 
 impl<'a> Arg<'a> {
-    /// Returns `Ok(T)` if the argument is [`Arg::Owned`].
-    pub fn take_owned<T: Reflect>(self, info: &ArgInfo) -> Result<T, ArgError> {
-        match self {
-            Arg::Owned(arg) => arg.take().map_err(|arg| ArgError::UnexpectedType {
-                id: info.id().clone(),
-                expected: ::std::borrow::Cow::Borrowed(info.type_path()),
-                received: ::std::borrow::Cow::Owned(arg.reflect_type_path().to_string()),
+    /// Create a new [`Arg`] with the given index and value.
+    pub fn new(index: usize, value: ArgValue<'a>) -> Self {
+        Self { index, value }
+    }
+
+    /// The index of the argument.
+    pub fn index(&self) -> usize {
+        self.index
+    }
+
+    /// Set the index of the argument.
+    pub(crate) fn set_index(&mut self, index: usize) {
+        self.index = index;
+    }
+
+    /// The value of the argument.
+    pub fn value(&self) -> &ArgValue<'a> {
+        &self.value
+    }
+
+    /// Take the value of the argument.
+    pub fn take_value(self) -> ArgValue<'a> {
+        self.value
+    }
+
+    /// Take the value of the argument and attempt to convert it to a concrete value, `T`.
+    ///
+    /// This is a convenience method for calling [`FromArg::from_arg`] on the argument.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use bevy_reflect::func::ArgList;
+    /// let a = 1u32;
+    /// let b = 2u32;
+    /// let mut c = 3u32;
+    /// let mut args = ArgList::new().push_owned(a).push_ref(&b).push_mut(&mut c);
+    ///
+    /// let a = args.take::<u32>().unwrap();
+    /// assert_eq!(a, 1);
+    ///
+    /// let b = args.take::<&u32>().unwrap();
+    /// assert_eq!(*b, 2);
+    ///
+    /// let c = args.take::<&mut u32>().unwrap();
+    /// assert_eq!(*c, 3);
+    /// ```
+    pub fn take<T: FromArg>(self) -> Result<T::This<'a>, ArgError> {
+        T::from_arg(self)
+    }
+
+    /// Returns `Ok(T)` if the argument is [`ArgValue::Owned`].
+    ///
+    /// If the argument is not owned, returns an error.
+    ///
+    /// It's generally preferred to use [`Self::take`] instead of this method.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use bevy_reflect::func::ArgList;
+    /// let value = 123u32;
+    /// let mut args = ArgList::new().push_owned(value);
+    /// let value = args.take_owned::<u32>().unwrap();
+    /// assert_eq!(value, 123);
+    /// ```
+    pub fn take_owned<T: Reflect + TypePath>(self) -> Result<T, ArgError> {
+        match self.value {
+            ArgValue::Owned(arg) => arg.take().map_err(|arg| ArgError::UnexpectedType {
+                index: self.index,
+                expected: std::borrow::Cow::Borrowed(T::type_path()),
+                received: std::borrow::Cow::Owned(arg.reflect_type_path().to_string()),
             }),
-            Arg::Ref(_) => Err(ArgError::InvalidOwnership {
-                id: info.id().clone(),
+            ArgValue::Ref(_) => Err(ArgError::InvalidOwnership {
+                index: self.index,
                 expected: Ownership::Owned,
                 received: Ownership::Ref,
             }),
-            Arg::Mut(_) => Err(ArgError::InvalidOwnership {
-                id: info.id().clone(),
+            ArgValue::Mut(_) => Err(ArgError::InvalidOwnership {
+                index: self.index,
                 expected: Ownership::Owned,
                 received: Ownership::Mut,
             }),
         }
     }
 
-    /// Returns `Ok(&T)` if the argument is [`Arg::Ref`].
-    pub fn take_ref<T: Reflect>(self, info: &ArgInfo) -> Result<&'a T, ArgError> {
-        match self {
-            Arg::Owned(_) => Err(ArgError::InvalidOwnership {
-                id: info.id().clone(),
+    /// Returns `Ok(&T)` if the argument is [`ArgValue::Ref`].
+    ///
+    /// If the argument is not a reference, returns an error.
+    ///
+    /// It's generally preferred to use [`Self::take`] instead of this method.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use bevy_reflect::func::ArgList;
+    /// let value = 123u32;
+    /// let mut args = ArgList::new().push_ref(&value);
+    /// let value = args.take_ref::<u32>().unwrap();
+    /// assert_eq!(*value, 123);
+    /// ```
+    pub fn take_ref<T: Reflect + TypePath>(self) -> Result<&'a T, ArgError> {
+        match self.value {
+            ArgValue::Owned(_) => Err(ArgError::InvalidOwnership {
+                index: self.index,
                 expected: Ownership::Ref,
                 received: Ownership::Owned,
             }),
-            Arg::Ref(arg) => Ok(arg.downcast_ref().ok_or_else(|| ArgError::UnexpectedType {
-                id: info.id().clone(),
-                expected: ::std::borrow::Cow::Borrowed(info.type_path()),
-                received: ::std::borrow::Cow::Owned(arg.reflect_type_path().to_string()),
-            })?),
-            Arg::Mut(_) => Err(ArgError::InvalidOwnership {
-                id: info.id().clone(),
+            ArgValue::Ref(arg) => {
+                Ok(arg.downcast_ref().ok_or_else(|| ArgError::UnexpectedType {
+                    index: self.index,
+                    expected: std::borrow::Cow::Borrowed(T::type_path()),
+                    received: std::borrow::Cow::Owned(arg.reflect_type_path().to_string()),
+                })?)
+            }
+            ArgValue::Mut(_) => Err(ArgError::InvalidOwnership {
+                index: self.index,
                 expected: Ownership::Ref,
                 received: Ownership::Mut,
             }),
         }
     }
 
-    /// Returns `Ok(&mut T)` if the argument is [`Arg::Mut`].
-    pub fn take_mut<T: Reflect>(self, info: &ArgInfo) -> Result<&'a mut T, ArgError> {
-        match self {
-            Arg::Owned(_) => Err(ArgError::InvalidOwnership {
-                id: info.id().clone(),
+    /// Returns `Ok(&mut T)` if the argument is [`ArgValue::Mut`].
+    ///
+    /// If the argument is not a mutable reference, returns an error.
+    ///
+    /// It's generally preferred to use [`Self::take`] instead of this method.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use bevy_reflect::func::ArgList;
+    /// let mut value = 123u32;
+    /// let mut args = ArgList::new().push_mut(&mut value);
+    /// let value = args.take_mut::<u32>().unwrap();
+    /// assert_eq!(*value, 123);
+    /// ```
+    pub fn take_mut<T: Reflect + TypePath>(self) -> Result<&'a mut T, ArgError> {
+        match self.value {
+            ArgValue::Owned(_) => Err(ArgError::InvalidOwnership {
+                index: self.index,
                 expected: Ownership::Mut,
                 received: Ownership::Owned,
             }),
-            Arg::Ref(_) => Err(ArgError::InvalidOwnership {
-                id: info.id().clone(),
+            ArgValue::Ref(_) => Err(ArgError::InvalidOwnership {
+                index: self.index,
                 expected: Ownership::Mut,
                 received: Ownership::Ref,
             }),
-            Arg::Mut(arg) => {
-                let received = ::std::borrow::Cow::Owned(arg.reflect_type_path().to_string());
+            ArgValue::Mut(arg) => {
+                let received = std::borrow::Cow::Owned(arg.reflect_type_path().to_string());
                 Ok(arg.downcast_mut().ok_or_else(|| ArgError::UnexpectedType {
-                    id: info.id().clone(),
-                    expected: ::std::borrow::Cow::Borrowed(info.type_path()),
+                    index: self.index,
+                    expected: std::borrow::Cow::Borrowed(T::type_path()),
                     received,
                 })?)
             }
         }
     }
 }
+
+/// Represents an argument that can be passed to a [`DynamicFunction`].
+///
+/// [`DynamicFunction`]: crate::func::DynamicFunction
+#[derive(Debug)]
+pub enum ArgValue<'a> {
+    Owned(Box<dyn Reflect>),
+    Ref(&'a dyn Reflect),
+    Mut(&'a mut dyn Reflect),
+}
+
+impl<'a> Deref for ArgValue<'a> {
+    type Target = dyn Reflect;
+
+    fn deref(&self) -> &Self::Target {
+        match self {
+            ArgValue::Owned(arg) => arg.as_ref(),
+            ArgValue::Ref(arg) => *arg,
+            ArgValue::Mut(arg) => *arg,
+        }
+    }
+}

crates/bevy_reflect/src/func/args/error.rs

@@ -2,25 +2,30 @@ use alloc::borrow::Cow;
 
 use thiserror::Error;
 
-use crate::func::args::{ArgId, Ownership};
+use crate::func::args::Ownership;
 
 /// An error that occurs when converting an [argument].
 ///
-/// [argument]: crate::func::Arg
+/// [argument]: crate::func::args::Arg
 #[derive(Debug, Error, PartialEq)]
 pub enum ArgError {
     /// The argument is not the expected type.
-    #[error("expected `{expected}` but received `{received}` (@ {id:?})")]
+    #[error("expected `{expected}` but received `{received}` (@ argument index {index})")]
     UnexpectedType {
-        id: ArgId,
+        index: usize,
         expected: Cow<'static, str>,
         received: Cow<'static, str>,
     },
     /// The argument has the wrong ownership.
-    #[error("expected {expected} value but received {received} value (@ {id:?})")]
+    #[error("expected {expected} value but received {received} value (@ argument index {index})")]
     InvalidOwnership {
-        id: ArgId,
+        index: usize,
         expected: Ownership,
         received: Ownership,
     },
+    /// Occurs when attempting to access an argument from an empty [`ArgList`].
+    ///
+    /// [`ArgList`]: crate::func::args::ArgList
+    #[error("expected an argument but received none")]
+    EmptyArgList,
 }