Merge pull request #1075 from godot-rust/bugfix/signal-visibility

Explicit signal visibility

Author: Bromeon

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

@@ -188,7 +188,7 @@ pub struct SignatureInfo {
     pub receiver_type: ReceiverType,
     pub param_idents: Vec<Ident>,
     pub param_types: Vec<venial::TypeExpr>,
-    pub ret_type: TokenStream,
+    pub return_type: TokenStream,
 }
 
 impl SignatureInfo {
@@ -198,7 +198,7 @@ impl SignatureInfo {
             receiver_type: ReceiverType::Mut,
             param_idents: vec![],
             param_types: vec![],
-            ret_type: quote! { () },
+            return_type: quote! { () },
         }
     }
 
@@ -207,7 +207,7 @@ impl SignatureInfo {
 
     pub fn tuple_type(&self) -> TokenStream {
         // Note: for GdSelf receivers, first parameter is not even part of SignatureInfo anymore.
-        util::make_signature_tuple_type(&self.ret_type, &self.param_types)
+        util::make_signature_tuple_type(&self.return_type, &self.param_types)
     }
 }
 
@@ -392,7 +392,7 @@ pub(crate) fn into_signature_info(
         receiver_type,
         param_idents,
         param_types,
-        ret_type,
+        return_type: ret_type,
     }
 }
 

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

@@ -319,18 +319,14 @@ fn process_godot_fns(
                         "#[signal] must not have a body; declare the function with a semicolon"
                     );
                 }
-                if function.vis_marker.is_some() {
-                    return bail!(
-                        &function.vis_marker,
-                        "#[signal] must not have a visibility specifier; signals are always public"
-                    );
-                }
 
                 let external_attributes = function.attributes.clone();
-                let sig = util::reduce_to_signature(function);
+
+                let mut fn_signature = util::reduce_to_signature(function);
+                fn_signature.vis_marker = function.vis_marker.clone();
 
                 signal_definitions.push(SignalDefinition {
-                    signature: sig,
+                    fn_signature,
                     external_attributes,
                     has_builder: !signal.no_builder,
                 });

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

