Merge godot-rust#114

114: Fix base class registration, better panic coverage r=Bromeon a=Bromeon Fixes godot-rust#88. Co-authored-by: Jan Haller <[email protected]>
ttencate · Feb 5, 2023 · f25f65c · f25f65c
2 parents 17487d6 + 2a94b6c
commit f25f65c
Show file tree

Hide file tree

Showing 10 changed files with 173 additions and 72 deletions.
diff --git a/godot-codegen/src/lib.rs b/godot-codegen/src/lib.rs
@@ -269,6 +269,7 @@ const SELECTED_CLASSES: &[&str] = &[
     "CollisionShape2D",
     "Control",
     "FileAccess",
+    "HTTPRequest",
     "Input",
     "Label",
     "MainLoop",

diff --git a/godot-core/src/init/mod.rs b/godot-core/src/init/mod.rs
@@ -18,26 +18,31 @@ pub unsafe fn __gdext_load_library<E: ExtensionLibrary>(
         panic!("Attempted to call Godot engine from unit-tests; use integration tests for this.")
     }
 
-    sys::initialize(interface, library);
+    let init_code = || {
+        sys::initialize(interface, library);
 
-    let mut handle = InitHandle::new();
+        let mut handle = InitHandle::new();
 
-    let success = E::load_library(&mut handle);
-    // No early exit, unclear if Godot still requires output parameters to be set
+        let success = E::load_library(&mut handle);
+        // No early exit, unclear if Godot still requires output parameters to be set
 
-    let godot_init_params = sys::GDExtensionInitialization {
-        minimum_initialization_level: handle.lowest_init_level().to_sys(),
-        userdata: std::ptr::null_mut(),
-        initialize: Some(ffi_initialize_layer),
-        deinitialize: Some(ffi_deinitialize_layer),
-    };
+        let godot_init_params = sys::GDExtensionInitialization {
+            minimum_initialization_level: handle.lowest_init_level().to_sys(),
+            userdata: std::ptr::null_mut(),
+            initialize: Some(ffi_initialize_layer),
+            deinitialize: Some(ffi_deinitialize_layer),
+        };
+
+        *init = godot_init_params;
+        INIT_HANDLE = Some(handle);
 
-    let is_success = /*handle.*/success as u8;
+        success as u8
+    };
 
-    *init = godot_init_params;
-    INIT_HANDLE = Some(handle);
+    let ctx = || "error when loading GDExtension library";
+    let is_success = crate::private::handle_panic(ctx, init_code);
 
-    is_success
+    is_success.unwrap_or(0)
 }
 
 #[doc(hidden)]
@@ -49,16 +54,34 @@ unsafe extern "C" fn ffi_initialize_layer(
     _userdata: *mut std::ffi::c_void,
     init_level: sys::GDExtensionInitializationLevel,
 ) {
-    let handle = INIT_HANDLE.as_mut().unwrap();
-    handle.run_init_function(InitLevel::from_sys(init_level));
+    let ctx = || {
+        format!(
+            "failed to initialize GDExtension layer `{:?}`",
+            InitLevel::from_sys(init_level)
+        )
+    };
+
+    crate::private::handle_panic(ctx, || {
+        let handle = INIT_HANDLE.as_mut().unwrap();
+        handle.run_init_function(InitLevel::from_sys(init_level));
+    });
 }
 
 unsafe extern "C" fn ffi_deinitialize_layer(
     _userdata: *mut std::ffi::c_void,
     init_level: sys::GDExtensionInitializationLevel,
 ) {
-    let handle = INIT_HANDLE.as_mut().unwrap();
-    handle.run_deinit_function(InitLevel::from_sys(init_level));
+    let ctx = || {
+        format!(
+            "failed to deinitialize GDExtension layer `{:?}`",
+            InitLevel::from_sys(init_level)
+        )
+    };
+
+    crate::private::handle_panic(ctx, || {
+        let handle = INIT_HANDLE.as_mut().unwrap();
+        handle.run_deinit_function(InitLevel::from_sys(init_level));
+    });
 }
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------

diff --git a/godot-core/src/lib.rs b/godot-core/src/lib.rs
@@ -48,13 +48,32 @@ pub mod private {
         sys::plugin_foreach!(__GODOT_PLUGIN_REGISTRY; visitor);
     }
 
