Merge #737

737: InitHandle and NativeClass::new_instance() now refer to a global class registry to check if the class has been added. r=toasteater a=jacobsky

This is related to issue #602 where and it allows the `InitHandle` struct and `NativeClass` trait to properly track whether a class has been added to the godot API.

This is just my first pass at a naming scheme, so if there is a preferred naming convention to go by or modifying the error messages, please let me know.

For the "class_registry", I decided to use the RwLock as there shouldn't be any need for locking after initialization. I thought this would be a performance be a minimal number of writes after initialization. This should lower the performance cost for reads after that point. The registry consists of a simple hash set as outlined in the Issue

For the `NativeClass::new_instance()` method, I chose to use a debug_assert!(), to give a more concrete error message when using a debug-build, but remove the additional check during instancing from Release builds.

If there are any other requests regarding this, I would be happy to make the necessary changes.

Co-authored-by: Jacobsky <cael.jacobsen@gmail.com>

Author: bors[bot]

gdnative-core/Cargo.toml

@@ -23,6 +23,7 @@ approx = "0.4.0"
 glam = "0.14.0"
 indexmap = "1.6.0"
 ahash = "0.7.0"
+once_cell = "1.7.2"
 
 gdnative-impl-proc-macros = { path = "../impl/proc_macros", version = "=0.9.3" }
 

gdnative-core/src/nativescript/class.rs

@@ -14,8 +14,8 @@ use crate::private::{get_api, ReferenceCountedClassPlaceholder};
 use crate::ref_kind::{ManuallyManaged, RefCounted};
 use crate::thread_access::{NonUniqueThreadAccess, Shared, ThreadAccess, ThreadLocal, Unique};
 
+use super::class_registry;
 use super::emplace;
-
 /// Trait used for describing and initializing a Godot script class.
 ///
 /// This trait is used to provide data and functionality to the
@@ -321,6 +321,8 @@ impl<T: NativeClass> Instance<T, Unique> {
                 std::ptr::null_mut(),
             );
 
+            debug_assert!(class_registry::is_class_registered::<T>(), "`{type_name}` must be registered before it can be used; call `handle.add_class::<{type_name}>()` in your `nativescript_init` callback", type_name = std::any::type_name::<T>());
+
             assert!(
                 emplace::take::<T>().is_none(),
                 "emplacement value should be taken by the constructor wrapper (this is a bug in the bindings)",
@@ -330,7 +332,7 @@ impl<T: NativeClass> Instance<T, Unique> {
 
             let owner = variant
                 .try_to_object::<T::Base>()
-                .expect("base object should be of the correct type (is the script registered?)")
+                .expect("the engine should return a base object of the correct type")
                 .assume_unique();
 
             let script_ptr =

gdnative-core/src/nativescript/class_registry.rs

@@ -0,0 +1,30 @@
+use crate::nativescript::NativeClass;
+use once_cell::sync::Lazy;
+use parking_lot::RwLock;
+use std::any::TypeId;
+use std::collections::HashSet;
+
+static CLASS_REGISTRY: Lazy<RwLock<HashSet<TypeId>>> = Lazy::new(|| RwLock::new(HashSet::new()));
+
+/// Can be used to validate whether or not `C` has been added using `InitHandle::add_class<C>()`
+/// Returns true if added otherwise false.
+#[inline]
+pub(crate) fn is_class_registered<C: NativeClass>() -> bool {
+    let type_id = TypeId::of::<C>();
+    CLASS_REGISTRY.read().contains(&type_id)
+}
+
+/// Registers the class `C` in the class registry.
+/// Returns `true` if `C` was not already present in the list.
+/// Returns `false` if `C` was already added.
+#[inline]
+pub(crate) fn register_class<C: NativeClass>() -> bool {
+    let type_id = TypeId::of::<C>();
+    CLASS_REGISTRY.write().insert(type_id)
+}
+
+/// Clears the registry
+#[inline]
+pub(crate) fn cleanup() {
+    CLASS_REGISTRY.write().clear();
+}

gdnative-core/src/nativescript/init.rs

@@ -46,8 +46,8 @@ use crate::nativescript::NativeClassMethods;
 use crate::nativescript::UserData;
 use crate::private::get_api;
 
+use super::class_registry;
 use super::emplace;
-
 pub mod method;
 pub mod property;
 
@@ -96,6 +96,12 @@ impl InitHandle {
     where
         C: NativeClassMethods,
     {
+        if !class_registry::register_class::<C>() {
+            panic!(
+                "`{type_name}` has already been registered",
+                type_name = std::any::type_name::<C>()
+            );
+        }
         unsafe {
             let class_name = CString::new(C::class_name()).unwrap();
             let base_name = CString::new(C::Base::class_name()).unwrap();

gdnative-core/src/nativescript/mod.rs

@@ -1,5 +1,6 @@
 //! Types and functions related to the NativeScript extension of GDNative.
 
+pub(crate) mod class_registry;
 mod emplace;
 mod macros;
 

gdnative-core/src/private.rs

@@ -109,8 +109,10 @@ pub fn get_gdnative_library_sys() -> *mut sys::godot_object {
 #[inline]
 pub unsafe fn cleanup_internal_state() {
     #[cfg(feature = "nativescript")]
-    crate::nativescript::type_tag::cleanup();
-
+    {
+        crate::nativescript::type_tag::cleanup();
+        crate::nativescript::class_registry::cleanup();
+    }
     GODOT_API = None;
 }