Implement trait-based entry point declaration #1026

chitoyuu · 2023-02-03T17:25:34Z

Adds the #[gdnative::init::callbacks] attribute macro and GDNativeCallbacks trait for entry point declaration, replacing the old system of init macros. godot_init! is kept as a shim and deprecated.

This also adds support for the following callbacks:

gdnative_singleton
nativescript_terminate
nativescript_frame
nativescript_thread_enter
nativescript_thread_exit

As a side effect, most of the init boilerplate are no longer emitted into the user crate, instead staying in gdnative-core. Some further internal refactoring might be necessary to improve code organization.

Close #985, Close #338

chitoyuu · 2023-02-03T17:34:54Z

Example code looks like this:

struct HelloWorldLibrary;

#[gdnative::init::callbacks]
unsafe impl GDNativeCallbacks for HelloWorldLibrary {
    fn nativescript_init(handle: InitHandle) {
        handle.add_class::<HelloWorld>();
    }
}

The safety section is still TODO. GDNativeCallbacks currently contains definitions for all known GDNative symbols, including many niche ones that aren't previously exposed (for many years!) and are of dubious usefulness. I wonder if it's a little bit too low-level for the average use case of simply registering a bunch of types, but still it should be a step up from the previous system.

I don't know why the TODO check is somehow passing on CI. It's failing on my local machine. Might need to look into this later.

Bromeon

Very cool to see this pattern also in GDNative 🙂

I haven't looked at all the details yet, but looks good so far! Some of those are indeed very low-level callbacks. I'm not sure if Rust developers have even needed them, but you spent quite some effort and likely documented them better than Godot itself, so I support their addition!

You marked this as breaking-change, but is it when the old macros are kept around? I would probably even keep them at least during the next minor version, to give people time to migrate. Keep in mind that removing the init macros breaks every single godot-rust application.

Bromeon · 2023-02-03T22:56:58Z

gdnative-core/src/init/mod.rs

+    /// Callback invoked after startup, immediately after `gdnative_init`, if the `singleton`
+    /// option is set for the `GDNativeLibrary` resource.
+    ///
+    /// **Attention:** This is **NOT** what you're looking for! `gdnative_singleton` has nothing
+    /// to do with exposing "singleton" objects or static-looking methods to GDScript. Instead,
+    /// create a [`NativeClass`][crate::export::NativeClass] inheriting `Node` and make that an
+    /// auto-load in your project settings. See [the FAQ][auto-load-faq] for an example.
+    ///
+    /// Despite the confusing name, what the `singleton` option does is to mark the GDNative
+    /// library for automatic initialization at the startup of the application before any game
+    /// content and scripts are loaded, which might be useful in some use cases. The extra
+    /// callback itself doesn't have any special qualities.
+    ///
+    /// At the time [`Self::gdnative_singleton`] is called, it is guaranteed that:
+    ///
+    /// - [`Self::gdnative_init`] has been invoked exactly once, immediately before.
+    /// - No other API callbacks have been invoked.
+    ///
+    /// It is NOT guaranteed that:
+    ///
+    /// - There are any practical uses to this callback. **You do not need this to create a
+    ///   singleton object, nor does it help you.** Use an [auto-load singleton][auto-load-faq]
+    ///   instead.
+    ///
+    /// [auto-load-faq]: https://godot-rust.github.io/book/gdnative/faq/code.html#can-i-implement-static-methods-in-gdnative
+    #[inline]
+    fn gdnative_singleton() {}


Should this maybe be declared at the end then? Would appear lowest in docs (main view, not alphabetic sidebar).

I assume the current ordering is by group (first gdnative_, then nativescript_) and top-down (first the whole library, then the NativeScript functionality). If you want to preserve the grouping, could make sense to mention the nativescript_ ones first -- after all, nativescript_init is by far the most used method.

Yes, it should make sense to mention nativescript_init in the docs, although given its prominence I wonder if it should be given its own layer of abstraction. We should certainly leave the possibility of using these low-level callbacks open for completeness nevertheless.

