From fe50efcf051cab220882b3066487a539f0d78738 Mon Sep 17 00:00:00 2001
From: Jan Haller <bromeon@gmail.com>
Date: Sun, 2 Feb 2025 18:12:07 +0100
Subject: [PATCH] Support Variant::object_id() for Godot <4.4; add
 object_id_unchecked()

Changes semantics to panic for dead objects.
---
 godot-core/src/builtin/variant/mod.rs         | 45 +++++++++++++++++--
 godot-core/src/meta/error/convert_error.rs    |  4 ++
 .../builtin_tests/containers/variant_test.rs  | 26 ++++++++++-
 3 files changed, 69 insertions(+), 6 deletions(-)

diff --git a/godot-core/src/builtin/variant/mod.rs b/godot-core/src/builtin/variant/mod.rs
index abf647342..b88f19b66 100644
--- a/godot-core/src/builtin/variant/mod.rs
+++ b/godot-core/src/builtin/variant/mod.rs
@@ -8,7 +8,7 @@
 use crate::builtin::{
     GString, StringName, VariantArray, VariantDispatch, VariantOperator, VariantType,
 };
-use crate::meta::error::ConvertError;
+use crate::meta::error::{ConvertError, ErrorKind, FromVariantError};
 use crate::meta::{arg_into_ref, ArrayElement, AsArg, FromGodot, ToGodot};
 use godot_ffi as sys;
 use std::{fmt, ptr};
@@ -110,10 +110,47 @@ impl Variant {
     ///
     /// If the variant is not an object, returns `None`.
     ///
-    /// If the object is dead, the instance ID is still returned. Use [`Variant::try_to::<Gd<T>>()`][Self::try_to]
-    /// to retrieve only live objects.
-    #[cfg(since_api = "4.4")]
+    /// # Panics
+    /// If the variant holds an object and that object is dead.
+    ///
+    /// If you want to detect this case, use [`try_to::<Gd<...>>()`](Self::try_to). If you want to retrieve the previous instance ID of a
+    /// freed object for whatever reason, use [`object_id_unchecked()`][Self::object_id_unchecked]. This method is only available from
+    /// Godot 4.4 onwards.
     pub fn object_id(&self) -> Option<crate::obj::InstanceId> {
+        #[cfg(since_api = "4.4")]
+        {
+            assert!(
+                self.get_type() != VariantType::OBJECT || self.is_object_alive(),
+                "Variant::object_id(): object has been freed"
+            );
+            self.object_id_unchecked()
+        }
+
+        #[cfg(before_api = "4.4")]
+        {
+            match self.try_to::<crate::obj::Gd<crate::classes::Object>>() {
+                Ok(obj) => Some(obj.instance_id_unchecked()),
+                Err(c)
+                    if matches!(
+                        c.kind(),
+                        ErrorKind::FromVariant(FromVariantError::DeadObject)
+                    ) =>
+                {
+                    panic!("Variant::object_id(): object has been freed")
+                }
+                _ => None, // other conversion errors
+            }
+        }
+    }
+
+    /// For variants holding an object, returns the object's instance ID.
+    ///
+    /// If the variant is not an object, returns `None`.
+    ///
+    /// If the object is dead, the instance ID is still returned, similar to [`Gd::instance_id_unchecked()`][crate::obj::Gd::instance_id_unchecked].
+    /// Unless you have a very good reason to use this, we recommend using [`object_id()`][Self::object_id] instead.
+    #[cfg(since_api = "4.4")]
+    pub fn object_id_unchecked(&self) -> Option<crate::obj::InstanceId> {
         // SAFETY: safe to call for non-object variants (returns 0).
         let raw_id: u64 = unsafe { interface_fn!(variant_get_object_instance_id)(self.var_sys()) };
 
diff --git a/godot-core/src/meta/error/convert_error.rs b/godot-core/src/meta/error/convert_error.rs
index 80bdcad39..8621a537f 100644
--- a/godot-core/src/meta/error/convert_error.rs
+++ b/godot-core/src/meta/error/convert_error.rs
@@ -91,6 +91,10 @@ impl ConvertError {
     pub fn into_erased(self) -> impl Error + Send + Sync {
         ErasedConvertError::from(self)
     }
+
+    pub(crate) fn kind(&self) -> &ErrorKind {
+        &self.kind
+    }
 }
 
 impl fmt::Display for ConvertError {
diff --git a/itest/rust/src/builtin_tests/containers/variant_test.rs b/itest/rust/src/builtin_tests/containers/variant_test.rs
index 136f37b6d..892080488 100644
--- a/itest/rust/src/builtin_tests/containers/variant_test.rs
+++ b/itest/rust/src/builtin_tests/containers/variant_test.rs
@@ -239,7 +239,6 @@ fn variant_get_type() {
     assert_eq!(variant.get_type(), VariantType::BASIS)
 }
 
-#[cfg(since_api = "4.4")]
 #[itest]
 fn variant_object_id() {
     let variant = Variant::nil();
@@ -257,7 +256,30 @@ fn variant_object_id() {
     node.free();
 
     // When freed, variant still returns the object ID.
-    assert_eq!(variant.object_id(), Some(id));
+    expect_panic("Variant::object_id() with freed object", || {
+        let _ = variant.object_id();
+    });
+}
+
+#[itest]
+#[cfg(since_api = "4.4")]
+fn variant_object_id_unchecked() {
+    let variant = Variant::nil();
+    assert_eq!(variant.object_id_unchecked(), None);
+
+    let variant = Variant::from(77);
+    assert_eq!(variant.object_id_unchecked(), None);
+
+    let node = Node::new_alloc();
+    let id = node.instance_id();
+
+    let variant = node.to_variant();
+    assert_eq!(variant.object_id_unchecked(), Some(id));
+
+    node.free();
+
+    // When freed, unchecked function will still return old ID.
+    assert_eq!(variant.object_id_unchecked(), Some(id));
 }
 
 #[itest]