Remove impl AsObjectArg<T> for Gd<T> values

The idea is to require pass-by-ref for objects, in order to discourage
accidental/unnecessary cloning. This is also consistent with AsArg
by-refs, in particular `impl AsArg<Gd<T>>` when used in arrays and
packed arrays.

The downside is that it needs an extra `&` in some places, and one can
no longer use pass-by-value to "consume" objects, ending their lifetime
with the call. What is also unclear is whether there are situations
where we can benefit from moving "into" the engine (perf-wise). So this
may need some more discussion.

One option is also to move toward using Copy for manually managed Gd<T>
types, which would mean a) those could be passed by-value, and b) we
would anyway lose the "consume" semantics.

This is a more conservative choice for now, with minor syntactic
drawbacks, but in line with AsArg<T> and the potential to expand in the
future.

Author: Bromeon

godot-core/src/obj/object_arg.rs

@@ -27,7 +27,9 @@ use std::ptr;
 /// </div>
 
 #[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 +43,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 +62,7 @@ where
         ObjectCow::Owned(self.upcast())
     }
 }
+*/
 
 impl<T, U> AsObjectArg<T> for &Gd<U>
 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);
     ///     }
     /// }
     ///

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/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() {
@@ -43,6 +45,7 @@ fn object_arg_borrowed_mut() {
     });
 }
 
+/*
 #[itest]
 fn object_arg_option_owned() {
     with_objects(|manual, refc| {
@@ -52,6 +55,7 @@ fn object_arg_option_owned() {
         (a, b)
     });
 }
+*/
 
 #[itest]
 fn object_arg_option_borrowed() {
@@ -80,10 +84,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, "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, "value", &Variant::from(-123));
     assert_eq!(error, global::Error::ERR_UNAVAILABLE);
 }
 
@@ -106,14 +110,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);

itest/rust/src/object_tests/object_test.rs

@@ -294,7 +294,7 @@ fn object_engine_freed_argument_passing(ctx: &TestContext) {
     // Destroy object and then pass it to a Godot engine API.
     node.free();
     expect_panic("pass freed Gd<T> to Godot engine API (T=Node)", || {
-        tree.add_child(node2);
+        tree.add_child(&node2);
     });
 }
 
@@ -325,7 +325,7 @@ fn object_user_freed_argument_passing() {
     // Destroy object and then pass it to a Godot engine API (upcast itself works, see other tests).
     obj.free();
     expect_panic("pass freed Gd<T> to Godot engine API (T=user)", || {
-        engine.register_singleton("NeverRegistered", obj2);
+        engine.register_singleton("NeverRegistered", &obj2);
     });
 }
 
@@ -848,7 +848,7 @@ fn object_get_scene_tree(ctx: &TestContext) {
     let node = Node3D::new_alloc();
 
     let mut tree = ctx.scene_tree.clone();
-    tree.add_child(node);
+    tree.add_child(&node);
 
     let count = tree.get_child_count();
     assert_eq!(count, 1);

itest/rust/src/object_tests/onready_test.rs

@@ -157,7 +157,7 @@ fn init_attribute_node_key_lifecycle() {
 
     let mut child = Node::new_alloc();
     child.set_name("child");
-    obj.add_child(child);
+    obj.add_child(&child);
 
     obj.notify(NodeNotification::READY);
 

itest/rust/src/object_tests/virtual_methods_test.rs

@@ -519,7 +519,7 @@ fn test_format_loader(_test_context: &TestContext) {
         .unwrap();
     assert!(resource.try_cast::<BoxMesh>().is_ok());
 
-    loader.remove_resource_format_loader(format_loader);
+    loader.remove_resource_format_loader(&format_loader);
 }
 
 #[itest]