fix unsafeties

assets/tests/register_new_component/new_component_can_be_set.lua

@@ -0,0 +1,17 @@
+function on_test()
+    local NewComponent = world.register_new_component("ScriptComponentA")
+
+    local new_entity = world.spawn()
+    world.insert_component(new_entity, NewComponent, construct(types.ScriptComponent, {
+        data = "Hello World"
+    }))
+
+    local component_instance = world.get_component(new_entity, NewComponent)
+    assert(component_instance.data == "Hello World", "unexpected value: " .. component_instance.data)
+
+    component_instance.data = {
+        foo = "bar"
+    }
+
+    assert(component_instance.data.foo == "bar", "unexpected value: " .. component_instance.data.foo)
+end

crates/bevy_mod_scripting_core/src/bindings/function/from_ref.rs

@@ -1,15 +1,15 @@
 //! Contains the [`FromScriptRef`] trait and its implementations.
 
-use std::{any::TypeId, ffi::OsString, path::PathBuf};
-use bevy::reflect::{
-    DynamicEnum, DynamicList, DynamicMap, DynamicTuple, DynamicVariant, Map, PartialReflect,
-};
 use crate::{
-    bindings::{match_by_type, WorldGuard, FromScript},
+    bindings::{match_by_type, FromScript, WorldGuard},
     error::InteropError,
     reflection_extensions::TypeInfoExtensions,
     ScriptValue,
 };
+use bevy::reflect::{
+    DynamicEnum, DynamicList, DynamicMap, DynamicTuple, DynamicVariant, Map, PartialReflect,
+};
+use std::{any::TypeId, ffi::OsString, path::PathBuf};
 
 /// Converts from a [`ScriptValue`] to a value equivalent to the given [`TypeId`].
 ///
@@ -56,6 +56,7 @@ impl FromScriptRef for Box<dyn PartialReflect> {
                 tq : String  => return <String>::from_script(value, world).map(|a| Box::new(a) as _),
                 tr : PathBuf => return <PathBuf>::from_script(value, world).map(|a| Box::new(a) as _),
                 ts : OsString=> return <OsString>::from_script(value, world).map(|a| Box::new(a) as _),
+                tsv: ScriptValue => return <ScriptValue>::from_script(value, world).map(|a| Box::new(a) as _),
                 tn : ()      => return <()>::from_script(value, world).map(|a| Box::new(a) as _)
             }
         );

crates/bevy_mod_scripting_core/src/bindings/function/into_ref.rs

@@ -102,6 +102,7 @@ fn into_script_ref(
             },
             tr : PathBuf => return downcast_into_value!(r, PathBuf).clone().into_script(world),
             ts : OsString=> return downcast_into_value!(r, OsString).clone().into_script(world),
+            tsv: ScriptValue=> return Ok(downcast_into_value!(r, ScriptValue).clone()),
             tn : ()      => return Ok(ScriptValue::Unit)
         }
     );

crates/bevy_mod_scripting_core/src/bindings/query.rs

@@ -146,16 +146,19 @@ impl ScriptComponentRegistration {
                 let mut entity = world
                     .get_entity_mut(entity)
                     .map_err(|_| InteropError::missing_entity(entity))?;
-                let mut cast = instance.downcast::<ScriptComponent>().map_err(|v| {
+                let cast = instance.downcast::<ScriptComponent>().map_err(|v| {
                     InteropError::type_mismatch(TypeId::of::<ScriptComponent>(), Some(v.type_id()))
                 })?;
-                let ptr = (cast.as_mut() as *mut ScriptComponent).cast();
+                // the reason we leak the box, is because we don't want to double drop the owning ptr
+
+                let ptr = (Box::leak(cast) as *mut ScriptComponent).cast();
                 // Safety: cannot be null as we just created it from a valid reference
                 let non_null_ptr = unsafe { NonNull::new_unchecked(ptr) };
                 // Safety:
                 // - we know the type is ScriptComponent, as we just created the pointer
                 // - the box will stay valid for the life of this function, and we do not return the ptr
                 // - pointer is alligned correctly
+                // - nothing else will call drop on this
                 let owning_ptr = unsafe { OwningPtr::new(non_null_ptr) };
                 // Safety:
                 // - Owning Ptr is valid as we just created it

crates/bevy_mod_scripting_core/src/bindings/script_component.rs

@@ -80,8 +80,23 @@ impl WorldAccessGuard<'_> {
                 "script registered component name must start with 'Script'",
             ));
         }
+        let component_registry = self.component_registry();
+        let component_registry_read = component_registry.read();
+        if component_registry_read.get(&component_name).is_some() {
+            return Err(InteropError::unsupported_operation(
+                None,
+                None,
+                "script registered component already exists",
+            ));
+        }
 
         let component_id = self.with_global_access(|w| {
+            bevy::log::info!(
+                "components present: {}. script: {}. World id: {:?}",
+                w.components().len(),
+                component_registry_read.components.len(),
+                w.id()
+            );
             let descriptor = unsafe {
                 // Safety: same safety guarantees as ComponentDescriptor::new
                 // we know the type in advance
@@ -93,10 +108,11 @@ impl WorldAccessGuard<'_> {
                     needs_drop::<ScriptComponent>().then_some(|x| x.drop_as::<ScriptComponent>()),
                 )
             };
-            w.register_component_with_descriptor(descriptor)
+            let o = w.register_component_with_descriptor(descriptor);
+            bevy::log::info!("components present after: {}", w.components().len());
+            o
         })?;
-
-        let component_registry = self.component_registry();
+        drop(component_registry_read);
         let mut component_registry = component_registry.write();
 
         let registration = ScriptComponentRegistration::new(
@@ -135,51 +151,53 @@ impl Plugin for DynamicScriptComponentPlugin {
     }
 }
 
-// #[cfg(test)]
-// mod test {
-//     use std::ptr::NonNull;
-
-//     use super::*;
-//     use bevy::{ecs::world::World, ptr::OwningPtr};
-
-//     #[test]
-//     fn test_script_component() {
-//         let mut world = World::new();
-//         let component_name = "MyScriptComponent";
-
-//         #[derive(Reflect, Component)]
-//         struct UnderlyingComponent;
-
-//         // initialize component descriptor dynamically
-//         let descriptor = unsafe {
-//             // Safety: same safety guarantees as ComponentDescriptor::new
-//             // we know the type in advance
-//             // we only use this method to name the component
-//             ComponentDescriptor::new_with_layout(
-//                 component_name,
-//                 UnderlyingComponent::STORAGE_TYPE,
-//                 Layout::new::<UnderlyingComponent>(),
-//                 needs_drop::<UnderlyingComponent>()
-//                     .then_some(|x| x.drop_as::<UnderlyingComponent>()),
-//             )
-//         };
-
-//         // register with the world
-//         let component_id = world.register_component_with_descriptor(descriptor);
-
-//         // insert into the entity
-//         let entity = world.spawn_empty().id();
-//         let mut entity = world.entity_mut(entity);
-
-//         let value = Box::new(UnderlyingComponent);
-//         let value_ref = Box::into_raw(value).cast::<u8>();
-//         let ptr = unsafe { OwningPtr::new(NonNull::new(value_ref).unwrap()) };
-//         unsafe { entity.insert_by_id(component_id, ptr) };
-
-//         // check it gets inserted
-//         assert!(
-//             entity.contains_id(component_id),
-//             "entity does not contain freshly inserted component"
-//         )
-//     }
-// }
+#[cfg(test)]
+mod test {
+    use std::ptr::NonNull;
+
+    use super::*;
+    use bevy::{ecs::world::World, ptr::OwningPtr};
+
+    #[test]
+    fn test_script_component() {
+        let mut world = World::new();
+        let component_name = "MyScriptComponent";
+
+        #[derive(Reflect, Component)]
+        struct UnderlyingComponent;
+
+        // initialize component descriptor dynamically
+        let descriptor = unsafe {
+            // Safety: same safety guarantees as ComponentDescriptor::new
+            // we know the type in advance
+            // we only use this method to name the component
+            ComponentDescriptor::new_with_layout(
+                component_name,
+                UnderlyingComponent::STORAGE_TYPE,
+                Layout::new::<UnderlyingComponent>(),
+                needs_drop::<UnderlyingComponent>()
+                    .then_some(|x| x.drop_as::<UnderlyingComponent>()),
+            )
+        };
+
+        // register with the world
+        let component_id = world.register_component_with_descriptor(descriptor.clone());
+        let component_id_2 = world.register_component_with_descriptor(descriptor);
+        assert_eq!(component_id, component_id_2); // iam getting a double free for this in scritps somehow
+
+        // insert into the entity
+        let entity = world.spawn_empty().id();
+        let mut entity = world.entity_mut(entity);
+
+        let value = Box::new(UnderlyingComponent);
+        let value_ref = Box::into_raw(value).cast::<u8>();
+        let ptr = unsafe { OwningPtr::new(NonNull::new(value_ref).unwrap()) };
+        unsafe { entity.insert_by_id(component_id, ptr) };
+
+        // check it gets inserted
+        assert!(
+            entity.contains_id(component_id),
+            "entity does not contain freshly inserted component"
+        )
+    }
+}

