Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Derive NativeClass for known monomorphizations of generic types #601

Closed
Tracked by #767
ghost opened this issue Sep 23, 2020 · 6 comments · Fixed by #983
Closed
Tracked by #767

Derive NativeClass for known monomorphizations of generic types #601

ghost opened this issue Sep 23, 2020 · 6 comments · Fixed by #983
Labels
c: export Component: export (mod export, derive) feature Adds functionality to the library
Milestone

Comments

@ghost
Copy link

ghost commented Sep 23, 2020

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 ghost added feature Adds functionality to the library c: export Component: export (mod export, derive) labels Sep 23, 2020
@ghost ghost added the breaking-change Issues and PRs that are breaking to fix/merge. label Feb 3, 2021
@ghost ghost added this to the 0.10 milestone Feb 3, 2021
@ghost
Copy link
Author

ghost commented Feb 3, 2021

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

@Bromeon Bromeon modified the milestones: v0.10, v0.11 Nov 1, 2021
@chitoyuu chitoyuu mentioned this issue Jan 4, 2022
6 tasks
@Bromeon
Copy link
Member

Bromeon commented Jan 4, 2022

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 #842 (comment):

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 #603. I think it would be nice, if for non-monomorphized types, the following would be possible:

  • Adopting the Rust struct's name by default
    (I don't know if/how this would work with the name being extracted)
  • Allowing a custom name, e.g. as an attribute #[name = "MyCustomName"] or so
    (this can be done in a separate PR later)

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

@Bromeon Bromeon modified the milestones: v0.11, v0.10.0 Jan 4, 2022
@chitoyuu
Copy link
Contributor

chitoyuu commented Jan 4, 2022

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
Copy link
Member

Bromeon commented Jan 4, 2022

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
Copy link
Contributor

chitoyuu commented Jan 4, 2022

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
Copy link
Member

Bromeon commented Jan 4, 2022

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! 🙂

chitoyuu added a commit to chitoyuu/godot-rust that referenced this issue Jan 9, 2022
This allows NativeScript types to be registered under names only determined
at run time, enabling better ergonomics for libraries.

Ref godot-rust#601, godot-rust#603
chitoyuu added a commit to chitoyuu/godot-rust that referenced this issue Jan 9, 2022
This allows NativeScript types to be registered under names only determined
at run time, enabling better ergonomics for libraries.

Ref godot-rust#601, godot-rust#603
bors bot added a commit that referenced this issue Jan 10, 2022
847: Separate static name from `NativeClass` into new trait r=Bromeon a=chitoyuu

This allows NativeScript types to be registered under names only determined
at run time, enabling better ergonomics for libraries.

Ref #601, #603

## Unresolved questions

This does not implement the `#[name = "MyCustomName"]` static renaming syntax as suggested in #601 (comment), because it's very uncommon (if at all possible) for a key-value pair to be the top-level meta in an attribute. It should also be noted that at this moment, the `NativeClass` macro already utilizes a large number of type-level attributes:

- `inherit`
- `user_data`
- `register_with`
- `no_constructor`

...so it might not be wise to add another into the mix, especially under such a non-specific identifier as `name`. We might want to consider bundling (some of) these into a single top-level attribute (like `variant` for the `From`/`ToVariant` macros) instead. See #848.

Co-authored-by: Chitose Yuuzaki <chitoyuu@potatoes.gay>
@Bromeon Bromeon modified the milestones: v0.10.0, v0.11 Feb 6, 2022
@chitoyuu chitoyuu removed the breaking-change Issues and PRs that are breaking to fix/merge. label Nov 30, 2022
@chitoyuu chitoyuu modified the milestones: unplanned, v0.11.x Nov 30, 2022
@bors bors bot closed this as completed in 7eafdd7 Dec 6, 2022
@chitoyuu chitoyuu modified the milestones: v0.11.x, v0.11.1 Dec 6, 2022
KarimHamidou added a commit to KarimHamidou/godot-rust that referenced this issue Feb 6, 2023
847: Separate static name from `NativeClass` into new trait r=Bromeon a=chitoyuu

This allows NativeScript types to be registered under names only determined
at run time, enabling better ergonomics for libraries.

Ref #601, #603

## Unresolved questions

This does not implement the `#[name = "MyCustomName"]` static renaming syntax as suggested in godot-rust/gdnative#601 (comment), because it's very uncommon (if at all possible) for a key-value pair to be the top-level meta in an attribute. It should also be noted that at this moment, the `NativeClass` macro already utilizes a large number of type-level attributes:

- `inherit`
- `user_data`
- `register_with`
- `no_constructor`

...so it might not be wise to add another into the mix, especially under such a non-specific identifier as `name`. We might want to consider bundling (some of) these into a single top-level attribute (like `variant` for the `From`/`ToVariant` macros) instead. See #848.

Co-authored-by: Chitose Yuuzaki <chitoyuu@potatoes.gay>
GuilhermeOrceziae added a commit to GuilhermeOrceziae/godot-rust that referenced this issue Feb 9, 2023
847: Separate static name from `NativeClass` into new trait r=Bromeon a=chitoyuu

This allows NativeScript types to be registered under names only determined
at run time, enabling better ergonomics for libraries.

Ref #601, #603

## Unresolved questions

This does not implement the `#[name = "MyCustomName"]` static renaming syntax as suggested in godot-rust/gdnative#601 (comment), because it's very uncommon (if at all possible) for a key-value pair to be the top-level meta in an attribute. It should also be noted that at this moment, the `NativeClass` macro already utilizes a large number of type-level attributes:

- `inherit`
- `user_data`
- `register_with`
- `no_constructor`

...so it might not be wise to add another into the mix, especially under such a non-specific identifier as `name`. We might want to consider bundling (some of) these into a single top-level attribute (like `variant` for the `From`/`ToVariant` macros) instead. See #848.

Co-authored-by: Chitose Yuuzaki <chitoyuu@potatoes.gay>
hesuteia added a commit to hesuteia/godot-rust that referenced this issue Feb 11, 2023
847: Separate static name from `NativeClass` into new trait r=Bromeon a=chitoyuu

This allows NativeScript types to be registered under names only determined
at run time, enabling better ergonomics for libraries.

Ref #601, #603

## Unresolved questions

This does not implement the `#[name = "MyCustomName"]` static renaming syntax as suggested in godot-rust/gdnative#601 (comment), because it's very uncommon (if at all possible) for a key-value pair to be the top-level meta in an attribute. It should also be noted that at this moment, the `NativeClass` macro already utilizes a large number of type-level attributes:

- `inherit`
- `user_data`
- `register_with`
- `no_constructor`

...so it might not be wise to add another into the mix, especially under such a non-specific identifier as `name`. We might want to consider bundling (some of) these into a single top-level attribute (like `variant` for the `From`/`ToVariant` macros) instead. See #848.

Co-authored-by: Chitose Yuuzaki <chitoyuu@potatoes.gay>
ecobiubiu added a commit to ecobiubiu/open-rust that referenced this issue Mar 30, 2023
847: Separate static name from `NativeClass` into new trait r=Bromeon a=chitoyuu

This allows NativeScript types to be registered under names only determined
at run time, enabling better ergonomics for libraries.

Ref #601, #603

## Unresolved questions

This does not implement the `#[name = "MyCustomName"]` static renaming syntax as suggested in godot-rust/gdnative#601 (comment), because it's very uncommon (if at all possible) for a key-value pair to be the top-level meta in an attribute. It should also be noted that at this moment, the `NativeClass` macro already utilizes a large number of type-level attributes:

- `inherit`
- `user_data`
- `register_with`
- `no_constructor`

...so it might not be wise to add another into the mix, especially under such a non-specific identifier as `name`. We might want to consider bundling (some of) these into a single top-level attribute (like `variant` for the `From`/`ToVariant` macros) instead. See #848.

Co-authored-by: Chitose Yuuzaki <chitoyuu@potatoes.gay>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: export Component: export (mod export, derive) feature Adds functionality to the library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants