Explicit signal visibility

[Bromeon]

Fixes #1073.

New semantics:

1. #[signal] declarations can now have a visibility specifier: pub, pub(crate), etc.
2. Both the generated signal struct and the signals().my_sig() accessor method inherit this visibility.

---

Warning

#[signal] visibility must not exceed class visibility.

---

Otherwise it will lead to an error message:

can't leak private type

pointing to the class macro (not the signal). I'm aware this is not the best UX, but I don't know of any better approach. Suggestions welcome.

The core problem is that the generated signal struct has a Deref impl to TypedSignal<C, ...>, where C is the declaring class. So if the signal struct had wider visibility than C, it would implicitly make the other type public, which rustc doesn't like. Maybe there's a trick here... See also #1061 (comment).

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1075

[austin226]

Nice!

I tested the "can't leak private type" case with this diff:

diff --git a/itest/rust/src/builtin_tests/containers/signal_test.rs b/itest/rust/src/builtin_tests/containers/signal_test.rs
index 1d11a654..3cc25ef7 100644
--- a/itest/rust/src/builtin_tests/containers/signal_test.rs
+++ b/itest/rust/src/builtin_tests/containers/signal_test.rs
@@ -259,6 +259,20 @@ mod emitter {
         pub last_received_int: i64,
     }
 
+    #[derive(GodotClass)]
+    #[class(init, base=Object)]
+    struct Emitter2 {
+        _base: Base<Object>,
+        #[cfg(since_api = "4.2")]
+        pub last_received_int: i64,
+    }
+
+    #[godot_api]
+    impl Emitter2 {
+        #[signal]
+        pub fn signal_int_2(arg1: i64);
+    }
+
     #[godot_api]
     impl Emitter {
         #[signal]

and I do see the expected error:

error[E0446]: private type `Emitter2` in public interface
   --> itest/rust/src/builtin_tests/containers/signal_test.rs:270:5
    |
264 |     struct Emitter2 {
    |     --------------- `Emitter2` declared as private
...
270 |     #[godot_api]
    |     ^^^^^^^^^^^^ can't leak private type
    |
    = note: this error originates in the attribute macro `godot_api` (in Nightly builds, run with -Z macro-backtrace for more info)

warning: field `last_received_int` is never read
   --> itest/rust/src/builtin_tests/containers/signal_test.rs:267:13
    |
264 |     struct Emitter2 {
    |            -------- field in this struct
...
267 |         pub last_received_int: i64,
    |             ^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(dead_code)]` on by default

For more information about this error, try `rustc --explain E0446`.

Ideally the error would point to the line with the signal definition instead of the line with #[godot_api], but I have no idea now difficult of a change that would be. For now this seems good enough, since one can always search for the error string "can't leak private type" and find it in the comment at godot-macros/src/lib.rs.

[ColinWttt]

This looks good to me. If I accidentally forget pub/pub(crate), the compile-time error will indicate which signal is private.

error: type `__godot_Signal_CardCostArea_discard_cost_cards<'_>` is private
  --> src\class\card_hand.rs:82:9
   |
82 | /         card_cost_area
83 | |             .signals()
84 | |             .discard_cost_cards()
   | |_________________________________^ private type

[Bromeon]

Ideally the error would point to the line with the signal definition instead of the line with #[godot_api], but I have no idea now difficult of a change that would be.

Yes, as mentioned in the initial post, I'm aware of the bad UX here.

There are a few things holding me back:

1. An associated type (Deref::Target) in the impl for GeneratedSignalStruct (which can be public) cannot be a private type (the surrounding class).

   • Conceptually this makes sense. There's no point in accessing Gd<T>::signals().some_signal() from outside T's module, when T itself is private.

2. In the #[godot_api] macro, I have no access to the class' #[derive(GodotClass)] macro, and thus no idea what the actual visibility of the struct is. But the signal must not be "more public" than the class.

   • This is "technically correct", still.
   • However, it may be surprising because in a regular impl, you can have pub items, even if the Self struct/enum is private. They are then automatically limited to that visibility.

3. I don't have direct control over the compiler error and the fact that the macro obscures its origin (even nightly -Z macro-backtrace isn't much better). The only thing I could imagine is to generate another error first, which is more expressive. Ideas welcome.

[Bromeon]

This looks good to me. If I accidentally forget pub/pub(crate), the compile-time error will indicate which signal is private.

Yes, this part is quite nice.

It's the "accidentally left a signal pub when making the class private" which is troubling...