Merge pull request #947 from godot-rust/qol/objects-by-ref

Require `AsObjectArg` pass-by-ref, consistent with `AsArg`

Author: Bromeon

godot-core/src/meta/traits.rs

@@ -175,6 +175,7 @@ pub trait ArrayElement: GodotType + ToGodot + FromGodot + sealed::Sealed + meta:
         builtin_type_string::<Self>()
     }
 
+    #[doc(hidden)]
     fn debug_validate_elements(_array: &builtin::Array<Self>) -> Result<(), ConvertError> {
         // No-op for most element types.
         Ok(())

godot-core/src/obj/object_arg.rs

@@ -15,19 +15,37 @@ use std::ptr;
 
 /// Objects that can be passed as arguments to Godot engine functions.
 ///
-/// This trait is implemented for the following types:
-/// - [`Gd<T>`] and `&Gd<T>`, to pass objects. Subclasses of `T` are explicitly supported.
-/// - [`Option<Gd<T>>`] and `Option<&Gd<T>>`, to pass optional objects. `None` is mapped to a null argument.
+/// This trait is implemented for **shared references** in multiple ways:
+/// - [`&Gd<T>`][crate::obj::Gd]  to pass objects. Subclasses of `T` are explicitly supported.
+/// - [`Option<&Gd<T>>`][Option], to pass optional objects. `None` is mapped to a null argument.
 /// - [`Gd::null_arg()`], to pass `null` arguments without using `Option`.
 ///
 /// # Nullability
 /// <div class="warning">
 /// The GDExtension API does not inform about nullability of its function parameters. It is up to you to verify that the arguments you pass
 /// are only null when this is allowed. Doing this wrong should be safe, but can lead to the function call failing.
 /// </div>
-
+///
+/// # Different argument types
+/// Currently, the trait requires pass-by-ref, which helps detect accidental cloning when interfacing with Godot APIs. Plus, it is more
+/// consistent with the [`AsArg`][crate::meta::AsArg] trait (for strings, but also `AsArg<Gd<T>>` as used in
+/// [`Array::push()`][crate::builtin::Array::push] and similar methods).
+///
+/// The following table lists the possible argument types and how you can pass them. `Gd` is short for `Gd<T>`.
+///
+/// | Type              | Closest accepted type | How to transform |
+/// |-------------------|-----------------------|------------------|
+/// | `Gd`              | `&Gd`                 | `&arg`           |
+/// | `&Gd`             | `&Gd`                 | `arg`            |
+/// | `&mut Gd`         | `&Gd`                 | `&*arg`          |
+/// | `Option<Gd>`      | `Option<&Gd>`         | `arg.as_ref()`   |
+/// | `Option<&Gd>`     | `Option<&Gd>`         | `arg`            |
+/// | `Option<&mut Gd>` | `Option<&Gd>`         | `arg.as_deref()` |
+/// | (null literal)    |                       | `Gd::null_arg()` |
 #[diagnostic::on_unimplemented(
-    message = "The provided argument of type `{Self}` cannot be converted to a `Gd<{T}>` parameter"
+    message = "Argument of type `{Self}` cannot be passed to an `impl AsObjectArg<{T}>` parameter",
+    note = "If you pass by value, consider borrowing instead.",
+    note = "See also `AsObjectArg` docs: https://godot-rust.github.io/docs/gdext/master/godot/meta/trait.AsObjectArg.html"
 )]
 pub trait AsObjectArg<T>
 where
@@ -41,6 +59,12 @@ where
     fn consume_arg(self) -> ObjectCow<T>;
 }
 
+/*
+Currently not implemented for values, to be consistent with AsArg for by-ref builtins. The idea is that this can discover patterns like
+api.method(refc.clone()), and encourage better performance with api.method(&refc). However, we need to see if there's a notable ergonomic
+impact, and consider that for nodes, Gd<T> copies are relatively cheap (no ref-counting). There is also some value in prematurely ending
+the lifetime of a Gd<T> by moving out, so it's not accidentally used later.
+
 impl<T, U> AsObjectArg<T> for Gd<U>
 where
     T: GodotClass + Bounds<Declarer = bounds::DeclEngine>,
@@ -54,6 +78,7 @@ where
         ObjectCow::Owned(self.upcast())
     }
 }
+*/
 
 impl<T, U> AsObjectArg<T> for &Gd<U>
 where
