Replace Deref/DerefMut with traits using supertraits for subtyping

[lilizoey]

TODO: proper writeup

Makes objects use traits with supertraits for inheritance.

see also this discord thread.

closes #23

provides a different solution to #131 where we now just implement each AsX trait on user-defined classes that have a base field.

also splits up godot-core/src/gen/classes/mod.rs a bit which hopefully makes IDEs and stuff a bit happier when looking at the file.

[GodotRust]

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

[Bromeon]

Thanks a lot for experimenting with alternative approaches! Really cool to see you got something working 🙂

Maybe a small usage example in the intro text would help (I guess that falls under your TODO 😉), possibly with and without prelude.

---

This generates a lot of symbols:

• for each class 1 AsClass trait
• for each class N As* impls, where N is the number of base classes
• something else?

It would be interesting to see the impact on compile times, especially since we already have some problems with those...

---

One disadvantage I see is that now, all the methods are in traits rather than inherent. This means that a class is no longer self-contained, but all related traits need to be explicitly imported.

It also means that the documentation is now quite a bit harder to read, because the most-derived methods are no longer at the beginning, but in an impl somewhere down (ordered alphabetically).

master | #278

---

It's nice that it solves #23. Apart from that, where do you see the biggest advantages of this approach vs. a class-based one? Because from a user perspective, it does add some complexity for everyday programming (needing to know 2 symbols instead of 1 per class, and how they relate, plus the imports).

[lilizoey]

Thanks a lot for experimenting with alternative approaches! Really cool to see you got something working 🙂

Maybe a small usage example in the intro text would help (I guess that falls under your TODO 😉), possibly with and without prelude.

This generates a lot of symbols:

* for each class 1 `AsClass` trait

* for each class N `As*` impls, where N is the number of base classes

* something else?

It would be interesting to see the impact on compile times, especially since we already have some problems with those...

One disadvantage I see is that now, all the methods are in traits rather than inherent. This means that a class is no longer self-contained, but all related traits need to be explicitly imported.

It also means that the documentation is now quite a bit harder to read, because the most-derived methods are no longer at the beginning, but in an impl somewhere down (ordered alphabetically).

master | #278

It's nice that it solves #23. Apart from that, where do you see the biggest advantages of this approach vs. a class-based one? Because from a user perspective, it does add some complexity for everyday programming (needing to know 2 symbols instead of 1 per class, and how they relate, plus the imports).

This adds 2N new symbols, one AsClass for each class, which needs to be exported in the prelude for ergonomic reasons. and one impl_as_Class macro. for impls. each class now has one impl for each reflexive superclass, though this is rarely more than 5. but we do remove deref/derefmut impls. so it's likely closer to a 2-3 ish increase in number of impls per class.

i agree that these are all concerns. im still not sure about all the benefits/disadvantages of this approach, beyond fixing #23. im gonna take a closer look at it all. though im gonna be a bit busy the rest of this week and probably gonna prioritize finalizing #275 first.

a couple of thoughts regarding benefits though. for one the subtyping relationships now actually lives within the rust type system properly, which to me seems like an advantage. and now every class clearly shows every function it inherits from superclasses, rather than having it come from Deref/DerefMut. in addition it's easier to create trait bounds for classes.

There's also the development benefit of having Gd<T> provide the safety guarantee that Gd<T> is in fact a smart-pointer to a T. Which is certainly easier to reason about than if we provide DerefMut impls. And we mostly dont lose the ergonomics that keeping Deref but removing DerefMut implies.

There's also the benefit of no longer using Deref/DerefMut in ways that are generally seen as unidiomatic among the rust community. Which has a learnability and understandability benefit.

im also very curious about compile times. subjectively it doesn't seem to have much impact so it's likely not very different from the previous solution. im unsure if splitting up the classes/mod.rs did much. though it seemed to take somewhat less time for rust analyzer to finish check for broken builds.

