Merge #157

157: `Gd::eq()` + fix `Gd::from_variant(nil)` r=Bromeon a=Bromeon

* Fix crash when converting `Variant::nil()` to `Gd<T>`
* Implement `Eq` for `Gd<T>`

Fixes #155.
bors r+

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

Author: bors[bot]

godot-core/src/obj/gd.rs

@@ -28,6 +28,8 @@ use crate::{callbacks, engine, out};
 /// This smart pointer can only hold _objects_ in the Godot sense: instances of Godot classes (`Node`, `RefCounted`, etc.)
 /// or user-declared structs (`#[derive(GodotClass)]`). It does **not** hold built-in types (`Vector3`, `Color`, `i32`).
 ///
+/// `Gd<T>` never holds null objects. If you need nullability, use `Option<Gd<T>>`.
+///
 /// This smart pointer behaves differently depending on `T`'s associated types, see [`GodotClass`] for their documentation.
 /// In particular, the memory management strategy is fully dependent on `T`:
 ///
@@ -591,16 +593,18 @@ impl<T: GodotClass> Share for Gd<T> {
 
 impl<T: GodotClass> FromVariant for Gd<T> {
     fn try_from_variant(variant: &Variant) -> Result<Self, VariantConversionError> {
-        let result = unsafe {
-            Self::from_sys_init(|self_ptr| {
+        let result_or_none = unsafe {
+            Self::from_sys_init_opt(|self_ptr| {
                 let converter = sys::builtin_fn!(object_from_variant);
                 converter(self_ptr, variant.var_sys());
             })
         };
 
         // The conversion method `variant_to_object` does NOT increment the reference-count of the object; we need to do that manually.
         // (This behaves differently in the opposite direction `object_to_variant`.)
-        Ok(result.with_inc_refcount())
+        result_or_none
+            .map(|obj| obj.with_inc_refcount())
+            .ok_or(VariantConversionError)
     }
 }
 
@@ -626,6 +630,19 @@ impl<T: GodotClass> ToVariant for Gd<T> {
     }
 }
 
+impl<T: GodotClass> PartialEq for Gd<T> {
+    /// ⚠️ Returns whether two `Gd` pointers point to the same object.
+    ///
+    /// # Panics
+    /// When `self` or `other` is dead.
+    fn eq(&self, other: &Self) -> bool {
+        // Panics when one is dead
+        self.instance_id() == other.instance_id()
+    }
+}
+
+impl<T: GodotClass> Eq for Gd<T> {}
+
 impl<T> Display for Gd<T>
 where
     T: GodotClass<Declarer = dom::EngineDomain>,

itest/rust/src/object_test.rs

@@ -6,8 +6,10 @@
 
 use crate::{expect_panic, itest};
 use godot::bind::{godot_api, GodotClass, GodotExt};
-use godot::builtin::{FromVariant, GodotString, StringName, ToVariant, Variant, Vector3};
-use godot::engine::{file_access, Camera3D, FileAccess, Node, Node3D, Object, RefCounted};
+use godot::builtin::{
+    FromVariant, GodotString, StringName, ToVariant, Variant, VariantConversionError, Vector3,
+};
+use godot::engine::{file_access, Area2D, Camera3D, FileAccess, Node, Node3D, Object, RefCounted};
 use godot::obj::{Base, Gd, InstanceId};
 use godot::obj::{Inherits, Share};
 use godot::sys::GodotFfi;
@@ -161,6 +163,35 @@ fn object_from_instance_id_unrelated_type() {
     node.free();
 }
 
+#[itest]
+fn object_user_eq() {
+    let value: i16 = 17943;
+    let a = ObjPayload { value };
+    let b = ObjPayload { value };
+
+    let a1 = Gd::new(a);
+    let a2 = a1.share();
+    let b1 = Gd::new(b);
+
+    assert_eq!(a1, a2);
+    assert_ne!(a1, b1);
+    assert_ne!(a2, b1);
+}
+
+#[itest]
+fn object_engine_eq() {
+    let a1 = Node3D::new_alloc();
+    let a2 = a1.share();
+    let b1 = Node3D::new_alloc();
+
+    assert_eq!(a1, a2);
+    assert_ne!(a1, b1);
+    assert_ne!(a2, b1);
+
+    a1.free();
+    b1.free();
+}
+
 #[itest]
 fn object_user_convert_variant() {
     let value: i16 = 17943;
@@ -225,6 +256,21 @@ fn check_convert_variant_refcount(obj: Gd<RefCounted>) {
     assert_eq!(obj.get_reference_count(), 1);
 }
 
+#[itest]
+fn object_engine_convert_variant_nil() {
+    let nil = Variant::nil();
+
+    assert_eq!(
+        Gd::<Area2D>::try_from_variant(&nil),
+        Err(VariantConversionError),
+        "try_from_variant(&nil)"
+    );
+
+    expect_panic("from_variant(&nil)", || {
+        Gd::<Area2D>::from_variant(&nil);
+    });
+}
+
 #[itest]
 fn object_engine_returned_refcount() {
     let Some(file) = FileAccess::open("res://itest.gdextension".into(), file_access::ModeFlags::READ) else {