Share trait: deprecate + replace with Clone

[Bromeon]

Addresses part of #297.

What I'm not yet sure is whether the impl Property will still be needed for Gd, Array and Dictionary, now that they support cloning.

---

In a 2nd step, I would like to revisit the names of the Gd constructors. Some ideas:

• new_default -> with_init, new, ...
• new -> from_standalone, from_object, manage, ...
• with_base -> from_base_fn, from_base_ctor, ...

I don't want to implement Default for now due to the risk of manually managed types leaking.

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-409

[bluenote10]

Personally, I always thought it is nice that we have .share() instead of .clone(), and I actually often wished Rust's smart pointer would also use .share() instead of .clone() 😉 . It just makes things less ambiguous, i.e., one cannot confuse duplicating a pointer from duplicating actual content. And it doesn't split .clone() calls into things that are very cheap and things that are rather expensive. With .share() I never had to wonder if that's a "real" clone...

But I guess that's just what the ecosystem demands...

[Bromeon]

I somewhat agree, which is why I added Share initially 🙂

Clone makes interop a bit easier though, and is more consistent with Variant::clone.

What do you think about the arguments in #297 (comment) and following?

[bluenote10]

What do you think about the arguments in #297 (comment) and following?

I don't really have a strong opinion on this 😉

[Ogeon]

I suppose it depends on one's perspective. The data is shared by cloning the reference, but I understand the ambiguity. Clone is what it became. Gd could be seen as similar to Rc and Arc in that sense, except that it may or may not count the references. I guess if it's not reference counted, It may or may not be intended to be shared on clone. 🤔 That's currently all in the programmer's head and not encoded in the type.

Personally, though, I'm not against having a share method (and other options) for clarity, I just miss having the Clone trait with an ok default behavior. 🙂 The inconvenience can be reduced on my end with a custom SharedGd<T> wrapper, otherwise. 🤷

[Bromeon]

I also thought making "sharing" explicit was nice -- but we can't fully escape that it is treated like a copy by Godot.
This concerns Gd<T>, Array<T> and Dictionary equally.

# GDScript
let a: Type = [1, 2, 3]
let b = a    # same syntax for copy _and_ share

# Type = PackedInt32Array -> copy
# Type = Array            -> share

Also in Rust:

let a: VariantArray = varray![1, 2, 3];
let b = a.share();

// but:
let a = Variant::from(a);
let b = a.clone();

Same semantics, different method.

The only thing I could possibly imagine is to have both:

• Clone uses "Godot copy semantics" (value copy, copy-on-write, ref-counting, pointer copy).
• Share ensures that no content is copied (only ref-counting and pointer copy).
  • Implemented for Gd<T>, Array<T>, Dictionary
  • Possibly also for Variant (but panicking for other contained type)? I don't want to go down a TryShare route...

But is Share truly useful? I expect it to appear much less often once we have argument inference, aka param: impl Into<ParamType>.

Then there's also a question of expressing deep-copies somehow -- some Godot types have a duplicate(bool deep=false) (arrays, dictionaries) or duplicate(int flags) method.

This conundrum is not new either: godot-rust/gdnative#712 🙉

[lilizoey]

A couple of things that having Clone enables are:

• Option::cloned
• Iterator::cloned

These are ones i've stumbled upon wanting but not being able to use, like how do we properly clone a Option<Gd<T>>? With only Share this gets a bit tricky, extension trait is probably the best solution. But by implementing Clone we can just use clone/cloned.

And several other similar methods that don't easily work with Share. Of course we could work around this by providing our own explicit impls here, but if other libraries have their own types with similar methods then this doesn't really scale.

In addition having Clone lets us #[derive(Clone)] for types containing these. Though we could again work-around this by providing a #[derive(Share)], but then we need to figure out what to do for types from third party crates that don't implement Share.

[Ogeon]

I agree that Clone is such a fundamental building block (or glue?) in the Rust ecosystem that it's hard to replace it or try to "fix" it with only Share. I gave that kind of situation a lot of thought when I realized that the traits in num_traits don't work exactly as I would have wished them to, and realized that a utility trait like that is most useful in its own crate that everything has to depend on. Otherwise, you end up having to make workarounds for third party types, as @lilizoey mentioned. But I'm not sure Share can be derived in a meaningful way either (what does Share do with a bool?), so Clone is still required for least #[derive(Clone)] to work without adapter wrappers.

Clone is essentially "recursively share or copy", either selected depending on the encountered data type. Share is a specific subset or an implementation of Clone, so one doesn't exclude the other. In fact, there could be another trait that explicitly only makes a deep copy as well, that's not available for shared references. As for if any of those are better than an explicit T::clone(...) call, I don't know. Better for call chaining? A bit nicer to read, maybe?

[Bromeon]

Thanks for everyone's input. It seems like there is consensus that having Clone makes gdext types more versatile in the Rust ecosystem, but I'm also acknowledging that there might be minor confusion regarding it not actually "cloning" things.

As such, I'm going to merge this and deprecate Share for the time being. If there is demand for more specialized traits (Share as it exists now, DeepCopy/ShallowCopy as variants of duplicate, or directly Duplicate taking parameters...), we could then discuss that separately.