Postpone the ref-counting problem for native structures

Also rename collider() -> get_collider().

Author: Bromeon

godot-codegen/src/generator/native_structures.rs

@@ -71,14 +71,20 @@ fn make_native_structure(
 ) -> builtins::GeneratedBuiltin {
     let class_name = &class_name.rust_ty;
 
+    // TODO for native structures holding object pointers, we should encapsulate the raw object, as follows:
+    // - Make raw object pointer field private (and maybe also ID field, to keep in sync).
+    // - Add constructor that takes Gd<Object> instead of ID/raw-ptr fields.
+    // - Add getter and setter methods with Gd<Object> (already present).
+    // - Add Drop impl, which decrements refcount if the constructor was used, and does nothing if FromGodot pointer conversion was used.
+
     let imports = util::make_imports();
-    let (fields, methods) = make_native_structure_fields_and_methods(&structure.format, ctx);
+    let (fields, methods) = make_native_structure_fields_and_methods(structure, ctx);
     let doc = format!("[`ToGodot`] and [`FromGodot`] are implemented for `*mut {class_name}` and `*const {class_name}`.");
 
     // mod re_export needed, because class should not appear inside the file module, and we can't re-export private struct as pub
     let tokens = quote! {
         #imports
-        use std::ffi::c_void; // for opaque object pointers
+        use std::ffi::c_void; // for opaque object pointer fields
         use crate::meta::{GodotConvert, FromGodot, ToGodot};
 
         /// Native structure; can be passed via pointer in APIs that are not exposed to GDScript.
@@ -132,10 +138,10 @@ fn make_native_structure(
 }
 
 fn make_native_structure_fields_and_methods(
-    format_str: &str,
+    structure: &NativeStructure,
     ctx: &mut Context,
 ) -> (TokenStream, TokenStream) {
-    let fields = parse_native_structures_format(format_str)
+    let fields = parse_native_structures_format(&structure.format)
         .expect("Could not parse native_structures format field");
 
     let mut field_definitions = vec![];
@@ -151,6 +157,7 @@ fn make_native_structure_fields_and_methods(
 
     let fields = quote! { #( #field_definitions )* };
     let methods = quote! { #( #accessors )* };
+
     (fields, methods)
 }
 
@@ -176,12 +183,15 @@ fn make_native_structure_field_and_accessor(
         field_name = format_ident!("raw_{}_ptr", snake_field_name);
 
         // Generate method that converts from instance ID.
-        let getter_name = &snake_field_name;
+        let getter_name = format_ident!("get_{}", snake_field_name);
         let setter_name = format_ident!("set_{}", snake_field_name);
         let id_field_name = format_ident!("{}_id", snake_field_name);
 
+        // Current native structures treat all object pointers as Object (even if concrete ones like `collider` might be Node).
+        // Having node also avoids the lifetime issue with reference-counted objects (unclear if gdext should increment refcount or not).
+        // If other fields use different classes in the future, we'll need to update this.
         accessor = Some(quote! {
-            /// Returns the object as a `Gd<T>`, or `None` if it no longer exists.
+            /// Returns the object as a `Gd<Node>`, or `None` if it no longer exists.
             pub fn #getter_name(&self) -> Option<Gd<Object>> {
                 crate::obj::InstanceId::try_from_u64(self.#id_field_name.id)
                     .and_then(|id| Gd::try_from_instance_id(id).ok())
@@ -191,22 +201,16 @@ fn make_native_structure_field_and_accessor(
                 // unsafe { Gd::from_obj_sys(ptr) }
             }
 
-            /// Sets the object from a `Gd<T>` pointer.
-            ///
-            /// `increment_refcount` is only relevant for ref-counted objects (inheriting `RefCounted`). It is ignored otherwise.
-            /// - Set it to true if you transfer `self` to Godot, e.g. via output parameter in a virtual function call.
-            ///   In this case, you can drop your own references and the object will remain alive.
-            ///   However, if you drop the native structure `self` without handing it over to Godot, you'll have a memory leak.
-            /// - Set it to false if you just manage the native structure yourself.
-            pub fn #setter_name(&mut self, mut obj: Gd<Object>, increment_refcount: bool) {
+            /// Sets the object from a `Gd` pointer holding `Node` or a derived class.
+            pub fn #setter_name<T>(&mut self, #snake_field_name: Gd<T>)
+            where T: crate::obj::Inherits<Object> {
                 use crate::meta::GodotType as _;
 
-                assert!(obj.is_instance_valid(), "provided object is dead");
+                let obj = #snake_field_name.upcast();
+
+                assert!(obj.is_instance_valid(), "provided node is dead");
 
                 let id = obj.instance_id().to_u64();
-                if increment_refcount {
-                    obj = obj.with_inc_refcount();
-                }
 
                 self.#id_field_name = ObjectId { id };
                 self.#field_name = obj.obj_sys() as *mut std::ffi::c_void;

godot-core/src/obj/gd.rs

@@ -46,7 +46,7 @@ use crate::{classes, out};
 /// - **Manual**<br>
 ///   Objects inheriting from [`Object`] which are not `RefCounted` (or inherited) are **manually-managed**.
 ///   Their destructor is not automatically called (unless they are part of the scene tree). Creating a `Gd<T>` means that
-///   you are responsible of explicitly deallocating such objects using [`free()`][Self::free].<br><br>
+///   you are responsible for explicitly deallocating such objects using [`free()`][Self::free].<br><br>
 ///
 /// - **Dynamic**<br>
 ///   For `T=Object`, the memory strategy is determined **dynamically**. Due to polymorphism, a `Gd<Object>` can point to either
@@ -77,7 +77,7 @@ use crate::{classes, out};
 /// These provide interior mutability similar to [`RefCell`][std::cell::RefCell], with the addition that `Gd` simultaneously handles reference
 /// counting (for some types `T`).
 ///
-/// When you declare a `#[func]` method on your own class and it accepts `&self` or `&mut self`, an implicit `bind()` or `bind_mut()` call
+/// When you declare a `#[func]` method on your own class, and it accepts `&self` or `&mut self`, an implicit `bind()` or `bind_mut()` call
 /// on the owning `Gd<T>` is performed. This is important to keep in mind, as you can get into situations that violate dynamic borrow rules; for
 /// example if you are inside a `&mut self` method, make a call to GDScript and indirectly call another method on the same object (re-entrancy).
 ///
@@ -424,12 +424,6 @@ impl<T: GodotClass> Gd<T> {
     {
         self.raw.script_sys()
     }
-
-    pub(crate) fn with_inc_refcount(self) -> Self {
-        Self {
-            raw: self.raw.with_inc_refcount(),
-        }
-    }
 }
 
 impl<T: GodotClass> Deref for Gd<T> {

godot-core/src/obj/raw.rs

@@ -82,8 +82,7 @@ impl<T: GodotClass> RawGd<T> {
     }
 
     /// Returns `self` but with initialized ref-count.
-    // Could be private if not for Gd::with_inc_refcount(), which is used by native structure setters.
-    pub(crate) fn with_inc_refcount(mut self) -> Self {
+    fn with_inc_refcount(mut self) -> Self {
         // Note: use init_ref and not inc_ref, since this might be the first reference increment.
         // Godot expects RefCounted::init_ref to be called instead of RefCounted::reference in that case.
         // init_ref also doesn't hurt (except 1 possibly unnecessary check).

itest/rust/src/engine_tests/native_structures_test.rs

@@ -13,7 +13,7 @@ use godot::classes::native::{
 };
 use godot::classes::text_server::Direction;
 use godot::classes::{ITextServerExtension, Node3D, RefCounted, TextServer, TextServerExtension};
-use godot::obj::{Base, Gd, NewAlloc, NewGd};
+use godot::obj::{Base, NewAlloc, NewGd};
 use godot::register::{godot_api, GodotClass};
 
 use std::cell::Cell;
@@ -194,50 +194,61 @@ fn native_structure_debug() {
 
 #[itest]
 fn native_structure_object_pointers() {
-    // Object.
-    let object = Node3D::new_alloc();
-
     let mut result = PhysicsServer2DExtensionShapeResult {
         rid: Rid::new(12),
         collider_id: ObjectId { id: 0 },
         raw_collider_ptr: std::ptr::null_mut(),
         shape: 0,
     };
 
-    result.set_collider(object.clone().upcast(), true);
+    let retrieved = result.get_collider();
+    assert_eq!(retrieved, None);
+
+    let object = Node3D::new_alloc();
+    result.set_collider(object.clone());
     assert_eq!(result.collider_id.id, object.instance_id().to_i64() as u64);
 
-    let retrieved = result.collider();
+    let retrieved = result.get_collider();
     assert_eq!(retrieved, Some(object.clone().upcast()));
 
     object.free();
-    assert_eq!(result.collider(), None);
+    assert_eq!(result.get_collider(), None);
+}
+
+#[itest(skip)] // Not yet implemented.
+fn native_structure_refcounted_pointers() {
+    let mut result = PhysicsServer2DExtensionShapeResult {
+        rid: Rid::new(12),
+        collider_id: ObjectId { id: 0 },
+        raw_collider_ptr: std::ptr::null_mut(),
+        shape: 0,
+    };
 
     // RefCounted, increment ref-count.
     let object = RefCounted::new_gd();
     let id = object.instance_id();
-    result.set_collider(object.clone().upcast(), true);
+    result.set_collider(object.clone());
     assert_eq!(result.collider_id.id, id.to_i64() as u64);
 
     drop(object); // Test if Godot keeps ref-count.
 
     let retrieved = result
-        .collider()
+        .get_collider()
         .expect("Ref-counted objects don't drop if ref-count is incremented");
     assert_eq!(retrieved.instance_id(), id);
 
     // Manually decrement refcount (method unexposed).
-    Gd::<RefCounted>::from_instance_id(id).call("unreference".into(), &[]);
+    //Gd::<RefCounted>::from_instance_id(id).call("unreference".into(), &[]);
 
     // RefCounted, do NOT increment ref-count.
     let object = RefCounted::new_gd();
     let id = object.instance_id();
-    result.set_collider(object.clone().upcast(), false);
+    result.set_collider(object.clone());
     assert_eq!(result.collider_id.id, id.to_i64() as u64);
 
     drop(object); // Test if Godot keeps ref-count.
 
-    let retrieved = result.collider();
+    let retrieved = result.get_collider();
     assert!(
         retrieved.is_none(),
         "Ref-counted objects drop if ref-count is not incremented"