Skip to content
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

Improve warning for Send resources marked as non_send #8000

Merged
merged 2 commits into from
Apr 17, 2023

Conversation

NiseVoid
Copy link
Contributor

@NiseVoid NiseVoid commented Mar 9, 2023

Objective

  • Fixes unclear warning when insert_non_send_resource is called on a Send resource

Solution

  • Add a message to the asssert statement that checks this

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2023

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

Comment on lines 275 to 281
if SEND {
assert!(component_info.is_send_and_sync());
assert!(
component_info.is_send_and_sync(),
"Send + Sync resource {} initialized as non_send",
component_info.name(),
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I already asked on Discord but nobody answered: this check only triggers if a resource is first inserted as non-Send and then as Send. If the order is swapped this won't trigger (there's even a test for this). IMO this is inconsistent, it should either always trigger or never.

Copy link
Member

Choose a reason for hiding this comment

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

It's perfectly safe to store a Send type in !Send storage, though probably a mistake to do so. We could fire a warning in the opposite case, but I'm against hard-failing when there's no safety issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's perfectly safe to store a Send type in !Send storage

It's also safe to store a Send type in Send storage though. The problem is that component_info is shared between Send and non-Send resources with the same TypeId. When a resource is inserted as both Send and non-Send then component_info.is_send_and_sync() will report only the value of the first one, which is however wrong for when the resource is first inserted as non-Send. This can be fixed by either:

  • giving two different ComponentIds to Send and non-Send resources with the same TypeId
    • for example this could be done by duplicating Components::resource_indices for non-Send resources
    • in this case the given assertion would just become a sanity check and should never trigger in practice
  • or making it a hard fail
    • this would essentially consist in making this assertion trigger in the opposite direction too, that is when a resource is inserted as Send first and then as non-Send
  • or updating the component_info to set is_send_and_sync when it is false but a Send Resource is being inserted
    • this feels the worst option since it doesn't really fix the issue, it just hides it

At the end I would expect these two functions to either both succeed or both fail (currently only the first one succeeds):

#[derive(Resource)]
struct Foo;
fn send_then_non_send() {
    let mut world = World::new();
    world.insert_resource(Foo);
    world.insert_non_send_resource(Foo);
}
fn non_send_then_send() {
    let mut world = World::new();
    world.insert_non_send_resource(Foo);
    world.insert_resource(Foo);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The case where I ran into this error is much simpler and simply requires insert_non_send_resource, then adding a system that uses it. Doing insert_resource first does prevent the error, which seems a bit odd but that doesn't really cause any usability problems

Copy link
Member

Choose a reason for hiding this comment

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

While this is a pretty big footgun as is, changing the behavior here would be a breaking change, and probably not suited for a PR aiming at 0.10.1. I'm OK with just changing the panic message for now, and have a separate PR resolve this for 0.11.

Hopefully we get !Send resources out of the World before then though.

@james7132 james7132 added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Mar 9, 2023
@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 16, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 17, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 17, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 17, 2023
Merged via the queue into bevyengine:main with commit 65292fd Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants