Debug impl for Variant now holds objects, to not modify ref-count

Previously, Debug for Variant converted objects by creating a new Gd<T> pointer. This is
subject to the observer effect, and changes the reference count of objects being inspected.

Use an alternative direct way to fetch data, using Variant::call().

Author: Bromeon

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

@@ -8,6 +8,7 @@
 use crate::builtin::{
     GString, StringName, VariantArray, VariantDispatch, VariantOperator, VariantType,
 };
+use crate::classes;
 use crate::meta::error::{ConvertError, FromVariantError};
 use crate::meta::{
     arg_into_ref, ffi_variant_type, ArrayElement, AsArg, ExtVariantType, FromGodot, GodotType,
@@ -583,6 +584,10 @@ impl fmt::Debug for Variant {
                 array.fmt(f)
             }
 
+            // Converting to objects before printing causes their refcount to increment, leading to an Observer effect
+            // where `Debug` actually changes the object statistics. As such, fetch information without instantiating Gd<T>.
+            VariantType::OBJECT => classes::debug_string_variant(self, f, "VariantGd"),
+
             // VariantDispatch also includes dead objects via `FreedObject` enumerator, which maps to "<Freed Object>".
             _ => VariantDispatch::from_variant(self).fmt(f),
         }

godot-core/src/classes/class_runtime.rs

@@ -7,7 +7,7 @@
 
 //! Runtime checks and inspection of Godot classes.
 
-use crate::builtin::{GString, StringName};
+use crate::builtin::{GString, StringName, Variant};
 #[cfg(debug_assertions)]
 use crate::classes::{ClassDb, Object};
 use crate::meta::CallContext;
@@ -22,27 +22,67 @@ pub(crate) fn debug_string<T: GodotClass>(
     ty: &str,
 ) -> std::fmt::Result {
     if let Some(id) = obj.instance_id_or_none() {
-        // Sample the reference count FIRST, before any other operations that might affect it.
-        // This avoids the +1 temporary reference from subsequent FFI calls during debug formatting.
-        let refcount = obj.maybe_refcount();
-
         let class: StringName = obj.dynamic_class_string();
+        debug_string_parts(f, ty, id, class, obj.maybe_refcount(), None)
+    } else {
+        write!(f, "{ty} {{ freed obj }}")
+    }
+}
 
-        let mut builder = f.debug_struct(ty);
-        builder
-            .field("id", &id.to_i64())
-            .field("class", &format_args!("{class}"));
-
-        if let Some(refcount) = refcount {
-            builder.field("refc", &refcount);
-        }
-
-        builder.finish()
+#[cfg(since_api = "4.4")]
+pub(crate) fn debug_string_variant(
+    obj: &Variant,
+    f: &mut std::fmt::Formatter<'_>,
+    ty: &str,
+) -> std::fmt::Result {
+    let id = obj
+        .object_id_unchecked()
+        .expect("Variant must be of type OBJECT");
+
+    if id.lookup_validity() {
+        // Object::get_class() currently returns String, but this is future-proof if the return type changes to StringName.
+        let class = obj
+            .call("get_class", &[])
+            .try_to_relaxed::<StringName>()
+            .expect("get_class() must be compatible with StringName");
+
+        let refcount = id.is_ref_counted().then(|| {
+            obj.call("get_reference_count", &[])
+                .try_to_relaxed::<i32>()
+                .expect("get_reference_count() must return integer") as usize
+        });
+
+        debug_string_parts(f, ty, id, class, refcount, None)
     } else {
         write!(f, "{ty} {{ freed obj }}")
     }
 }
 
