feat: stabilise script systems (lifecycle, errors, soundness)

assets/tests/add_system/adding_system_again_replaces_it.lua

@@ -0,0 +1,37 @@
+-- adding a system with the same callback label for the same script replaces the old one
+local ScriptAttachment = import_namespace("bms").ScriptAttachment
+local bms = import_namespace("bms")
+local world = bms.world
+runs = {}
+local ResourceType = world.get_type_by_name("test_utils::test_data::TestResource")
+
+function on_test()
+    local post_update_schedule = world.get_schedule_by_name("PostUpdate")
+    local script_attachment = ScriptAttachment.new_entity_script(bms.current_entity(), bms.current_script())
+
+    world.add_system(
+        post_update_schedule,
+        bms.system_builder("my_replaced_system", script_attachment)
+    )
+    world.add_system(
+        post_update_schedule,
+        bms.system_builder("my_replaced_system", script_attachment)
+        :resource(ResourceType)
+    )
+
+    return true
+end
+
+function my_replaced_system(resource)
+    runs[#runs + 1] = resource
+end
+
+function on_test_post_update()
+    return true
+end
+
+function on_test_last()
+    assert(#runs == 1, "Expected 1 run, got: " .. #runs)
+    assert(runs[1] ~= nil, "Expected the replacement with the resource parameter to run")
+    return true
+end

assets/tests/add_system/system_cannot_access_component_outside_query_archetypes.lua

@@ -0,0 +1,64 @@
+-- bevy runs a system querying `A` with `B` in parallel with systems accessing `A` without `B`,
+-- so the script system must not reach `A` on entities without `B`, even though it has access to `A`
+local ScriptAttachment = import_namespace("bms").ScriptAttachment
+local bms = import_namespace("bms")
+local world = bms.world
+runs = {}
+local ComponentA = world.get_type_by_name("test_utils::test_data::CompWithFromWorldAndComponentData")
+local ComponentB = world.get_type_by_name("test_utils::test_data::CompWithDefaultAndComponentData")
+
+function on_test()
+    local post_update_schedule = world.get_schedule_by_name("PostUpdate")
+    local script_attachment = ScriptAttachment.new_entity_script(bms.current_entity(), bms.current_script())
+
+    local matched = world.spawn()
+    world.add_default_component(matched, ComponentA)
+    world.add_default_component(matched, ComponentB)
+
+    outsider = world.spawn()
+    world.add_default_component(outsider, ComponentA)
+    assert(world._archetype_of(matched) ~= world._archetype_of(outsider),
+        "the entities should live in different archetypes")
+
+    world.add_system(
+        post_update_schedule,
+        bms.system_builder("my_filtered_system", script_attachment)
+        :query(world.query():component(ComponentA):with(ComponentB))
+    )
+
+    return true
+end
+
+function my_filtered_system(query)
+    runs[#runs + 1] = "my_filtered_system"
+    local ok, err = pcall(check_accesses, query)
+    runs[#runs + 1] = ok and "finished" or tostring(err)
+end
+
+function check_accesses(query)
+    assert(#query == 1, "Expected 1 result, got: " .. #query)
+    local matched = query[1]
+    assert(matched:components()[1][1] == "Default", "the matched component should be accessible")
+    assert(world.get_component(matched:entity(), ComponentA)[1] == "Default",
+        "the matched component should be accessible through the world too")
+
+    assert_throws(function()
+        local component = world.get_component(outsider, ComponentA)
+        local _ = component[1]
+    end, ".*not matched by any query.*")
+
+    -- `with` filters by `B` without accessing it
+    assert_throws(function()
+        local component = world.get_component(matched:entity(), ComponentB)
+        local _ = component[1]
+    end, ".*annot claim access.*")
+end
+
+function on_test_post_update()
+    return true
+end
+
+function on_test_last()
+    assert(#runs == 2 and runs[2] == "finished", "the system did not finish, runs: " .. table.concat(runs, ","))
+    return true
+end

assets/tests/add_system/system_with_parameters.lua

@@ -49,16 +49,18 @@ function my_parameterised_system(resourceA, query, resourceB)
         assert(#components == 2, "Expected 2 components, got " .. #components)
         local componentA = components[1]
         local componentB = components[2]
-        assert(componentA._1 == "Default", "Expected 'Default', got: " .. componentA._1)
-        assert(componentB._1 == "Default", "Expected 'Default', got: " .. componentA._1)
+        assert(componentA[1] == "Default", "Expected 'Default', got: " .. componentA[1])
+        assert(componentB[1] == "Default", "Expected 'Default', got: " .. componentB[1])
     end
+    -- errors inside systems are only logged, so record that we got to the end
+    runs[#runs + 1] = "finished"
 end
 
 function on_test_post_update()
     return true
 end
 
 function on_test_last()
-    assert(#runs == 1, "Expected 1 runs, got: " .. #runs)
+    assert(#runs == 2 and runs[2] == "finished", "the system did not finish, runs: " .. table.concat(runs, ","))
     return true
 end

crates/bevy_mod_scripting_bindings/src/reference.rs

@@ -484,6 +484,10 @@ impl ReflectReference {
             .get_type_data(self.base.type_id)
             .ok_or_else(|| InteropError::unregistered_base(self.base.clone()))?;
 
+        if let ReflectBase::Component(entity, component_id) = &self.base.base_id {
+            world.check_archetype_access(*entity, *component_id)?;
+        }
+
         let ptr = unsafe {
             self.base
                 .base_id
@@ -556,6 +560,10 @@ impl ReflectReference {
             .get_type_data(self.base.type_id)
             .ok_or_else(|| InteropError::unregistered_base(self.base.clone()))?;
 
+        if let ReflectBase::Component(entity, component_id) = &self.base.base_id {
+            world.check_archetype_access(*entity, *component_id)?;
+        }
+
         let ptr = unsafe {
             self.base
                 .base_id

crates/bevy_mod_scripting_core/src/handler.rs

@@ -1126,6 +1126,7 @@ mod test {
             WorldAccessGuard::new_non_exclusive(
                 world.as_unsafe_world_cell(),
                 AccessByteSet::from_allowed_list(&[]),
+                None,
                 type_registry,
                 cache,
             )

crates/bevy_mod_scripting_core/src/lib.rs

@@ -16,8 +16,9 @@ use crate::{
     },
     pipeline::ScriptLoadingPipeline,
     script::{ScriptComponentsChangeCache, script_component_changed_handler},
+    script_system::{ScriptSystems, apply_script_system_changes},
 };
-use bevy_app::{App, Last, Plugin, PostUpdate, PreUpdate};
+use bevy_app::{App, First, Last, Plugin, PostUpdate, PreUpdate};
 use bevy_asset::{AssetApp, Handle};
 use bevy_ecs::schedule::IntoScheduleConfigs;
 use bevy_ecs::{
@@ -392,6 +393,15 @@ impl Plugin for BMSScriptingInfrastructurePlugin {
             run_queued_callbacks_system.in_set(ScriptingSystemSet::RunQueuedCallbacks),
         );
 
+        // changes to script systems in running schedules are applied in between schedules, twice so that
+        // changes to `Last` itself are applied too
+        app.init_resource::<ScriptSystems>()
+            .add_systems(First, apply_script_system_changes)
+            .add_systems(
+                Last,
+                apply_script_system_changes.after(ScriptingSystemSet::RunQueuedCallbacks),
+            );
+
         if !self.dont_log_script_event_errors {
             app.add_systems(PostUpdate, script_error_logger);
         }

crates/bevy_mod_scripting_core/src/pipeline/hooks.rs

@@ -9,6 +9,7 @@ use bevy_mod_scripting_script::ScriptAttachment;
 use crate::{
     commands::RunScriptCallback,
     event::{IntoCallbackLabel, OnScriptLoaded, OnScriptReloaded, OnScriptUnloaded},
+    script_system::remove_script_systems,
 };
 
 use super::*;
@@ -56,78 +57,89 @@ pub fn on_script_unloaded_for_unload_pipeline_handler<P: IntoScriptPluginParams>
     trigger: On<UnloadingInitialized<P>>,
     mut commands: Commands,
     world_id: WorldId,
+    run_callback: bool,
 ) {
     let event = trigger.event();
-    let emit_responses = P::readonly_configuration(world_id).emit_responses;
-    // let guard = WorldGuard::new_exclusive(world);
-    bevy_log::debug!(
-        "Running on_script_unloaded hook for script: {}, due to unload",
-        event.attachment
-    );
-    commands.queue(
-        RunScriptCallback::<P>::new(
-            event.attachment.clone(),
-            OnScriptUnloaded::into_callback_label(),
-            vec![],
-            emit_responses,
-        )
-        .with_context_override(trigger.existing_context.clone())
-        .with_post_callback_handler(|world, attachment, response| {
-            if let Ok(v) = response.as_ref() {
-                bevy_log::debug!(
-                    "on_script_unloaded hook for script: {} setting reload_state: {:?}",
-                    attachment,
-                    v
-                );
-                let mut data = world.get_resource_or_init::<ActiveMachinesData>();
-                let data = data.0.entry(attachment).or_default();
-
-                data.reload_state = v.clone();
-            }
-        }),
-    );
+    if run_callback {
+        let emit_responses = P::readonly_configuration(world_id).emit_responses;
+        // let guard = WorldGuard::new_exclusive(world);
+        bevy_log::debug!(
+            "Running on_script_unloaded hook for script: {}, due to unload",
+            event.attachment
+        );
+        commands.queue(
+            RunScriptCallback::<P>::new(
+                event.attachment.clone(),
+                OnScriptUnloaded::into_callback_label(),
+                vec![],
+                emit_responses,
+            )
+            .with_context_override(trigger.existing_context.clone())
+            .with_post_callback_handler(|world, attachment, response| {
+                if let Ok(v) = response.as_ref() {
+                    bevy_log::debug!(
+                        "on_script_unloaded hook for script: {} setting reload_state: {:?}",
+                        attachment,
+                        v
+                    );
+                    let mut data = world.get_resource_or_init::<ActiveMachinesData>();
+                    let data = data.0.entry(attachment).or_default();
+
+                    data.reload_state = v.clone();
+                }
+            }),
+        );
+    }
     remove_registrations_when_unloaded(&mut commands, event.attachment.clone());
 }
 
-/// Once the unload callback ran, removes what the script registered which only makes sense while it runs, i.e. its exports and observers.
+/// Removes the registrations and script systems of a script which is unloaded, or reloaded (it adds them again once loaded).
+///
+/// Runs whether or not the unload callback is enabled, after the callback.
 fn remove_registrations_when_unloaded(commands: &mut Commands, attachment: ScriptAttachment) {
-    commands.queue(move |world: &mut World| remove_script_registrations(world, &attachment));
+    commands.queue(move |world: &mut World| {
+        remove_script_registrations(world, &attachment);
+        remove_script_systems(world, &attachment);
+    });
 }
-// todo unify the two, they just need a context extractor trait
+
 pub fn on_script_unloaded_for_reload_pipeline_handler<P: IntoScriptPluginParams>(
     trigger: On<ReloadingInitialized<P>>,
     mut commands: Commands,
     world_id: WorldId,
+    run_callback: bool,
 ) {
     let event = trigger.event();
-    let emit_responses = P::readonly_configuration(world_id).emit_responses;
-    bevy_log::debug!(
-        "Running on_script_unloaded hook for script: {}, due to reload",
-        event.attachment
-    );
-    commands.queue(
-        RunScriptCallback::<P>::new(
-            event.attachment.clone(),
-            OnScriptUnloaded::into_callback_label(),
-            vec![],
-            emit_responses,
-        )
-        .with_context_override(trigger.existing_context.clone())
-        .with_post_callback_handler(|world, attachment, response| {
-            if let Ok(v) = response.as_ref() {
-                bevy_log::debug!(
-                    "on_script_unloaded hook for script: {} setting reload_state: {:?}",
-                    attachment,
-                    v
-                );
-                let mut datas = world.get_resource_or_init::<ActiveMachinesData>();
-                let data = datas.0.entry(attachment).or_default();
-
-                data.reload_state = v.clone();
-                bevy_log::debug!("Datas: {}", datas.0.len());
-            }
-        }),
-    );
+    if run_callback {
+        let emit_responses = P::readonly_configuration(world_id).emit_responses;
+        bevy_log::debug!(
+            "Running on_script_unloaded hook for script: {}, due to reload",
+            event.attachment
+        );
+        commands.queue(
+            RunScriptCallback::<P>::new(
+                event.attachment.clone(),
+                OnScriptUnloaded::into_callback_label(),
+                vec![],
+                emit_responses,
+            )
+            .with_context_override(trigger.existing_context.clone())
+            .with_post_callback_handler(|world, attachment, response| {
+                if let Ok(v) = response.as_ref() {
+                    bevy_log::debug!(
+                        "on_script_unloaded hook for script: {} setting reload_state: {:?}",
+                        attachment,
+                        v
+                    );
+                    let mut datas = world.get_resource_or_init::<ActiveMachinesData>();
+                    let data = datas.0.entry(attachment).or_default();
+
+                    data.reload_state = v.clone();
+                    bevy_log::debug!("Datas: {}", datas.0.len());
+                }
+            }),
+        );
+    }
     remove_registrations_when_unloaded(&mut commands, event.attachment.clone());
 }
 

crates/bevy_mod_scripting_core/src/pipeline/mod.rs

@@ -240,11 +240,23 @@ impl<P: IntoScriptPluginParams> Plugin for ScriptLoadingPipeline<P> {
             );
         }
 
-        // these are the only triggers on their events
-        if self.on_script_unloaded_callback {
-            app.add_observer(on_script_unloaded_for_unload_pipeline_handler::<P>);
-            app.add_observer(on_script_unloaded_for_reload_pipeline_handler::<P>);
-        }
+        // these are the only triggers on their events, so the unload callback runs before the script's
+        // registrations and systems are removed
+        let unloaded = self.on_script_unloaded_callback;
+        app.add_observer(
+            move |trigger: On<UnloadingInitialized<P>>, commands: Commands, world_id: WorldId| {
+                on_script_unloaded_for_unload_pipeline_handler(
+                    trigger, commands, world_id, unloaded,
+                )
+            },
+        );
+        app.add_observer(
+            move |trigger: On<ReloadingInitialized<P>>, commands: Commands, world_id: WorldId| {
+                on_script_unloaded_for_reload_pipeline_handler(
+                    trigger, commands, world_id, unloaded,
+                )
+            },
+        );
 
         // clear hanging machine data
         app.add_observer(