Merge pull request #1146 from godot-rust/qol/withsignals-everywhere

User classes expose typed-signal API even without `#[signal]`

Author: Bromeon

godot-core/src/obj/gd.rs

@@ -326,7 +326,8 @@ impl<T: GodotClass> Gd<T> {
 
     /// Equivalent to [`upcast::<Object>()`][Self::upcast], but without bounds.
     // Not yet public because it might need _mut/_ref overloads, and 6 upcast methods are a bit much...
-    pub(crate) fn upcast_object(self) -> Gd<classes::Object> {
+    #[doc(hidden)] // no public API, but used by #[signal].
+    pub fn upcast_object(self) -> Gd<classes::Object> {
         self.owned_cast()
             .expect("Upcast to Object failed. This is a bug; please report it.")
     }

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

@@ -71,14 +71,17 @@ struct SignalAttr {
     pub no_builder: bool,
 }
 
-// ----------------------------------------------------------------------------------------------------------------------------------------------
-
-pub struct InherentImplAttr {
+pub(crate) 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 'godot_api' attribute.
     pub secondary: bool,
+
+    /// When typed signal generation is explicitly disabled by the user.
+    pub no_typed_signals: bool,
 }
 
+// ----------------------------------------------------------------------------------------------------------------------------------------------
+
 /// Codegen for `#[godot_api] impl MyType`
 pub fn transform_inherent_impl(
     meta: InherentImplAttr,
@@ -107,8 +110,12 @@ pub fn transform_inherent_impl(
 
     // For each #[func] in this impl block, create one constant.
     let func_name_constants = make_funcs_collection_constants(&funcs, &class_name);
-    let (signal_registrations, signal_symbol_types) =
-        make_signal_registrations(&signals, &class_name, &class_name_obj)?;
+    let (signal_registrations, signal_symbol_types) = make_signal_registrations(
+        &signals,
+        &class_name,
+        &class_name_obj,
+        meta.no_typed_signals,
+    )?;
 
     #[cfg(feature = "codegen-full")]
     let rpc_registrations = crate::class::make_rpc_registrations_fn(&class_name, &funcs);
@@ -206,6 +213,23 @@ pub fn transform_inherent_impl(
     }
 }
 
+/* Re-enable if we allow controlling declarative macros for signals (base_field_macro, visibility_macros).
+fn extract_hint_attribute(impl_block: &mut venial:: Impl) -> ParseResult<GodotApiHints> {
+    // #[hint(has_base_field = BOOL)]
+    let has_base_field;
+    if let Some(mut hints) = KvParser::parse_remove(&mut impl_block.attributes, "hint")? {
+        has_base_field = hints.handle_bool("has_base_field")?;
+    } else {
+        has_base_field = None;
+    }
+
+    // #[hint(class_visibility = pub(crate))]
+    // ...
+
+    Ok(GodotApiHints { has_base_field })
+}
+*/
+
 fn process_godot_fns(
     class_name: &Ident,
     impl_block: &mut venial::Impl,

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

@@ -182,6 +182,7 @@ pub fn make_signal_registrations(
     signals: &[SignalDefinition],
     class_name: &Ident,
     class_name_obj: &TokenStream,
+    no_typed_signals: bool,
 ) -> ParseResult<(Vec<TokenStream>, Option<TokenStream>)> {
     let mut signal_registrations = Vec::new();
 
@@ -210,8 +211,11 @@ pub fn make_signal_registrations(
         signal_registrations.push(registration);
     }
 
+    // Rewrite the above using #[cfg].
     #[cfg(since_api = "4.2")]
-    let signal_symbols = make_signal_symbols(class_name, collection_api);
+    let signal_symbols =
+        (!no_typed_signals).then(|| make_signal_symbols(class_name, collection_api));
+
     #[cfg(before_api = "4.2")]
     let signal_symbols = None;
 
@@ -311,7 +315,7 @@ impl SignalCollection {
             .push(make_signal_individual_struct(details))
     }
 
-    pub fn is_empty(&self) -> bool {
+    fn is_empty(&self) -> bool {
         self.individual_structs.is_empty()
     }
 }
@@ -380,18 +384,26 @@ fn make_signal_individual_struct(details: &SignalDetails) -> TokenStream {
 
 /// Generates symbolic API for signals:
 /// * collection (unspecified-name struct holding methods to access each signal)
+///   * can be absent if the class declares no own #[signal]s.
 /// * individual signal types
 /// * trait impls
 fn make_signal_symbols(
     class_name: &Ident,
     collection_api: SignalCollection,
     // max_visibility: SignalVisibility,
-) -> Option<TokenStream> {
-    // Future note: if we add Rust->Rust inheritance, then the WithSignals trait must be unconditionally implemented.
-    if collection_api.is_empty() {
-        return None;
-    }
+) -> TokenStream {
+    // Earlier implementation generated a simplified code when no #[signal] was declared: only WithSignals/WithUserSignals impl, but no own
+    // collection, instead the associated type pointing to the base class. This has however some problems:
+    // * Part of the reason for user-defined collection is to store UserSignalObject instead of Gd, which can store &mut self.
+    //   This is necessary for self.signals().some_base_signal().emit(), if such a signal is connected to Self::method_mut;
+    //   Gd would cause a borrow error.
+    // * Once we add Rust-Rust inheritance, we'd need to differentiate case again, which can be tricky since #[godot_api] has no information
+    //   about the base class.
+    //
+    // As compromise, we always generate a collection struct if either at least 1 #[signal] is declared or the struct has a Base<T> field.
+    // We also provide opt-out via #[godot_api(no_typed_signals)].
 
+    let declares_no_signals = collection_api.is_empty();
     let collection_struct_name = format_ident!("__godot_Signals_{}", class_name);
     let collection_struct_methods = &collection_api.provider_methods;
     let with_signals_impl = make_with_signals_impl(class_name, &collection_struct_name);
@@ -431,7 +443,7 @@ fn make_signal_symbols(
 
     let visibility_macro = util::format_class_visibility_macro(class_name);
 
-    let code = quote! {
+    let mut code = quote! {
         #visibility_macro! {
             #[allow(non_camel_case_types)]
             #[doc(hidden)] // Only on struct, not methods, to allow completion in IDEs.
@@ -455,9 +467,21 @@ fn make_signal_symbols(
         #( #individual_structs )*
     };
 
-    Some(code)
+    // base_field_macro! is a macro that expands to all input tokens if the class declares a Base<T> field, and to nothing otherwise.
+    // This makes sure that WithSignals is only implemented for classes with a base field, and avoids compile errors about it.
+    // Only when no #[signal] is declared -> otherwise the user explicitly requests it, and a compile error is better to guide them.
+    if declares_no_signals {
+        let base_field_macro = util::format_class_base_field_macro(class_name);
+
+        code = quote! {
+            #base_field_macro! { #code }
+        };
+    }
+
+    code
 }
 
+/// Declare `impl WithSignals` and `impl WithUserSignals` with own signal collection.
 fn make_with_signals_impl(class_name: &Ident, collection_struct_name: &Ident) -> TokenStream {
     quote! {
         impl ::godot::obj::WithSignals for #class_name {
@@ -467,10 +491,10 @@ fn make_with_signals_impl(class_name: &Ident, collection_struct_name: &Ident) ->
             type __SignalObj<'c> = ::godot::private::UserSignalObject<'c, Self>;
 
             #[doc(hidden)]
-            fn __signals_from_external(external: &mut Gd<Self>) -> Self::SignalCollection<'_, Self> {
+            fn __signals_from_external(external: &mut ::godot::obj::Gd<Self>) -> Self::SignalCollection<'_, Self> {
                 Self::SignalCollection {
                     __internal_obj: Some(::godot::private::UserSignalObject::External {
-                        gd: external.clone().upcast::<Object>()
+                        gd: external.clone().upcast_object()
                     })
                 }
             }

godot-macros/src/class/derive_godot_class.rs

@@ -142,7 +142,9 @@ pub fn derive_godot_class(item: venial::Item) -> ParseResult<TokenStream> {
         pub struct #funcs_collection_struct_name {}
     };
 
+    // Note: one limitation is that macros don't work for `impl nested::MyClass` blocks.
     let visibility_macro = make_visibility_macro(class_name, class.vis_marker.as_ref());
+    let base_field_macro = make_base_field_macro(class_name, fields.base_field.is_some());
 
     Ok(quote! {
         impl ::godot::obj::GodotClass for #class_name {
@@ -174,6 +176,7 @@ pub fn derive_godot_class(item: venial::Item) -> ParseResult<TokenStream> {
         #user_class_impl
         #init_expecter
         #visibility_macro
+        #base_field_macro
         #( #deprecations )*
         #( #errors )*
 
@@ -211,6 +214,25 @@ fn make_visibility_macro(
     }
 }
 
+/// Generates code for a decl-macro, which evaluates to nothing if the class has no base field.
+fn make_base_field_macro(class_name: &Ident, has_base_field: bool) -> TokenStream {
+    let macro_name = util::format_class_base_field_macro(class_name);
+
+    if has_base_field {
+        quote! {
+            macro_rules! #macro_name {
+                ( $( $tt:tt )* ) => { $( $tt )* };
+            }
+        }
+    } else {
+        quote! {
+            macro_rules! #macro_name {
+                ( $( $tt:tt )* ) => {};
+            }
+        }
+    }
+}
+
 /// Checks at compile time that a function with the given name exists on `Self`.
 #[must_use]
 pub fn make_existence_check(ident: &Ident) -> TokenStream {

godot-macros/src/class/godot_api.rs

@@ -17,9 +17,20 @@ fn parse_inherent_impl_attr(meta: TokenStream) -> Result<super::InherentImplAttr
     let item = venial_parse_meta(&meta, format_ident!("godot_api"), &quote! { fn func(); })?;
     let mut attr = KvParser::parse_required(item.attributes(), "godot_api", &meta)?;
     let secondary = attr.handle_alone("secondary")?;
+    let no_typed_signals = attr.handle_alone("no_typed_signals")?;
     attr.finish()?;
 
-    Ok(super::InherentImplAttr { secondary })
+    if no_typed_signals && secondary {
+        return bail!(
+            meta,
+            "#[godot_api]: keys `secondary` and `no_typed_signals` are mutually exclusive; secondary blocks allow no signals anyway"
+        )?;
+    }
+
+    Ok(super::InherentImplAttr {
+        secondary,
+        no_typed_signals,
+    })
 }
 
 pub fn attribute_godot_api(

godot-macros/src/util/kv_parser.rs

@@ -38,14 +38,16 @@ impl KvParser {
     }
 
     /// Create a new parser which checks for presence of an `#[expected]` attribute.
+    ///
+    /// Returns `Ok(None)` if the attribute is not present.
     pub fn parse(attributes: &[venial::Attribute], expected: &str) -> ParseResult<Option<Self>> {
         let mut found_attr: Option<Self> = None;
 
         for attr in attributes.iter() {
             let path = &attr.path;
             if path_is_single(path, expected) {
                 if found_attr.is_some() {
-                    return bail!(attr, "only a single #[{expected}] attribute allowed",);
+                    return bail!(attr, "only a single #[{expected}] attribute allowed");
                 }
 
                 let attr_name = expected.to_string();
@@ -59,6 +61,40 @@ impl KvParser {
         Ok(found_attr)
     }
 
+    /// Like `parse()`, but removes the attribute from the list.
+    ///
+    /// Useful for `#[proc_macro_attributes]`, where handled attributes must not show up in resulting code.
+    // Currently unused.
+    pub fn parse_remove(
+        attributes: &mut Vec<venial::Attribute>,
+        expected: &str,
+    ) -> ParseResult<Option<Self>> {
+        let mut found_attr: Option<Self> = None;
+        let mut found_index: Option<usize> = None;
+
+        for (i, attr) in attributes.iter().enumerate() {
+            let path = &attr.path;
+            if path_is_single(path, expected) {
+                if found_attr.is_some() {
+                    return bail!(attr, "only a single #[{expected}] attribute allowed");
+                }
+
+                let attr_name = expected.to_string();
+                found_index = Some(i);
+                found_attr = Some(Self {
+                    span: attr.tk_brackets.span,
+                    map: ParserState::parse(attr_name, &attr.value)?,
+                });
+            }
+        }
+
+        if let Some(index) = found_index {
+            attributes.remove(index);
+        }
+
+        Ok(found_attr)
+    }
+
     pub fn span(&self) -> Span {
         self.span
     }

godot-macros/src/util/mod.rs

@@ -409,3 +409,8 @@ pub fn format_funcs_collection_struct(class_name: &Ident) -> Ident {
 pub fn format_class_visibility_macro(class_name: &Ident) -> Ident {
     format_ident!("__godot_{class_name}_vis_macro")
 }
+
+/// Returns the name of the macro used to communicate whether the `struct` (class) contains a base field.
+pub fn format_class_base_field_macro(class_name: &Ident) -> Ident {
+    format_ident!("__godot_{class_name}_has_base_field_macro")
+}

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

@@ -327,6 +327,24 @@ fn signal_symbols_engine_inherited_internal() {
     node.free();
 }
 
+// Test that Node signals are accessible from a derived class, when the class itself has no #[signal] declarations.
+// Verifies the code path that only generates the traits, no dedicated signal collection.
+#[cfg(since_api = "4.2")]
+#[itest]
+fn signal_symbols_engine_inherited_no_own_signals() {
+    let mut obj = Receiver::new_alloc();
+
+    let mut sig = obj.signals().property_list_changed();
+    sig.connect_self(|this: &mut Receiver| {
+        this.receive_int(941);
+    });
+
+    obj.notify_property_list_changed();
+    assert_eq!(obj.bind().last_received.get(), LastReceived::Int(941));
+
+    obj.free();
+}
+
 #[itest]
 fn signal_construction_and_id() {
     let mut object = RefCounted::new_gd();
@@ -451,6 +469,8 @@ struct Receiver {
 
 #[godot_api]
 impl Receiver {
+    // Do not declare any #[signal]s here -- explicitly test this implements WithSignal without them.
+
     fn last_received(&self) -> LastReceived {
         self.last_received.get()
     }

itest/rust/src/object_tests/object_test.rs

@@ -997,7 +997,9 @@ pub mod object_test_gd {
     }
     use nested::ObjectTest;
 
-    #[godot_api]
+    // Disabling signals allows nested::ObjectTest, which would fail otherwise due to generated decl-macro being out-of-scope.
+    #[godot_api(no_typed_signals)]
+    // #[hint(has_base_field = false)] // if we allow more fine-grained control in the future
     impl nested::ObjectTest {
         #[func]
         fn pass_object(&self, object: Gd<Object>) -> i64 {