Also thinking about it, notify and notify_reverse can probably be moved into AsObject. Thinking about it again, we could actually implement notify and notify_reverse as extension traits that are blanket implemented for anything implementing AsObject. Which is an advantage of this method. Though whether we want to or not is a bit more unclear. But doing this in general, implementing extension traits for AsClass to add functionality on top of godot is useful. And means we dont have to dive in to the codegen to add new functionality as often. It also lets library developers easily add functionality to existing godot classes if they want. This can be done with impl<T: Inherits<U>> Trait for Gd<T>, but this has some ergonomics issues last i checked, which this solution should avoid. (largely you need to go through base every time).

But that last point can probably be sorted out using Deref/DerefMut somehow. The main advantage is that we mostly get it for free with this method.

I kinda feel like it'd be useful to do some user-testing for these when this PR is more developed. Because it's a bit unclear to me which one is more understandable at this point.

[Bromeon]

Thanks a lot for the great answer! 👍

I also think it's hard to make such a big decision without using it in real-world code for some time. However, there may be quite some maintenance effort involved to keep this PR open and mergeable (there is #272, I also plan to do some codegen changes in the next days, etc). Maybe this would be easier if changes that are not directly related to the model (such as moving classes into engine::classes) would be removed from the core PR for now? Just to reduce the number of lines that can merge-conflict...

[lilizoey]

definitely, i can probably make a separate PR for splitting up mod.rs a bit too so we can see if it has any benefits on its own.

[lilizoey]

update: it seems like compile times are largely unchanged, it's possible this solution is slightly slower (around 2%) but it's a bit hard to be confident especially when a clean check/build takes a while. it's possible that some of the compile time differences are improved by splitting up mod.rs like initially but it also seems like it's a very minor effect if it is there.

[Bromeon]

However, there may be quite some maintenance effort involved to keep this PR open and mergeable

That's exactly what happened 😬

But before spending effort on rebasing, we'd need to have a compelling reason to even consider switching.
My previous concerns still apply:

It's nice that it solves #23. Apart from that, where do you see the biggest advantages of this approach vs. a class-based one? Because from a user perspective, it does add some complexity for everyday programming (needing to know 2 symbols instead of 1 per class, and how they relate, plus the imports).

In other words, do we gain something for day-to-day game development? To me it seems usage of gdext becomes more complex with the trait-based approach, not simpler. Solving #23 is nice, but that issue alone is not critical enough to warrant ergonomic downgrades. (I'm not expecting to have ready answers for the above; as mentioned previously, we'd likely need some real-world usage).

If we don't plan to make progress, we should maybe close the PR, or just have another branch to work on.

[lilizoey]

However, there may be quite some maintenance effort involved to keep this PR open and mergeable

That's exactly what happened grimacing

But before spending effort on rebasing, we'd need to have a compelling reason to even consider switching. My previous concerns still apply:

It's nice that it solves #23. Apart from that, where do you see the biggest advantages of this approach vs. a class-based one? Because from a user perspective, it does add some complexity for everyday programming (needing to know 2 symbols instead of 1 per class, and how they relate, plus the imports).

In other words, do we gain something for day-to-day game development? To me it seems usage of gdext becomes more complex with the trait-based approach, not simpler. Solving #23 is nice, but that issue alone is not critical enough to warrant ergonomic downgrades. (I'm not expecting to have ready answers for the above; as mentioned previously, we'd likely need some real-world usage).

If we don't plan to make progress, we should maybe close the PR, or just have another branch to work on.

yeah i'm fine with closing it for now, it's at least good to know that this works. we can come back to it later probably.

[lilizoey]

This could probably be used to fix #348 without making free unsafe, would need a fair bit of other stuff to make it nice though. But with this we could:

1. Remove Deref and DerefMut for Gd<T>.

2. Have an unsafe Gd::as_inner_unchecked()

3. Have each Gd<T> implement *Trait for that class, something like:

fn foo(&self, arg1...) {
    if !self.is_instance_valid() {
        panic!("attempted to dereference freed object")
    }
    unsafe { self.as_inner_unchecked().foo(arg1, ...) }
}