diff --git a/flecs_ecs/src/core/components/component.rs b/flecs_ecs/src/core/components/component.rs index 11254d57..c7ffa550 100644 --- a/flecs_ecs/src/core/components/component.rs +++ b/flecs_ecs/src/core/components/component.rs @@ -89,6 +89,37 @@ impl<'a, T: ComponentId> Component<'a, T> { _marker: PhantomData, } } + + /// Register on replace hook. Needs to implement the Default constructor. + /// This is because + pub fn on_replace(self, func: Func) -> Self + where + Func: FnMut(EntityView, &mut T, &mut T) + 'static, + { + ecs_assert!( + T::IMPLS_DEFAULT, + FlecsErrorCode::InvalidOperation, + "setting on_replace hook on {} requires it to implement Default.", + core::any::type_name::() + ); + + let mut type_hooks: sys::ecs_type_hooks_t = self.get_hooks(); + ecs_assert!( + type_hooks.on_replace.is_none(), + FlecsErrorCode::InvalidOperation, + "on_replace hook already set for component {}", + core::any::type_name::() + ); + + let binding_ctx = Self::get_binding_context(&mut type_hooks); + let boxed_func = Box::new(func); + let static_ref = Box::leak(boxed_func); + binding_ctx.on_replace = Some(static_ref as *mut _ as *mut c_void); + binding_ctx.free_on_replace = Some(Self::on_replace_drop::); + type_hooks.on_replace = Some(Self::run_replace::); + unsafe { sys::ecs_set_hooks_id(self.world.world_ptr_mut(), *self.id, &type_hooks) }; + self + } } impl<'a, T> Component<'a, T> { /// Create a new component that is not marked within Rust. @@ -250,30 +281,6 @@ impl<'a, T> Component<'a, T> { self } - /// Register on replace hook. - pub fn on_replace(self, func: Func) -> Self - where - Func: FnMut(EntityView, &mut T, &mut T) + 'static, - { - let mut type_hooks: sys::ecs_type_hooks_t = self.get_hooks(); - - ecs_assert!( - type_hooks.on_replace.is_none(), - FlecsErrorCode::InvalidOperation, - "on_replace hook already set for component {}", - core::any::type_name::() - ); - - let binding_ctx = Self::get_binding_context(&mut type_hooks); - let boxed_func = Box::new(func); - let static_ref = Box::leak(boxed_func); - binding_ctx.on_replace = Some(static_ref as *mut _ as *mut c_void); - binding_ctx.free_on_replace = Some(Self::on_replace_drop::); - type_hooks.on_replace = Some(Self::run_replace::); - unsafe { sys::ecs_set_hooks_id(self.world.world_ptr_mut(), *self.id, &type_hooks) }; - self - } - /// Function to free the on add hook. #[extern_abi] unsafe fn on_add_drop(func: *mut c_void) diff --git a/flecs_ecs/src/core/components/lifecycle_traits.rs b/flecs_ecs/src/core/components/lifecycle_traits.rs index cfa4e691..b4b27e9b 100644 --- a/flecs_ecs/src/core/components/lifecycle_traits.rs +++ b/flecs_ecs/src/core/components/lifecycle_traits.rs @@ -259,7 +259,8 @@ fn panic_copy( } /// This is the generic move for non-trivial types -/// It will move the memory +/// It will move the memory and deconstruct initialized memory +/// at the location it is moving to. #[extern_abi] fn move_dtor( dst_ptr: *mut c_void, @@ -279,6 +280,8 @@ fn move_dtor( let src_value = src_arr.offset(i); //get value of src let dst_value = dst_arr.offset(i); // get ptr to dest + core::ptr::drop_in_place(dst_value); + //memcpy the bytes of src to dest //src value and dest value point to the same thing core::ptr::copy_nonoverlapping(src_value, dst_value, 1); diff --git a/flecs_ecs/src/core/utility/functions.rs b/flecs_ecs/src/core/utility/functions.rs index 868828bb..6515d95a 100644 --- a/flecs_ecs/src/core/utility/functions.rs +++ b/flecs_ecs/src/core/utility/functions.rs @@ -364,23 +364,59 @@ pub(crate) fn set_helper( }; unsafe { - if T::NEEDS_DROP && sys::ecs_has_id(world, entity, id) { - assign_helper(world, entity, value, id); - return; - } - - let res = sys::ecs_cpp_set( - world, - entity, - id, - &value as *const _ as *const _, - const { core::mem::size_of::() }, - ); - - let comp = res.ptr as *mut T; - core::ptr::write(comp, value); + if T::IMPLS_DEFAULT { + if T::NEEDS_DROP && sys::ecs_has_id(world, entity, id) { + assign_helper(world, entity, value, id); + return; + } - if res.call_modified { + // The reason we can't use this on types that don't implement a default + // constructor is because ctor is called during `ecs_cpp_set` when deferred, + // before the new_ptr is moved into the value. + // + // This creates the expectation that the data must be initialized and is therefore + // dtor'd when moved into. This is a UB in rust for components without a ctor since + // the data is freed while uninitialized. + // + // For that reason, we could use `ecs_emplace_id`, but then on_replace hooks would not + // be registerable on any of our components. + // This code could be significantly more simple if ecs_emplace_id worked with `on_replace` + // hooks, but that will require modifications to flecs.c, so this will have to suffice for now. + let res = sys::ecs_cpp_set( + world, + entity, + id, + &value as *const _ as *const _, + const { core::mem::size_of::() }, + ); + + let comp = res.ptr as *mut T; + // The set pipeline for default constructed components works like so: + // Default constructed -> Dropped -> Moved into from value + // Default construction happens in sys::ecs_cpp_set. + core::ptr::drop_in_place(comp); + core::ptr::write(comp, value); + + if res.call_modified { + sys::ecs_modified_id(world, entity, id); + } + } else { + // If T does not impl Default, we need to construct the data ourselves. + // so that uninitialized data is not dropped. + let mut changed: bool = false; + + let comp = sys::ecs_emplace_id( + world, + entity, + id, + const { core::mem::size_of::() }, + &mut changed, + ) as *mut T; + + if !changed { + core::ptr::drop_in_place(comp); + } + core::ptr::write(comp, value); sys::ecs_modified_id(world, entity, id); } } diff --git a/flecs_ecs/src/core/world/world.rs b/flecs_ecs/src/core/world/world.rs index 465626e1..cfa8d6fe 100644 --- a/flecs_ecs/src/core/world/world.rs +++ b/flecs_ecs/src/core/world/world.rs @@ -4,7 +4,6 @@ use flecs_ecs_sys as sys; use crate::core::{ FlecsArray, FlecsIdMap, QueryBuilderImpl, SystemAPI, WorldCtx, ecs_os_api, flecs, - has_default_hook, }; /// The `World` is the container for all ECS data. It stores the entities and @@ -72,21 +71,11 @@ impl Default for World { .observer::() .with((flecs::With, flecs::Wildcard)) .run(|mut it| { - let world = it.world().ptr_mut(); - while it.next() { - for _ in it.iter() { - let pair_id = it.pair(0); - let second_id = pair_id.second_id(); - let raw_second_id = **second_id; - let is_not_tag = unsafe { sys::ecs_get_typeid(world, raw_second_id) != 0 }; - - if is_not_tag { - assert!( - has_default_hook(world, raw_second_id), - "flecs::with requires a default-constructible target or be a ZST. Implement Default for {second_id}." - ); - } - } + if it.next() { + // Known bug in flecs that allows operating on uninitialized data in safe code using flecs::With rather trivially when using ecs_emplace_id. + panic!( + "flecs::with is inherently unsafe as the default constructor will not be called under certain circumstances. This is a known bug within flecs." + ); } }); diff --git a/flecs_ecs/tests/flecs/component_lifecycle_test.rs b/flecs_ecs/tests/flecs/component_lifecycle_test.rs index c0454dd3..7c77a892 100644 --- a/flecs_ecs/tests/flecs/component_lifecycle_test.rs +++ b/flecs_ecs/tests/flecs/component_lifecycle_test.rs @@ -1,4 +1,9 @@ #![allow(dead_code)] +use std::{ + collections::HashSet, + sync::{LazyLock, Mutex}, +}; + use crate::common_test::*; #[test] @@ -52,3 +57,127 @@ fn component_lifecycle_count_in_remove_hook() { assert_eq!(world.cloned::<&Count>().0, 0); } + +static INITIALIZED_BOXES: LazyLock>> = + LazyLock::new(|| Mutex::new(HashSet::new())); + +#[derive(Component, Clone)] +struct BoxedNumber { + number: Box, + id: String, + dropped: bool, +} + +impl BoxedNumber { + fn new(id: &str) -> Self { + INITIALIZED_BOXES.lock().unwrap().insert(id.to_string()); + Self { + number: Box::new(10), + id: id.to_string(), + dropped: false, + } + } +} + +impl Drop for BoxedNumber { + fn drop(&mut self) { + assert!(!self.dropped, "double-free detected"); + self.dropped = true; + INITIALIZED_BOXES.lock().unwrap().remove(&self.id); + } +} + +#[test] +fn component_lifecycle_drop_on_remove() { + let world = World::new(); + + world + .entity_named("object 1") + .set(BoxedNumber::new("Object 1")); + world + .entity_named("object 2") + .set(BoxedNumber::new("Object 2")); + world + .entity_named("object 3") + .set(BoxedNumber::new("Object 3")); + + world.defer_begin(); + world.query::<&BoxedNumber>().build().each_entity(|ent, _| { + ent.each_component(|e| { + ent.remove(e); + }); + }); + world.defer_end(); + + let init_boxes = INITIALIZED_BOXES.lock().unwrap(); + assert!( + init_boxes.is_empty(), + "Leaked memory, objects not properly deleted: {:?}", + init_boxes + ); +} + +#[test] +fn component_lifecycle_set_singleton() { + { + let world = World::new(); + + world + .component::() + .add_trait::(); + + world.system::<()>().run(|it| { + let world = it.world(); + world.set(BoxedNumber::new("Object 1")); + }); + + world.progress(); + } +} + +#[test] +fn component_lifecycle_drop_on_world_delete() { + { + let world = World::new(); + + world + .entity_named("object 1") + .set(BoxedNumber::new("Object 1")); + world + .entity_named("object 2") + .set(BoxedNumber::new("Object 2")); + world + .entity_named("object 3") + .set(BoxedNumber::new("Object 3")); + + world.quit(); + + world.progress(); + } + + let init_boxes = INITIALIZED_BOXES.lock().unwrap(); + assert!( + init_boxes.is_empty(), + "Leaked memory, objects not properly deleted: {:?}", + init_boxes + ); +} + +#[test] +fn component_lifecycle_set_multiple_times() { + let world = World::new(); + + let ent = world + .entity_named("object 1") + .set(BoxedNumber::new("Object 1")); + ent.set(BoxedNumber::new("Object 2")); + ent.set(BoxedNumber::new("Object 3")); + ent.destruct(); + + let init_boxes = INITIALIZED_BOXES.lock().unwrap(); + assert!( + init_boxes.is_empty(), + "Leaked memory, objects not properly deleted: {:?}", + init_boxes + ); +} diff --git a/flecs_ecs/tests/flecs/derive_attr_component_traits.rs b/flecs_ecs/tests/flecs/derive_attr_component_traits.rs index 1362fe73..b07bffe2 100644 --- a/flecs_ecs/tests/flecs/derive_attr_component_traits.rs +++ b/flecs_ecs/tests/flecs/derive_attr_component_traits.rs @@ -72,9 +72,9 @@ mod component_traits_attributes { struct TDontFragment; // Pair-trait components (relationship, target) - #[derive(Component)] - #[flecs(traits((With, _Y)))] - struct TWithY; + // #[derive(Component)] + // #[flecs(traits((With, _Y)))] + // struct TWithY; #[derive(Component)] #[flecs(traits((OneOf, Group)))] @@ -133,8 +133,8 @@ mod component_traits_attributes { assert!(c.has(flecs::DontFragment)); // Pairs - let c = world.component::(); - assert!(c.has((flecs::With, _Y))); + // let c = world.component::(); + // assert!(c.has((flecs::With, _Y))); let c = world.component::(); assert!(c.has((flecs::OneOf, self::Group))); diff --git a/flecs_ecs_sys/Cargo.toml b/flecs_ecs_sys/Cargo.toml index 08c2cff2..e0cdf6b6 100644 --- a/flecs_ecs_sys/Cargo.toml +++ b/flecs_ecs_sys/Cargo.toml @@ -136,5 +136,4 @@ flecs_base = [ # Default features default = [ "flecs_meta_c", - "regenerate_binding", ] diff --git a/flecs_ecs_sys/src/flecs.c b/flecs_ecs_sys/src/flecs.c index 86aef27f..368de751 100644 --- a/flecs_ecs_sys/src/flecs.c +++ b/flecs_ecs_sys/src/flecs.c @@ -41464,11 +41464,12 @@ void flecs_table_delete( /* If neither move nor move_ctor are set, this indicates that * non-destructive move semantics are not supported for this - * type. In such cases, we set the move_dtor as ctor_move_dtor, - * which indicates a destructive move operation. This adjustment - * ensures compatibility with different language bindings. */ - if (!ti->hooks.move_ctor && ti->hooks.ctor_move_dtor) { - move_dtor = ti->hooks.ctor_move_dtor; + * type. In such cases when we are performing a non-destructive deletion, + * we set the move_dtor as ctor_move_dtor, which indicates we are operating + * on memory already cleaned up. This adjustment ensures compatibility + * with different language bindings. */ + if (!destruct && !ti->hooks.move_ctor) { + move_dtor = ti->hooks.ctor_move_dtor; } if (move_dtor) { @@ -41605,8 +41606,14 @@ void flecs_table_move( flecs_table_invoke_add_hooks(world, dst_table, i_new, &dst_entity, dst_index, 1, construct); } else { + ecs_type_info_t *ti = dst_column->ti; + + /* If the component doesn't have a move/move_ctor, + * then it doesn't support non-destructive moves and must + * be destructed during its removal. + */ flecs_table_invoke_remove_hooks(world, src_table, - src_column, &src_entity, src_index, 1, use_move_dtor); + src_column, &src_entity, src_index, 1, use_move_dtor || !ti->hooks.move_ctor); } } @@ -41620,8 +41627,11 @@ void flecs_table_move( } for (; (i_old < src_column_count); i_old ++) { + ecs_column_t *src_column = &src_columns[i_new]; + ecs_type_info_t *ti = src_column->ti; + flecs_table_invoke_remove_hooks(world, src_table, &src_columns[i_old], - &src_entity, src_index, 1, use_move_dtor); + &src_entity, src_index, 1, use_move_dtor || !ti->hooks.move_ctor); } flecs_table_check_sanity(dst_table);