crates/bevy_mod_scripting_core/src/bindings/world.rs

@@ -961,31 +961,20 @@ impl WorldAccessGuard<'_> {
         registration: ScriptComponentRegistration,
         value: ReflectReference,
     ) -> Result<(), InteropError> {
-        let component_data = registration
-            .type_registration()
-            .type_registration()
-            .data::<ReflectComponent>()
-            .ok_or_else(|| {
-                InteropError::missing_type_data(
-                    registration.registration.type_id(),
-                    "ReflectComponent".to_owned(),
-                )
-            })?;
-
-        with_global_access!(&self.inner.accesses, "Could not insert element", {
-            let cell = self.as_unsafe_world_cell()?;
-            let type_registry = self.type_registry();
-            let type_registry = type_registry.read();
-            let world_mut = unsafe { cell.world_mut() };
-            let mut entity = world_mut
-                .get_entity_mut(entity)
-                .map_err(|_| InteropError::missing_entity(entity))?;
+        let instance = <Box<dyn PartialReflect>>::from_script_ref(
+            registration.type_registration().type_id(),
+            ScriptValue::Reference(value),
+            self.clone(),
+        )?;
 
-            let ref_ = unsafe { value.reflect_unsafe(self.clone())? };
-            component_data.apply_or_insert(&mut entity, ref_, &type_registry);
+        let reflect = instance.try_into_reflect().map_err(|v| {
+            InteropError::failed_from_reflect(
+                Some(registration.type_registration().type_id()),
+                format!("instance produced by conversion to target type when inserting component is not a full reflect type: {v:?}"),
+            )
+        })?;
 
-            Ok(())
-        })?
+        registration.insert_into_entity(self.clone(), entity, reflect)
     }
 
     /// get the component from the entity

crates/bevy_mod_scripting_core/src/error.rs

@@ -401,10 +401,10 @@ impl InteropError {
 
     /// Thrown if a type cannot be converted from reflect, this can happen if the type was unable to
     /// re-construct itself from a dynamic value.
-    pub fn failed_from_reflect(type_id: Option<TypeId>, reason: String) -> Self {
+    pub fn failed_from_reflect(type_id: Option<TypeId>, reason: impl Into<String>) -> Self {
         Self(Arc::new(InteropErrorInner::FailedFromReflect {
             type_id,
-            reason,
+            reason: reason.into(),
         }))
     }
 

crates/bevy_mod_scripting_core/src/lib.rs

@@ -252,6 +252,7 @@ fn once_per_app_finalize(app: &mut App) {
     if app.world().contains_resource::<BMSFinalized>() {
         return;
     }
+    println!("Running init");
     app.insert_resource(BMSFinalized);
 
     // read extensions from asset settings

crates/bevy_mod_scripting_functions/src/core.rs

@@ -1287,7 +1287,7 @@ impl GlobalNamespace {
         let reflect_val = val.try_into_reflect().map_err(|_| {
             InteropError::failed_from_reflect(
                 Some(registration.type_id()),
-                "Could not construct the type".into(),
+                "Could not construct the type",
             )
         })?;
 

crates/languages/bevy_mod_scripting_lua/tests/lua_tests.rs

@@ -14,12 +14,15 @@ use std::{
     path::{Path, PathBuf},
 };
 
+#[derive(Debug)]
 struct Test {
     path: PathBuf,
 }
 
 impl Test {
     fn execute(self) -> Result<(), Failed> {
+        println!("Running test: {:?}", self.path);
+
         execute_integration_test::<LuaScriptingPlugin, _, _>(
             |world, type_registry| {
                 let _ = world;
@@ -126,10 +129,11 @@ fn main() {
     let args = Arguments::from_args();
 
     // Create a list of tests and/or benchmarks (in this case: two dummy tests).
-    let tests = discover_all_tests()
+    let all_tests = discover_all_tests();
+    println!("discovered {} tests. {:?}", all_tests.len(), all_tests);
+    let tests = all_tests
         .into_iter()
         .map(|t| Trial::test(t.name(), move || t.execute()));
-
     // Run all tests and exit the application appropriatly.
     libtest_mimic::run(&args, tests.collect()).exit();
 }