-    pub fn print_panic(err: Box<dyn std::any::Any + Send>) {
+    fn print_panic(err: Box<dyn std::any::Any + Send>) {
         if let Some(s) = err.downcast_ref::<&'static str>() {
-            log::godot_error!("rust-panic:  {s}");
+            log::godot_error!("Panic msg:  {s}");
         } else if let Some(s) = err.downcast_ref::<String>() {
-            log::godot_error!("rust-panic:  {s}");
+            log::godot_error!("Panic msg:  {s}");
         } else {
-            log::godot_error!("rust-panic of type ID {:?}", err.type_id());
+            log::godot_error!("Rust panic of type ID {:?}", err.type_id());
+        }
+    }
+
+    /// Executes `code`. If a panic is thrown, it is caught and an error message is printed to Godot.
+    ///
+    /// Returns `None` if a panic occurred, and `Some(result)` with the result of `code` otherwise.
+    pub fn handle_panic<E, F, R, S>(error_context: E, code: F) -> Option<R>
+    where
+        E: FnOnce() -> S,
+        F: FnOnce() -> R + std::panic::UnwindSafe,
+        S: std::fmt::Display,
+    {
+        match std::panic::catch_unwind(code) {
+            Ok(result) => Some(result),
+            Err(err) => {
+                log::godot_error!("Rust function panicked. Context: {}", error_context());
+                print_panic(err);
+                None
+            }
         }
     }
 }

