DerefMut on Gd pointer may be used to break subtyping relations

[chitoyuu]

The following code snippet can be used to put an arbitrary Node into a Gd<Node3D>:

let mut a: Gd<Node> = ...;
let mut b: Gd<Node3D> = ...;

mem::swap(&mut *a, &mut *b); // hm...

[lilizoey]

So the issue here is that while shared references are covariant, mutable references are invariant. So if we want our subtyping relationship to be correct, then we cannot implement DerefMut like we currently do. I.e if A is a subtype of B, then we cannot implement DerefMut on A with B as the target. I dont think this will necessarily cause UB at the moment, but im not sure. However if we keep this then we cannot rely on Gd<T> being a pointer to a valid T and we will need to runtime check this anywhere we need this to be true.

So as i see it there are a couple of options here:

1. Keep this as is
2. Remove DerefMut impls on subtypes
3. Stop using Deref/DerefMut entirely for subtyping

1 will require us to now reexamine our usage of Gd and see if we every rely on Gd<T> actually being a pointer to a T. And possibly add runtime checks where needed.

2 will require rewriting some functions (such as notification), and possibly worse ergonomics than currently. But it might not be too bad overall, hard to say without trying it.

3 will require a big rewrite, and it's a bit unclear what the best method of supporting subtyping then is. it's likely that we'd need more code duplication than currently. how much is unclear to me at the moment. traits with default methods and blanket impls seem feasible to me, but im not fully convinced of it.

[Bromeon]

I went for approach 1) in the above list: we keep the behavior as-is, but do runtime checks + panics to prevent UB.
See PR #546 for details.

Rationale: I was the one to originally bring up this scenario on Discord, and in more than a year of gdext usage, I haven't seen a single user reporting this problem. It's unlikely to happen in practice, and for the handful of cases where it may happen, runtime checks are good enough. We can disable them in Release mode.

Why not enforce this via type system, by only providing Deref? Because it comes at quite a cost: we can no longer use const-correctness for Godot types and would need to use &self receivers everywhere. While const-correctness is currently a hint rather than critical for safety and Godot's API is not 100% perfect in this regard, the distinction of &self and &mut self in Godot methods has been very useful overall. It makes code more explicit and allows IDEs to highlight modifications more easily.

With our declared focus on pragmatism, I believe retaining DerefMut is the better choice. Technically this issue is not solved, as it's at least theoretically possible to have Gd<T> with a mismatching runtime type, so I'll close this as wontfix. In practice, such cases are rare to occur accidentally, and easy to detect through the debug checks. Situations where this can still cause UB should be considered bugs and fixed.