Merge pull request #774 from lilizoey/refactor/godot-ffi-cleanup-unsafe

More thoroughly document `unsafe` in `godot-ffi`

Author: Bromeon

godot-codegen/src/generator/extension_interface.rs

@@ -84,6 +84,9 @@ fn generate_proc_address_funcs(h_path: &Path) -> TokenStream {
         }
 
         impl GDExtensionInterface {
+            // TODO: Figure out the right safety preconditions. This currently does not have any because incomplete safety docs
+            // can cause issues with people assuming they are sufficient.
+            #[allow(clippy::missing_safety_doc)]
             pub(crate) unsafe fn load(
                 get_proc_address: crate::GDExtensionInterfaceGetProcAddress,
             ) -> Self {

godot-codegen/src/generator/method_tables.rs

@@ -286,7 +286,10 @@ fn make_named_method_table(info: NamedMethodTable) -> TokenStream {
             pub const CLASS_COUNT: usize = #class_count;
             pub const METHOD_COUNT: usize = #method_count;
 
-            pub fn load(
+            // TODO: Figure out the right safety preconditions. This currently does not have any because incomplete safety docs
+            // can cause issues with people assuming they are sufficient.
+            #[allow(clippy::missing_safety_doc)]
+            pub unsafe fn load(
                 #ctor_parameters
             ) -> Self {
                 #pre_init_code
@@ -383,8 +386,11 @@ fn make_method_table(info: IndexedMethodTable) -> TokenStream {
             pub const CLASS_COUNT: usize = #class_count;
             pub const METHOD_COUNT: usize = #method_count;
 
+            // TODO: Figure out the right safety preconditions. This currently does not have any because incomplete safety docs
+            // can cause issues with people assuming they are sufficient.
+            #[allow(clippy::missing_safety_doc)]
             #unused_attr
-            pub fn load(
+            pub unsafe fn load(
                 #ctor_parameters
             ) -> Self {
                 #pre_init_code
@@ -455,8 +461,11 @@ fn make_method_table(info: IndexedMethodTable) -> TokenStream {
             pub const CLASS_COUNT: usize = #class_count;
             pub const METHOD_COUNT: usize = #method_count;
 
+            // TODO: Figure out the right safety preconditions. This currently does not have any because incomplete safety docs
+            // can cause issues with people assuming they are sufficient.
+            #[allow(clippy::missing_safety_doc)]
             #unused_attr
-            pub fn load() -> Self {
+            pub unsafe fn load() -> Self {
                 // SAFETY: interface and lifecycle tables are initialized at this point, so we can get 'static references to them.
                 let (interface, lifecycle_table) = unsafe {
                     (crate::get_interface(), crate::builtin_lifecycle_api())

godot-core/src/init/mod.rs

@@ -90,7 +90,9 @@ unsafe extern "C" fn ffi_initialize_layer<E: ExtensionLibrary>(
             LEVEL_SERVERS_CORE_LOADED.store(true, Relaxed);
         }
 
-        gdext_on_level_init(level);
+        // SAFETY: Godot will call this from the main thread, after `__gdext_load_library` where the library is initialized,
+        // and only once per level.
+        unsafe { gdext_on_level_init(level) };
         E::on_level_init(level);
     }
 
@@ -120,27 +122,27 @@ unsafe extern "C" fn ffi_deinitialize_layer<E: ExtensionLibrary>(
 }
 
 /// Tasks needed to be done by gdext internally upon loading an initialization level. Called before user code.
-fn gdext_on_level_init(level: InitLevel) {
-    // SAFETY: we are in the main thread, during initialization, no other logic is happening.
+///
+/// # Safety
+///
+/// - Must be called from the main thread.
+/// - The interface must have been initialized.
+/// - Must only be called once per level.
+#[deny(unsafe_op_in_unsafe_fn)]
+unsafe fn gdext_on_level_init(level: InitLevel) {
     // TODO: in theory, a user could start a thread in one of the early levels, and run concurrent code that messes with the global state
     // (e.g. class registration). This would break the assumption that the load_class_method_table() calls are exclusive.
     // We could maybe protect globals with a mutex until initialization is complete, and then move it to a directly-accessible, read-only static.
-    unsafe {
-        match level {
-            InitLevel::Core => {}
-            InitLevel::Servers => {
-                sys::load_class_method_table(sys::ClassApiLevel::Server);
-            }
-            InitLevel::Scene => {
-                sys::load_class_method_table(sys::ClassApiLevel::Scene);
-                ensure_godot_features_compatible();
-            }
-            InitLevel::Editor => {
-                sys::load_class_method_table(sys::ClassApiLevel::Editor);
-            }
-        }
-        crate::registry::class::auto_register_classes(level);
+
+    // SAFETY: we are in the main thread, initialize has been called, has never been called with this level before.
+    unsafe { sys::load_class_method_table(level) };
+
+    if level == InitLevel::Scene {
+        // SAFETY: On the main thread, api initialized, `Scene` was initialized above.
+        unsafe { ensure_godot_features_compatible() };
     }
+
+    crate::registry::class::auto_register_classes(level);
 }
 
 /// Tasks needed to be done by gdext internally upon unloading an initialization level. Called after user code.
@@ -271,49 +273,17 @@ pub enum EditorRunBehavior {
 /// See also:
 /// - [`ExtensionLibrary::on_level_init()`]
 /// - [`ExtensionLibrary::on_level_deinit()`]
-#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug)]
-pub enum InitLevel {
-    /// First level loaded by Godot. Builtin types are available, classes are not.
-    Core,
-
-    /// Second level loaded by Godot. Only server classes and builtins are available.
-    Servers,
-
-    /// Third level loaded by Godot. Most classes are available.
-    Scene,
-
-    /// Fourth level loaded by Godot, only in the editor. All classes are available.
-    Editor,
-}
-
-impl InitLevel {
-    #[doc(hidden)]
-    pub fn from_sys(level: godot_ffi::GDExtensionInitializationLevel) -> Self {
-        match level {
-            sys::GDEXTENSION_INITIALIZATION_CORE => Self::Core,
-            sys::GDEXTENSION_INITIALIZATION_SERVERS => Self::Servers,
-            sys::GDEXTENSION_INITIALIZATION_SCENE => Self::Scene,
-            sys::GDEXTENSION_INITIALIZATION_EDITOR => Self::Editor,
-            _ => {
-                eprintln!("WARNING: unknown initialization level {level}");
-                Self::Scene
-            }
-        }
-    }
-    #[doc(hidden)]
-    pub fn to_sys(self) -> godot_ffi::GDExtensionInitializationLevel {
-        match self {
-            Self::Core => sys::GDEXTENSION_INITIALIZATION_CORE,
-            Self::Servers => sys::GDEXTENSION_INITIALIZATION_SERVERS,
-            Self::Scene => sys::GDEXTENSION_INITIALIZATION_SCENE,
-            Self::Editor => sys::GDEXTENSION_INITIALIZATION_EDITOR,
-        }
-    }
-}
+pub type InitLevel = sys::InitLevel;
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 
-fn ensure_godot_features_compatible() {
+/// # Safety
+///
+/// - Must be called from the main thread.
+/// - The interface must be initialized.
+/// - The `Scene` api level must have been initialized.
+#[deny(unsafe_op_in_unsafe_fn)]
+unsafe fn ensure_godot_features_compatible() {
     // The reason why we don't simply call Os::has_feature() here is that we might move the high-level engine classes out of godot-core
     // later, and godot-core would only depend on godot-sys. This makes future migrations easier. We still have access to builtins though.
 
@@ -323,8 +293,9 @@ fn ensure_godot_features_compatible() {
     let single = GString::from("single");
     let double = GString::from("double");
 
-    // SAFETY: main thread, after initialize(), valid string pointers.
     let gdext_is_double = cfg!(feature = "double-precision");
+
+    // SAFETY: main thread, after initialize(), valid string pointers, `Scene` initialized.
     let godot_is_double = unsafe {
         let is_single = sys::godot_has_feature(os_class.string_sys(), single.sys());
         let is_double = sys::godot_has_feature(os_class.string_sys(), double.sys());

godot-ffi/src/binding/mod.rs

@@ -79,6 +79,27 @@ unsafe impl Sync for ClassLibraryPtr {}
 // SAFETY: See `Sync` impl safety doc.
 unsafe impl Send for ClassLibraryPtr {}
 
+// ----------------------------------------------------------------------------------------------------------------------------------------------
+
+/// # Safety
+/// The table must not have been initialized yet.
+unsafe fn initialize_table<T>(table: &ManualInitCell<T>, value: T, what: &str) {
+    debug_assert!(
+        !table.is_initialized(),
+        "method table for {what} should only be initialized once"
+    );
+
+    table.set(value)
+}
+
+/// # Safety
+/// The table must have been initialized.
+unsafe fn get_table<T>(table: &'static ManualInitCell<T>, msg: &str) -> &'static T {
+    debug_assert!(table.is_initialized(), "{msg}");
+
+    table.get_unchecked()
+}
+
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Public API
 
@@ -154,25 +175,6 @@ pub unsafe fn class_editor_api() -> &'static ClassEditorMethodTable {
     )
 }
 
-/// # Safety
-/// The table must not have been initialized yet.
-unsafe fn initialize_table<T>(table: &ManualInitCell<T>, value: T, what: &str) {
-    debug_assert!(
-        !table.is_initialized(),
-        "method table for {what} should only be initialized once"
-    );
-
-    table.set(value)
-}
-
-/// # Safety
-/// The table must have been initialized.
-unsafe fn get_table<T>(table: &'static ManualInitCell<T>, msg: &str) -> &'static T {
-    debug_assert!(table.is_initialized(), "{msg}");
-
-    table.get_unchecked()
-}
-
 /// # Safety
 ///
 /// - The Godot binding must have been initialized before calling this function.

godot-ffi/src/binding/single_threaded.rs

@@ -143,7 +143,9 @@ impl BindingStorage {
     /// - The binding must be initialized.
     #[inline(always)]
     pub unsafe fn get_binding_unchecked() -> &'static GodotBinding {
-        let storage = Self::storage();
+        // SAFETY: The bindings were initialized on the main thread because `initialize` must be called from the main thread,
+        // and this function is called from the main thread.
+        let storage = unsafe { Self::storage() };
 
         // We only check if we are in the main thread in debug builds if we aren't building for a non-threaded Godot build,
         // since we could otherwise assume there won't be multi-threading.

godot-ffi/src/compat/compat_4_0.rs

@@ -15,7 +15,8 @@ use crate::compat::BindingCompat;
 
 pub type InitCompat = *const sys::GDExtensionInterface;
 
-impl BindingCompat for *const sys::GDExtensionInterface {
+// SAFETY: If `ensure_static_runtime_compatibility` succeeds then the other two functions should be safe to call.
+unsafe impl BindingCompat for *const sys::GDExtensionInterface {
     fn ensure_static_runtime_compatibility(&self) {
         // We try to read the first fields of the GDExtensionInterface struct, which are version numbers.
         // If those are unrealistic numbers, chances are high that `self` is in fact a function pointer (used for Godot 4.1.x).
@@ -40,7 +41,7 @@ impl BindingCompat for *const sys::GDExtensionInterface {
         );
     }
 
-    fn runtime_version(&self) -> sys::GDExtensionGodotVersion {
+    unsafe fn runtime_version(&self) -> sys::GDExtensionGodotVersion {
         // SAFETY: this method is only invoked after the static compatibility check has passed.
         // We thus know that Godot 4.0.x runs, and *self is a GDExtensionInterface pointer.
         let interface = unsafe { &**self };
@@ -52,7 +53,7 @@ impl BindingCompat for *const sys::GDExtensionInterface {
         }
     }
 
-    fn load_interface(&self) -> sys::GDExtensionInterface {
+    unsafe fn load_interface(&self) -> sys::GDExtensionInterface {
         unsafe { **self }
     }
 }

godot-ffi/src/compat/compat_4_1plus.rs

@@ -17,7 +17,10 @@ use crate::compat::BindingCompat;
 
 pub type InitCompat = sys::GDExtensionInterfaceGetProcAddress;
 
-impl BindingCompat for sys::GDExtensionInterfaceGetProcAddress {
+// SAFETY: If `ensure_static_runtime_compatibility` succeeds then the other two functions should be safe to call, except on wasm where
+// `ensure_static_runtime_compatibility` is a no-op.
+// TODO: fix `ensure_static_runtime_compatibility` on wasm.
+unsafe impl BindingCompat for sys::GDExtensionInterfaceGetProcAddress {
     // In WebAssembly, function references and data pointers live in different memory spaces, so trying to read the "memory"
     // at a function pointer (an index into a table) to heuristically determine which API we have (as is done below) won't work.
     #[cfg(target_family = "wasm")]
@@ -82,7 +85,9 @@ impl BindingCompat for sys::GDExtensionInterfaceGetProcAddress {
         // From here we can assume Godot 4.1+. We need to make sure that the runtime version is >= static version.
         // Lexicographical tuple comparison does that.
         let static_version = crate::GdextBuild::godot_static_version_triple();
-        let runtime_version_raw = self.runtime_version();
+
+        // SAFETY: We are now reasonably sure the runtime version is 4.1.
+        let runtime_version_raw = unsafe { self.runtime_version() };
 
         // SAFETY: Godot provides this version struct.
         let runtime_version = (
@@ -105,21 +110,28 @@ impl BindingCompat for sys::GDExtensionInterfaceGetProcAddress {
         }
     }
 
-    fn runtime_version(&self) -> sys::GDExtensionGodotVersion {
-        unsafe {
-            let get_proc_address = self.expect("get_proc_address unexpectedly null");
-            let get_godot_version = get_proc_address(sys::c_str(b"get_godot_version\0")); //.expect("get_godot_version unexpectedly null");
+    unsafe fn runtime_version(&self) -> sys::GDExtensionGodotVersion {
+        let get_proc_address = self.expect("get_proc_address unexpectedly null");
 
-            let get_godot_version =
-                crate::cast_fn_ptr!(get_godot_version as sys::GDExtensionInterfaceGetGodotVersion);
+        // SAFETY: `self.0` is a valid `get_proc_address` pointer.
+        let get_godot_version = unsafe { get_proc_address(sys::c_str(b"get_godot_version\0")) }; //.expect("get_godot_version unexpectedly null");
 
-            let mut version = std::mem::MaybeUninit::<sys::GDExtensionGodotVersion>::zeroed();
-            get_godot_version(version.as_mut_ptr());
-            version.assume_init()
-        }
+        // SAFETY: `sys::GDExtensionInterfaceGetGodotVersion` is an `Option` of an `unsafe extern "C"` function pointer.
+        let get_godot_version = crate::cast_fn_ptr!(unsafe {
+            get_godot_version as sys::GDExtensionInterfaceGetGodotVersion
+        });
+
+        let mut version = std::mem::MaybeUninit::<sys::GDExtensionGodotVersion>::zeroed();
+
+        // SAFETY: `get_proc_address` with "get_godot_version" does return a valid `sys::GDExtensionInterfaceGetGodotVersion` pointer, and since we have a valid
+        // `get_proc_address` pointer then it must be callable.
+        unsafe { get_godot_version(version.as_mut_ptr()) };
+
+        // SAFETY: `get_godot_version` initializes `version`.
+        unsafe { version.assume_init() }
     }
 
-    fn load_interface(&self) -> sys::GDExtensionInterface {
+    unsafe fn load_interface(&self) -> sys::GDExtensionInterface {
         unsafe { sys::GDExtensionInterface::load(*self) }
     }
 }

godot-ffi/src/compat/mod.rs

@@ -21,7 +21,12 @@ pub use compat_4_0::*;
 ///
 /// Provides a compatibility layer to be able to use 4.0.x extensions under Godot versions >= 4.1.
 /// Also performs deterministic checks and expressive errors for cases where compatibility cannot be provided.
-pub(crate) trait BindingCompat {
+///
+/// # Safety
+///
+/// [`ensure_static_runtime_compatibility`](BindingCompat::ensure_static_runtime_compatibility) succeeding should be sufficient to ensure that
+/// both [`runtime_version`](BindingCompat::runtime_version) and [`load_interface`](BindingCompat::load_interface) can be called safely.
+pub(crate) unsafe trait BindingCompat {
     // Implementation note: these methods could be unsafe, but that would remove any `unsafe` statements _inside_
     // the function bodies, making reasoning about them harder. Also, the call site is already an unsafe function,
     // so it would not add safety there, either.
@@ -42,8 +47,16 @@ pub(crate) trait BindingCompat {
     fn ensure_static_runtime_compatibility(&self);
 
     /// Return version dynamically passed via `gdextension_interface.h` file.
-    fn runtime_version(&self) -> sys::GDExtensionGodotVersion;
+    ///
+    /// # Safety
+    ///
+    /// `self` must be a valid interface or get proc address pointer.
+    unsafe fn runtime_version(&self) -> sys::GDExtensionGodotVersion;
 
     /// Return the interface, either as-is from the header (legacy) or code-generated (modern API).
-    fn load_interface(&self) -> sys::GDExtensionInterface;
+    ///
+    /// # Safety
+    ///
+    /// `self` must be a valid interface or get proc address pointer.
+    unsafe fn load_interface(&self) -> sys::GDExtensionInterface;
 }