@@ -71,30 +96,44 @@ where
     }
 }
 
-impl<T, U> AsObjectArg<T> for &mut Gd<U>
+impl<T, U> AsObjectArg<T> for Option<U>
 where
     T: GodotClass + Bounds<Declarer = bounds::DeclEngine>,
-    U: Inherits<T>,
+    U: AsObjectArg<T>,
 {
-    // Delegate to &Gd impl.
-
     fn as_object_arg(&self) -> ObjectArg<T> {
-        <&Gd<U>>::as_object_arg(&&**self)
+        self.as_ref()
+            .map_or_else(ObjectArg::null, AsObjectArg::as_object_arg)
     }
 
     fn consume_arg(self) -> ObjectCow<T> {
-        <&Gd<U>>::consume_arg(&*self)
+        match self {
+            Some(obj) => obj.consume_arg(),
+            None => Gd::null_arg().consume_arg(),
+        }
     }
 }
 
-impl<T, U> AsObjectArg<T> for Option<U>
+/*
+It's relatively common that Godot APIs return `Option<Gd<T>>` or pass this type in virtual functions. To avoid excessive `as_ref()` calls, we
+**could** directly support `&Option<Gd>` in addition to `Option<&Gd>`. However, this is currently not done as it hides nullability,
+especially in situations where a return type is directly propagated:
+    api(create_obj().as_ref())
+    api(&create_obj())
+While the first is slightly longer, it looks different from a function create_obj() that returns Gd<T> and thus can never be null.
+In some scenarios, it's better to immediately ensure non-null (e.g. through `unwrap()`) instead of propagating nulls to the engine.
+It's also quite idiomatic to use as_ref() for inner-option transforms in Rust.
+
+impl<T, U> AsObjectArg<T> for &Option<U>
 where
     T: GodotClass + Bounds<Declarer = bounds::DeclEngine>,
-    U: AsObjectArg<T>,
+    for<'a> &'a U: AsObjectArg<T>,
 {
     fn as_object_arg(&self) -> ObjectArg<T> {
-        self.as_ref()
-            .map_or_else(ObjectArg::null, AsObjectArg::as_object_arg)
+        match self {
+            Some(obj) => obj.as_object_arg(),
+            None => ObjectArg::null(),
+        }
     }
 
     fn consume_arg(self) -> ObjectCow<T> {
@@ -104,6 +143,7 @@ where
         }
     }
 }
+*/
 
 impl<T> AsObjectArg<T> for ObjectNullArg<T>
 where

godot-core/src/obj/script.rs

