Fix visibility where class is wider than signal

Addresses immediate issue where a `pub struct MyClass` has a private
#[signal] declaration. This caused "leaks private type" problems with
the associated type <MyClass as WithSignals>::SignalCollection, that
mentions a generated (private) type.

This approach can possibly be extended to the inverse case later, to
allow the inverse case: pub #[signal]s in private classes.

Author: Bromeon

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

@@ -112,8 +112,8 @@ struct SignalDetails<'a> {
     individual_struct_name: Ident,
     /// Visibility, e.g. `pub(crate)`
     vis_marker: Option<venial::VisMarker>,
-    /// Detected visibility as strongly typed enum.
-    vis_classified: SignalVisibility,
+    // /// Detected visibility as strongly typed enum.
+    // vis_classified: SignalVisibility,
 }
 
 impl<'a> SignalDetails<'a> {
@@ -150,7 +150,7 @@ impl<'a> SignalDetails<'a> {
         let individual_struct_name = format_ident!("__godot_Signal_{}_{}", class_name, signal_name);
 
         let vis_marker = &fn_signature.vis_marker;
-        let Some(vis_classified) = SignalVisibility::try_parse(vis_marker.as_ref()) else {
+        let Some(_vis_classified) = SignalVisibility::try_parse(vis_marker.as_ref()) else {
             return bail!(
                 vis_marker,
                 "invalid visibility `{}` for #[signal]; supported are `pub`, `pub(crate)`, `pub(super)` and private (no visibility marker)",
@@ -170,7 +170,7 @@ impl<'a> SignalDetails<'a> {
             signal_cfg_attrs,
             individual_struct_name,
             vis_marker: vis_marker.clone(),
-            vis_classified,
+            // vis_classified,
         })
     }
 }
@@ -187,8 +187,8 @@ pub fn make_signal_registrations(
 
     #[cfg(since_api = "4.2")]
     let mut collection_api = SignalCollection::default();
-    #[cfg(since_api = "4.2")]
-    let mut max_visibility = SignalVisibility::Priv;
+    // #[cfg(since_api = "4.2")]
+    // let mut max_visibility = SignalVisibility::Priv;
 
     for signal in signals {
         let SignalDefinition {
@@ -203,15 +203,15 @@ pub fn make_signal_registrations(
         #[cfg(since_api = "4.2")]
         if *has_builder {
             collection_api.extend_with(&details);
-            max_visibility = max_visibility.max(details.vis_classified);
+            // max_visibility = max_visibility.max(details.vis_classified);
         }
 
         let registration = make_signal_registration(&details, class_name_obj);
         signal_registrations.push(registration);
     }
 
     #[cfg(since_api = "4.2")]
-    let signal_symbols = make_signal_symbols(class_name, collection_api, max_visibility);
+    let signal_symbols = make_signal_symbols(class_name, collection_api);
     #[cfg(before_api = "4.2")]
     let signal_symbols = None;
 
@@ -391,7 +391,7 @@ fn make_signal_individual_struct(details: &SignalDetails) -> TokenStream {
 fn make_signal_symbols(
     class_name: &Ident,
     collection_api: SignalCollection,
-    max_visibility: SignalVisibility,
+    // 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() {
@@ -405,8 +405,13 @@ fn make_signal_symbols(
     let individual_structs = collection_api.individual_structs;
 
     // The collection cannot be `pub` because `Deref::Target` contains the class type, which leads to "leak private type" errors.
-    // Max visibility: the user decides which visibility is acceptable for individual #[signal]s, which is bounded by the class' own
-    // visibility. Since we assume that decision is correct, the collection itself can also share the widest visibility of any #[signal].
+    // We thus adopt the visibility of the #[derive(GodotClass)] struct, imported via macro trick.
+    //
+    // A previous approach (that cannot access the struct visibility) used "max visibility": the user decides which visibility is acceptable f
+    // or individual #[signal]s. They can all be at most the class visibility. Since we assume that decision is correct, the signal collection
+    // itself can also share the widest visibility of any #[signal]. This approach however still led to problems because there's a 2-way
+    // dependency: `impl WithSignals for MyClass` has an associated type `SignalCollection` that mentions the generated collection type. If
+    // that collection type has *lower* visibility than the class, we *also* run into "leak private type" errors.
 
     // Unrelated, we could use the following for encapsulation:
     //     #[cfg(since_api = "4.2")]
@@ -430,13 +435,17 @@ fn make_signal_symbols(
     // Downside is slightly higher complexity and introducing signals in secondary blocks becomes harder (although we could use another
     // module name, we'd need a way to create unique names).
 
+    let visibility_macro = util::format_class_visibility_macro(class_name);
+
     let code = quote! {
-        #[allow(non_camel_case_types)]
-        #[doc(hidden)] // Only on struct, not methods, to allow completion in IDEs.
-        #max_visibility struct #collection_struct_name<'c, C> {
-            // Hiding necessary because it's in the same scope as the user-defined class, so appearing in IDE completion.
-            #[doc(hidden)]
-            __internal_obj: Option<::godot::private::UserSignalObject<'c, C>>
+        #visibility_macro! {
+            #[allow(non_camel_case_types)]
+            #[doc(hidden)] // Only on struct, not methods, to allow completion in IDEs.
+            struct #collection_struct_name<'c, C> {
+                // Hiding necessary because it's in the same scope as the user-defined class, so appearing in IDE completion.
+                #[doc(hidden)]
+                __internal_obj: Option<::godot::private::UserSignalObject<'c, C>>
+            }
         }
 
         impl<'c, C> #collection_struct_name<'c, C>

godot-macros/src/class/derive_godot_class.rs

@@ -5,9 +5,6 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
 
-use proc_macro2::{Ident, Punct, TokenStream};
-use quote::{format_ident, quote, quote_spanned};
-
 use crate::class::{
     make_property_impl, make_virtual_callback, BeforeKind, Field, FieldCond, FieldDefault,
     FieldExport, FieldVar, Fields, SignatureInfo,
@@ -17,6 +14,8 @@ use crate::util::{
     require_api_version, KvParser,
 };
 use crate::{handle_mutually_exclusive_keys, util, ParseResult};
+use proc_macro2::{Ident, Punct, TokenStream};
+use quote::{format_ident, quote, quote_spanned};
 
 pub fn derive_godot_class(item: venial::Item) -> ParseResult<TokenStream> {
     let class = item.as_struct().ok_or_else(|| {
@@ -143,6 +142,8 @@ pub fn derive_godot_class(item: venial::Item) -> ParseResult<TokenStream> {
         pub struct #funcs_collection_struct_name {}
     };
 
+    let visibility_macro = make_visibility_macro(class_name, class.vis_marker.as_ref());
+
     Ok(quote! {
         impl ::godot::obj::GodotClass for #class_name {
             type Base = #base_class;
@@ -172,6 +173,7 @@ pub fn derive_godot_class(item: venial::Item) -> ParseResult<TokenStream> {
         #godot_exports_impl
         #user_class_impl
         #init_expecter
+        #visibility_macro
         #( #deprecations )*
         #( #errors )*
 
@@ -185,6 +187,30 @@ pub fn derive_godot_class(item: venial::Item) -> ParseResult<TokenStream> {
     })
 }
 
+/// Generates code for a decl-macro, which takes any item and prepends it with the visibility marker of the class.
+///
+/// Used to access the visibility of the class in other proc-macros like `#[godot_api]`.
+fn make_visibility_macro(
+    class_name: &Ident,
+    vis_marker: Option<&venial::VisMarker>,
+) -> TokenStream {
+    let macro_name = util::format_class_visibility_macro(class_name);
+
+    quote! {
+        macro_rules! #macro_name {
+            (
+                $( #[$meta:meta] )*
+                struct $( $tt:tt )+
+            ) => {
+                $( #[$meta] )*
+                #vis_marker struct $( $tt )+
+            };
+
+            // Can be expanded to `fn` etc. if needed.
+        }
+    }
+}
+
 /// 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/util/mod.rs

@@ -407,3 +407,8 @@ pub fn format_funcs_collection_constant(_class_name: &Ident, func_name: &Ident)
 pub fn format_funcs_collection_struct(class_name: &Ident) -> Ident {
     format_ident!("__godot_{class_name}_Funcs")
 }
+
+/// Returns the name of the macro used to communicate the `struct` (class) visibility to other symbols.
+pub fn format_class_visibility_macro(class_name: &Ident) -> Ident {
+    format_ident!("__godot_{class_name}_vis_macro")
+}

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

@@ -446,6 +446,22 @@ impl Receiver {
     }
 }
 
+// ----------------------------------------------------------------------------------------------------------------------------------------------
+
+// Class which is deliberately `pub` but has only private `#[signal]` declaration.
+// Regression test, as this caused "leaked private types" in the past.
+#[derive(GodotClass)]
+#[class(init, base=Object)]
+pub struct PubClassPrivSignal {
+    _base: Base<Object>,
+}
+
+#[godot_api]
+impl PubClassPrivSignal {
+    #[signal]
+    fn private_signal();
+}
+
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // 4.2+ custom callables