Merge godot-rust#116

116: Make `Inherits<T>` reflexive, with implied `GodotClass` bound r=Bromeon a=Bromeon

Previously, it was hard to express polymorphism such as "anything that is a Node", including both Node and derived classes (for example in function parameters).

This is now straightforward and also doesn't require a GodotClass bound anymore:

```rs
fn print_node<T>(node: Gd<T>)
    where T: Inherits<Node>
{
    let node = node.upcast(); // Gd<Node> inferred
    ... // use it
}
```

Also adds a couple of tests, as well as a minor panic improvement when invoking Godot from test/doctest.

Co-authored-by: Jan Haller <[email protected]>

godot-core/src/obj/traits.rs

@@ -48,13 +48,53 @@ pub trait Share {
     fn share(&self) -> Self;
 }
 
-/// A struct `Derived` implementing `Inherits<Base>` expresses that `Derived` _strictly_ inherits `Base` in the Godot hierarchy.
+/// Non-strict inheritance relationship in the Godot class hierarchy.
 ///
-/// This trait is implemented for all Godot engine classes, even for non-direct relations (e.g. `Node3D` implements `Inherits<Object>`). Deriving [`GodotClass`] for custom classes will achieve the same: all direct and indirect base
-/// classes of your extension class will be wired up using the `Inherits` relation.
+/// `Derived: Inherits<Base>` means that either `Derived` is a subclass of `Base`, or the class `Base` itself (hence "non-strict").
 ///
-/// The trait is not reflexive: `T` never implements `Inherits<T>`.
-pub trait Inherits<Base> {}
+/// This trait is automatically implemented for all Godot engine classes and user-defined classes that derive [`GodotClass`].
+/// It has `GodotClass` as a supertrait, allowing your code to have bounds solely on `Derived: Inherits<Base>` rather than
+/// `Derived: Inherits<Base> + GodotClass`.
+///
+/// Inheritance is transitive across indirect base classes: `Node3D` implements `Inherits<Node>` and `Inherits<Object>`.
+///
+/// The trait is also reflexive: `T` always implements `Inherits<T>`.
+///
+/// # Usage
+///
+/// The primary use case for this trait is polymorphism: you write a function that accepts anything that derives from a certain class
+/// (including the class itself):
+/// ```no_run
+/// # use godot::prelude::*;
+/// fn print_node<T>(node: Gd<T>)
+/// where
+///     T: Inherits<Node>,
+/// {
+///     let up = node.upcast(); // type Gd<Node> inferred
+///     println!("Node #{} with name {}", up.instance_id(), up.get_name());
+///     up.free();
+/// }
+///
+/// // Call with different types
+/// print_node(Node::new_alloc());   // works on T=Node as well
+/// print_node(Node2D::new_alloc()); // or derived classes
+/// print_node(Node3D::new_alloc());
+/// ```
+///
+/// A variation of the above pattern works without `Inherits` or generics, if you move the `upcast()` into the call site:
+/// ```no_run
+/// # use godot::prelude::*;
+/// fn print_node(node: Gd<Node>) { /* ... */ }
+///
+/// // Call with different types
+/// print_node(Node::new_alloc());            // no upcast needed
+/// print_node(Node2D::new_alloc().upcast());
+/// print_node(Node3D::new_alloc().upcast());
+/// ```
+///
+pub trait Inherits<Base>: GodotClass {}
+
+impl<T: GodotClass> Inherits<T> for T {}
 
 /// Auto-implemented for all engine-provided classes
 pub trait EngineClass: GodotClass {

godot-ffi/src/lib.rs

@@ -109,18 +109,30 @@ pub unsafe fn get_registry() -> &'static mut GlobalRegistry {
     &mut unwrap_ref_unchecked_mut(&mut BINDING).registry
 }
 