@@ -386,7 +386,7 @@ impl<'a, T: ScriptInstance> SiMut<'a, T> {
     ///         let name = this.base().get_name();
     ///         godot_print!("name is {name}");
     ///         // However, we cannot call methods that require `&mut Base`, such as:
-    ///         // this.base().add_child(node);
+    ///         // this.base().add_child(&node);
     ///         Ok(Variant::nil())
     ///     }
     ///     # fn class_name(&self) -> GString { todo!() }

godot-core/src/obj/traits.rs

@@ -304,7 +304,7 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
     ///     fn process(&mut self, _delta: f64) {
     ///         let node = Node::new_alloc();
     ///         // fails because `add_child` requires a mutable reference.
-    ///         self.base().add_child(node);
+    ///         self.base().add_child(&node);
     ///     }
     /// }
     ///
@@ -344,7 +344,7 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
     /// impl INode for MyClass {
     ///     fn process(&mut self, _delta: f64) {
     ///         let node = Node::new_alloc();
-    ///         self.base_mut().add_child(node);
+    ///         self.base_mut().add_child(&node);
     ///     }
     /// }
     ///

godot-core/src/registry/property.rs

@@ -60,6 +60,7 @@ pub trait Export: Var {
     ///
     /// Only overridden for `Gd<T>`, to detect erroneous exports of `Node` inside a `Resource` class.
     #[allow(clippy::wrong_self_convention)]
+    #[doc(hidden)]
     fn as_node_class() -> Option<ClassName> {
         None
     }

godot-core/src/tools/save_load.rs

@@ -78,17 +78,18 @@ where
 /// See [`try_save`] for more information.
 ///
 /// # Panics
-/// If the resouce cannot be saved.
+/// If the resource cannot be saved.
 ///
 /// # Example
 /// ```no_run
 /// use godot::prelude::*;
 ///
-/// save(Resource::new_gd(), "res://BaseResource.tres")
+/// let obj = Resource::new_gd();
+/// save(&obj, "res://BaseResource.tres")
 /// ```
 /// use godot::
 #[inline]
-pub fn save<T>(obj: Gd<T>, path: impl AsArg<GString>)
+pub fn save<T>(obj: &Gd<T>, path: impl AsArg<GString>)
 where
     T: Inherits<Resource>,
 {
@@ -120,12 +121,12 @@ where
 /// };
 ///
 /// let save_state = SavedGame::new_gd();
-/// let res = try_save(save_state, "user://save.tres");
+/// let res = try_save(&save_state, "user://save.tres");
 ///
 /// assert!(res.is_ok());
 /// ```
 #[inline]
-pub fn try_save<T>(obj: Gd<T>, path: impl AsArg<GString>) -> Result<(), IoError>
+pub fn try_save<T>(obj: &Gd<T>, path: impl AsArg<GString>) -> Result<(), IoError>
 where
     T: Inherits<Resource>,
 {
@@ -163,7 +164,7 @@ where
     }
 }
 
-fn save_impl<T>(obj: Gd<T>, path: &GString) -> Result<(), IoError>
+fn save_impl<T>(obj: &Gd<T>, path: &GString) -> Result<(), IoError>
 where
     T: Inherits<Resource>,
 {

itest/rust/src/engine_tests/native_audio_structures_test.rs

@@ -113,7 +113,7 @@ fn native_audio_structure_out_parameter() {
         .cast::<SceneTree>();
 
     tree.get_root().unwrap().add_child(&player);
-    player.set_stream(generator);
+    player.set_stream(&generator);
 
     // Start playback so we can push audio frames through the audio pipeline.
     player.play();

itest/rust/src/engine_tests/node_test.rs

@@ -22,11 +22,11 @@ fn node_get_node() {
 
     let mut parent = Node3D::new_alloc();
     parent.set_name("parent");
-    parent.add_child(child);
+    parent.add_child(&child);
 
     let mut grandparent = Node::new_alloc();
     grandparent.set_name("grandparent");
-    grandparent.add_child(parent);
+    grandparent.add_child(&parent);
 
     // Directly on Gd<T>
     let found = grandparent.get_node_as::<Node3D>("parent/child");
@@ -74,7 +74,7 @@ fn node_scene_tree() {
     assert_eq!(err, global::Error::OK);
 
     let mut tree = SceneTree::new_alloc();
-    let err = tree.change_scene_to_packed(scene);
+    let err = tree.change_scene_to_packed(&scene);
     assert_eq!(err, global::Error::OK);
 
     // Note: parent + child are not owned by PackedScene, thus need to be freed

itest/rust/src/engine_tests/save_load_test.rs

@@ -34,13 +34,13 @@ fn save_test() {
 
     let resource = SavedGame::new_gd();
 
-    let res = try_save(resource.clone(), FAULTY_PATH);
+    let res = try_save(&resource, FAULTY_PATH);
     assert!(res.is_err());
 
-    let res = try_save(resource.clone(), &res_path);
+    let res = try_save(&resource, &res_path);
     assert!(res.is_ok());
 
-    save(resource.clone(), &res_path);
+    save(&resource, &res_path);
 
     remove_test_file(RESOURCE_NAME);
 }
@@ -53,7 +53,7 @@ fn load_test() {
     let mut resource = SavedGame::new_gd();
     resource.bind_mut().set_level(level);
 
-    save(resource.clone(), &res_path);
+    save(&resource, &res_path);
 
     let res = try_load::<SavedGame>(FAULTY_PATH);
     assert!(res.is_err());

itest/rust/src/object_tests/object_arg_test.rs

@@ -13,6 +13,7 @@ use godot::obj::{Gd, NewAlloc, NewGd};
 use crate::framework::itest;
 use crate::object_tests::object_test::{user_refc_instance, RefcPayload};
 
+/*
 #[itest]
 fn object_arg_owned() {
     with_objects(|manual, refc| {
@@ -22,6 +23,7 @@ fn object_arg_owned() {
         (a, b)
     });
 }
+*/
 
 #[itest]
 fn object_arg_borrowed() {
@@ -37,12 +39,17 @@ fn object_arg_borrowed() {
 fn object_arg_borrowed_mut() {
     with_objects(|mut manual, mut refc| {
         let db = ClassDb::singleton();
-        let a = db.class_set_property(&mut manual, "name", &Variant::from("hello"));
-        let b = db.class_set_property(&mut refc, "value", &Variant::from(-123));
+
+        let manual_ref = &mut manual;
+        let refc_ref = &mut refc;
+
+        let a = db.class_set_property(&*manual_ref, "name", &Variant::from("hello"));
+        let b = db.class_set_property(&*refc_ref, "value", &Variant::from(-123));
         (a, b)
     });
 }
 
+/*
 #[itest]
 fn object_arg_option_owned() {
     with_objects(|manual, refc| {
@@ -52,6 +59,7 @@ fn object_arg_option_owned() {
         (a, b)
     });
 }
+*/
 
 #[itest]
 fn object_arg_option_borrowed() {
@@ -63,12 +71,30 @@ fn object_arg_option_borrowed() {
     });
 }
 
+/*
+#[itest]
+fn object_arg_option_borrowed_outer() {
+    with_objects(|manual, refc| {
+        let db = ClassDb::singleton();
+        let a = db.class_set_property(&Some(manual), "name", &Variant::from("hello"));
+        let b = db.class_set_property(&Some(refc), "value", &Variant::from(-123));
+        (a, b)
+    });
+}
+*/
+
 #[itest]
 fn object_arg_option_borrowed_mut() {
+    // If you have an Option<&mut Gd<T>>, you can use as_deref() to get Option<&Gd<T>>.
+
     with_objects(|mut manual, mut refc| {
         let db = ClassDb::singleton();
-        let a = db.class_set_property(Some(&mut manual), "name", &Variant::from("hello"));
-        let b = db.class_set_property(Some(&mut refc), "value", &Variant::from(-123));
+
+        let manual_opt: Option<&mut Gd<Node>> = Some(&mut manual);
+        let refc_opt: Option<&mut Gd<RefcPayload>> = Some(&mut refc);
+
+        let a = db.class_set_property(manual_opt.as_deref(), "name", &Variant::from("hello"));
+        let b = db.class_set_property(refc_opt.as_deref(), "value", &Variant::from(-123));
         (a, b)
     });
 }
@@ -80,10 +106,10 @@ fn object_arg_option_none() {
 
     // Will emit errors but should not crash.
     let db = ClassDb::singleton();
-    let error = db.class_set_property(manual, "name", &Variant::from("hello"));
+    let error = db.class_set_property(manual.as_ref(), "name", &Variant::from("hello"));
     assert_eq!(error, global::Error::ERR_UNAVAILABLE);
 
-    let error = db.class_set_property(refc, "value", &Variant::from(-123));
+    let error = db.class_set_property(refc.as_ref(), "value", &Variant::from(-123));
     assert_eq!(error, global::Error::ERR_UNAVAILABLE);
 }
 
@@ -106,14 +132,14 @@ fn object_arg_owned_default_params() {
     let b = ResourceFormatLoader::new_gd();
 
     // Use direct and explicit _ex() call syntax.
-    ResourceLoader::singleton().add_resource_format_loader(a.clone()); // by value
+    ResourceLoader::singleton().add_resource_format_loader(&a);
     ResourceLoader::singleton()
-        .add_resource_format_loader_ex(b.clone()) // by value
+        .add_resource_format_loader_ex(&b)
         .done();
 
     // Clean up (no leaks).
-    ResourceLoader::singleton().remove_resource_format_loader(a);
-    ResourceLoader::singleton().remove_resource_format_loader(b);
+    ResourceLoader::singleton().remove_resource_format_loader(&a);
+    ResourceLoader::singleton().remove_resource_format_loader(&b);
 }
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------

itest/rust/src/object_tests/object_swap_test.rs

@@ -119,7 +119,7 @@ fn object_subtype_swap_argument_passing(ctx: &TestContext) {
 
     let mut tree = ctx.scene_tree.clone();
     expect_panic("pass badly typed Gd<T> to Godot engine API", || {
-        tree.add_child(node);
+        tree.add_child(&node);
     });
 
     swapped_free!(obj, node2);