chitoyuu · 2023-02-04T04:08:31Z

You marked this as breaking-change, but is it when the old macros are kept around? I would probably even keep them at least during the next minor version, to give people time to migrate. Keep in mind that removing the init macros breaks every single godot-rust application.

Yes, indeed I'm aware of that. It has to be a breaking change because the separate macros can't be implemented in terms of the new system, only godot_init! can (which is). I think it should be possible to keep the empty variants of the macros as no-ops, but anything outside than the godot_init! pattern will have to be broken. Perhaps we can do that.

bluenote10 · 2023-02-04T11:36:30Z

From a user perspective the most puzzling part about this change is that now an unsafe is needed, while the old pattern didn't. The notion of an unsafe trait is a bit niche, and probably not familiar to everyone. For instance, I was at first wondering if this means that all the init functions now run in an unsafe scope. After experimenting a bit, I don't think this is the case fortunately, right? But then I still don't fully understand what it means for me as a user having to provide an unsafe impl. Reading up on the topic I came across statements like

When a trait is marked as unsafe, it means the trait implementor needs to implement carefully.

but I wouldn't know what exactly I have to worry about implementing my init functions.

To avoid frequent questions, it would be great if the trait docstring could briefly explain what the unsafe is trying to communicate / accomplish.

Bromeon

@chitoyuu Makes sense, thanks! Sorry if I created extra effort with the 2nd commit, only meant to clarify what the compatibility strategy is.

@bluenote10 The idea is that unsafe on the usage side can be interpreted as an "opt-in":

unsafe { function() } means I have personally ensured that invariants for the function call are maintained, which the Rust type system and compiler cannot verify.
unsafe impl Trait means I have personally ensured that implementing the trait for the given type is sound (again because the compiler cannot verify). Think of Send/Sync impls.

In the godot-rust context, unsafe concretely means that the user opts in to ensure that non-Rust code follows certain rules: GDScript does not invoke UB or create data races, and so on. Since this applies on the entire project level, doing it at the entry point is the most straightforward approach.

I'm currently writing an exhaustive article about the safety guidelines in GDExtension with all the background and rationale behind certain designs, and plan to link to it from the API doc. Likely a large part of it applies to GDNative as well.

The question is, is this unsafe really needed at the moment? The GDNative Rust binding has a much more sophisticated type system than the GDExtension one and carefully makes sure that the user is made aware when they must uphold invariants (e.g. in assume_safe()). There may be edge cases, but does it warrant having both unsafes?

Bromeon · 2023-02-04T11:36:40Z

gdnative-core/src/init/macros.rs

