feat: warn on duplicate globals (#486)

Author: makspll

benches/benchmarks.rs

@@ -266,22 +266,13 @@ fn conversion_benchmarks(criterion: &mut Criterion) {
 
 fn script_load_benchmarks(criterion: &mut Criterion) {
     let mut group = criterion.benchmark_group("loading");
-    let reload_probability = 0.5;
     // lua
-    let plugin = make_test_lua_plugin();
     let content = include_str!("../assets/macro_benchmarks/loading/empty.lua");
-    run_plugin_script_load_benchmark(plugin, "empty Lua", content, &mut group, reload_probability);
+    run_plugin_script_load_benchmark(make_test_lua_plugin, "empty Lua", content, &mut group);
 
     // rhai
-    let plugin = make_test_rhai_plugin();
     let content = include_str!("../assets/macro_benchmarks/loading/empty.rhai");
-    run_plugin_script_load_benchmark(
-        plugin,
-        "empty Rhai",
-        content,
-        &mut group,
-        reload_probability,
-    );
+    run_plugin_script_load_benchmark(make_test_rhai_plugin, "empty Rhai", content, &mut group);
 }
 
 pub fn benches() {

crates/bevy_mod_scripting_bindings/src/globals/core.rs

@@ -7,6 +7,7 @@ use ::{
     bevy_reflect::TypeRegistration,
 };
 use bevy_app::App;
+use bevy_log::warn;
 use bevy_mod_scripting_asset::ScriptAsset;
 use bevy_mod_scripting_derive::script_globals;
 use bevy_platform::collections::HashMap;
@@ -63,6 +64,7 @@ impl Plugin for CoreScriptGlobalsPlugin {
         register_core_globals(app.world_mut());
     }
 }
+const MSG_DUPLICATE_GLOBAL: &str = "This can cause confusing issues for script users, use `CoreScriptGlobalsPlugin.filter` to filter out uneeded duplicate types.";
 
 #[profiling::function]
 fn register_static_core_globals(world: &mut World, filter: fn(&TypeRegistration) -> bool) {
@@ -82,20 +84,41 @@ fn register_static_core_globals(world: &mut World, filter: fn(&TypeRegistration)
         if let Some(global_name) = registration.type_info().type_path_table().ident() {
             let documentation = "A reference to the type, allowing you to call static methods.";
             let type_info = registration.type_info();
-            global_registry.register_static_documented_dynamic(
-                registration.type_id(),
-                into_through_type_info(type_info),
-                global_name.into(),
-                documentation.into(),
-            );
+            if global_registry
+                .register_static_documented_dynamic(
+                    registration.type_id(),
+                    into_through_type_info(type_info),
+                    global_name.into(),
+                    documentation.into(),
+                )
+                .is_some()
+            {
+                warn!(
+                    "Duplicate global registration for type: '{}'. {MSG_DUPLICATE_GLOBAL}",
+                    type_info.type_path_table().short_path()
+                )
+            };
         }
     }
 
     // register basic globals
-    global_registry.register_dummy::<World>("world", "The current ECS world.");
-    global_registry
-        .register_dummy::<Entity>("entity", "The entity this script is attached to if any.");
-    global_registry.register_dummy_typed::<Val<Handle<ScriptAsset>>>("script_asset", "the asset handle for this script. If the asset is ever unloaded, the handle will be less useful.");
+    if global_registry
+        .register_dummy::<World>("world", "The current ECS world.")
+        .is_some()
+    {
+        warn!("existing `world` global was replaced by the core `world` dummy type.")
+    };
+
+    if global_registry
+        .register_dummy::<Entity>("entity", "The entity this script is attached to if any.")
+        .is_some()
+    {
+        warn!("existing `entity` global was replaced by the core `entity` dummy type.")
+    }
+
+    if global_registry.register_dummy_typed::<Val<Handle<ScriptAsset>>>("script_asset", "the asset handle for this script. If the asset is ever unloaded, the handle will be less useful.").is_some() {
+        warn!("existing `script_asset` global was replaced by the core `script_asset` dummy type.")
+    };
 }
 
 #[script_globals(bms_bindings_path = "crate", name = "core_globals")]
@@ -128,7 +151,15 @@ impl CoreGlobals {
             let registration = guard.clone().get_type_registration(registration)?;
             let registration =
                 registration.map_both(Val::from, |u| u.map_both(Val::from, Val::from));
-            type_cache.insert(type_path.to_owned(), registration);
+            if type_cache
+                .insert(type_path.to_owned(), registration)
+                .is_some()
+            {
+                warn!(
+                    "duplicate entry inside `types` global for type: {}. {MSG_DUPLICATE_GLOBAL}",
+                    type_path
+                )
+            };
         }
 
         Ok(type_cache)

crates/bevy_mod_scripting_bindings/src/globals/mod.rs

@@ -116,6 +116,7 @@ impl ScriptGlobalsRegistry {
     }
 
     /// Inserts a global into the registry, returns the previous value if it existed
+    #[must_use]
     pub fn register<
         T: ScriptReturn + 'static + Typed,
         F: Fn(WorldGuard) -> Result<T, InteropError> + 'static + Send + Sync,
@@ -140,40 +141,43 @@ impl ScriptGlobalsRegistry {
     /// This can be useful for globals which you cannot expose normally.
     ///
     /// Dummy globals are stored as non-static instances, i.e. they're expected to be values not type references.
+    #[must_use]
     pub fn register_dummy<T: 'static>(
         &mut self,
         name: impl Into<Cow<'static, str>>,
         documentation: impl Into<Cow<'static, str>>,
-    ) {
+    ) -> Option<ScriptGlobalDummy> {
         self.dummies.insert(
             name.into(),
             ScriptGlobalDummy {
                 documentation: Some(documentation.into()),
                 type_id: TypeId::of::<T>(),
                 type_information: None,
             },
-        );
+        )
     }
 
     /// Typed equivalent to [`Self::register_dummy`].
+    #[must_use]
     pub fn register_dummy_typed<T: 'static + TypedThrough>(
         &mut self,
         name: impl Into<Cow<'static, str>>,
         documentation: impl Into<Cow<'static, str>>,
-    ) {
+    ) -> Option<ScriptGlobalDummy> {
         self.dummies.insert(
             name.into(),
             ScriptGlobalDummy {
                 documentation: Some(documentation.into()),
                 type_id: TypeId::of::<T>(),
                 type_information: Some(T::through_type_info()),
             },
-        );
+        )
     }
 
     /// Inserts a global into the registry, returns the previous value if it existed.
     ///
     /// This is a version of [`Self::register`] which stores type information regarding the global.