diff --git a/godot-core/src/macros.rs b/godot-core/src/macros.rs
@@ -66,24 +66,24 @@ macro_rules! gdext_register_method_inner {
                     ret: sys::GDExtensionVariantPtr,
                     err: *mut sys::GDExtensionCallError,
                 ) {
-                    let result = ::std::panic::catch_unwind(|| {
-                        <Sig as SignatureTuple>::varcall::< $Class >(
-                            instance_ptr,
-                            args,
-                            ret,
-                            err,
-                            |inst, params| {
-                                let ( $($param,)* ) = params;
-                                inst.$method_name( $( $param, )* )
-                            },
-                            stringify!($method_name),
-                        )
-                    });
-
-                    if let Err(e) = result {
-                        $crate::log::godot_error!("Rust function panicked: {}", stringify!($method_name));
-                        $crate::private::print_panic(e);
-
+                    let success = $crate::private::handle_panic(
+                        || stringify!($method_name),
+                        || {
+                            <Sig as SignatureTuple>::varcall::< $Class >(
+                                instance_ptr,
+                                args,
+                                ret,
+                                err,
+                                |inst, params| {
+                                    let ( $($param,)* ) = params;
+                                    inst.$method_name( $( $param, )* )
+                                },
+                                stringify!($method_name),
+                            );
+                        }
+                    );
+
+                    if success.is_none() {
                         // Signal error and set return type to Nil
                         (*err).error = sys::GDEXTENSION_CALL_ERROR_INVALID_METHOD; // no better fitting enum?
                         sys::interface_fn!(variant_new_nil)(ret);
@@ -100,23 +100,23 @@ macro_rules! gdext_register_method_inner {
                     args: *const sys::GDExtensionConstTypePtr,
                     ret: sys::GDExtensionTypePtr,
                 ) {
-                    let result = ::std::panic::catch_unwind(|| {
-                        <Sig as SignatureTuple>::ptrcall::< $Class >(
-                            instance_ptr,
-                            args,
-                            ret,
-                            |inst, params| {
-                                let ( $($param,)* ) = params;
-                                inst.$method_name( $( $param, )* )
-                            },
-                            stringify!($method_name),
-                        );
-                    });
-
-                    if let Err(e) = result {
-                        $crate::log::godot_error!("Rust function panicked: {}", stringify!($method_name));
-                        $crate::private::print_panic(e);
-
+                    let success = $crate::private::handle_panic(
+                        || stringify!($method_name),
+                        || {
+                            <Sig as SignatureTuple>::ptrcall::< $Class >(
+                                instance_ptr,
+                                args,
+                                ret,
+                                |inst, params| {
+                                    let ( $($param,)* ) = params;
+                                    inst.$method_name( $( $param, )* )
+                                },
+                                stringify!($method_name),
+                            );
+                        }
+                    );
+
+                    if success.is_none() {
                         // TODO set return value to T::default()?
                     }
                 }

diff --git a/godot-core/src/obj/traits.rs b/godot-core/src/obj/traits.rs
@@ -25,6 +25,9 @@ where
     /// Defines the memory strategy.
     type Mem: mem::Memory;
 
+    /// The name of the class, under which it is registered in Godot.
+    ///
+    /// This may deviate from the Rust struct name: `HttpRequest::CLASS_NAME == "HTTPRequest"`.
     const CLASS_NAME: &'static str;
 }
 

diff --git a/godot-core/src/registry.rs b/godot-core/src/registry.rs
@@ -233,16 +233,23 @@ fn register_class_raw(info: ClassRegistrationInfo) {
         .expect("class defined (parent_class_name)");
 
     unsafe {
-        interface_fn!(classdb_register_extension_class)(
+        // Try to register class...
+        #[allow(clippy::let_unit_value)] // notifies us if Godot API ever adds a return type.
+        let _: () = interface_fn!(classdb_register_extension_class)(
             sys::get_library(),
             class_name.string_sys(),
             parent_class_name.string_sys(),
             ptr::addr_of!(info.godot_params),
         );
-    }
 
-    // std::mem::forget(class_name);
-    // std::mem::forget(parent_class_name);
+        // ...then see if it worked.
+        // This is necessary because the above registration does not report errors (apart from console output).
+        let tag = interface_fn!(classdb_get_class_tag)(class_name.string_sys());
+        assert!(
+            !tag.is_null(),
+            "failed to register class `{class_name}`; check preceding Godot stderr messages",
+        );
+    }
 
     // ...then custom symbols
 

diff --git a/godot-macros/src/derive_godot_class.rs b/godot-macros/src/derive_godot_class.rs
@@ -22,10 +22,9 @@ pub fn transform(input: TokenStream) -> ParseResult<TokenStream> {
     let fields = parse_fields(class)?;
 
     let base_ty = &struct_cfg.base_ty;
-    let base_ty_str = struct_cfg.base_ty.to_string();
     let class_name = &class.name;
     let class_name_str = class.name.to_string();
-    let inherits_macro = format_ident!("inherits_transitive_{}", &base_ty_str);
+    let inherits_macro = format_ident!("inherits_transitive_{}", base_ty);
 
     let prv = quote! { ::godot::private };
     let deref_impl = make_deref_impl(class_name, &fields);
@@ -57,7 +56,7 @@ pub fn transform(input: TokenStream) -> ParseResult<TokenStream> {
         ::godot::sys::plugin_add!(__GODOT_PLUGIN_REGISTRY in #prv; #prv::ClassPlugin {
             class_name: #class_name_str,
             component: #prv::PluginComponent::ClassDef {
-                base_class_name: #base_ty_str,
+                base_class_name: <::godot::engine::#base_ty as ::godot::obj::GodotClass>::CLASS_NAME,
                 generated_create_fn: #create_fn,
                 free_fn: #prv::callbacks::free::<#class_name>,
             },

diff --git a/godot-macros/src/itest.rs b/godot-macros/src/itest.rs
@@ -40,17 +40,13 @@ pub fn transform(input: TokenStream) -> Result<TokenStream, Error> {
         pub fn #test_name() -> bool {
             println!(#init_msg);
 
-            let result = ::std::panic::catch_unwind(
+            // Explicit type to prevent tests from returning a value
+            let success: Option<()> = godot::private::handle_panic(
+                || #error_msg,
                 || #body
             );
 
-            if let Err(e) = result {
-                ::godot::log::godot_error!(#error_msg);
-                ::godot::private::print_panic(e);
-                false
-            } else {
-                true
-            }
+            success.is_some()
         }
     })
 }
diff --git a/itest/rust/src/codegen_test.rs b/itest/rust/src/codegen_test.rs
@@ -0,0 +1,51 @@
+/*
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at https://mozilla.org/MPL/2.0/.
+ */
+
+// This file tests the presence and naming of generated symbols, not their functionality.
+
+use crate::itest;
+
+use godot::engine::HttpRequest;
+use godot::prelude::*;
+
+pub fn run() -> bool {
+    let mut ok = true;
+    ok &= codegen_class_renamed();
+    ok &= codegen_base_renamed();
+    ok
+}
+
+#[itest]
+fn codegen_class_renamed() {
+    // Known as `HTTPRequest` in Godot
+    let obj = HttpRequest::new_alloc();
+    obj.free();
+}
+
+#[itest]
+fn codegen_base_renamed() {
+    // The registration is done at startup time, so it may already fail during GDExtension init.
+    // Nevertheless, try to instantiate an object with base HttpRequest here.
+
+    let obj = Gd::with_base(|base| TestBaseRenamed { base });
+    let _id = obj.instance_id();
+
+    obj.free();
+}
+
+#[derive(GodotClass)]
+#[class(base=HttpRequest)]
+pub struct TestBaseRenamed {
+    #[base]
+    base: Base<HttpRequest>,
+}
+
+#[godot_api]
+impl GodotExt for TestBaseRenamed {
+    fn init(base: Base<HttpRequest>) -> Self {
+        TestBaseRenamed { base }
+    }
+}
diff --git a/itest/rust/src/lib.rs b/itest/rust/src/lib.rs
@@ -12,6 +12,7 @@ use std::panic::UnwindSafe;
 mod array_test;
 mod base_test;
 mod builtin_test;
+mod codegen_test;
 mod dictionary_test;
 mod enum_test;
 mod export_test;
@@ -29,6 +30,7 @@ fn run_tests() -> bool {
     let mut ok = true;
     ok &= base_test::run();
     ok &= builtin_test::run();
+    ok &= codegen_test::run();
     ok &= enum_test::run();
     ok &= export_test::run();
     ok &= gdscript_ffi_test::run();