+        };
+    };
+    (_ as $fn_name:ident) => {
+        ::core::compile_error!("this syntax is no longer supported. use the #[gdnative::init::callbacks] attribute macro with a prefix instead");


Do we explicitly use core and not std? The distinction should only matter in no_std environments, something we anyway can't provide 🤔

Regarding no_std, I don't actually remember there being anything from std proper (not in alloc) that we depend for core functionalities. It should be irrelevant for all the openly available platforms, but perhaps it can be slightly relevant for consoles? Not necessarily saying that it's something we need to have, but I think it is a possibility.

Of course it's also fine here to just use std, if you think that's better.

It's mostly a matter of consistency and surprise factor -- when I see core::, my association is immediately no_std, but maybe the Rust community sees this differently, and using core when it suffices is good practice.

Most of the codebase currently uses std:: though, with a handful of core:: occurrences for things like core::cmp::Ord, core::marker::PhantomData -- but even those are not used consistently. It's only a cosmetic issue though.

I understand. For this PR, I'll switch to std. I've created an issue for investigating no_std later: #1028

chitoyuu · 2023-02-04T13:05:44Z

The question is, is this unsafe really needed at the moment? The GDNative Rust binding has a much more sophisticated type system than the GDExtension one and carefully makes sure that the user is made aware when they must uphold invariants (e.g. in assume_safe()). There may be edge cases, but does it warrant having both unsafes?

You're right that it isn't immediately necessary. Perhaps it makes sense to remove it for now.

Adds the `#[gdnative::init::callbacks]` attribute macro and `GDNativeCallbacks` trait for entry point declaration, replacing the old system of init macros. `godot_init!` is kept as a shim and deprecated. This also adds support for the following callbacks: - `gdnative_singleton` - `nativescript_terminate` - `nativescript_frame` - `nativescript_thread_enter` - `nativescript_thread_exit` As a side effect, most of the init boilerplate are no longer emitted into the user crate, instead staying in `gdnative-core`. Some further internal refactoring might be necessary to improve code organization. Close godot-rust#985, Close godot-rust#338

chitoyuu · 2023-02-06T07:39:09Z

Applied suggestions in the thread.

bors try

bors · 2023-02-06T07:53:29Z

try

Build succeeded:

full-ci

chitoyuu · 2023-02-06T23:18:51Z

bors r+

bors · 2023-02-06T23:34:18Z

Build succeeded:

full-ci

chitoyuu added feature Adds functionality to the library c: core Component: core (mod core_types, object, log, init, ...) breaking-change Issues and PRs that are breaking to fix/merge. labels Feb 3, 2023

chitoyuu added this to the v0.12.0 milestone Feb 3, 2023

Bromeon reviewed Feb 3, 2023

View reviewed changes

Bromeon reviewed Feb 4, 2023

View reviewed changes

chitoyuu mentioned this pull request Feb 4, 2023

Investigate no_std, improve core / std consistency #1028

Open

chitoyuu added 2 commits February 6, 2023 07:37

Shim legacy init macros whenever possible.

bdd6ecd

chitoyuu force-pushed the feature/trait-based-init branch from 732dedc to bdd6ecd Compare February 6, 2023 07:38

bors bot added a commit that referenced this pull request Feb 6, 2023

Try #1026:

f2b26ce

chitoyuu marked this pull request as ready for review February 6, 2023 09:19

bors bot merged commit 25f108f into godot-rust:master Feb 6, 2023

chitoyuu mentioned this pull request Feb 11, 2023

Proof of concept: Register wrapped functions #1033

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement trait-based entry point declaration #1026

Implement trait-based entry point declaration #1026

chitoyuu commented Feb 3, 2023

chitoyuu commented Feb 3, 2023

Bromeon left a comment

Bromeon Feb 3, 2023

chitoyuu Feb 4, 2023

chitoyuu commented Feb 4, 2023

bluenote10 commented Feb 4, 2023

Bromeon left a comment •

edited

Loading

Bromeon Feb 4, 2023

chitoyuu Feb 4, 2023 •

edited

Loading

Bromeon Feb 4, 2023

chitoyuu Feb 4, 2023

chitoyuu commented Feb 4, 2023

chitoyuu commented Feb 6, 2023

bors bot commented Feb 6, 2023

chitoyuu commented Feb 6, 2023

bors bot commented Feb 6, 2023

Implement trait-based entry point declaration #1026

Implement trait-based entry point declaration #1026

Conversation

chitoyuu commented Feb 3, 2023

chitoyuu commented Feb 3, 2023

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon Feb 3, 2023

Choose a reason for hiding this comment

chitoyuu Feb 4, 2023

Choose a reason for hiding this comment

chitoyuu commented Feb 4, 2023

bluenote10 commented Feb 4, 2023

Bromeon left a comment • edited Loading

Choose a reason for hiding this comment

Bromeon Feb 4, 2023

Choose a reason for hiding this comment

chitoyuu Feb 4, 2023 • edited Loading

Choose a reason for hiding this comment

Bromeon Feb 4, 2023

Choose a reason for hiding this comment

chitoyuu Feb 4, 2023

Choose a reason for hiding this comment

chitoyuu commented Feb 4, 2023

chitoyuu commented Feb 6, 2023

bors bot commented Feb 6, 2023

try

chitoyuu commented Feb 6, 2023

bors bot commented Feb 6, 2023

Bromeon left a comment •

edited

Loading

chitoyuu Feb 4, 2023 •

edited

Loading