@@ -12,8 +12,8 @@ use quote::{format_ident, quote};
 
 /// Holds information known from a signal's definition
 pub struct SignalDefinition {
-    /// The signal's function signature.
-    pub signature: venial::Function,
+    /// The signal's function signature (simplified, not original declaration).
+    pub fn_signature: venial::Function,
 
     /// The signal's non-gdext attributes (all except #[signal]).
     pub external_attributes: Vec<venial::Attribute>,
@@ -24,8 +24,8 @@ pub struct SignalDefinition {
 
 /// Extracted syntax info for a declared signal.
 struct SignalDetails<'a> {
-    /// `fn my_signal(i: i32, s: GString)`
-    original_decl: &'a venial::Function,
+    /// `fn my_signal(i: i32, s: GString)` -- simplified from original declaration.
+    fn_signature: &'a venial::Function,
     /// `MyClass`
     class_name: &'a Ident,
     /// `i32`, `GString`
@@ -44,19 +44,21 @@ struct SignalDetails<'a> {
     signal_cfg_attrs: Vec<&'a venial::Attribute>,
     /// `MyClass_MySignal`
     individual_struct_name: Ident,
+    /// Visibility, e.g. `pub(crate)`
+    vis_marker: Option<venial::VisMarker>,
 }
 
 impl<'a> SignalDetails<'a> {
     pub fn extract(
-        original_decl: &'a venial::Function,
+        fn_signature: &'a venial::Function, // *Not* the original #[signal], just the signature part (no attributes, body, etc).
         class_name: &'a Ident,
         external_attributes: &'a [venial::Attribute],
     ) -> ParseResult<SignalDetails<'a>> {
         let mut param_types = vec![];
         let mut param_names = vec![];
         let mut param_names_str = vec![];
 
-        for (param, _punct) in original_decl.params.inner.iter() {
+        for (param, _punct) in fn_signature.params.inner.iter() {
             match param {
                 venial::FnParam::Typed(param) => {
                     param_types.push(param.ty.clone());
@@ -76,20 +78,21 @@ impl<'a> SignalDetails<'a> {
             .collect();
 
         let param_tuple = quote! { ( #( #param_types, )* ) };
-        let signal_name = &original_decl.name;
+        let signal_name = &fn_signature.name;
         let individual_struct_name = format_ident!("__godot_Signal_{}_{}", class_name, signal_name);
 
         Ok(Self {
-            original_decl,
+            fn_signature,
             class_name,
             param_types,
             param_names,
             param_names_str,
             param_tuple,
             signal_name,
-            signal_name_str: original_decl.name.to_string(),
+            signal_name_str: fn_signature.name.to_string(),
             signal_cfg_attrs,
             individual_struct_name,
+            vis_marker: fn_signature.vis_marker.clone(),
         })
     }
 }
@@ -104,12 +107,12 @@ pub fn make_signal_registrations(
 
     for signal in signals {
         let SignalDefinition {
-            signature,
+            fn_signature,
             external_attributes,
             has_builder,
         } = signal;
 
-        let details = SignalDetails::extract(signature, class_name, external_attributes)?;
+        let details = SignalDetails::extract(fn_signature, class_name, external_attributes)?;
 
         // Callable custom functions are only supported in 4.2+, upon which custom signals rely.
         #[cfg(since_api = "4.2")]
@@ -196,13 +199,19 @@ impl SignalCollection {
             signal_name_str,
             signal_cfg_attrs,
             individual_struct_name,
+            vis_marker,
             ..
         } = details;
 
         self.collection_methods.push(quote! {
             // Deliberately not #[doc(hidden)] for IDE completion.
             #(#signal_cfg_attrs)*
-            fn #signal_name(self) -> #individual_struct_name<'a> {
+            // Note: this could be `pub` always and would still compile (maybe warning with the following message).
+            //   associated function `SignalCollection::my_signal` is reachable at visibility `pub(crate)`
+            //
+            // However, it would still lead to a compile error when declaring the individual signal struct `pub` (or any other
+            // visibility that exceeds the class visibility). So, we can as well declare the visibility here.
+            #vis_marker fn #signal_name(self) -> #individual_struct_name<'a> {
                 #individual_struct_name {
                     typed: ::godot::register::TypedSignal::new(self.__internal_obj, #signal_name_str)
                 }
@@ -219,14 +228,15 @@ impl SignalCollection {
 }
 
 fn make_signal_individual_struct(details: &SignalDetails) -> TokenStream {
-    let emit_params = &details.original_decl.params;
+    let emit_params = &details.fn_signature.params;
 
     let SignalDetails {
         class_name,
         param_names,
         param_tuple,
         signal_cfg_attrs,
         individual_struct_name,
+        vis_marker,
         ..
     } = details;
 
@@ -235,9 +245,9 @@ fn make_signal_individual_struct(details: &SignalDetails) -> TokenStream {
     //   #(#signal_cfg_attrs)* pub mod #module_name { use super::*; ... }
     //   #(#signal_cfg_attrs)* pub(crate) use #module_name::#individual_struct_name;
     // However, there are some challenges:
-    // - Visibility becomes a pain to handle (rustc doesn't like re-exporting private symbols as pub, and we can't know the visibility of the
-    //   surrounding class struct). Having signals always-public is much less of a headache, requires less choice on the user side
-    //   (pub/pub(crate)/nothing on #[signal]), and likely good enough for the moment.
+    // - Visibility is a pain to handle: rustc doesn't like re-exporting private symbols as pub, and we can't know the visibility of the
+    //   surrounding class struct. Users must explicitly declare #[signal]s `pub` if they want wider visibility; this must not exceed the
+    //   visibility of the class itself.
     // - Not yet clear if we should have each signal + related types in separate module. If #[signal] is supported in #[godot_api(secondary)]
     //   impl blocks, then we would have to group them by the impl block. Rust doesn't allow partial modules, so they'd need to have individual
     //   names as well, possibly explicitly chosen by the user.
@@ -248,7 +258,7 @@ fn make_signal_individual_struct(details: &SignalDetails) -> TokenStream {
         #(#signal_cfg_attrs)*
         #[allow(non_camel_case_types)]
         #[doc(hidden)] // Signal struct is hidden, but the method returning it is not (IDE completion).
-        struct #individual_struct_name<'a> {
+        #vis_marker struct #individual_struct_name<'a> {
             #[doc(hidden)]
             typed: ::godot::register::TypedSignal<'a, #class_name, #param_tuple>,
         }

godot-macros/src/docs.rs

@@ -154,11 +154,17 @@ fn make_docs_from_attributes(doc: &[Attribute]) -> Option<String> {
 }
 
 fn make_signal_docs(signal: &SignalDefinition) -> Option<String> {
-    let name = &signal.signature.name;
-    let params = params(signal.signature.params.iter().filter_map(|(x, _)| match x {
-        FnParam::Receiver(_) => None,
-        FnParam::Typed(y) => Some((&y.name, &y.ty)),
-    }));
+    let name = &signal.fn_signature.name;
+    let params = params(
+        signal
+            .fn_signature
+            .params
+            .iter()
+            .filter_map(|(x, _)| match x {
+                FnParam::Receiver(_) => None,
+                FnParam::Typed(y) => Some((&y.name, &y.ty)),
+            }),
+    );
     let desc = make_docs_from_attributes(&signal.external_attributes)?;
     Some(format!(
         r#"
@@ -251,7 +257,11 @@ pub fn make_method_docs(method: &FuncDefinition) -> Option<String> {
         .registered_name
         .clone()
         .unwrap_or_else(|| method.rust_ident().to_string());
-    let ret = method.signature_info.ret_type.to_token_stream().to_string();
+    let ret = method
+        .signature_info
+        .return_type
+        .to_token_stream()
+        .to_string();
     let params = params(
         method
             .signature_info

godot-macros/src/lib.rs

@@ -774,6 +774,9 @@ pub fn derive_godot_class(input: TokenStream) -> TokenStream {
 /// method, you can access all declared signals in `self.signals().some_signal()` or `gd.signals().some_signal()`. The returned object is
 /// of type [`TypedSignal`], which provides further APIs for emitting and connecting, among others.
 ///
+/// Visibility of signals **must not exceed class visibility**. If your class is private (as above) and you declare your signal as `pub fn`,
+/// you will get a compile error "can't leak private type".
+///
 /// A detailed explanation with examples is available in the [book chapter _Registering signals_](https://godot-rust.github.io/book/register/signals.html).
 ///
 /// [`WithSignals`]: ../obj/trait.WithSignals.html

godot-macros/src/util/mod.rs

@@ -83,7 +83,7 @@ pub(crate) use require_api_version;
 
 pub fn reduce_to_signature(function: &venial::Function) -> venial::Function {
     let mut reduced = function.clone();
-    reduced.vis_marker = None; // TODO needed?
+    reduced.vis_marker = None; // retained outside in the case of #[signal].
     reduced.attributes.clear();
     reduced.tk_semicolon = None;
     reduced.body = None;

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

@@ -245,51 +245,59 @@ fn signal_construction_and_id() {
 /// Global sets the value of the received argument and whether it was a static function.
 static LAST_STATIC_FUNCTION_ARG: Global<i64> = Global::default();
 
-#[derive(GodotClass)]
-#[class(init, base=Object)]
-struct Emitter {
-    _base: Base<Object>,
-    #[cfg(since_api = "4.2")]
-    last_received_int: i64,
-}
+// Separate module to test signal visibility.
+use emitter::Emitter;
 
-#[godot_api]
-impl Emitter {
-    #[signal]
-    fn signal_unit();
+mod emitter {
+    use super::*;
+
+    #[derive(GodotClass)]
+    #[class(init, base=Object)]
+    pub struct Emitter {
+        _base: Base<Object>,
+        #[cfg(since_api = "4.2")]
+        pub last_received_int: i64,
+    }
 
-    #[signal]
-    fn signal_int(arg1: i64);
+    #[godot_api]
+    impl Emitter {
+        #[signal]
+        fn signal_unit();
 
-    #[signal]
-    fn signal_obj(arg1: Gd<Object>, arg2: GString);
+        // Public to demonstrate usage inside module.
+        #[signal]
+        pub fn signal_int(arg1: i64);
 
-    #[func]
-    fn self_receive(&mut self, arg1: i64) {
-        #[cfg(since_api = "4.2")]
-        {
-            self.last_received_int = arg1;
+        #[signal]
+        fn signal_obj(arg1: Gd<Object>, arg2: GString);
+
+        #[func]
+        pub fn self_receive(&mut self, arg1: i64) {
+            #[cfg(since_api = "4.2")]
+            {
+                self.last_received_int = arg1;
+            }
         }
-    }
 
-    #[func]
-    fn self_receive_static(arg1: i64) {
-        *LAST_STATIC_FUNCTION_ARG.lock() = arg1;
-    }
+        #[func]
+        fn self_receive_static(arg1: i64) {
+            *LAST_STATIC_FUNCTION_ARG.lock() = arg1;
+        }
 
-    // "Internal" means connect/emit happens from within the class (via &mut self).
+        // "Internal" means connect/emit happens from within the class (via &mut self).
 
-    #[cfg(since_api = "4.2")]
-    fn connect_signals_internal(&mut self, tracker: Rc<Cell<i64>>) {
-        let mut sig = self.signals().signal_int();
-        sig.connect_self(Self::self_receive);
-        sig.connect(Self::self_receive_static);
-        sig.connect(move |i| tracker.set(i));
-    }
+        #[cfg(since_api = "4.2")]
+        pub fn connect_signals_internal(&mut self, tracker: Rc<Cell<i64>>) {
+            let mut sig = self.signals().signal_int();
+            sig.connect_self(Self::self_receive);
+            sig.connect(Self::self_receive_static);
+            sig.connect(move |i| tracker.set(i));
+        }
 
-    #[cfg(since_api = "4.2")]
-    fn emit_signals_internal(&mut self) {
-        self.signals().signal_int().emit(1234);
+        #[cfg(since_api = "4.2")]
+        pub fn emit_signals_internal(&mut self) {
+            self.signals().signal_int().emit(1234);
+        }
     }
 }