`GodotClass` should be an unsafe trait

[lilizoey]

If you implement GodotClass for T with Declarer = EngineDomain, then you can use Deref to transmute a &OpaqueObject to a &T. It is the responsibility of the implementer of GodotClass to ensure that this is a safe transmute.

See:

impl<T> Deref for Gd<T>
where
    T: GodotClass<Declarer = dom::EngineDomain>,
{
    type Target = T;

    fn deref(&self) -> &Self::Target {
        // SAFETY:
        // This relies on Gd<Node3D> having the layout as Node3D (as an example),
        // which also needs #[repr(transparent)]:
        //
        // struct Gd<T: GodotClass> {
        //     opaque: OpaqueObject,         <- size of GDExtensionObjectPtr
        //     _marker: PhantomData,         <- ZST
        // }
        // struct Node3D {
        //     object_ptr: sys::GDExtensionObjectPtr,
        // }
        unsafe { std::mem::transmute::<&OpaqueObject, &T>(&self.opaque) }
    }
}

[Bromeon]

If you implement GodotClass for T with Declarer = EngineDomain

This trait is not intended to be implemented manually. You need several internal symbols, which means it's not a supported workflow. The documentation also mentions it; could be worded even more strictly:

You wouldn’t usually implement this trait yourself; use the GodotClass derive macro instead.

If we ever want to support manual implementation, this needs to be fleshed out alongside #4.

As to the unsafeness, maybe. But it's an implementation detail that will likely confuse users when they see it. #[derive(GodotClass)] is not unsafe after all. And if we provide a public way to implement it, we should try to make it safe 🤔

[lilizoey]

If you implement GodotClass for T with Declarer = EngineDomain

This trait is not intended to be implemented manually. You need several internal symbols, which means it's not a supported workflow. The documentation also mentions it; could be worded even more strictly:

You wouldn’t usually implement this trait yourself; use the GodotClass derive macro instead.

If we ever want to support manual implementation, this needs to be fleshed out alongside #4.

Whether we support manually implementing it or not is kinda irrelevant. The trait can be implemented manually, and cause UB even while upholding all other safety invariants (perhaps not the blanket unsafe on the unsafe impl ExtensionLibrary .. if we lump "dont implement GodotClass manually" in with that). So it should be marked unsafe unless we ensure that you can't trigger UB by implementing it.

In fact "you should not implement this trait manually" when implementing the trait manually can cause UB is definitely a good reason to make it an unsafe trait. Because that is a safety invariant for implementing the trait.

As to the unsafeness, maybe. But it's an implementation detail that will likely confuse users when they see it. #[derive(GodotClass)] is not unsafe after all. And if we provide a public way to implement it, we should try to make it safe thinking

I've definitely seen traits before that are marked unsafe, but are safe to implement using derives.

[lilizoey]

Here is an example:

// imports...
struct Testing;

#[gdextension]
unsafe impl ExtensionLibrary for Testing {}

#[derive(Debug, Copy, Clone)]
pub struct Foo {
    a: u64,
    b: u64,
    c: u64,
}

impl ::godot::obj::GodotClass for Foo {
    type Base = ::godot::engine::RefCounted;
    type Declarer = ::godot::obj::dom::EngineDomain;
    type Mem = <Self::Base as ::godot::obj::GodotClass>::Mem;
    const CLASS_NAME: &'static str = "Foo";
}
// The rest that `#[derive(GodotClass)]` would generate with generated `init`. (contains no unsafe)
#[godot_api]
impl Foo {
    #[func]
    fn do_ub() {
        let object_ptr = unsafe { callbacks::create::<Foo>(std::ptr::null_mut()) };
        let foo: Gd<Foo> = unsafe { Gd::from_obj_sys(object_ptr) };

        let bar: Foo = *foo;

        godot_print!("{bar:?}");
    }
}

This code triggers UB, but which unsafe block here is it we violate safety invariants in?

