Document some various stuff better

Cleanup a couple of things

Author: lilizoey

godot-core/src/builtin/meta/mod.rs

@@ -20,9 +20,10 @@ pub use signature::*;
 
 pub(crate) use godot_convert::convert_error::*;
 
+use crate::builtin::*;
 use crate::engine::global::{self, PropertyHint, PropertyUsageFlags};
+use crate::property::Var;
 use crate::property::{Export, PropertyHintInfo};
-use crate::{builtin::*, property::Var};
 use godot_ffi as sys;
 use registration::method::MethodParamOrReturnInfo;
 use sys::{GodotFfi, GodotNullableFfi};
@@ -278,7 +279,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 +318,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 +327,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::<f64>("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 +341,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 +416,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 +434,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.

godot-core/src/builtin/string/gstring.rs

@@ -175,6 +175,9 @@ impl GString {
         sys::static_assert_eq_size_align!(StringName, sys::types::OpaqueString);
 
         let ptr = ptr.cast::<Self>();
+
+        // 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
     }

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::<Self>();
+
+        // 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
     }

godot-core/src/registry/callbacks.rs

@@ -201,6 +201,9 @@ pub unsafe extern "C" fn unreference<T: GodotClass>(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<T: cap::GodotGetPropertyList>(
     instance: sys::GDExtensionClassInstancePtr,
@@ -216,6 +219,7 @@ pub unsafe extern "C" fn get_property_list<T: cap::GodotGetPropertyList>(
         .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,20 +230,32 @@ pub unsafe extern "C" fn get_property_list<T: cap::GodotGetPropertyList>(
     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<T: cap::GodotGetPropertyList>(
     _instance: sys::GDExtensionClassInstancePtr,
     list: *const sys::GDExtensionPropertyInfo,
     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: cap::GodotPropertyGetRevert>(
     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<T: cap::GodotPropertyGetRevert>(
     instance: sys::GDExtensionClassInstancePtr,
@@ -278,6 +297,9 @@ pub unsafe extern "C" fn property_can_revert<T: cap::GodotPropertyGetRevert>(
     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<T: cap::GodotPropertyGetRevert>(
     instance: sys::GDExtensionClassInstancePtr,

godot-core/src/registry/mod.rs

@@ -191,6 +191,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(

godot-core/src/registry/property.rs

@@ -139,7 +139,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;
@@ -247,7 +247,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<T>(variants: &[T]) -> PropertyHintInfo
     where
@@ -279,7 +280,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<T>(bits: &[T]) -> PropertyHintInfo
     where

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

@@ -228,7 +228,8 @@ pub fn transform_trait_impl(original_impl: venial::Impl) -> ParseResult<TokenStr
                     #(#cfg_attrs)*
                     impl ::godot::obj::cap::GodotGetPropertyList for #class_name {
                         fn __godot_get_property_list(&mut self) -> Vec<::godot::builtin::meta::PropertyInfo> {
-                            // Only supported in godot api > 4.3. If support is added for earlier versions this is still needed.
+                            // `get_property_list` is only supported in Godot API >= 4.3. If we add support for `get_property_list` to earlier
+                            // versions of Godot then this code is still needed and should be uncommented.
                             //
                             // use ::godot::obj::UserClass as _;
                             //

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