Skip to content

bevy_reflect: Add DynamicClosure and DynamicClosureMut #14141

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

Merged
merged 10 commits into from
Jul 16, 2024

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented Jul 4, 2024

Objective

As mentioned in this comment, creating a function registry (see #14098) is a bit difficult due to the requirements of DynamicFunction. Internally, a DynamicFunction contains a Box<dyn FnMut> (the function that reifies reflected arguments and calls the actual function), which requires &mut self in order to be called.

This means that users would require a mutable reference to the function registry for it to be useful— which isn't great. And they can't clone the DynamicFunction either because cloning an FnMut isn't really feasible (wrapping it in an Arc would allow it to be cloned but we wouldn't be able to call the clone since we need a mutable reference to the FnMut, which we can't get with multiple Arcs still alive, requiring us to also slap in a Mutex, which adds additional overhead).

And we don't want to just replace the dyn FnMut with dyn Fn as that would prevent reflecting closures that mutate their environment.

Instead, we need to introduce a new type to split the requirements of DynamicFunction.

Solution

Introduce new types for representing closures.

Specifically, this PR introduces DynamicClosure and DynamicClosureMut. Similar to how IntoFunction exists for DynamicFunction, two new traits were introduced: IntoClosure and IntoClosureMut.

Now DynamicFunction stores a dyn Fn with a 'static lifetime. DynamicClosure also uses a dyn Fn but has a lifetime, 'env, tied to its environment. DynamicClosureMut is most like the old DynamicFunction, keeping the dyn FnMut and also typing its lifetime, 'env, to the environment

Here are some comparison tables:

DynamicFunction DynamicClosure DynamicClosureMut
Callable with &self
Callable with &mut self
Allows for non-'static lifetimes
IntoFunction IntoClosure IntoClosureMut
Convert fn functions
Convert fn methods
Convert anonymous functions
Convert closures that capture immutable references
Convert closures that capture mutable references
Convert closures that capture owned values 1
let mut list: Vec<i32> = vec![1, 2, 3];

// `replace` is a closure that captures a mutable reference to `list`
let mut replace = |index: usize, value: i32| -> i32 {
  let old_value = list[index];
  list[index] = value;
  old_value
};

// Convert the closure into a dynamic closure using `IntoClosureMut::into_closure_mut`
let mut func: DynamicClosureMut = replace.into_closure_mut();

// Dynamically call the closure:
let args = ArgList::default().push_owned(1_usize).push_owned(-2_i32);
let value = func.call_once(args).unwrap().unwrap_owned();

// Check the result:
assert_eq!(value.take::<i32>().unwrap(), 2);
assert_eq!(list, vec![1, -2, 3]);

ReflectFn/ReflectFnMut

To make extending the function reflection system easier (the blanket impls for IntoFunction, IntoClosure, and IntoClosureMut are all incredibly short), this PR generalizes callables with two new traits: ReflectFn and ReflectFnMut.

These traits mimic Fn and FnMut but allow for being called via reflection. In fact, their blanket implementations are identical save for ReflectFn being implemented over Fn types and ReflectFnMut being implemented over FnMut types.

And just as Fn is a subtrait of FnMut, ReflectFn is a subtrait of ReflectFnMut. So anywhere that expects a ReflectFnMut can also be given a ReflectFn.

To reiterate, these traits aren't 100% necessary. They were added in purely for extensibility. If we decide to split things up differently or add new traits/types in the future, then those changes should be much simpler to implement.

TypedFunction

Because of the split into ReflectFn and ReflectFnMut, we needed a new way to access the function type information. This PR moves that concept over into TypedFunction.

Much like Typed, this provides a way to access a function's FunctionInfo.

By splitting this trait out, it helps to ensure the other traits are focused on a single responsibility.

Internal Macros

The original function PR (#13152) implemented IntoFunction using a macro which was passed into an all_tuples! macro invocation. Because we needed the same functionality for these new traits, this PR has copy+pasted that code for ReflectFn, ReflectFnMut, and TypedFunction— albeit with some differences between them.

Originally, I was going to try and macro-ify the impls and where clauses such that we wouldn't have to straight up duplicate a lot of this logic. However, aside from being more complex in general, autocomplete just does not play nice with such heavily nested macros (tried in both RustRover and VSCode). And both of those problems told me that it just wasn't worth it: we need to ensure the crate is easily maintainable, even at the cost of duplicating code.

So instead, I made sure to simplify the macro code by removing all fully-qualified syntax and cutting the where clauses down to the bare essentials, which helps to clean up a lot of the visual noise. I also tried my best to document the macro logic in certain areas (I may even add a bit more) to help with maintainability for future devs.

Documentation

Documentation for this module was a bit difficult for me. So many of these traits and types are very interconnected. And each trait/type has subtle differences that make documenting it in a single place, like at the module level, difficult to do cleanly. Describing the valid signatures is also challenging to do well.

Hopefully what I have here is okay. I think I did an okay job, but let me know if there any thoughts on ways to improve it. We can also move such a task to a followup PR for more focused discussion.

Testing

You can test locally by running:

cargo test --package bevy_reflect

Changelog

  • Added DynamicClosure struct
  • Added DynamicClosureMut struct
  • Added IntoClosure trait
  • Added IntoClosureMut trait
  • Added ReflectFn trait
  • Added ReflectFnMut trait
  • Added TypedFunction trait
  • IntoFunction now only works for standard Rust functions
  • IntoFunction no longer takes a lifetime parameter
  • DynamicFunction::call now only requires &self
  • Removed DynamicFunction::call_once
  • Changed the IntoReturn::into_return signature to include a where clause

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.

IntoClosure

IntoFunction now only works for standard Rust functions. Calling IntoFunction::into_function on a closure that captures references to its environment (either mutable or immutable), will no longer compile.

Instead, you will need to use either IntoClosure::into_closure to create a DynamicClosure or IntoClosureMut::into_closure_mut to create a DynamicClosureMut, depending on your needs:

let punct = String::from("!");
let print = |value: String| {
    println!("{value}{punct}");
};

// BEFORE
let func: DynamicFunction = print.into_function();

// AFTER
let func: DynamicClosure = print.into_closure();

IntoFunction lifetime

Additionally, IntoFunction no longer takes a lifetime parameter as it always expects a 'static lifetime. Usages will need to remove any lifetime parameters:

// BEFORE
fn execute<'env, F: IntoFunction<'env, Marker>, Marker>(f: F) {/* ... */}

// AFTER
fn execute<F: IntoFunction<Marker>, Marker>(f: F) {/* ... */}

IntoReturn

IntoReturn::into_return now has a where clause. Any manual implementors will need to add this where clause to their implementation.

Footnotes

  1. Due to limitations in Rust, IntoFunction can't be implemented for just functions (unless we forced users to manually coerce them to function pointers first). So closures that meet the trait requirements can technically be converted into a DynamicFunction as well. To both future-proof and reduce confusion, though, we'll just pretend like this isn't a thing.

@MrGVSV MrGVSV added C-Feature A new feature, making something new possible A-Reflection Runtime information about types D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 4, 2024
@MrGVSV MrGVSV added this to the 0.15 milestone Jul 4, 2024
@MrGVSV MrGVSV requested a review from NthTensor July 4, 2024 23:59
Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't get through a full review of this tonight, but I think these changes are really good and important for hooking up assets to code efficiently.

The design is great, but the changes may take a bit for me to work through.

@MrGVSV MrGVSV force-pushed the mrgvsv/reflect/reflect-closures branch from 912d502 to 272af95 Compare July 14, 2024 03:41
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent docs and implementation quality. I'm a bit nervous about using slightly different semantics than Rust, but I think it's the right choice. Altering the names might help make this more comfortable,

I would love to see a table explaining all of the different traits, but that can definitely wait.

@MrGVSV MrGVSV force-pushed the mrgvsv/reflect/reflect-closures branch from 272af95 to 99e8958 Compare July 15, 2024 07:16
@MrGVSV MrGVSV changed the title bevy_reflect: Add DynamicClosure bevy_reflect: Add DynamicClosure and DynamicClosureMut Jul 15, 2024
@MrGVSV MrGVSV requested a review from alice-i-cecile July 15, 2024 07:26
MrGVSV added 3 commits July 15, 2024 00:28
DynamicFunction is now further restricted to standard function types
(well, almost, Rust doesn't like impls on `fn` types)
@MrGVSV MrGVSV force-pushed the mrgvsv/reflect/reflect-closures branch from 99e8958 to fb58ba7 Compare July 15, 2024 07:28
@MrGVSV MrGVSV requested a review from NthTensor July 15, 2024 07:38
@MrGVSV
Copy link
Member Author

MrGVSV commented Jul 15, 2024

Updated the PR to further split DynamicClosure into DynamicClosure and DynamicClosureMut. This greatly simplifies the mental model for users as it now matches Rust definitions of functions and closures, at the cost of increasing the size of the API.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the right way to model it. Lots of code / API, but this is fundamentally complex.

Copy link
Contributor

@mweatherley mweatherley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor quibbles over notation aside, I think I'm pretty happy with this split.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 15, 2024
Avoid using function-pointer syntax when discussin general
function signatures.

Also fixes a broken doc link
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 16, 2024
Merged via the queue into bevyengine:main with commit 1042f09 Jul 16, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Feature A new feature, making something new possible D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants