Merge pull request #409 from godot-rust/qol/traits

`Share` trait: deprecate + replace with `Clone`

Author: Bromeon

examples/dodge-the-creeps/rust/src/main_scene.rs

@@ -91,7 +91,7 @@ impl Main {
 
         mob_scene.set_rotation(direction);
 
-        self.base.add_child(mob_scene.share().upcast());
+        self.base.add_child(mob_scene.clone().upcast());
 
         let mut mob = mob_scene.cast::<mob::Mob>();
         let range = {

godot-core/src/builtin/array.rs

@@ -34,9 +34,9 @@ use sys::{ffi_methods, interface_fn, GodotFfi};
 /// Like in GDScript, `Array` acts as a reference type: multiple `Array` instances may
 /// refer to the same underlying array, and changes to one are visible in the other.
 ///
-/// To create a copy that shares data with the original array, use [`Share::share()`]. If you want
-/// to create a copy of the data, use [`duplicate_shallow()`][Self::duplicate_shallow] or
-/// [`duplicate_deep()`][Self::duplicate_deep].
+/// To create a copy that shares data with the original array, use [`Clone::clone()`].
+/// If you want to create a copy of the data, use [`duplicate_shallow()`][Self::duplicate_shallow]
+/// or [`duplicate_deep()`][Self::duplicate_deep].
 ///
 /// # Thread safety
 ///
@@ -245,7 +245,7 @@ impl<T: VariantMetadata> Array<T> {
     /// (such as `Array`, `Dictionary` and `Object`) will still refer to the same value.
     ///
     /// To create a deep copy, use [`duplicate_deep()`][Self::duplicate_deep] instead.
-    /// To create a new reference to the same array data, use [`share()`][Share::share].
+    /// To create a new reference to the same array data, use [`clone()`][Clone::clone].
     pub fn duplicate_shallow(&self) -> Self {
         let duplicate: VariantArray = self.as_inner().duplicate(false);
         // SAFETY: duplicate() returns a typed array with the same type as Self
@@ -257,7 +257,7 @@ impl<T: VariantMetadata> Array<T> {
     /// still be shallow copied.
     ///
     /// To create a shallow copy, use [`duplicate_shallow()`][Self::duplicate_shallow] instead.
-    /// To create a new reference to the same array data, use [`share()`][Share::share].
+    /// To create a new reference to the same array data, use [`clone()`][Clone::clone].
     pub fn duplicate_deep(&self) -> Self {
         let duplicate: VariantArray = self.as_inner().duplicate(true);
         // SAFETY: duplicate() returns a typed array with the same type as Self
@@ -592,7 +592,7 @@ impl<T: VariantMetadata + ToVariant> Array<T> {
 //
 // - `from_arg_ptr`
 //   Arrays are properly initialized through a `from_sys` call, but the ref-count should be incremented
-//   as that is the callee's responsibility. Which we do by calling `std::mem::forget(array.share())`.
+//   as that is the callee's responsibility. Which we do by calling `std::mem::forget(array.clone())`.
 unsafe impl<T: VariantMetadata> GodotFfi for Array<T> {
     ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque;
         fn from_sys;
@@ -609,7 +609,7 @@ unsafe impl<T: VariantMetadata> GodotFfi for Array<T> {
 
     unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::PtrcallType) -> Self {
         let array = Self::from_sys(ptr);
-        std::mem::forget(array.share());
+        std::mem::forget(array.clone());
         array
     }
 }
@@ -624,23 +624,31 @@ impl<T: VariantMetadata> fmt::Debug for Array<T> {
 /// Creates a new reference to the data in this array. Changes to the original array will be
 /// reflected in the copy and vice versa.
 ///
-/// To create a (mostly) independent copy instead, see [`VariantArray::duplicate_shallow()`] and
-/// [`VariantArray::duplicate_deep()`].
-impl<T: VariantMetadata> Share for Array<T> {
-    fn share(&self) -> Self {
+/// To create a (mostly) independent copy instead, see [`Array::duplicate_shallow()`] and
+/// [`Array::duplicate_deep()`].
+impl<T: VariantMetadata> Clone for Array<T> {
+    fn clone(&self) -> Self {
+        // SAFETY: `self` is a valid array, since we have a reference that keeps it alive.
         let array = unsafe {
             Self::from_sys_init(|self_ptr| {
                 let ctor = ::godot_ffi::builtin_fn!(array_construct_copy);
                 let args = [self.sys_const()];
                 ctor(self_ptr, args.as_ptr());
             })
         };
+
         array
             .with_checked_type()
             .expect("copied array should have same type as original array")
     }
 }
 
+impl<T: VariantMetadata> Share for Array<T> {
+    fn share(&self) -> Self {
+        self.clone()
+    }
+}
+
 impl<T: VariantMetadata + TypeStringHint> TypeStringHint for Array<T> {
     fn type_string() -> String {
         format!("{}:{}", VariantType::Array as i32, T::type_string())
@@ -651,7 +659,7 @@ impl<T: VariantMetadata> Property for Array<T> {
     type Intermediate = Self;
 
     fn get_property(&self) -> Self::Intermediate {
-        self.share()
+        self.clone()
     }
 
     fn set_property(&mut self, value: Self::Intermediate) {

godot-core/src/builtin/dictionary.rs

@@ -49,8 +49,8 @@ impl Dictionary {
     /// will not be shared with the original dictionary. Note that any `Object`-derived elements will
     /// still be shallow copied.
     ///
-    /// To create a shallow copy, use [`Self::duplicate_shallow`] instead. To create a new reference to
-    /// the same array data, use [`Share::share`].
+    /// To create a shallow copy, use [`Self::duplicate_shallow()`] instead.
+    /// To create a new reference to the same dictionary data, use [`clone()`][Clone::clone].
     ///
     /// _Godot equivalent: `dict.duplicate(true)`_
     pub fn duplicate_deep(&self) -> Self {
@@ -61,8 +61,8 @@ impl Dictionary {
     /// any reference types (such as `Array`, `Dictionary` and `Object`) will still refer to the
     /// same value.
     ///
-    /// To create a deep copy, use [`Self::duplicate_deep`] instead. To create a new reference to the
-    /// same dictionary data, use [`Share::share`].
+    /// To create a deep copy, use [`Self::duplicate_deep()`] instead.
+    /// To create a new reference to the same dictionary data, use [`clone()`][Clone::clone].
     ///
     /// _Godot equivalent: `dict.duplicate(false)`_
     pub fn duplicate_shallow(&self) -> Self {
@@ -263,7 +263,7 @@ impl Dictionary {
 // - `from_arg_ptr`
 //   Dictionaries are properly initialized through a `from_sys` call, but the ref-count should be
 //   incremented as that is the callee's responsibility. Which we do by calling
-//   `std::mem::forget(dictionary.share())`.
+//   `std::mem::forget(dictionary.clone())`.
 unsafe impl GodotFfi for Dictionary {
     ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque;
         fn from_sys;
@@ -280,7 +280,7 @@ unsafe impl GodotFfi for Dictionary {
 
     unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::PtrcallType) -> Self {
         let dictionary = Self::from_sys(ptr);
-        std::mem::forget(dictionary.share());
+        std::mem::forget(dictionary.clone());
         dictionary
     }
 }
@@ -304,8 +304,9 @@ impl fmt::Debug for Dictionary {
 ///
 /// To create a (mostly) independent copy instead, see [`Dictionary::duplicate_shallow()`] and
 /// [`Dictionary::duplicate_deep()`].
-impl Share for Dictionary {
-    fn share(&self) -> Self {
+impl Clone for Dictionary {
+    fn clone(&self) -> Self {
+        // SAFETY: `self` is a valid dictionary, since we have a reference that keeps it alive.
         unsafe {
             Self::from_sys_init(|self_ptr| {
                 let ctor = sys::builtin_fn!(dictionary_construct_copy);
@@ -316,11 +317,17 @@ impl Share for Dictionary {
     }
 }
 
+impl Share for Dictionary {
+    fn share(&self) -> Self {
+        self.clone()
+    }
+}
+
 impl Property for Dictionary {
     type Intermediate = Self;
 
     fn get_property(&self) -> Self::Intermediate {
-        self.share()
+        self.clone()
     }
 
     fn set_property(&mut self, value: Self::Intermediate) {

godot-core/src/builtin/string/godot_string.rs

@@ -108,7 +108,7 @@ impl GodotString {
 // - `from_arg_ptr`
 //   Strings are properly initialized through a `from_sys` call, but the ref-count should be
 //   incremented as that is the callee's responsibility. Which we do by calling
-//   `std::mem::forget(string.share())`.
+//   `std::mem::forget(string.clone())`.
 unsafe impl GodotFfi for GodotString {
     ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque;
         fn from_sys;

godot-core/src/builtin/string/node_path.rs

@@ -46,7 +46,7 @@ impl NodePath {
 // - `from_arg_ptr`
 //   NodePaths are properly initialized through a `from_sys` call, but the ref-count should be
 //   incremented as that is the callee's responsibility. Which we do by calling
-//   `std::mem::forget(node_path.share())`.
+//   `std::mem::forget(node_path.clone())`.
 unsafe impl GodotFfi for NodePath {
     ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque;
         fn from_sys;

godot-core/src/builtin/string/string_name.rs

@@ -73,7 +73,7 @@ impl StringName {
 // - `from_arg_ptr`
 //   StringNames are properly initialized through a `from_sys` call, but the ref-count should be
 //   incremented as that is the callee's responsibility. Which we do by calling
-//   `std::mem::forget(string_name.share())`.
+//   `std::mem::forget(string_name.clone())`.
 unsafe impl GodotFfi for StringName {
     ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque;
         fn from_sys;

godot-core/src/engine.rs

@@ -112,10 +112,8 @@ where
         // This would need more sophisticated upcast design, e.g. T::upcast_{ref|mut}::<U>() for indirect relations
         // to make the indirect Deref more explicit
 
-        use crate::obj::Share;
-
         let path = path.into();
-        let node = self.share().upcast::<Node>();
+        let node = self.clone().upcast::<Node>();
 
         <Node as NodeExt>::try_get_node_as(&*node, path)
     }

godot-core/src/obj/gd.rs

@@ -39,7 +39,7 @@ use crate::{callbacks, engine, out};
 /// In particular, the memory management strategy is fully dependent on `T`:
 ///
 /// * Objects of type [`RefCounted`] or inherited from it are **reference-counted**. This means that every time a smart pointer is
-///   shared using [`Share::share()`], the reference counter is incremented, and every time one is dropped, it is decremented.
+///   shared using [`Clone::clone()`], the reference counter is incremented, and every time one is dropped, it is decremented.
 ///   This ensures that the last reference (either in Rust or Godot) will deallocate the object and call `T`'s destructor.
 ///
 /// * Objects inheriting from [`Object`] which are not `RefCounted` (or inherited) are **manually-managed**.
@@ -309,7 +309,7 @@ impl<T: GodotClass> Gd<T> {
     /// struct MyClass {}
     ///
     /// let obj: Gd<MyClass> = Gd::new_default();
-    /// let base = obj.share().upcast::<Node>();
+    /// let base = obj.clone().upcast::<Node>();
     /// ```
     pub fn upcast<Base>(self) -> Gd<Base>
     where
@@ -462,7 +462,7 @@ impl<T: GodotClass> Gd<T> {
 
     /// Returns a callable referencing a method from this object named `method_name`.
     pub fn callable<S: Into<StringName>>(&self, method_name: S) -> Callable {
-        Callable::from_object_method(self.share(), method_name)
+        Callable::from_object_method(self.clone(), method_name)
     }
 }
 
@@ -590,7 +590,7 @@ where
         if T::Mem::pass_as_ref(call_type) {
             interface_fn!(ref_set_object)(ptr as sys::GDExtensionRefPtr, self.obj_sys())
         } else {
-            std::ptr::write(ptr as *mut _, self.opaque)
+            ptr::write(ptr as *mut _, self.opaque)
         }
         // We've passed ownership to caller.
         std::mem::forget(self);
@@ -709,10 +709,16 @@ impl<T: GodotClass> Drop for Gd<T> {
     }
 }
 
+impl<T: GodotClass> Clone for Gd<T> {
+    fn clone(&self) -> Self {
+        out!("Gd::clone");
+        Self::from_opaque(self.opaque).with_inc_refcount()
+    }
+}
+
 impl<T: GodotClass> Share for Gd<T> {
     fn share(&self) -> Self {
-        out!("Gd::share");
-        Self::from_opaque(self.opaque).with_inc_refcount()
+        self.clone()
     }
 }
 
@@ -739,7 +745,7 @@ impl<T: GodotClass> Property for Gd<T> {
     type Intermediate = Self;
 
     fn get_property(&self) -> Self {
-        self.share()
+        self.clone()
     }
 
     fn set_property(&mut self, value: Self) {

godot-core/src/obj/traits.rs

@@ -71,6 +71,7 @@ pub trait Share {
     /// Creates a new reference that points to the same object.
     ///
     /// If the referred-to object is reference-counted, this will increment the count.
+    #[deprecated = "Replaced with `Clone::clone()`."]
     fn share(&self) -> Self;
 }
 

godot/src/lib.rs

@@ -12,7 +12,13 @@
 //!
 //! Godot is written in C++, which doesn't have the same strict guarantees about safety and
 //! mutability that Rust does. As a result, not everything in this crate will look and feel
-//! entirely "rusty". We distinguish four different kinds of types:
+//! entirely "rusty".
+//!
+//! Traits such as `Clone`, `PartialEq` or `PartialOrd` are designed to mirror Godot semantics,
+//! except in cases where Rust is stricter (e.g. float ordering). Cloning a type results in the
+//! same observable behavior as assignment or parameter-passing of a GDScript variable.
+//!
+//! We distinguish four different kinds of types:
 //!
 //! 1. **Value types**: `i64`, `f64`, and mathematical types like
 //!    [`Vector2`][crate::builtin::Vector2] and [`Color`][crate::builtin::Color].
@@ -43,11 +49,9 @@
 //!    careful when using such types. For example, when iterating over an `Array`, make sure that
 //!    it isn't being modified at the same time through another reference.
 //!
-//!    To avoid confusion these types don't implement `Clone`. You can use
-//!    [`Share`][crate::obj::Share] to create a new reference to the same instance, and
-//!    type-specific methods such as
-//!    [`Array::duplicate_deep()`][crate::builtin::Array::duplicate_deep] to make actual
-//!    copies. <br><br>
+//!    `Clone::clone()` on these types creates a new reference to the same instance, while
+//!    type-specific methods such as [`Array::duplicate_deep()`][crate::builtin::Array::duplicate_deep]
+//!    can be used to make actual copies. <br><br>
 //!
 //! 4. **Manually managed types**: [`Gd<T>`][crate::obj::Gd] where `T` inherits from
 //!    [`Object`][crate::engine::Object] but not from [`RefCounted`][crate::engine::RefCounted];

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

@@ -99,7 +99,7 @@ fn array_hash() {
 #[itest]
 fn array_share() {
     let mut array = array![1, 2];
-    let shared = array.share();
+    let shared = array.clone();
     array.set(0, 3);
     assert_eq!(shared.get(0), 3);
 }
@@ -372,7 +372,7 @@ fn untyped_array_return_from_godot_func() {
     let mut node = Node::new_alloc();
     let mut child = Node::new_alloc();
     child.set_name("child_node".into());
-    node.add_child(child.share());
+    node.add_child(child.clone());
     node.queue_free(); // Do not leak even if the test fails.
     let result = node.get_node_and_resource("child_node".into());
 
@@ -409,7 +409,7 @@ fn typed_array_return_from_godot_func() {
     let mut node = Node::new_alloc();
     let mut child = Node::new_alloc();
     child.set_name("child_node".into());
-    node.add_child(child.share());
+    node.add_child(child.clone());
     node.queue_free(); // Do not leak even if the test fails.
     let children = node.get_children();
 
@@ -419,7 +419,7 @@ fn typed_array_return_from_godot_func() {
 #[itest]
 fn typed_array_try_from_untyped() {
     let node = Node::new_alloc();
-    let array = VariantArray::from(&[node.share().to_variant()]);
+    let array = VariantArray::from(&[node.clone().to_variant()]);
     assert_eq!(
         array.to_variant().try_to::<Array<Option<Gd<Node>>>>(),
         Err(VariantConversionError::BadType)
@@ -430,7 +430,7 @@ fn typed_array_try_from_untyped() {
 #[itest]
 fn untyped_array_try_from_typed() {
     let node = Node::new_alloc();
-    let array = Array::<Option<Gd<Node>>>::from(&[Some(node.share())]);
+    let array = Array::<Option<Gd<Node>>>::from(&[Some(node.clone())]);
     assert_eq!(
         array.to_variant().try_to::<VariantArray>(),
         Err(VariantConversionError::BadType)

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

@@ -8,7 +8,7 @@ use godot::bind::{godot_api, GodotClass};
 use godot::builtin::inner::InnerCallable;
 use godot::builtin::{varray, Callable, GodotString, StringName, ToVariant, Variant};
 use godot::engine::{Node2D, Object};
-use godot::obj::{Gd, Share};
+use godot::obj::Gd;
 
 use crate::framework::itest;
 
@@ -66,7 +66,7 @@ fn callable_object_method() {
     let obj = Gd::<CallableTestObj>::new_default();
     let callable = obj.callable("foo");
 
-    assert_eq!(callable.object(), Some(obj.share().upcast::<Object>()));
+    assert_eq!(callable.object(), Some(obj.clone().upcast::<Object>()));
     assert_eq!(callable.object_id(), Some(obj.instance_id()));
     assert_eq!(callable.method_name(), Some("foo".into()));
 
@@ -109,7 +109,7 @@ fn callable_call_return() {
 #[itest]
 fn callable_call_engine() {
     let obj = Node2D::new_alloc();
-    let cb = Callable::from_object_method(obj.share(), "set_position");
+    let cb = Callable::from_object_method(obj.clone(), "set_position");
     let inner: InnerCallable = cb.as_inner();
 
     assert!(!inner.is_null());