Merge #274

274: Optimize array random access r=Bromeon a=Bromeon

As of godotengine/godot#66185, the FFI methods 
* `array_operator_index`
* `array_operator_index_const`

no longer print an error on out-of-bounds access. As before, they return a null pointer in that case.

This means we can reduce the number of FFI calls from 2 to 1, and directly check the returned pointer instead of doing a bounds check in advance. We can also make the `ptr[_mut]_unchecked` safe, and just let them return a null pointer.

Co-authored-by: Jan Haller <[email protected]>

Author: bors[bot]

godot-core/src/builtin/array.rs

@@ -78,11 +78,17 @@ impl<T: VariantMetadata> Array<T> {
     }
 
     /// Returns the number of elements in the array. Equivalent of `size()` in Godot.
+    ///
+    /// Retrieving the size incurs an FFI call. If you know the size hasn't changed, you may consider storing
+    /// it in a variable. For loops, prefer iterators.
     pub fn len(&self) -> usize {
         to_usize(self.as_inner().size())
     }
 
     /// Returns `true` if the array is empty.
+    ///
+    /// Checking for emptiness incurs an FFI call. If you know the size hasn't changed, you may consider storing
+    /// it in a variable. For loops, prefer iterators.
     pub fn is_empty(&self) -> bool {
         self.as_inner().is_empty()
     }
