From 8b2fbfa7117ebe0f5b0ed945ff5c2595a25345e0 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Fri, 28 Feb 2025 23:19:30 +0100 Subject: [PATCH 1/2] Create test for #[signal] generated method visibility --- .../builtin_tests/containers/signal_test.rs | 79 ++++++++++--------- 1 file changed, 43 insertions(+), 36 deletions(-) diff --git a/itest/rust/src/builtin_tests/containers/signal_test.rs b/itest/rust/src/builtin_tests/containers/signal_test.rs index 6aa404948..dca4cd20c 100644 --- a/itest/rust/src/builtin_tests/containers/signal_test.rs +++ b/itest/rust/src/builtin_tests/containers/signal_test.rs @@ -245,51 +245,58 @@ 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 = Global::default(); -#[derive(GodotClass)] -#[class(init, base=Object)] -struct Emitter { - _base: Base, - #[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, + #[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, arg2: GString); + #[signal] + 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, 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>) { - 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>) { + 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); + } } } From 084f764184ee0fc0040e40b5e1771dd27bf45f77 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Wed, 12 Mar 2025 14:48:28 +0100 Subject: [PATCH 2/2] Propagate signal visibility from #[signal] declaration --- godot-macros/src/class/data_models/func.rs | 8 ++-- .../src/class/data_models/inherent_impl.rs | 12 ++--- godot-macros/src/class/data_models/signal.rs | 44 ++++++++++++------- godot-macros/src/docs.rs | 22 +++++++--- godot-macros/src/lib.rs | 3 ++ godot-macros/src/util/mod.rs | 2 +- .../builtin_tests/containers/signal_test.rs | 3 +- 7 files changed, 57 insertions(+), 37 deletions(-) diff --git a/godot-macros/src/class/data_models/func.rs b/godot-macros/src/class/data_models/func.rs index 70e971f7e..8959719cb 100644 --- a/godot-macros/src/class/data_models/func.rs +++ b/godot-macros/src/class/data_models/func.rs @@ -188,7 +188,7 @@ pub struct SignatureInfo { pub receiver_type: ReceiverType, pub param_idents: Vec, pub param_types: Vec, - 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, } } diff --git a/godot-macros/src/class/data_models/inherent_impl.rs b/godot-macros/src/class/data_models/inherent_impl.rs index 1019b96fe..8f50a481f 100644 --- a/godot-macros/src/class/data_models/inherent_impl.rs +++ b/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, }); diff --git a/godot-macros/src/class/data_models/signal.rs b/godot-macros/src/class/data_models/signal.rs index b2b20e629..db1e4e20b 100644 --- a/godot-macros/src/class/data_models/signal.rs +++ b/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, @@ -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,11 +44,13 @@ struct SignalDetails<'a> { signal_cfg_attrs: Vec<&'a venial::Attribute>, /// `MyClass_MySignal` individual_struct_name: Ident, + /// Visibility, e.g. `pub(crate)` + vis_marker: Option, } 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> { @@ -56,7 +58,7 @@ impl<'a> SignalDetails<'a> { 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,7 +228,7 @@ 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, @@ -227,6 +236,7 @@ fn make_signal_individual_struct(details: &SignalDetails) -> TokenStream { 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>, } diff --git a/godot-macros/src/docs.rs b/godot-macros/src/docs.rs index 895d58f07..7fbf7e78e 100644 --- a/godot-macros/src/docs.rs +++ b/godot-macros/src/docs.rs @@ -154,11 +154,17 @@ fn make_docs_from_attributes(doc: &[Attribute]) -> Option { } fn make_signal_docs(signal: &SignalDefinition) -> Option { - 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 { .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 diff --git a/godot-macros/src/lib.rs b/godot-macros/src/lib.rs index c16074225..00029d4df 100644 --- a/godot-macros/src/lib.rs +++ b/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 diff --git a/godot-macros/src/util/mod.rs b/godot-macros/src/util/mod.rs index a14b1d283..d7a27d45e 100644 --- a/godot-macros/src/util/mod.rs +++ b/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; diff --git a/itest/rust/src/builtin_tests/containers/signal_test.rs b/itest/rust/src/builtin_tests/containers/signal_test.rs index dca4cd20c..1d11a6547 100644 --- a/itest/rust/src/builtin_tests/containers/signal_test.rs +++ b/itest/rust/src/builtin_tests/containers/signal_test.rs @@ -264,8 +264,9 @@ mod emitter { #[signal] fn signal_unit(); + // Public to demonstrate usage inside module. #[signal] - fn signal_int(arg1: i64); + pub fn signal_int(arg1: i64); #[signal] fn signal_obj(arg1: Gd, arg2: GString);