+/// Makes sure that Godot is running, or panics. Debug mode only!
+macro_rules! debug_assert_godot {
+    ($expr:expr) => {
+        debug_assert!(
+            $expr,
+            "Godot engine not available; make sure you are do not call it from unit/doc tests"
+        ); // previous message: "unchecked access to Option::None"
+    };
+}
+
 /// Combination of `as_ref()` and `unwrap_unchecked()`, but without the case differentiation in
 /// the former (thus raw pointer access in release mode)
 unsafe fn unwrap_ref_unchecked<T>(opt: &Option<T>) -> &T {
-    debug_assert!(opt.is_some(), "unchecked access to Option::None");
+    debug_assert_godot!(opt.is_some());
+
     match opt {
         Some(ref val) => val,
         None => std::hint::unreachable_unchecked(),
     }
 }
 
 unsafe fn unwrap_ref_unchecked_mut<T>(opt: &mut Option<T>) -> &mut T {
-    debug_assert!(opt.is_some(), "unchecked access to Option::None");
+    debug_assert_godot!(opt.is_some());
+
     match opt {
         Some(ref mut val) => val,
         None => std::hint::unreachable_unchecked(),

itest/rust/src/object_test.rs

@@ -7,9 +7,9 @@
 use crate::{expect_panic, itest};
 use godot::bind::{godot_api, GodotClass, GodotExt};
 use godot::builtin::{FromVariant, GodotString, StringName, ToVariant, Variant, Vector3};
-use godot::engine::{file_access, FileAccess, Node, Node3D, Object, RefCounted};
-use godot::obj::Share;
+use godot::engine::{file_access, Camera3D, FileAccess, Node, Node3D, Object, RefCounted};
 use godot::obj::{Base, Gd, InstanceId};
+use godot::obj::{Inherits, Share};
 use godot::sys::GodotFfi;
 
 use std::cell::RefCell;
@@ -42,11 +42,15 @@ pub fn run() -> bool {
     ok &= object_engine_up_deref();
     ok &= object_engine_up_deref_mut();
     ok &= object_engine_upcast();
+    ok &= object_engine_upcast_reflexive();
     ok &= object_engine_downcast();
+    ok &= object_engine_downcast_reflexive();
     ok &= object_engine_bad_downcast();
+    ok &= object_engine_accept_polymorphic();
     ok &= object_user_upcast();
     ok &= object_user_downcast();
     ok &= object_user_bad_downcast();
+    ok &= object_user_accept_polymorphic();
     ok &= object_engine_manual_free();
     ok &= object_engine_manual_double_free();
     ok &= object_engine_refcounted_free();
@@ -314,7 +318,19 @@ fn object_engine_upcast() {
     assert_eq!(object.instance_id(), id);
     assert_eq!(object.get_class(), GodotString::from("Node3D"));
 
-    // Deliberate free on upcast obj
+    // Deliberate free on upcast object
+    object.free();
+}
+
+#[itest]
+fn object_engine_upcast_reflexive() {
+    let node3d: Gd<Node3D> = Node3D::new_alloc();
+    let id = node3d.instance_id();
+
+    let object = node3d.upcast::<Node3D>();
+    assert_eq!(object.instance_id(), id);
+    assert_eq!(object.get_class(), GodotString::from("Node3D"));
+
     object.free();
 }
 
@@ -335,6 +351,17 @@ fn object_engine_downcast() {
     node3d.free();
 }
 
+#[itest]
+fn object_engine_downcast_reflexive() {
+    let node3d: Gd<Node3D> = Node3D::new_alloc();
+    let id = node3d.instance_id();
+
+    let node3d: Gd<Node3D> = node3d.cast::<Node3D>();
+    assert_eq!(node3d.instance_id(), id);
+
+    node3d.free();
+}
+
 #[itest]
 fn object_engine_bad_downcast() {
     let object: Gd<Object> = Object::new_alloc();
@@ -345,6 +372,59 @@ fn object_engine_bad_downcast() {
     free_ref.free();
 }
 
+#[itest]
+fn object_engine_accept_polymorphic() {
+    let mut node = Camera3D::new_alloc();
+    let expected_name = StringName::from("Node name");
+    let expected_class = GodotString::from("Camera3D");
+
+    node.set_name(GodotString::from(&expected_name));
+
+    let actual_name = accept_node(node.share());
+    assert_eq!(actual_name, expected_name);
+
+    let actual_class = accept_object(node.share());
+    assert_eq!(actual_class, expected_class);
+
+    node.free();
+}
+
+#[itest]
+fn object_user_accept_polymorphic() {
+    let obj = Gd::new(ObjPayload { value: 123 });
+    let expected_class = GodotString::from("ObjPayload");
+
+    let actual_class = accept_refcounted(obj.share());
+    assert_eq!(actual_class, expected_class);
+
+    let actual_class = accept_object(obj);
+    assert_eq!(actual_class, expected_class);
+}
+
+fn accept_node<T>(node: Gd<T>) -> StringName
+where
+    T: Inherits<Node>,
+{
+    let up = node.upcast();
+    up.get_name()
+}
+
+fn accept_refcounted<T>(node: Gd<T>) -> GodotString
+where
+    T: Inherits<RefCounted>,
+{
+    let up = node.upcast();
+    up.get_class()
+}
+
+fn accept_object<T>(node: Gd<T>) -> GodotString
+where
+    T: Inherits<Object>,
+{
+    let up = node.upcast();
+    up.get_class()
+}
+
 #[itest]
 fn object_user_upcast() {
     let obj = user_object();
@@ -402,7 +482,7 @@ fn object_engine_manual_double_free() {
 #[itest]
 fn object_engine_refcounted_free() {
     let node = RefCounted::new();
-    let node2 = node.share().upcast();
+    let node2 = node.share().upcast::<Object>();
 
     expect_panic("calling free() on RefCounted obj", || node2.free())
 }