+// Polyfill for Godot < 4.4, where Variant::object_id_unchecked() is not available.
+#[cfg(before_api = "4.4")]
+pub(crate) fn debug_string_variant(
+    obj: &Variant,
+    f: &mut std::fmt::Formatter<'_>,
+    ty: &str,
+) -> std::fmt::Result {
+    debug_assert_eq!(obj.get_type(), VariantType::OBJECT);
+
+    match obj.try_to::<Gd<crate::classes::Object>>() {
+        Ok(obj) => {
+            let id = obj.instance_id(); // Guaranteed valid, since conversion would have failed otherwise.
+            let class = obj.dynamic_class_string();
+
+            // Refcount is off-by-one due to now-created Gd<T> from conversion; correct by -1.
+            let refcount = obj.maybe_refcount().map(|rc| rc.saturating_sub(1));
+
+            debug_string_parts(f, ty, id, class, refcount, None)
+        }
+        Err(_) => {
+            write!(f, "{ty} {{ freed obj }}")
+        }
+    }
+}
+
 pub(crate) fn debug_string_nullable<T: GodotClass>(
     obj: &RawGd<T>,
     f: &mut std::fmt::Formatter<'_>,
@@ -67,25 +107,35 @@ pub(crate) fn debug_string_with_trait<T: GodotClass>(
     trt: &str,
 ) -> std::fmt::Result {
     if let Some(id) = obj.instance_id_or_none() {
-        // Sample the reference count FIRST, before any other operations that might affect it.
-        let refcount = obj.maybe_refcount();
-
         let class: StringName = obj.dynamic_class_string();
+        debug_string_parts(f, ty, id, class, obj.maybe_refcount(), Some(trt))
+    } else {
+        write!(f, "{ty} {{ freed obj }}")
+    }
+}
 
-        let mut builder = f.debug_struct(ty);
-        builder
-            .field("id", &id.to_i64())
-            .field("class", &format_args!("{class}"))
-            .field("trait", &format_args!("{trt}"));
+fn debug_string_parts(
+    f: &mut std::fmt::Formatter<'_>,
+    ty: &str,
+    id: InstanceId,
+    class: StringName,
+    refcount: Option<usize>,
+    trait_name: Option<&str>,
+) -> std::fmt::Result {
+    let mut builder = f.debug_struct(ty);
+    builder
+        .field("id", &id.to_i64())
+        .field("class", &format_args!("{class}"));
 
-        if let Some(refcount) = refcount {
-            builder.field("refc", &refcount);
-        }
+    if let Some(trait_name) = trait_name {
+        builder.field("trait", &format_args!("{trait_name}"));
+    }
 
-        builder.finish()
-    } else {
-        write!(f, "{ty} {{ freed obj }}")
+    if let Some(refcount) = refcount {
+        builder.field("refc", &refcount);
     }
+
+    builder.finish()
 }
 
 pub(crate) fn display_string<T: GodotClass>(

itest/rust/src/builtin_tests/containers/variant_test.rs

@@ -261,14 +261,14 @@ fn variant_dead_object_conversions() {
 
     // Verify Display + Debug impl.
     assert_eq!(format!("{variant}"), "<Freed Object>");
-    assert_eq!(format!("{variant:?}"), "<Freed Object>");
+    assert_eq!(format!("{variant:?}"), "VariantGd { freed obj }");
 
     // Variant::try_to().
     let result = variant.try_to::<Gd<Node>>();
     let err = result.expect_err("Variant::try_to::<Gd>() with dead object should fail");
     assert_eq!(
         err.to_string(),
-        "variant holds object which is no longer alive: <Freed Object>"
+        "variant holds object which is no longer alive: VariantGd { freed obj }"
     );
 
     // Variant::to().
@@ -282,7 +282,7 @@ fn variant_dead_object_conversions() {
     let err = result.expect_err("Variant::try_to::<Option<Gd>>() with dead object should fail");
     assert_eq!(
         err.to_string(),
-        "variant holds object which is no longer alive: <Freed Object>"
+        "variant holds object which is no longer alive: VariantGd { freed obj }"
     );
 }
 

itest/rust/src/object_tests/dyn_gd_test.rs

@@ -375,15 +375,16 @@ fn dyn_gd_error_unregistered_trait() {
     let node = node.into_gd();
 
     let err = back.expect_err("DynGd::try_to() should have failed");
-    let expected_err =
-        format!("trait `dyn UnrelatedTrait` has not been registered with #[godot_dyn]: {node:?}");
+    let expected_err = // Variant Debug uses "VariantGd" prefix.
+        format!("trait `dyn UnrelatedTrait` has not been registered with #[godot_dyn]: Variant{node:?}");
 
     assert_eq!(err.to_string(), expected_err);
 
     let back = variant.try_to::<DynGd<foreign::NodeHealth, dyn InstanceIdProvider<Id = i32>>>();
 
+    // Variant Debug uses "VariantGd" prefix.
     let err = back.expect_err("DynGd::try_to() should have failed");
-    let expected_err = format!("trait `dyn InstanceIdProvider<Id = i32>` has not been registered with #[godot_dyn]: {node:?}");
+    let expected_err = format!("trait `dyn InstanceIdProvider<Id = i32>` has not been registered with #[godot_dyn]: Variant{node:?}");
 
     assert_eq!(err.to_string(), expected_err);
 
@@ -401,10 +402,9 @@ fn dyn_gd_error_unimplemented_trait() {
 
     let refc_id = obj.instance_id().to_i64();
     let expected_debug = format!(
-            "none of the classes derived from `RefCounted` have been linked to trait `dyn Health` with #[godot_dyn]: \
-         Gd {{ id: {refc_id}, class: RefCounted, refc: 3 }}")
-    ;
-    // was: {obj:?}
+        "none of the classes derived from `RefCounted` have been linked to trait `dyn Health` with #[godot_dyn]: \
+         VariantGd {{ id: {refc_id}, class: RefCounted, refc: 3 }}"
+    );
     assert_eq!(err.to_string(), expected_debug);
 
     let node = foreign::NodeHealth::new_alloc();
@@ -415,9 +415,9 @@ fn dyn_gd_error_unimplemented_trait() {
 
     // NodeHealth is manually managed (inherits Node), so no refcount in debug output.
     let node_id = node.instance_id().to_i64();
-    let expected_debug =        format!(
-            "none of the classes derived from `NodeHealth` have been linked to trait `dyn InstanceIdProvider<Id = f32>` with #[godot_dyn]: \
-         Gd {{ id: {node_id}, class: NodeHealth }}"
+    let expected_debug = format!(
+        "none of the classes derived from `NodeHealth` have been linked to trait `dyn InstanceIdProvider<Id = f32>` with #[godot_dyn]: \
+         VariantGd {{ id: {node_id}, class: NodeHealth }}"
     );
     assert_eq!(err.to_string(), expected_debug);
 

itest/rust/src/object_tests/object_test.rs

@@ -496,11 +496,10 @@ fn object_engine_convert_variant_nil() {
     });
 }
 
-#[itest(focus)]
+#[itest]
 fn object_engine_convert_variant_error() {
     let refc = RefCounted::new_gd();
     let variant = refc.to_variant();
-
     assert_eq!(refc.test_refcount(), Some(2));
 
     let err = Gd::<Node2D>::try_from_variant(&variant)
@@ -510,11 +509,10 @@ fn object_engine_convert_variant_error() {
     assert_eq!(refc.test_refcount(), Some(3));
 
     let expected_debug = format!(
-        "cannot convert to class Node2D: Gd {{ id: {}, class: RefCounted, refc: 3 }}",
+        "cannot convert to class Node2D: VariantGd {{ id: {}, class: RefCounted, refc: 3 }}",
         refc.instance_id().to_i64()
     );
     assert_eq!(err.to_string(), expected_debug);
-    // was: format!("cannot convert to class Node2D: {refc:?}")
 }
 
 #[itest]