Skip to content

Commit 5595733

Browse files
drop old value in insert_resource_by_id if exists (#5587)
# 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
1 parent 166279e commit 5595733

File tree

1 file changed

+44
-8
lines changed
  • crates/bevy_ecs/src/world

1 file changed

+44
-8
lines changed

crates/bevy_ecs/src/world/mod.rs

Lines changed: 44 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1248,13 +1248,7 @@ impl World {
12481248
// SAFETY: column is of type R and has been allocated above
12491249
column.push(value, ComponentTicks::new(change_tick));
12501250
} else {
1251-
let ptr = column.get_data_unchecked_mut(0);
1252-
std::ptr::copy_nonoverlapping::<u8>(
1253-
value.as_ptr(),
1254-
ptr.as_ptr(),
1255-
column.item_layout().size(),
1256-
);
1257-
column.get_ticks_unchecked_mut(0).set_changed(change_tick);
1251+
column.replace(0, value, change_tick);
12581252
}
12591253
}
12601254

@@ -1593,7 +1587,7 @@ mod tests {
15931587
Drop(ID),
15941588
}
15951589

1596-
#[derive(Component)]
1590+
#[derive(Resource, Component)]
15971591
struct MayPanicInDrop {
15981592
drop_log: Arc<Mutex<Vec<DropLogItem>>>,
15991593
expected_panic_flag: Arc<AtomicBool>,
@@ -1786,6 +1780,48 @@ mod tests {
17861780
assert_eq!(DROP_COUNT.load(std::sync::atomic::Ordering::SeqCst), 1);
17871781
}
17881782

1783+
#[test]
1784+
fn insert_resource_by_id_drop_old() {
1785+
let drop_test_helper = DropTestHelper::new();
1786+
let res = std::panic::catch_unwind(|| {
1787+
let mut world = World::new();
1788+
1789+
world.insert_resource(drop_test_helper.make_component(false, 0));
1790+
let component_id = world
1791+
.components()
1792+
.get_resource_id(std::any::TypeId::of::<MayPanicInDrop>())
1793+
.unwrap();
1794+
1795+
OwningPtr::make(drop_test_helper.make_component(true, 1), |ptr| {
1796+
// SAFETY: value is valid for the component layout
1797+
unsafe {
1798+
world.insert_resource_by_id(component_id, ptr);
1799+
}
1800+
});
1801+
1802+
OwningPtr::make(drop_test_helper.make_component(false, 2), |ptr| {
1803+
// SAFETY: value is valid for the component layout
1804+
unsafe {
1805+
world.insert_resource_by_id(component_id, ptr);
1806+
}
1807+
});
1808+
});
1809+
1810+
let drop_log = drop_test_helper.finish(res);
1811+
1812+
assert_eq!(
1813+
drop_log,
1814+
[
1815+
DropLogItem::Create(0),
1816+
DropLogItem::Create(1),
1817+
DropLogItem::Drop(0), // inserting 1 drops 0
1818+
DropLogItem::Create(2),
1819+
DropLogItem::Drop(1), // inserting 2 drops 1
1820+
DropLogItem::Drop(2), // 2 could not be inserted because dropping 1 panicked, so it is dropped as well
1821+
]
1822+
);
1823+
}
1824+
17891825
#[test]
17901826
#[should_panic = "insert_resource_by_id called with component id which doesn't exist in this world"]
17911827
fn insert_resource_by_id_invalid_component_id() {

0 commit comments

Comments
 (0)