Skip to content

Remove Send + Sync bounds from PartialReflect and friends (and move the constraint to usages thereof) #16993

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Remove Send + Sync bounds from PartialReflect and friends (and move the constraint to usages thereof) #16993

wants to merge 3 commits into from

Conversation

BenjaminBrienen
Copy link
Contributor

Objective

Fixes #16713

Solution

Move the bounds to the usage sites

Disclaimer

If you read this while it is a draft, be aware that this is not ready for a round of full feedback.

@BenjaminBrienen BenjaminBrienen changed the title Remove Send + Sync bounds from PartialReflect Remove Send + Sync bounds from PartialReflect and Reflect Dec 27, 2024
@BenjaminBrienen BenjaminBrienen changed the title Remove Send + Sync bounds from PartialReflect and Reflect Remove Send + Sync bounds from PartialReflect and Reflect Dec 27, 2024
@BenjaminBrienen BenjaminBrienen changed the title Remove Send + Sync bounds from PartialReflect and Reflect Remove Send + Sync bounds from PartialReflect and friends Dec 27, 2024
@BenjaminBrienen BenjaminBrienen marked this pull request as ready for review December 27, 2024 20:42
@BenjaminBrienen BenjaminBrienen self-assigned this Dec 27, 2024
@BenjaminBrienen BenjaminBrienen added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 27, 2024
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Jan 6, 2025
@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 22, 2025
@BenjaminBrienen BenjaminBrienen added S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Mar 9, 2025
@BenjaminBrienen
Copy link
Contributor Author

I might pick this up again in 4 or 5 weeks when rust 1.86 lands, which includes rust-lang/rust#65991

@alice-i-cecile alice-i-cecile removed this from the 0.16 milestone Mar 16, 2025
@alice-i-cecile
Copy link
Member

Cutting from the milestone: there's nothing urgent about this, as much as I think it's great.

@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! labels Apr 3, 2025
@BenjaminBrienen BenjaminBrienen added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Apr 3, 2025
@alice-i-cecile alice-i-cecile added the C-Bug An unexpected or incorrect behavior label Apr 4, 2025
@alice-i-cecile alice-i-cecile added X-Uncontroversial This work is generally agreed upon D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes and removed D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Apr 7, 2025
@BenjaminBrienen BenjaminBrienen changed the title Remove Send + Sync bounds from PartialReflect and friends Remove Send + Sync bounds from PartialReflect and friends (and move the constraint to usages thereof) Apr 7, 2025
@BenjaminBrienen
Copy link
Contributor Author

BenjaminBrienen commented Apr 7, 2025 •
edited
Loading

A follow up is needed to remove the + Send + Sync and truly make the engine more generic. This is just the first step.

MrGVSV
MrGVSV reviewed Apr 12, 2025
View reviewed changes
crates/bevy_asset/src/handle.rs
@@ -691,16 +691,17 @@ mod tests {
"Inserting the asset should result in a strong count of 1"
);

let reflected: &dyn Reflect = &handle;
let _cloned_handle: Box<dyn Reflect> = reflected.reflect_clone().unwrap();
let reflected: &(dyn Reflect + Send + Sync) = &handle;
Copy link
Member

Choose a reason for hiding this comment

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

Are the bounds necessary here?

crates/bevy_reflect/src/reflect.rs
@@ -106,32 +106,34 @@ where
/// Casts this type to a boxed, reflected value.
///
/// This is useful for coercing trait objects.
fn into_partial_reflect(self: Box<Self>) -> Box<dyn PartialReflect>;
fn into_partial_reflect(self: Box<Self>) -> Box<dyn PartialReflect + Send + Sync>;
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I feel like this sort of defeats the purpose, no?

The only way we can return Box::new(self) here is if Self: Send + Sync.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrGVSV MrGVSV MrGVSV left review comments

Assignees

@BenjaminBrienen BenjaminBrienen

Labels
A-Reflection Runtime information about types C-Bug An unexpected or incorrect behavior C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Uncontroversial This work is generally agreed upon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reflect trait should not require Send + Sync bounds
3 participants
@BenjaminBrienen @alice-i-cecile @MrGVSV