1. unsafe impl ExtensionLibrary for Testing {}? Works, but i dont think we should add too much stuff to the safety invariant for that trait. And it seems very off to have "dont implement these traits manually" as safety invariants in here. My understanding was that this unsafe was mainly for guaranteeing you didn't violate safety invariants from godot. Not for cases where the UB occurs entirely in rust code.

2. unsafe { callbacks::create::<Foo>(std::ptr::null_mut()) }? Maybe, but what is the safety invariant that we violate here? "do not call this method unless the GodotClass implementation for T is UserDomain? Then why doesn't it require that in the signature? We could also inline the entire function, and make all the ffi calls manually and at that point it's even more unclear what safety invariant we violate. And it'd still depend on how you decide to implement GodotClass for T.

Or maybe it is that we shouldn't have implemented GodotInit? That seems to go against its description, but also would mean that GodotInit should be an unsafe trait.

3. unsafe { Gd::from_obj_sys(object_ptr) }? Similar to above. I guess we are violating the from_sys invariant of:

ptr must be a valid type ptr: it must follow Godot's convention to encode Self, which is different depending on the type.

But why does whether we choose to implement GodotClass with EngineDomain or UserDomain change the way which Godot encodes Gd<T>?

In the end it seems most natural to me that we make it a safety invariant of GodotClass that you only choose EngineDomain for the Declarer if the type in question actually just has the same layout and semantics as the code genned godot classes. The other options just seem very non-local and hard to reason about. And if you ever make a mistake like this, the solution is almost always going to be "you shouldn't have implemented GodotClass that way" or "since you implemented GodotClass that way you can't do that". I.e GodotClass has certain safety invariants that you must uphold to safely implement it.

Full Code

use godot::{
    prelude::{meta::ClassName, *},
    private::callbacks,
    sys,
};
struct Testing;

#[gdextension]
unsafe impl ExtensionLibrary for Testing {}

#[derive(Debug, Copy, Clone)]
pub struct Foo {
    a: u64,
    b: u64,
    c: u64,
}

impl ::godot::obj::GodotClass for Foo {
    type Base = ::godot::engine::RefCounted;
    type Declarer = ::godot::obj::dom::EngineDomain;
    type Mem = <Self::Base as ::godot::obj::GodotClass>::Mem;
    const CLASS_NAME: &'static str = "Foo";
}
impl ::godot::obj::cap::GodotInit for Foo {
    fn __godot_init(base: ::godot::obj::Base<Self::Base>) -> Self {
        Self {
            a: ::std::default::Default::default(),
            b: ::std::default::Default::default(),
            c: ::std::default::Default::default(),
        }
    }
}
impl Foo {}

impl ::godot::obj::cap::ImplementsGodotExports for Foo {
    fn __register_exports() {}
}
::godot::sys::plugin_add!(__GODOT_PLUGIN_REGISTRY in::godot::private;
::godot::private::ClassPlugin {
  class_name:"Foo",component: ::godot::private::PluginComponent::ClassDef {
    base_class_name: < ::godot::engine::RefCounted as ::godot::obj::GodotClass> ::CLASS_NAME,generated_create_fn:Some(::godot::private::callbacks::create:: <Foo>),free_fn: ::godot::private::callbacks::free:: <Foo> ,
  },
});
::godot::private::class_macros::inherits_transitive_RefCounted!(Foo);

#[godot_api]
impl Foo {
    #[func]
    fn do_ub() {
        let object_ptr = unsafe { callbacks::create::<Foo>(std::ptr::null_mut()) };
        let foo: Gd<Foo> = unsafe { Gd::from_obj_sys(object_ptr) };

        let bar: Foo = *foo;

        godot_print!("{bar:?}");
    }
}

[Bromeon]

Whether we support manually implementing it or not is kinda irrelevant. The trait can be implemented manually [...]

What can be done doesn't matter if the documentation advises against it -- I reworded this to make it clearer.

Thanks for the elaborated example! I think the unsafe trait is somewhat orthogonal to "whether the user can use it" and was already working on a PR.

To be clear though, this does not legitimate a) manual implementation of GodotClass, b) use of private APIs.