[Merged by Bors] - drop old value in `insert_resource_by_id` if exists #5587

jakobhellermann · 2022-08-05T20:22:09Z

Objective

While trying out the lint unsafe_op_in_unsafe_fn I noticed that insert_resource_by_id didn't drop the old value if it already existed, and reimplemented Column::replace manually for no apparent reason.

Solution

use Column::replace and add a test expecting the correct drop count

Changelog

World::insert_resource_by_id will now correctly drop the old resource value, if one already existed

BoxyUwU · 2022-08-07T18:25:59Z

crates/bevy_ecs/src/world/mod.rs

@@ -1813,6 +1807,46 @@ mod tests {
        assert_eq!(DROP_COUNT.load(std::sync::atomic::Ordering::SeqCst), 1);
    }

+    #[test]
+    fn insert_resource_by_id_drop_old() {


You should use DropTestHelper and a regular rust type for the resource instead of doing this all dynamically. You can get the ComponentId representing the rust type via world.components.get_resource_id(TypeId::of::<Whatever>()).unwrap() (after you've inserted the resource the first time otherwise it wont have been registered lol)

I changed the test to use DropTestHelper: https://github.com/bevyengine/bevy/pull/5587/files#diff-8799631eaab5e61e5dbc20251e08d83474df566ba159061cf2a78ce8f1fd59d5R1810-R1854

Unfortunately I had to use AssertUnwindSafe to be able to create the world outside of the catch_unwind and reference it afterwards, otherwise I was getting
| `&mut world::World` may not be safely transferred across an unwind boundary
I don't know much about unwind safety, do you know if this assertion is justified?
Alternatively I can create the world inside the catch_unwind and not remove the !contains_resource assert.

Alternatively I can create the world inside the catch_unwind and not remove the !contains_resource assert.

I prefer this approach as it's a bit less spooky.

You can set make_component(false, 1) and get rid of the catch unwind/assertunwindsafe. I don't think panic in drop was relevent to this issue was it?

You can set make_component(false, 1) and get rid of the catch unwind/assertunwindsafe. I don't think panic in drop was relevent to this issue was it?

That is currently not possible with the DropTestHelper because it needs at least one dropped make_component(true) to "defuse" itself: #5615

I removed the AssertUnwindSafe and the !contains_resource assert.

Oh thats unfortunate oh well ...

alice-i-cecile · 2022-08-09T16:19:36Z

bors r+

# Objective While trying out the lint `unsafe_op_in_unsafe_fn` I noticed that `insert_resource_by_id` didn't drop the old value if it already existed, and reimplemented `Column::replace` manually for no apparent reason. ## Solution - use `Column::replace` and add a test expecting the correct drop count --- ## Changelog - `World::insert_resource_by_id` will now correctly drop the old resource value, if one already existed

bors · 2022-08-09T16:43:29Z

Build failed (retrying...):

ci

# Objective While trying out the lint `unsafe_op_in_unsafe_fn` I noticed that `insert_resource_by_id` didn't drop the old value if it already existed, and reimplemented `Column::replace` manually for no apparent reason. ## Solution - use `Column::replace` and add a test expecting the correct drop count --- ## Changelog - `World::insert_resource_by_id` will now correctly drop the old resource value, if one already existed

alice-i-cecile · 2022-08-09T16:45:28Z

bors r-

bors · 2022-08-09T16:45:29Z

Canceled.

alice-i-cecile · 2022-08-09T16:45:47Z

@jakobhellermann you'll need to update this PR to reflect the new explicit #[derive(Resource)] requirement.

jakobhellermann · 2022-08-09T17:48:50Z

@jakobhellermann you'll need to update this PR to reflect the new explicit #[derive(Resource)] requirement.

done

alice-i-cecile · 2022-08-09T18:05:27Z

bors r+

# Objective While trying out the lint `unsafe_op_in_unsafe_fn` I noticed that `insert_resource_by_id` didn't drop the old value if it already existed, and reimplemented `Column::replace` manually for no apparent reason. ## Solution - use `Column::replace` and add a test expecting the correct drop count --- ## Changelog - `World::insert_resource_by_id` will now correctly drop the old resource value, if one already existed

bors · 2022-08-09T18:20:52Z

Pull request successfully merged into main.

Build succeeded:

# Objective remove `insert_resource_with_id` because `insert_resource_by_id` exists and does almost exactly the same thing blocked on #5587 because otherwise we will leak a resource when it's inserted ## Solution remove the function and also add a safety invariant of to `insert_resource_by_id` that the id be valid for the world. I didn't see any discussion in #4447 about this safety invariant being left off in favor of a panic so I'm curious if there was one or if it just seemed nicer to have less safety invariants for callers to uphold 😅 --- ## Changelog - safety invariant added to `insert_resource_by_id` requiring the id to be valid for world ## Migration Guide - audit any calls to `insert_resource_by_id` making sure that the id is valid for the world Co-authored-by: Carter Anderson <mcanders1@gmail.com>

# Objective While trying out the lint `unsafe_op_in_unsafe_fn` I noticed that `insert_resource_by_id` didn't drop the old value if it already existed, and reimplemented `Column::replace` manually for no apparent reason. ## Solution - use `Column::replace` and add a test expecting the correct drop count --- ## Changelog - `World::insert_resource_by_id` will now correctly drop the old resource value, if one already existed

# Objective remove `insert_resource_with_id` because `insert_resource_by_id` exists and does almost exactly the same thing blocked on bevyengine#5587 because otherwise we will leak a resource when it's inserted ## Solution remove the function and also add a safety invariant of to `insert_resource_by_id` that the id be valid for the world. I didn't see any discussion in bevyengine#4447 about this safety invariant being left off in favor of a panic so I'm curious if there was one or if it just seemed nicer to have less safety invariants for callers to uphold 😅 --- ## Changelog - safety invariant added to `insert_resource_by_id` requiring the id to be valid for world ## Migration Guide - audit any calls to `insert_resource_by_id` making sure that the id is valid for the world Co-authored-by: Carter Anderson <mcanders1@gmail.com>

# Objective While trying out the lint `unsafe_op_in_unsafe_fn` I noticed that `insert_resource_by_id` didn't drop the old value if it already existed, and reimplemented `Column::replace` manually for no apparent reason. ## Solution - use `Column::replace` and add a test expecting the correct drop count --- ## Changelog - `World::insert_resource_by_id` will now correctly drop the old resource value, if one already existed

# Objective remove `insert_resource_with_id` because `insert_resource_by_id` exists and does almost exactly the same thing blocked on bevyengine#5587 because otherwise we will leak a resource when it's inserted ## Solution remove the function and also add a safety invariant of to `insert_resource_by_id` that the id be valid for the world. I didn't see any discussion in bevyengine#4447 about this safety invariant being left off in favor of a panic so I'm curious if there was one or if it just seemed nicer to have less safety invariants for callers to uphold 😅 --- ## Changelog - safety invariant added to `insert_resource_by_id` requiring the id to be valid for world ## Migration Guide - audit any calls to `insert_resource_by_id` making sure that the id is valid for the world Co-authored-by: Carter Anderson <mcanders1@gmail.com>

jakobhellermann added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events labels Aug 5, 2022

bjorn3 approved these changes Aug 6, 2022

View reviewed changes

alice-i-cecile approved these changes Aug 7, 2022

View reviewed changes

alice-i-cecile 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 Aug 7, 2022

BoxyUwU suggested changes Aug 7, 2022

View reviewed changes

BoxyUwU mentioned this pull request Aug 7, 2022

[Merged by Bors] - Remove insert_resource_with_id #5608

Closed

jakobhellermann force-pushed the insert-resource-by-id-drop-old branch from 100a69b to ee98a94 Compare August 8, 2022 12:52

alice-i-cecile approved these changes Aug 8, 2022

View reviewed changes

BoxyUwU approved these changes Aug 8, 2022

View reviewed changes

jakobhellermann added 4 commits August 9, 2022 19:47

drop old value in insert_resource_by_id

64d01dd

use DropTestHelper

df3ec96

remove AssertUnwindSafe

a7f6c92

derive resource

d3b7c4d

jakobhellermann force-pushed the insert-resource-by-id-drop-old branch from 84694e0 to d3b7c4d Compare August 9, 2022 17:48

bors bot changed the title ~~drop old value in insert_resource_by_id if exists~~ [Merged by Bors] - drop old value in insert_resource_by_id if exists Aug 9, 2022

bors bot closed this Aug 9, 2022

jakobhellermann deleted the insert-resource-by-id-drop-old branch August 10, 2022 13:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - drop old value in `insert_resource_by_id` if exists #5587

[Merged by Bors] - drop old value in `insert_resource_by_id` if exists #5587

jakobhellermann commented Aug 5, 2022

BoxyUwU Aug 7, 2022

jakobhellermann Aug 8, 2022 •

edited

Loading

alice-i-cecile Aug 8, 2022

BoxyUwU Aug 8, 2022

jakobhellermann Aug 8, 2022

jakobhellermann Aug 8, 2022

BoxyUwU Aug 8, 2022

alice-i-cecile commented Aug 9, 2022

bors bot commented Aug 9, 2022

alice-i-cecile commented Aug 9, 2022

bors bot commented Aug 9, 2022

alice-i-cecile commented Aug 9, 2022

jakobhellermann commented Aug 9, 2022

alice-i-cecile commented Aug 9, 2022

bors bot commented Aug 9, 2022

[Merged by Bors] - drop old value in insert_resource_by_id if exists #5587

[Merged by Bors] - drop old value in insert_resource_by_id if exists #5587

Conversation

jakobhellermann commented Aug 5, 2022

Objective

Solution

Changelog

BoxyUwU Aug 7, 2022

Choose a reason for hiding this comment

jakobhellermann Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

alice-i-cecile Aug 8, 2022

Choose a reason for hiding this comment

BoxyUwU Aug 8, 2022

Choose a reason for hiding this comment

jakobhellermann Aug 8, 2022

Choose a reason for hiding this comment

jakobhellermann Aug 8, 2022

Choose a reason for hiding this comment

BoxyUwU Aug 8, 2022

Choose a reason for hiding this comment

alice-i-cecile commented Aug 9, 2022

bors bot commented Aug 9, 2022

alice-i-cecile commented Aug 9, 2022

bors bot commented Aug 9, 2022

alice-i-cecile commented Aug 9, 2022

jakobhellermann commented Aug 9, 2022

alice-i-cecile commented Aug 9, 2022

bors bot commented Aug 9, 2022

[Merged by Bors] - drop old value in `insert_resource_by_id` if exists #5587

[Merged by Bors] - drop old value in `insert_resource_by_id` if exists #5587

jakobhellermann Aug 8, 2022 •

edited

Loading