Merge pull request #964 from godot-rust/qol/signal-cleanup

Minor signal cleanup; prevent `#[signal]` from being used in secondary impl blocks

Author: Bromeon

godot-core/src/init/mod.rs

@@ -17,7 +17,7 @@ use crate::out;
 pub use sys::GdextBuild;
 
 #[doc(hidden)]
-// TODO consider body safe despite unsafe function, and explicitly mark unsafe {} locations
+#[deny(unsafe_op_in_unsafe_fn)]
 pub unsafe fn __gdext_load_library<E: ExtensionLibrary>(
     get_proc_address: sys::GDExtensionInterfaceGetProcAddress,
     library: sys::GDExtensionClassLibraryPtr,
@@ -41,7 +41,10 @@ pub unsafe fn __gdext_load_library<E: ExtensionLibrary>(
 
         let config = sys::GdextConfig::new(tool_only_in_editor);
 
-        sys::initialize(get_proc_address, library, config);
+        // SAFETY: no custom code has run yet + no other thread is accessing global handle.
+        unsafe {
+            sys::initialize(get_proc_address, library, config);
+        }
 
         // Currently no way to express failure; could be exposed to E if necessary.
         // No early exit, unclear if Godot still requires output parameters to be set.
@@ -54,7 +57,10 @@ pub unsafe fn __gdext_load_library<E: ExtensionLibrary>(
             deinitialize: Some(ffi_deinitialize_layer::<E>),
         };
 
-        *init = godot_init_params;
+        // SAFETY: Godot is responsible for passing us a valid pointer.
+        unsafe {
+            *init = godot_init_params;
+        }
 
         success as u8
     };

godot-macros/src/class/data_models/inherent_impl.rs

@@ -4,6 +4,7 @@
  * 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/.
  */
+
 use crate::class::{
     into_signature_info, make_constant_registration, make_method_registration,
     make_signal_registrations, ConstDefinition, FuncDefinition, RpcAttr, RpcMode, SignalDefinition,
@@ -66,7 +67,7 @@ struct FuncAttr {
 
 pub struct InherentImplAttr {
     /// For implementation reasons, there can be a single 'primary' impl block and 0 or more 'secondary' impl blocks.
-    /// For now this is controlled by a key in the the 'godot_api' attribute
+    /// For now, this is controlled by a key in the 'godot_api' attribute.
     pub secondary: bool,
 }
 
@@ -80,7 +81,7 @@ pub fn transform_inherent_impl(
     let prv = quote! { ::godot::private };
 
     // Can add extra functions to the end of the impl block.
-    let (funcs, signals) = process_godot_fns(&class_name, &mut impl_block)?;
+    let (funcs, signals) = process_godot_fns(&class_name, &mut impl_block, meta.secondary)?;
     let consts = process_godot_constants(&mut impl_block)?;
 
     #[cfg(all(feature = "register-docs", since_api = "4.3"))]
@@ -107,16 +108,13 @@ pub fn transform_inherent_impl(
 
     let fill_storage = quote! {
         ::godot::sys::plugin_execute_pre_main!({
-            #method_storage_name.lock().unwrap().push(||{
-
+            #method_storage_name.lock().unwrap().push(|| {
                 #( #method_registrations )*
                 #( #signal_registrations )*
-
             });
-            #constants_storage_name.lock().unwrap().push(||{
 
+            #constants_storage_name.lock().unwrap().push(|| {
                 #constant_registration
-
             });
         });
     };
@@ -155,7 +153,6 @@ pub fn transform_inherent_impl(
         };
 
         let class_registration = quote! {
-
             ::godot::sys::plugin_add!(__GODOT_PLUGIN_REGISTRY in #prv; #prv::ClassPlugin {
                 class_name: #class_name_obj,
                 item: #prv::PluginItem::InherentImpl(#prv::InherentImpl {
@@ -169,7 +166,6 @@ pub fn transform_inherent_impl(
                 }),
                 init_level: <#class_name as ::godot::obj::GodotClass>::INIT_LEVEL,
             });
