From e9efb15d2a33b699a8b47952c6dacb5111c29301 Mon Sep 17 00:00:00 2001 From: Lili Zoey Zyli Date: Fri, 10 May 2024 19:01:41 +0200 Subject: [PATCH] Document some various stuff better Cleanup a couple of things --- godot-core/src/builtin/meta/mod.rs | 18 +++++------ godot-core/src/builtin/string/gstring.rs | 3 ++ godot-core/src/builtin/string/string_name.rs | 3 ++ godot-core/src/property.rs | 8 +++-- godot-core/src/registry/callbacks.rs | 32 ++++++++++++++++--- godot-core/src/registry/mod.rs | 2 ++ .../src/object_tests/virtual_methods_test.rs | 2 ++ 7 files changed, 51 insertions(+), 17 deletions(-) diff --git a/godot-core/src/builtin/meta/mod.rs b/godot-core/src/builtin/meta/mod.rs index 448a9c652..6506fbbd2 100644 --- a/godot-core/src/builtin/meta/mod.rs +++ b/godot-core/src/builtin/meta/mod.rs @@ -278,7 +278,7 @@ pub struct PropertyInfo { /// Which class this property is. /// - /// This should be set to [`ClassName::none()`] unless the variant type is object. You can use + /// This should be set to [`ClassName::none()`] unless the variant type is `Object`. You can use /// [`GodotClass::class_name()`](crate::obj::GodotClass::class_name()) to get the right name to use here. pub class_name: ClassName, @@ -317,7 +317,7 @@ impl PropertyInfo { /// Change the `hint` and `hint_string` to be the given `hint_info`. /// - /// See [`export_info_functions`](crate::property::export_info_function) for functions that return appropriate `PropertyHintInfo`s for + /// See [`export_info_functions`](crate::property::export_info_functions) for functions that return appropriate `PropertyHintInfo`s for /// various Godot annotations. /// /// # Examples @@ -326,11 +326,11 @@ impl PropertyInfo { /// // TODO: Make this nicer to use. /// ```no_run - /// # use crate::property::export_info_function; - /// # use crate::builtin::meta::PropertyInfo; + /// use godot::register::property::export_info_functions; + /// use godot::builtin::meta::PropertyInfo; /// /// let property = PropertyInfo::new_export::("my_range_property") - /// .with_hint_info(export_info_function::export_range( + /// .with_hint_info(export_info_functions::export_range( /// 0.0, /// 10.0, /// Some(0.1), @@ -340,7 +340,7 @@ impl PropertyInfo { /// false, /// false, /// false, - /// )) + /// )); /// ``` pub fn with_hint_info(self, hint_info: PropertyHintInfo) -> Self { let PropertyHintInfo { hint, hint_string } = hint_info; @@ -415,7 +415,7 @@ impl PropertyInfo { /// [`free_owned_property_sys`](Self::free_owned_property_sys). /// /// This will leak memory unless used together with `free_owned_property_sys`. - pub fn into_owned_property_sys(self) -> sys::GDExtensionPropertyInfo { + pub(crate) fn into_owned_property_sys(self) -> sys::GDExtensionPropertyInfo { use crate::obj::EngineBitfield as _; use crate::obj::EngineEnum as _; @@ -433,9 +433,9 @@ impl PropertyInfo { /// /// # Safety /// - /// * Must only be used on a struct returned from a call to [`into_owned_property_sys`], without modification. + /// * Must only be used on a struct returned from a call to `into_owned_property_sys`, without modification. /// * Must not be called more than once on a `sys::GDExtensionPropertyInfo` struct. - pub unsafe fn free_owned_property_sys(info: sys::GDExtensionPropertyInfo) { + pub(crate) unsafe fn free_owned_property_sys(info: sys::GDExtensionPropertyInfo) { // SAFETY: This function was called on a pointer returned from `into_owned_property_sys`, thus both `info.name` and // `info.hint_string` were created from calls to `into_owned_string_sys` on their respective types. // Additionally this function isn't called more than once on a struct containing the same `name` or `hint_string` pointers. diff --git a/godot-core/src/builtin/string/gstring.rs b/godot-core/src/builtin/string/gstring.rs index fd390c41d..42fd5346f 100644 --- a/godot-core/src/builtin/string/gstring.rs +++ b/godot-core/src/builtin/string/gstring.rs @@ -153,6 +153,9 @@ impl GString { sys::static_assert_eq_size_align!(StringName, sys::types::OpaqueString); let ptr = ptr.cast::(); + + // SAFETY: `ptr` was returned from a call to `into_owned_string_sys`, which means it was created by a call to + // `Box::into_raw`, thus we can use `Box::from_raw` here. Additionally this is only called once on this pointer. let boxed = unsafe { Box::from_raw(ptr) }; *boxed } diff --git a/godot-core/src/builtin/string/string_name.rs b/godot-core/src/builtin/string/string_name.rs index 676149f71..c4338e18e 100644 --- a/godot-core/src/builtin/string/string_name.rs +++ b/godot-core/src/builtin/string/string_name.rs @@ -134,6 +134,9 @@ impl StringName { sys::static_assert_eq_size_align!(StringName, sys::types::OpaqueStringName); let ptr = ptr.cast::(); + + // SAFETY: `ptr` was returned from a call to `into_owned_string_sys`, which means it was created by a call to + // `Box::into_raw`, thus we can use `Box::from_raw` here. Additionally this is only called once on this pointer. let boxed = unsafe { Box::from_raw(ptr) }; *boxed } diff --git a/godot-core/src/property.rs b/godot-core/src/property.rs index 4ed2f754a..3a726fdf4 100644 --- a/godot-core/src/property.rs +++ b/godot-core/src/property.rs @@ -131,7 +131,7 @@ impl PropertyHintInfo { /// Functions used to translate user-provided arguments into export hints. /// -/// Each function is named the same as the equivalent Godot annotation. E.g `@export_range` is `fn export_range`. +/// Each function is named the same as the equivalent Godot annotation. For instance `@export_range` in Godot is `fn export_range` here. pub mod export_info_functions { use crate::builtin::GString; use crate::engine::global::PropertyHint; @@ -239,7 +239,8 @@ pub mod export_info_functions { /// # Examples /// /// ```no_run - /// export_enum(&[("a", None), ("b", "Some(10)")]); + /// # use godot::register::property::export_info_functions::export_enum; + /// export_enum(&[("a", None), ("b", Some(10))]); /// ``` pub fn export_enum(variants: &[T]) -> PropertyHintInfo where @@ -271,7 +272,8 @@ pub mod export_info_functions { /// # Examples /// /// ```no_run - /// export_flags(&[("a", None), ("b", "Some(10)")]); + /// # use godot::register::property::export_info_functions::export_flags; + /// export_flags(&[("a", None), ("b", Some(10))]); /// ``` pub fn export_flags(bits: &[T]) -> PropertyHintInfo where diff --git a/godot-core/src/registry/callbacks.rs b/godot-core/src/registry/callbacks.rs index 61f44df61..7f6206817 100644 --- a/godot-core/src/registry/callbacks.rs +++ b/godot-core/src/registry/callbacks.rs @@ -201,6 +201,9 @@ pub unsafe extern "C" fn unreference(instance: sys::GDExtensionCl storage.on_dec_ref(); } +/// # Safety +/// +/// Must only be called by Godot as a callback for `get_property_list` for a rust-defined class of type `T`. #[deny(unsafe_op_in_unsafe_fn)] pub unsafe extern "C" fn get_property_list( instance: sys::GDExtensionClassInstancePtr, @@ -216,6 +219,7 @@ pub unsafe extern "C" fn get_property_list( .map(|prop| prop.into_owned_property_sys()) .collect(); + // SAFETY: Godot ensures that `count` is initialized and valid to write into. unsafe { *count = property_list_sys .len() @@ -226,6 +230,10 @@ pub unsafe extern "C" fn get_property_list( Box::leak(property_list_sys).as_mut_ptr() } +/// # Safety +/// +/// - Must only be called by Godot as a callback for `free_property_list` for a rust-defined class of type `T`. +/// - Must only be passed to Godot as a callback when [`get_property_list`] is the corresponding `get_property_list` callback. #[deny(unsafe_op_in_unsafe_fn)] pub unsafe extern "C" fn free_property_list( _instance: sys::GDExtensionClassInstancePtr, @@ -233,13 +241,21 @@ pub unsafe extern "C" fn free_property_list( count: u32, ) { let list = list as *mut sys::GDExtensionPropertyInfo; - let property_list_sys = unsafe { - Box::from_raw(std::slice::from_raw_parts_mut( - list, - count.try_into().expect("`u32` should fit in `usize`"), - )) + + // SAFETY: `list` comes from `get_property_list` above, and `count` also comes from the same function. + // This means that `list` is a pointer to a `&[sys::GDExtensionPropertyInfo]` slice of length `count`. + // This means all the preconditions of this function are satisfied except uniqueness of this point. + // Uniqueness is guaranteed as Godot called this function at a point where the list is no longer accessed + // through any other pointer, and we dont access the slice through any other pointer after this call either. + let property_list_slice = unsafe { + std::slice::from_raw_parts_mut(list, count.try_into().expect("`u32` should fit in `usize`")) }; + // SAFETY: This slice was created by calling `Box::leak` on a `Box<[sys::GDExtensionPropertyInfo]>`, we can thus + // call `Box::from_raw` on this slice to get back the original boxed slice. + // Note that this relies on coercion of `&mut` -> `*mut`. + let property_list_sys = unsafe { Box::from_raw(property_list_slice) }; + for property_info in property_list_sys.iter() { // SAFETY: The structs contained in this list were all returned from `into_owned_property_sys`. // We only call this method once for each struct and for each list. @@ -267,6 +283,9 @@ unsafe fn raw_property_get_revert( T::__godot_property_get_revert(&*instance, property.clone()) } +/// # Safety +/// +/// - Must only be called by Godot as a callback for `property_can_revert` for a rust-defined class of type `T`. #[deny(unsafe_op_in_unsafe_fn)] pub unsafe extern "C" fn property_can_revert( instance: sys::GDExtensionClassInstancePtr, @@ -278,6 +297,9 @@ pub unsafe extern "C" fn property_can_revert( revert.is_some() as sys::GDExtensionBool } +/// # Safety +/// +/// - Must only be called by Godot as a callback for `property_get_revert` for a rust-defined class of type `T`. #[deny(unsafe_op_in_unsafe_fn)] pub unsafe extern "C" fn property_get_revert( instance: sys::GDExtensionClassInstancePtr, diff --git a/godot-core/src/registry/mod.rs b/godot-core/src/registry/mod.rs index 5cec19d87..3b6074916 100644 --- a/godot-core/src/registry/mod.rs +++ b/godot-core/src/registry/mod.rs @@ -190,6 +190,8 @@ pub enum PluginItem { ) -> *const sys::GDExtensionPropertyInfo, >, + // We do not support using this in Godot < 4.3, however it's easier to leave this in and fail elsewhere when attempting to use + // this in Godot < 4.3. #[cfg(before_api = "4.3")] user_free_property_list_fn: Option< unsafe extern "C" fn( diff --git a/itest/rust/src/object_tests/virtual_methods_test.rs b/itest/rust/src/object_tests/virtual_methods_test.rs index b4226b78c..0fcb192b0 100644 --- a/itest/rust/src/object_tests/virtual_methods_test.rs +++ b/itest/rust/src/object_tests/virtual_methods_test.rs @@ -296,6 +296,8 @@ impl IRefCounted for RevertTest { match String::from(property).as_str() { "property_not_revert" => None, "property_do_revert" => Some(GString::from("hello!").to_variant()), + // No UB or anything else like a crash or panic should happen when `property_can_revert` and `property_get_revert` return + // inconsistent values, but in case something like that happens we should be able to detect it through this function. "property_changes" => { if INC.fetch_add(1, std::sync::atomic::Ordering::AcqRel) % 2 == 0 { None