godot-rust / gdnative

Rust bindings for Godot 3
https://godot-rust.github.io
MIT License
3.61k stars 210 forks source link

Derive NativeClass for known monomorphizations of generic types #601

Closed ghost closed 1 year ago

ghost commented 4 years ago

While it's impossible to derive NativeClass in the general case due to monomorphization, it should be possible to export a list of monomorphizations specified by the user. This is most useful when a user needs to wrap a Rust data structure in a NativeScript, and know the list of types necessary.

Suppose that we have a GridMap type that efficiently stores a value for each tile in a 3D space, for example. Currently, we can wrap it in a NativeClass like this:

#[derive(NativeClass)]
#[inherit(Reference)]
struct GridMapInt {
    map: GridMap<i32>,
}

#[methods]
impl GridMapInt {
    #[export]
    fn get(&self, _owner: &Reference, x: i32, y: i32, z: i32) -> i32 {
        self.map.get(x, y, z)
    }

    #[export]
    fn set(&self, _owner: &Reference, x: i32, y: i32, z: i32, value: i32) {
        self.map.set(x, y, z, value);
    }

    // - 100 extra wrapper methods -
}

We can, then, create and use GridMaps containing i32s in GDScript. However, suppose that we now also want to be able to create GridMaps that contain strings instead. Because NativeClass and methods don't support generics yet, we have no option but to copy and paste GridMapInt, along with its 100 extra wrapper methods, which is obviously terrible, or use code generation, which is rather inconvenient. Both options will also limit how one can interact with those wrapper types from Rust, since they are not generic.

Instead, with a syntax for explicitly exporting certain monomorphizations, we would be able to write something like:

#[derive(NativeClass)]
#[inherit(Reference)]
struct GridMap<T> {
    map: GridMap<T>,
}

#[gdnative::monomorphize]
type GridMapInt = GridMap<i32>;

#[gdnative::monomorphize]
type GridMapStr = GridMap<String>;

#[methods]
impl<T> GridMap<T> {
    #[export]
    fn get(&self, _owner: &Reference, x: i32, y: i32, z: i32) -> T {
        self.map.get(x, y, z)
    }

    #[export]
    fn set(&self, _owner: &Reference, x: i32, y: i32, z: i32, value: T) {
        self.map.set(x, y, z, value);
    }

    // - 100 extra methods -
}

...which could expand to:

struct GridMap<T> {
    map: GridMap<T>,
}

// `GenericNativeClass` will be a supertrait of `NativeClass` that does not have `class_name`.
impl<T> GenericNativeClass for GridMap<T> {
    // - snip -
}

type GridMapInt = GridMap<i32>;
impl NativeClass for GridMapInt {
    fn class_name() -> &'static str {
        "GridMapInt"
    }
}

type GridMapStr = GridMap<String>;
impl NativeClass for GridMapStr {
    fn class_name() -> &'static str {
        "GridMapStr"
    }
}

impl<T> GridMap<T> {
    fn get(&self, _owner: &Reference, x: i32, y: i32, z: i32) -> T {
        self.map.get(x, y, z)
    }

    fn set(&self, _owner: &Reference, x: i32, y: i32, z: i32, value: T) {
        self.map.set(x, y, z, value);
    }

    // - 100 extra methods -
}

impl<T> NativeClassMethods for GridMap<T>
where
    // bounds detected from method signatures
    T: FromVariant + OwnedToVariant,
{
    fn register(builder: &ClassBuilder<Self>) {
        // - snip -
    }
}

Alternatives

Attributes on the original type instead of type aliases

Instead, we could also put the list of generic arguments above the type for which NativeScript is derived:

#[derive(NativeClass)]
#[inherit(Reference)]
#[monomorphize(name = "GridMapInt", args = "i32")]
#[monomorphize(name = "GridMapStr", args = "String")]
struct GridMap<T> {
    map: GridMap<T>,
}

There shouldn't be any implication on implementation difficulty or auto-registration, but I find this syntax much less convenient compared to the one with type aliases, due to how attribute syntax works. It also won't work as well with macros:

// This will work:

macro_rules! decl_aliases {
    ($($name:ident : $t:ty),*) => {
        $(
            #[gdnative::monomorphize]
            type $name = MyStruct<With, An, Unusual, Number, Of, Arguments, $t>;
        )*
    }
}

decl_aliases!(
    MyStructFoo: Foo,
    MyStructBar: Bar
)

// ...while this won't, because attributes cannot be produced from ordinary macros in isolation:

macro_rules! decl_attrs {
    ($($name:ident : $t:ty),*) => {
        // ..and even if they did, look at this monstrosity!
        $(
            #[monomorphize(
                name = stringify!($name),
                args = concat!(
                    "With, An, Unusual, Number, Of, Arguments, ",
                    stringify!($t),
                ),
            )]
        )*
    }
}

Opinions are welcome!

ghost commented 3 years ago

This would likely require some reworking of the NativeClass and NativeClassMethods traits, so should be considered before releasing 0.10.

Bromeon commented 2 years ago

This would be a useful feature indeed.

May I ask how this affects existing code (since it's marked as a breaking change)? It technically splits NativeClass, but since most user code doesn't directly touch those types, this change wouldn't affect them?

From https://github.com/godot-rust/godot-rust/issues/842#issuecomment-1004504829:

I'm proposing that we separate the class name from the implementation types themselves, in the process also allowing users to change any class names at registration site. This is just a side-effect, but might be useful in "plugin"-style libraries, like gdnative-async, for avoiding name collisions with user types.

Static class names can still be supported through a separate trait (StaticallyNamed?), which could be used as a bound for the registration methods.

Regarding class name, we might also want to consider https://github.com/godot-rust/godot-rust/issues/603. I think it would be nice, if for non-monomorphized types, the following would be possible:

I have no problem with using String as a type instead of &'static str.

chitoyuu commented 2 years ago

May I ask how this affects existing code (since it's marked as a breaking change)? It technically splits NativeClass, but since most user code doesn't directly touch those types, this change wouldn't affect them?

Exactly what you said. It affects manual NativeClass implementations, that are currently required for generic or renamed types.

  • Adopting the Rust struct's name by default (I don't know if/how this would work with the name being extracted)

We can support that by using the new trait as a generic bound of add_class, so to be able to write something like:

impl InitHandle {
    pub fn add_class<C: NativeClass + StaticallyNamed>(self) {
        self.add_class_with_name(C::class_name()) // now a member of StaticallyNamed
    }

    pub fn add_class_with_name<C: NativeClass>(self, name: &str) {
        // -- original impl of add_class --
    }
}
Bromeon commented 2 years ago

And StaticallyNamed would be implemented during #[derive(NativeClass)]?

So the derive-macro would detect whether it's applied to a generic class (-> no name) or not (-> name)? Or the trait is always implemented but not always used?

Sorry for the many questions, maybe it's best if I just look at your implementation πŸ™‚

chitoyuu commented 2 years ago

So the derive-macro would detect whether it's applied to a generic class (-> no name) or not (-> name)? Or the trait is always implemented but not always used?

Having NativeClass implement it "magically" for non-generic types is the plan, but it would also be possible to make it explicit if you'd prefer that. In the explicit case attempting to derive StaticallyNamed for a generic type would be a compile-time error.

I'll try to make a PR this weekend.

Bromeon commented 2 years ago

No, I think having less work left to the user (i.e. implicit) for non-generic types is better.

Thanks a lot, and no rush! πŸ™‚