@@ -150,10 +156,24 @@ impl<T: VariantMetadata> Array<T> {
     ///
     /// If `index` is out of bounds.
     fn ptr(&self, index: usize) -> *const Variant {
-        self.check_bounds(index);
+        let ptr = self.ptr_or_null(index);
+        assert!(
+            !ptr.is_null(),
+            "Array index {index} out of bounds (len {len})",
+            len = self.len(),
+        );
+        ptr
+    }
+
+    /// Returns a pointer to the element at the given index, or null if out of bounds.
+    fn ptr_or_null(&self, index: usize) -> *const Variant {
+        // SAFETY: array_operator_index_const returns null for invalid indexes.
+        let variant_ptr = unsafe {
+            let index = to_i64(index);
+            interface_fn!(array_operator_index_const)(self.sys(), index)
+        };
 
-        // SAFETY: We just checked that the index is not out of bounds.
-        unsafe { self.ptr_unchecked(index) }
+        Variant::ptr_from_sys(variant_ptr)
     }
 
     /// Returns a mutable pointer to the element at the given index.
@@ -162,29 +182,23 @@ impl<T: VariantMetadata> Array<T> {
     ///
     /// If `index` is out of bounds.
     fn ptr_mut(&self, index: usize) -> *mut Variant {
-        self.check_bounds(index);
-
-        // SAFETY: We just checked that the index is not out of bounds.
-        unsafe { self.ptr_mut_unchecked(index) }
+        let ptr = self.ptr_mut_or_null(index);
+        assert!(
+            !ptr.is_null(),
+            "Array index {index} out of bounds (len {len})",
+            len = self.len(),
+        );
+        ptr
     }
 
-    /// Returns a pointer to the element at the given index.
-    ///
-    /// # Safety
-    ///
-    /// Calling this with an out-of-bounds index is undefined behavior.
-    unsafe fn ptr_unchecked(&self, index: usize) -> *const Variant {
-        let variant_ptr = interface_fn!(array_operator_index_const)(self.sys(), to_i64(index));
-        Variant::ptr_from_sys(variant_ptr)
-    }
+    /// Returns a pointer to the element at the given index, or null if out of bounds.
+    fn ptr_mut_or_null(&self, index: usize) -> *mut Variant {
+        // SAFETY: array_operator_index returns null for invalid indexes.
+        let variant_ptr = unsafe {
+            let index = to_i64(index);
+            interface_fn!(array_operator_index)(self.sys(), index)
+        };
 
-    /// Returns a mutable pointer to the element at the given index.
-    ///
-    /// # Safety
-    ///
-    /// Calling this with an out-of-bounds index is undefined behavior.
-    unsafe fn ptr_mut_unchecked(&self, index: usize) -> *mut Variant {
-        let variant_ptr = interface_fn!(array_operator_index)(self.sys(), to_i64(index));
         Variant::ptr_from_sys_mut(variant_ptr)
     }
 
@@ -365,6 +379,7 @@ impl<T: VariantMetadata + FromVariant> Array<T> {
     ///
     /// If `index` is out of bounds.
     pub fn get(&self, index: usize) -> T {
+        // Panics on out-of-bounds
         let ptr = self.ptr(index);
 
         // SAFETY: `ptr()` just verified that the index is not out of bounds.
@@ -582,17 +597,17 @@ unsafe impl<T: VariantMetadata> GodotFfi for Array<T> {
         fn move_return_ptr;
     }
 
-    unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::PtrcallType) -> Self {
-        let array = Self::from_sys(ptr);
-        std::mem::forget(array.share());
-        array
-    }
-
     unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
         let mut result = Self::default();
         init_fn(result.sys_mut());
         result
     }
+
+    unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::PtrcallType) -> Self {
+        let array = Self::from_sys(ptr);
+        std::mem::forget(array.share());
+        array
+    }
 }
 
 impl<T: VariantMetadata> fmt::Debug for Array<T> {
@@ -702,9 +717,11 @@ impl<T: VariantMetadata + ToVariant> From<&[T]> for Array<T> {
             return array;
         }
         array.resize(len);
-        let ptr = array.ptr_mut(0);
+
+        let ptr = array.ptr_mut_or_null(0);
         for (i, element) in slice.iter().enumerate() {
             // SAFETY: The array contains exactly `len` elements, stored contiguously in memory.
+            // Also, the pointer is non-null, as we checked for emptiness above.
             unsafe {
                 *ptr.offset(to_isize(i)) = element.to_variant();
             }
@@ -767,10 +784,11 @@ impl<'a, T: VariantMetadata + FromVariant> Iterator for Iter<'a, T> {
         if self.next_idx < self.array.len() {
             let idx = self.next_idx;
             self.next_idx += 1;
-            // Using `ptr_unchecked` rather than going through `get()` so we can avoid a second
-            // bounds check.
-            // SAFETY: We just checked that the index is not out of bounds.
-            let variant = unsafe { &*self.array.ptr_unchecked(idx) };
+
+            let element_ptr = self.array.ptr_or_null(idx);
+
+            // SAFETY: We just checked that the index is not out of bounds, so the pointer won't be null.
+            let variant = unsafe { &*element_ptr };
             let element = T::from_variant(variant);
             Some(element)
         } else {

godot-core/src/builtin/basis.rs

@@ -579,7 +579,7 @@ unsafe impl GodotFfi for Basis {
 
 /// The ordering used to interpret a set of euler angles as extrinsic
 /// rotations.
-#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+#[derive(Copy, Clone, Eq, PartialEq, Debug)]
 #[repr(C)]
 pub enum EulerOrder {
     XYZ = 0,

godot-core/src/builtin/color.rs

@@ -15,15 +15,18 @@ use sys::{ffi_methods, GodotFfi};
 /// Channel values are _typically_ in the range of 0 to 1, but this is not a requirement, and
 /// values outside this range are explicitly allowed for e.g. High Dynamic Range (HDR).
 #[repr(C)]
-#[derive(Copy, Clone, Debug, PartialEq, PartialOrd)]
+#[derive(Copy, Clone, PartialEq, PartialOrd, Debug)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 pub struct Color {
     /// The color's red component.
     pub r: f32,
+
     /// The color's green component.
     pub g: f32,
+
     /// The color's blue component.
     pub b: f32,
+
     /// The color's alpha component. A value of 0 means that the color is fully transparent. A
     /// value of 1 means that the color is fully opaque.
     pub a: f32,
@@ -318,12 +321,14 @@ unsafe impl GodotFfi for Color {
     ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. }
 }
 
-#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+#[derive(Copy, Clone, Eq, PartialEq, Debug)]
 pub enum ColorChannelOrder {
     /// RGBA channel order. Godot's default.
     Rgba,
+
     /// ABGR channel order. Reverse of the default RGBA order.
     Abgr,
+
     /// ARGB channel order. More compatible with DirectX.
     Argb,
 }

godot-core/src/builtin/dictionary.rs

@@ -192,6 +192,8 @@ impl Dictionary {
     /// _Godot equivalent: `dict[key] = value`_
     pub fn set<K: ToVariant, V: ToVariant>(&mut self, key: K, value: V) {
         let key = key.to_variant();
+
+        // SAFETY: always returns a valid pointer to a value in the dictionary; either pre-existing or newly inserted.
         unsafe {
             *self.get_ptr_mut(key) = value.to_variant();
         }
@@ -226,12 +228,16 @@ impl Dictionary {
 
     /// Get the pointer corresponding to the given key in the dictionary.
     ///
-    /// If there exists no value at the given key, a `NIL` variant will be created.
+    /// If there exists no value at the given key, a `NIL` variant will be inserted for that key.
     fn get_ptr_mut<K: ToVariant>(&mut self, key: K) -> *mut Variant {
         let key = key.to_variant();
+
+        // SAFETY: accessing an unknown key _mutably_ creates that entry in the dictionary, with value `NIL`.
         let ptr = unsafe {
             interface_fn!(dictionary_operator_index)(self.sys_mut(), key.var_sys_const())
         };
+
+        // Never a null pointer, since entry either existed already or was inserted above.
         Variant::ptr_from_sys_mut(ptr)
     }
 }
@@ -256,17 +262,17 @@ unsafe impl GodotFfi for Dictionary {
         fn move_return_ptr;
     }
 
-    unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::PtrcallType) -> Self {
-        let dictionary = Self::from_sys(ptr);
-        std::mem::forget(dictionary.share());
-        dictionary
-    }
-
     unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
         let mut result = Self::default();
         init_fn(result.sys_mut());
         result
     }
+
+    unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::PtrcallType) -> Self {
+        let dictionary = Self::from_sys(ptr);
+        std::mem::forget(dictionary.share());
+        dictionary
+    }
 }
 
 impl_builtin_traits! {

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

@@ -13,7 +13,7 @@ use crate::obj::GodotClass;
 
 /// Utility to construct class names known at compile time.
 /// Cannot be a function since the backing string must be retained.
-#[derive(Eq, PartialEq, Hash, Clone, Debug)]
+#[derive(Clone, Eq, PartialEq, Hash, Debug)]
 pub struct ClassName {
     backing: StringName,
 }

godot-core/src/builtin/projection.rs

@@ -494,7 +494,7 @@ unsafe impl GodotFfi for Projection {
 }
 
 /// A projections clipping plane.
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+#[derive(Copy, Clone, Eq, PartialEq, Hash, Debug)]
 #[repr(C)]
 pub enum ProjectionPlane {
     Near = 0,
@@ -507,7 +507,7 @@ pub enum ProjectionPlane {
 
 /// The eye to create a projection for, when creating a projection adjusted
 /// for head-mounted displays.
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+#[derive(Copy, Clone, Eq, PartialEq, Hash, Debug)]
 #[repr(C)]
 pub enum ProjectionEye {
     Left = 1,

godot-core/src/builtin/rect2i.rs

@@ -20,6 +20,7 @@ use super::{Rect2, RectSide, Vector2i};
 pub struct Rect2i {
     /// The position of the rectangle.
     pub position: Vector2i,
+
     /// The size of the rectangle.
     pub size: Vector2i,
 }

godot-core/src/builtin/rid.rs

@@ -30,10 +30,11 @@ use sys::{ffi_methods, GodotFfi};
 // eligible for it, it is also guaranteed to have it. Meaning the layout of this type is identical to `u64`.
 // See: https://doc.rust-lang.org/nomicon/ffi.html#the-nullable-pointer-optimization
 // Cannot use `#[repr(C)]` as it does not use the nullable pointer optimization.
-#[derive(Copy, Clone, Eq, PartialEq, Hash, Ord, PartialOrd, Debug)]
+#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug)]
 pub enum Rid {
     /// A valid RID may refer to some resource, but is not guaranteed to do so.
     Valid(NonZeroU64),
+
     /// An invalid RID will never refer to a resource. Internally it is represented as a 0.
     Invalid,
 }

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

@@ -200,16 +200,13 @@ impl Variant {
         sys::to_const_ptr(self.var_sys())
     }
 
+    /// Converts to variant pointer; can be a null pointer.
     pub(crate) fn ptr_from_sys(variant_ptr: sys::GDExtensionVariantPtr) -> *const Variant {
-        assert!(!variant_ptr.is_null(), "ptr_from_sys: null variant pointer");
         variant_ptr as *const Variant
     }
 
+    /// Converts to variant mut pointer; can be a null pointer.
     pub(crate) fn ptr_from_sys_mut(variant_ptr: sys::GDExtensionVariantPtr) -> *mut Variant {
-        assert!(
-            !variant_ptr.is_null(),
-            "ptr_from_sys_mut: null variant pointer"
-        );
         variant_ptr as *mut Variant
     }
 }

godot-core/src/builtin/variant/variant_traits.rs

@@ -49,7 +49,7 @@ pub trait ToVariant {
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 
-#[derive(Debug, Eq, PartialEq)]
+#[derive(Eq, PartialEq, Debug)]
 pub struct VariantConversionError;
 /*pub enum VariantConversionError {
     /// Variant type does not match expected type