-
         };
 
         let result = quote! {
@@ -182,7 +178,7 @@ pub fn transform_inherent_impl(
 
         Ok(result)
     } else {
-        // We are in a secondary `impl` block, so most of the work has already been done
+        // We are in a secondary `impl` block, so most of the work has already been done,
         // and we just need to add our registration functions in the storage defined by the primary `impl` block.
 
         let result = quote! {
@@ -197,6 +193,7 @@ pub fn transform_inherent_impl(
 fn process_godot_fns(
     class_name: &Ident,
     impl_block: &mut venial::Impl,
+    is_secondary_impl: bool,
 ) -> ParseResult<(Vec<FuncDefinition>, Vec<SignalDefinition>)> {
     let mut func_definitions = vec![];
     let mut signal_definitions = vec![];
@@ -286,9 +283,16 @@ fn process_godot_fns(
                     rpc_info,
                 });
             }
+
             ItemAttrType::Signal(ref _attr_val) => {
+                if is_secondary_impl {
+                    return attr.bail(
+                        "#[signal] is not currently supported in secondary impl blocks",
+                        function,
+                    );
+                }
                 if function.return_ty.is_some() {
-                    return attr.bail("return types are not supported", function);
+                    return attr.bail("return types in #[signal] are not supported", function);
                 }
 
                 let external_attributes = function.attributes.clone();
@@ -301,6 +305,7 @@ fn process_godot_fns(
 
                 removed_indexes.push(index);
             }
+
             ItemAttrType::Const(_) => {
                 return attr.bail(
                     "#[constant] can only be used on associated constant",
@@ -541,12 +546,7 @@ where
             }
 
             // #[signal]
-            name if name == "signal" => {
-                // TODO once parameters are supported, this should probably be moved to the struct definition
-                // E.g. a zero-sized type Signal<(i32, String)> with a provided emit(i32, String) method
-                // This could even be made public (callable on the struct obj itself)
-                AttrParseResult::Signal(attr.value.clone())
-            }
+            name if name == "signal" => AttrParseResult::Signal(attr.value.clone()),
 
             // #[constant]
             name if name == "constant" => AttrParseResult::Const(attr.value.clone()),

itest/rust/src/builtin_tests/containers/callable_test.rs

@@ -4,13 +4,13 @@
  * 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/.
  */
+
 use godot::builtin::inner::InnerCallable;
 use godot::builtin::{varray, Callable, GString, StringName, Variant};
 use godot::classes::{Node2D, Object};
 use godot::meta::ToGodot;
 use godot::obj::{NewAlloc, NewGd};
 use godot::register::{godot_api, GodotClass};
-use std::fmt::{Display, Formatter};
 use std::hash::Hasher;
 use std::sync::atomic::{AtomicU32, Ordering};
 
@@ -364,7 +364,7 @@ pub mod custom_callable {
     }
 
     impl Hash for Adder {
-        fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        fn hash<H: Hasher>(&self, state: &mut H) {
             let mut guard = self.tracker.lock().unwrap();
             guard.hash_counter += 1;
 
@@ -378,7 +378,7 @@ pub mod custom_callable {
         }
     }
 
-    impl godot::builtin::RustCallable for Adder {
+    impl RustCallable for Adder {
         fn invoke(&mut self, args: &[&Variant]) -> Result<Variant, ()> {
             for arg in args {
                 self.sum += arg.to::<i32>();
@@ -410,6 +410,7 @@ pub mod custom_callable {
         tracker.lock().unwrap().hash_counter
     }
 
+    // Also used in signal_test.
     pub struct PanicCallable(pub Arc<AtomicU32>);
 
     impl PartialEq for PanicCallable {
@@ -424,8 +425,8 @@ pub mod custom_callable {
         }
     }
 
-    impl Display for PanicCallable {
-        fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+    impl fmt::Display for PanicCallable {
+        fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
             write!(f, "test")
         }
     }