+    #[must_use]
     pub fn register_documented<
         T: TypedScriptReturn + 'static,
         F: Fn(WorldGuard) -> Result<T, InteropError> + 'static + Send + Sync,
@@ -195,7 +199,11 @@ impl ScriptGlobalsRegistry {
     }
 
     /// Registers a static global into the registry.
-    pub fn register_static<T: 'static + Typed>(&mut self, name: Cow<'static, str>) {
+    #[must_use]
+    pub fn register_static<T: 'static + Typed>(
+        &mut self,
+        name: Cow<'static, str>,
+    ) -> Option<ScriptGlobal> {
         self.globals.insert(
             name,
             ScriptGlobal {
@@ -204,17 +212,18 @@ impl ScriptGlobalsRegistry {
                 type_id: TypeId::of::<T>(),
                 type_information: into_through_type_info(T::type_info()),
             },
-        );
+        )
     }
 
     /// Registers a static global into the registry.
     ///
     /// This is a version of [`Self::register_static`] which stores rich type information regarding the global.
+    #[must_use]
     pub fn register_static_documented<T: TypedScriptReturn + 'static>(
         &mut self,
         name: Cow<'static, str>,
         documentation: Cow<'static, str>,
-    ) {
+    ) -> Option<ScriptGlobal> {
         self.globals.insert(
             name,
             ScriptGlobal {
@@ -223,19 +232,20 @@ impl ScriptGlobalsRegistry {
                 type_id: TypeId::of::<T>(),
                 type_information: T::through_type_info(),
             },
-        );
+        )
     }
 
     /// Registers a static global into the registry.
     ///
     /// This is a version of [`Self::register_static_documented`] which does not require compile time type knowledge.
+    #[must_use]
     pub fn register_static_documented_dynamic(
         &mut self,
         type_id: TypeId,
         type_information: ThroughTypeInfo,
         name: Cow<'static, str>,
         documentation: Cow<'static, str>,
-    ) {
+    ) -> Option<ScriptGlobal> {
         self.globals.insert(
             name,
             ScriptGlobal {
@@ -244,7 +254,7 @@ impl ScriptGlobalsRegistry {
                 type_id,
                 type_information,
             },
-        );
+        )
     }
 }
 
