Skip to content

Commit

Permalink
bevy_reflect: Function reflection terminology refactor (bevyengine#14813
Browse files Browse the repository at this point in the history
)

# Objective

One of the changes in bevyengine#14704 made `DynamicFunction` effectively the same
as `DynamicClosure<'static>`. This change meant that the de facto
function type would likely be `DynamicClosure<'static>` instead of the
intended `DynamicFunction`, since the former is much more flexible.

We _could_ explore ways of making `DynamicFunction` implement `Copy`
using some unsafe code, but it likely wouldn't be worth it. And users
would likely still reach for the convenience of
`DynamicClosure<'static>` over the copy-ability of `DynamicFunction`.

The goal of this PR is to fix this confusion between the two types.

## Solution

Firstly, the `DynamicFunction` type was removed. Again, it was no
different than `DynamicClosure<'static>` so it wasn't a huge deal to
remove.

Secondly, `DynamicClosure<'env>` and `DynamicClosureMut<'env>` were
renamed to `DynamicFunction<'env>` and `DynamicFunctionMut<'env>`,
respectively.

Yes, we still ultimately kept the naming of `DynamicFunction`, but
changed its behavior to that of `DynamicClosure<'env>`. We need a term
to refer to both functions and closures, and "function" was the best
option.


[Originally](https://discord.com/channels/691052431525675048/1002362493634629796/1274091992162242710),
I was going to go with "callable" as the replacement term to encompass
both functions and closures (e.g. `DynamciCallable<'env>`). However, it
was
[suggested](https://discord.com/channels/691052431525675048/1002362493634629796/1274653581777047625)
by @SkiFire13 that the simpler "function" term could be used instead.

While "callable" is perhaps the better umbrella term—being truly
ambiguous over functions and closures— "function" is more familiar, used
more often, easier to discover, and is subjectively just
"better-sounding".

## Testing

Most changes are purely swapping type names or updating documentation,
but you can verify everything still works by running the following
command:

```
cargo test --package bevy_reflect
```
  • Loading branch information
MrGVSV authored Aug 19, 2024
1 parent 75738ed commit 2b4180c
Show file tree
Hide file tree
Showing 29 changed files with 772 additions and 1,239 deletions.
6 changes: 3 additions & 3 deletions benches/benches/bevy_reflect/function.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use bevy_reflect::func::{ArgList, IntoClosure, TypedFunction};
use bevy_reflect::func::{ArgList, IntoFunction, TypedFunction};
use bevy_reflect::prelude::*;
use criterion::{criterion_group, criterion_main, BatchSize, Criterion};

Expand Down Expand Up @@ -29,7 +29,7 @@ fn into(c: &mut Criterion) {
.bench_function("closure", |b| {
let capture = 25;
let closure = |a: i32| a + capture;
b.iter(|| closure.into_closure());
b.iter(|| closure.into_function());
});
}

Expand All @@ -45,7 +45,7 @@ fn call(c: &mut Criterion) {
})
.bench_function("closure", |b| {
let capture = 25;
let add = (|a: i32| a + capture).into_closure();
let add = (|a: i32| a + capture).into_function();
b.iter_batched(
|| ArgList::new().push_owned(75_i32),
|args| add.call(args),
Expand Down
24 changes: 12 additions & 12 deletions crates/bevy_app/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -605,16 +605,16 @@ impl App {

/// Registers the given function into the [`AppFunctionRegistry`] resource.
///
/// The given function will internally be stored as a [`DynamicClosure`]
/// The given function will internally be stored as a [`DynamicFunction`]
/// and mapped according to its [name].
///
/// Because the function must have a name,
/// anonymous functions (e.g. `|a: i32, b: i32| { a + b }`) and closures must instead
/// be registered using [`register_function_with_name`] or converted to a [`DynamicClosure`]
/// and named using [`DynamicClosure::with_name`].
/// be registered using [`register_function_with_name`] or converted to a [`DynamicFunction`]
/// and named using [`DynamicFunction::with_name`].
/// Failure to do so will result in a panic.
///
/// Only types that implement [`IntoClosure`] may be registered via this method.
/// Only types that implement [`IntoFunction`] may be registered via this method.
///
/// See [`FunctionRegistry::register`] for more information.
///
Expand Down Expand Up @@ -650,7 +650,7 @@ impl App {
/// .register_function(add);
/// ```
///
/// Anonymous functions and closures should be registered using [`register_function_with_name`] or given a name using [`DynamicClosure::with_name`].
/// Anonymous functions and closures should be registered using [`register_function_with_name`] or given a name using [`DynamicFunction::with_name`].
///
/// ```should_panic
/// use bevy_app::App;
Expand All @@ -660,15 +660,15 @@ impl App {
/// ```
///
/// [`register_function_with_name`]: Self::register_function_with_name
/// [`DynamicClosure`]: bevy_reflect::func::DynamicClosure
/// [`DynamicFunction`]: bevy_reflect::func::DynamicFunction
/// [name]: bevy_reflect::func::FunctionInfo::name
/// [`DynamicClosure::with_name`]: bevy_reflect::func::DynamicClosure::with_name
/// [`IntoClosure`]: bevy_reflect::func::IntoClosure
/// [`DynamicFunction::with_name`]: bevy_reflect::func::DynamicFunction::with_name
/// [`IntoFunction`]: bevy_reflect::func::IntoFunction
/// [`FunctionRegistry::register`]: bevy_reflect::func::FunctionRegistry::register
#[cfg(feature = "reflect_functions")]
pub fn register_function<F, Marker>(&mut self, function: F) -> &mut Self
where
F: bevy_reflect::func::IntoClosure<'static, Marker> + 'static,
F: bevy_reflect::func::IntoFunction<'static, Marker> + 'static,
{
self.main_mut().register_function(function);
self
Expand All @@ -689,7 +689,7 @@ impl App {
/// For named functions (e.g. `fn add(a: i32, b: i32) -> i32 { a + b }`) where a custom name is not needed,
/// it's recommended to use [`register_function`] instead as the generated name is guaranteed to be unique.
///
/// Only types that implement [`IntoClosure`] may be registered via this method.
/// Only types that implement [`IntoFunction`] may be registered via this method.
///
/// See [`FunctionRegistry::register_with_name`] for more information.
///
Expand Down Expand Up @@ -738,7 +738,7 @@ impl App {
///
/// [type name]: std::any::type_name
/// [`register_function`]: Self::register_function
/// [`IntoClosure`]: bevy_reflect::func::IntoClosure
/// [`IntoFunction`]: bevy_reflect::func::IntoFunction
/// [`FunctionRegistry::register_with_name`]: bevy_reflect::func::FunctionRegistry::register_with_name
#[cfg(feature = "reflect_functions")]
pub fn register_function_with_name<F, Marker>(
Expand All @@ -747,7 +747,7 @@ impl App {
function: F,
) -> &mut Self
where
F: bevy_reflect::func::IntoClosure<'static, Marker> + 'static,
F: bevy_reflect::func::IntoFunction<'static, Marker> + 'static,
{
self.main_mut().register_function_with_name(name, function);
self
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_app/src/sub_app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ impl SubApp {
#[cfg(feature = "reflect_functions")]
pub fn register_function<F, Marker>(&mut self, function: F) -> &mut Self
where
F: bevy_reflect::func::IntoClosure<'static, Marker> + 'static,
F: bevy_reflect::func::IntoFunction<'static, Marker> + 'static,
{
let registry = self.world.resource_mut::<AppFunctionRegistry>();
registry.write().register(function).unwrap();
Expand All @@ -428,7 +428,7 @@ impl SubApp {
function: F,
) -> &mut Self
where
F: bevy_reflect::func::IntoClosure<'static, Marker> + 'static,
F: bevy_reflect::func::IntoFunction<'static, Marker> + 'static,
{
let registry = self.world.resource_mut::<AppFunctionRegistry>();
registry.write().register_with_name(name, function).unwrap();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,18 @@
#![allow(unused)]

use bevy_reflect::func::IntoFunction;
use bevy_reflect::func::{DynamicFunction, IntoFunction};
use bevy_reflect::Reflect;

fn main() {
let value = String::from("Hello, World!");
let closure_capture_owned = move || println!("{}", value);

let _ = closure_capture_owned.into_function();
//~^ E0277
// Pass:
let _: DynamicFunction<'static> = closure_capture_owned.into_function();

let value = String::from("Hello, World!");
let closure_capture_reference = || println!("{}", value);
//~^ ERROR: `value` does not live long enough

let _ = closure_capture_reference.into_function();
// ↑ This should be an error (E0277) but `compile_fail_utils` fails to pick it up
// when the `closure_capture_owned` test is present
let _: DynamicFunction<'static> = closure_capture_reference.into_function();
}
9 changes: 4 additions & 5 deletions crates/bevy_reflect/src/func/args/arg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,10 @@ use crate::func::args::{ArgError, FromArg, Ownership};
use crate::{PartialReflect, Reflect, TypePath};
use std::ops::Deref;

/// Represents an argument that can be passed to a [`DynamicFunction`], [`DynamicClosure`],
/// or [`DynamicClosureMut`].
/// Represents an argument that can be passed to a [`DynamicFunction`] or [`DynamicFunctionMut`].
///
/// [`DynamicFunction`]: crate::func::DynamicFunction
/// [`DynamicClosure`]: crate::func::DynamicClosure
/// [`DynamicClosureMut`]: crate::func::DynamicClosureMut
/// [`DynamicFunctionMut`]: crate::func::DynamicFunctionMut
#[derive(Debug)]
pub struct Arg<'a> {
index: usize,
Expand Down Expand Up @@ -181,9 +179,10 @@ impl<'a> Arg<'a> {
}
}

/// Represents an argument that can be passed to a [`DynamicFunction`].
/// Represents an argument that can be passed to a [`DynamicFunction`] or [`DynamicFunctionMut`].
///
/// [`DynamicFunction`]: crate::func::DynamicFunction
/// [`DynamicFunctionMut`]: crate::func::DynamicFunctionMut
#[derive(Debug)]
pub enum ArgValue<'a> {
Owned(Box<dyn PartialReflect>),
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_reflect/src/func/args/from_arg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,16 @@ use crate::func::args::{Arg, ArgError};
/// A trait for types that can be created from an [`Arg`].
///
/// This trait exists so that types can be automatically converted into an [`Arg`]
/// by [`IntoFunction`] so they can be passed to a [`DynamicFunction`] in an [`ArgList`].
/// so they can be put into an [`ArgList`] and passed to a [`DynamicFunction`] or [`DynamicFunctionMut`].
///
/// This trait is used instead of a blanket [`From`] implementation due to coherence issues:
/// we can't implement `From<T>` for both `T` and `&T`/`&mut T`.
///
/// This trait is automatically implemented when using the `Reflect` [derive macro].
///
/// [`IntoFunction`]: crate::func::IntoFunction
/// [`DynamicFunction`]: crate::func::DynamicFunction
/// [`ArgList`]: crate::func::args::ArgList
/// [`DynamicFunction`]: crate::func::DynamicFunction
/// [`DynamicFunctionMut`]: crate::func::DynamicFunctionMut
/// [derive macro]: derive@crate::Reflect
pub trait FromArg {
/// The type to convert into.
Expand Down
17 changes: 9 additions & 8 deletions crates/bevy_reflect/src/func/args/info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,11 @@ use alloc::borrow::Cow;
use crate::func::args::{GetOwnership, Ownership};
use crate::TypePath;

/// Type information for an [`Arg`] used in a [`DynamicFunction`], [`DynamicClosure`],
/// or [`DynamicClosureMut`].
/// Type information for an [`Arg`] used in a [`DynamicFunction`] or [`DynamicFunctionMut`].
///
/// [`Arg`]: crate::func::args::Arg
/// [`DynamicFunction`]: crate::func::function::DynamicFunction
/// [`DynamicClosure`]: crate::func::closures::DynamicClosure
/// [`DynamicClosureMut`]: crate::func::closures::DynamicClosureMut
/// [`DynamicFunction`]: crate::func::DynamicFunction
/// [`DynamicFunctionMut`]: crate::func::DynamicFunctionMut
#[derive(Debug, Clone)]
pub struct ArgInfo {
/// The index of the argument within its function.
Expand Down Expand Up @@ -58,13 +56,13 @@ impl ArgInfo {
/// since the name can't be inferred from the function type alone.
///
/// For [`DynamicFunctions`] created using [`IntoFunction`]
/// or [`DynamicClosures`] created using [`IntoClosure`],
/// and [`DynamicFunctionMuts`] created using [`IntoFunctionMut`],
/// the name will always be `None`.
///
/// [`DynamicFunctions`]: crate::func::DynamicFunction
/// [`IntoFunction`]: crate::func::IntoFunction
/// [`DynamicClosures`]: crate::func::DynamicClosure
/// [`IntoClosure`]: crate::func::IntoClosure
/// [`DynamicFunctionMuts`]: crate::func::DynamicFunctionMut
/// [`IntoFunctionMut`]: crate::func::IntoFunctionMut
pub fn name(&self) -> Option<&str> {
self.name.as_deref()
}
Expand All @@ -74,6 +72,9 @@ impl ArgInfo {
self.ownership
}

/// The [type path] of the argument.
///
/// [type path]: TypePath::type_path
pub fn type_path(&self) -> &'static str {
self.type_path
}
Expand Down
6 changes: 2 additions & 4 deletions crates/bevy_reflect/src/func/args/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@ use crate::func::ArgError;
use crate::{PartialReflect, Reflect, TypePath};
use std::collections::VecDeque;

/// A list of arguments that can be passed to a [`DynamicFunction`], [`DynamicClosure`],
/// or [`DynamicClosureMut`].
/// A list of arguments that can be passed to a [`DynamicFunction`] or [`DynamicFunctionMut`].
///
/// # Example
///
Expand All @@ -28,8 +27,7 @@ use std::collections::VecDeque;
///
/// [arguments]: Arg
/// [`DynamicFunction`]: crate::func::DynamicFunction
/// [`DynamicClosure`]: crate::func::DynamicClosure
/// [`DynamicClosureMut`]: crate::func::DynamicClosureMut
/// [`DynamicFunctionMut`]: crate::func::DynamicFunctionMut
#[derive(Default, Debug)]
pub struct ArgList<'a> {
list: VecDeque<Arg<'a>>,
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_reflect/src/func/args/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! Argument types and utilities for working with [`DynamicFunctions`] and [`DynamicClosures`].
//! Argument types and utilities for working with [`DynamicFunction`] and [`DynamicFunctionMut`].
//!
//! [`DynamicFunctions`]: crate::func::DynamicFunction
//! [`DynamicClosures`]: crate::func::DynamicClosure
//! [`DynamicFunction`]: crate::func::DynamicFunction
//! [`DynamicFunctionMut`]: crate::func::DynamicFunctionMut
pub use arg::*;
pub use error::*;
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_reflect/src/func/args/ownership.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@ use core::fmt::{Display, Formatter};

/// A trait for getting the ownership of a type.
///
/// This trait exists so that [`IntoFunction`] can automatically generate
/// This trait exists so that [`TypedFunction`] can automatically generate
/// [`FunctionInfo`] containing the proper [`Ownership`] for its [argument] types.
///
/// This trait is automatically implemented when using the `Reflect` [derive macro].
///
/// [`IntoFunction`]: crate::func::IntoFunction
/// [`TypedFunction`]: crate::func::TypedFunction
/// [`FunctionInfo`]: crate::func::FunctionInfo
/// [argument]: crate::func::args::Arg
/// [derive macro]: derive@crate::Reflect
Expand Down
Loading

0 comments on commit 2b4180c

Please sign in to comment.