@@ -307,14 +317,14 @@ mod test {
     fn test_static_globals() {
         let mut registry = ScriptGlobalsRegistry::default();
 
-        registry.register_static::<i32>(Cow::Borrowed("foo"));
+        _ = registry.register_static::<i32>(Cow::Borrowed("foo"));
 
         let global = registry.get("foo").unwrap();
         assert!(global.maker.is_none());
         assert_eq!(global.type_id, TypeId::of::<i32>());
 
         // the same but documented
-        registry.register_static_documented::<i32>(
+        _ = registry.register_static_documented::<i32>(
             Cow::Borrowed("bar"),
             Cow::Borrowed("This is a test"),
         );

crates/bevy_mod_scripting_derive/src/derive/script_globals.rs

@@ -42,8 +42,11 @@ pub fn script_globals(
             let registry = world.get_resource_or_init::<#bms_bindings_path::globals::AppScriptGlobalsRegistry>();
             let mut registry = registry.write();
 
-            registry
-                #(#function_registrations)*;
+            #(
+                if (registry #function_registrations).is_some() {
+                    warn!("conflicting global registration under name: {}. This might cause confusing problems, use `CoreScriptGlobalsPlugin.filter` to filter out uneeded duplicate types.", stringify!(#function_name))
+                }
+            )*;
         }
     };
 

crates/testing_crates/script_integration_test_harness/src/lib.rs

@@ -4,7 +4,6 @@ pub mod test_functions;
 
 use std::{
     path::PathBuf,
-    sync::Mutex,
     time::{Duration, Instant},
 };
 
@@ -22,7 +21,6 @@ use ::{
     bevy_reflect::Reflect,
 };
 use bevy_asset::Assets;
-use bevy_ecs::world::World;
 use bevy_mod_scripting_asset::ScriptAsset;
 use bevy_mod_scripting_bindings::{
     CoreScriptGlobalsPlugin, ReflectAccessId, ThreadWorldContainer, WorldAccessGuard,
@@ -340,61 +338,40 @@ where
 
 pub fn run_plugin_script_load_benchmark<
     P: IntoScriptPluginParams + Plugin + FromWorld,
+    F: Fn() -> P,
     M: Measurement,
 >(
-    plugin: P,
+    plugin_maker: F,
     benchmark_id: &str,
     content: &str,
     criterion: &mut criterion::BenchmarkGroup<M>,
-    reload_probability: f32,
 ) {
-    let mut app = setup_integration_test(|_, _| {});
-    install_test_plugin(&mut app, false);
-    app.add_plugins(plugin);
     let content_boxed = content.to_string().into_bytes().into_boxed_slice();
-    let mut rng_guard = RNG.lock().unwrap();
-    *rng_guard = rand_chacha::ChaCha12Rng::from_seed([42u8; 32]);
-    drop(rng_guard);
-
-    let world_ptr = app.world_mut() as *mut World;
-    let world_guard = Mutex::<()>::new(());
 
     criterion.bench_function(benchmark_id, |c| {
         c.iter_batched(
             || {
-                let mut rng = RNG.lock().unwrap();
-                let is_reload = rng.random_range(0f32..=1f32) < reload_probability;
+                let mut app = setup_integration_test(|_, _| {});
+                install_test_plugin(&mut app, false);
+                app.add_plugins(plugin_maker());
 
-                let guard = world_guard.lock().unwrap();
                 // Safety: we claimed a unique guard, only code accessing this will need to do the same
-                let world = unsafe { &mut *world_ptr };
+                let world = app.world_mut();
                 let mut assets = world.get_resource_or_init::<Assets<ScriptAsset>>();
 
-                let id = is_reload
-                    .then(|| assets.ids().next())
-                    .flatten()
-                    .and_then(|id| assets.get_strong_handle(id))
-                    .unwrap_or_else(|| {
-                        assets.add(ScriptAsset {
-                            content: content_boxed.clone(),
-                            language: P::LANGUAGE,
-                        })
-                    });
-                drop(guard);
+                let id = assets.add(ScriptAsset {
+                    content: content_boxed.clone(),
+                    language: P::LANGUAGE,
+                });
 
                 // We manually load the script inside a command.
                 (
+                    app,
                     AttachScript::<P>::new(ScriptAttachment::StaticScript(id)),
-                    is_reload,
                 )
             },
-            |(command, _is_reload)| {
-                tracing::event!(
-                    Level::TRACE,
-                    "profiling_iter {} is reload?: {}",
-                    benchmark_id,
-                    _is_reload
-                );
+            |(mut app, command)| {
+                tracing::event!(Level::TRACE, "profiling_iter {}", benchmark_id);
                 command.apply(app.world_mut());
             },
             BatchSize::LargeInput,