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

Mix-ins, manually registered #[methods] blocks applicable to multiple types #984

Closed
chitoyuu opened this issue Dec 5, 2022 · 3 comments · Fixed by #999
Closed

Mix-ins, manually registered #[methods] blocks applicable to multiple types #984

chitoyuu opened this issue Dec 5, 2022 · 3 comments · Fixed by #999
Labels
c: export Component: export (mod export, derive) feature Adds functionality to the library
Milestone

Comments

@chitoyuu
Copy link
Contributor

chitoyuu commented Dec 5, 2022

It has been a long-standing limitation that only one #[methods] blocks can be exposed for any given type. There are many reasons why it's desirable to remove this limitation:

  • Generally, Rust puts no restriction on the number of impl blocks a type might have. It's natural to want to use multiple impl blocks for code organization.
  • It's nice to be able use traits to group common functionality together, so a consistent GDScript interface can be enforced by the compiler.
  • Traits can also potentially be used to simplify the process of correctly overriding an engine virtual method (e.g. _ready). Some interesting experiments are being done in the GDExtension bindings, centered on a curated set of methods.

In light of #983, there's also a new important use for the ability: to declare multiple impl blocks with different bounds, which is critical for the usefulness of generic types.

Design

The public interface would be fairly simple, and compatible with the existing API:

#[derive(Clone, NativeClass)]
#[register_with(register_mixins)]
struct Foo;

#[methods(as = "InherentMixin")]
impl Foo {
    #[method]
    fn foo() {}
}

#[methods(as = "TraitMixin")]
impl<C> MyDeepCloneTrait for C
where
    C: NativeClass + Clone,
{
    #[method]
    fn duplicate(&self) -> Instance<C> {
        Instance::emplace(self.clone()).into_shared()
    }
}

fn register_mixins(builder: &ClassBuilder<Foo>) {
    builder.mixin::<InherentMixin>();
    builder.mixin::<TraitMixin>();
}

Here, the presence of the #[methods(as)] argument tells the macro to generate a mixin type called MyMixin, instead of an ordinary NativeClassMethods implementation. A mixin type is an opaque type with no public interface, that serves as a handle to pass into ClassBuilder::mixin, which will then perform registration.

It's important that a type is generated instead of a plain register function here: the latter does not have a nameable type. While it doesn't make a lot difference for simple use cases with manual registration, they can be useful for type-based shenanigans in the future, should we find ourselves having to expand the internal interface beyond just a single function.

Internally, #[methods] (without the as argument) could be refactored to generate an unnameable mixin that is immediately wrapped into a NativeClassMethods. This behavior can be removed in a future breaking version where we introduce automatic registration for inherent impls (on supported platforms, see #350), and remove the NativeClassMethods trait completely.

@chitoyuu chitoyuu added feature Adds functionality to the library c: export Component: export (mod export, derive) labels Dec 5, 2022
@chitoyuu chitoyuu added this to the v0.11.x milestone Dec 5, 2022
@Bromeon
Copy link
Member

Bromeon commented Dec 5, 2022

Nice to have a type name as a differentiator!

Question for my understanding:

#[methods(as = "TraitMixin")]
impl<C> MyDeepCloneTrait for C
where
    C: NativeClass + Clone,
{
    #[method]
    fn duplicate(&self) -> Instance<C> {
        Instance::emplace(self.clone()).into_shared()
    }
}

This would still need an explicit monomorphization as in #983, in order to expose duplicate to GDScript, correct? I assume the handle TraitMixin itself is not generic?

@chitoyuu
Copy link
Contributor Author

chitoyuu commented Dec 5, 2022

This would still need an explicit monomorphization as in #983, in order to expose duplicate to GDScript, correct? I assume the handle TraitMixin itself is not generic?

Yes. The TraitMixin itself is not generic, but will have a generic impl on it, so it can be registered to different types. For manual registration the impl will be monomorphized in builder.mixin::<TraitMixin>(), as shown later in the example.

I'm not very optimistic about automatic registration of generic impls like this in the near future. For one thing, it will likely require specialization in some form, which isn't coming to stable any time soon. Even with that cleared, there would still be the problem of somehow coercing Rust into monomorphizing NxM combinations, where both sets N and M are only known at runtime, as is the case with inventory.

@chitoyuu chitoyuu modified the milestones: v0.11.x, v0.11.1 Dec 6, 2022
@chitoyuu
Copy link
Contributor Author

chitoyuu commented Dec 7, 2022

I'm starting to think that perhaps this should just be rolled out together with #350, so we don't have to write all the diagnostic code and UI tests just to do it all over again. It would be a lot better if the code was aware of platform availability of auto registration right from the start.

#350 has to be a non-breaking change anyway (due to WASM), so that yak might as well get shaved right now.

bors bot added a commit that referenced this issue Jan 4, 2023
999: Implement automatic `NativeClass` registration via inventory. Implement mix-ins. r=chitoyuu a=chitoyuu

## Implement automatic NativeClass registration via inventory

This adds the optional `inventory` feature, which allows `NativeClass` types to be automatically registered on supported platforms (everything that OSS Godot currently supports except WASM).

Run-time diagnostic functions are added to help debug missing registration problems that are highly likely to arise when porting `inventory`-enabled projects to WASM.

An internal `cfg_ex` attribute macro is added to help manage cfg conditions.

Close #350. Note that the standalone registration attribute syntax proposed by the original issue isn't implemented, for the limited usefulness -- there are much fewer cases where manual `NativeClass` impls are necessary thanks to all the improvements since the original issue.

## Implement mix-in `#[methods]` blocks

Adds the `#[methods(mixin = "Name")]` syntax for declaring mix-in blocks. Mix-in blocks have a many-to-many relationship with `NativeClass` types. Both `impl Type` and `impl Trait for Type` blocks are accepted.

The argument name is changed from `as` in the original proposal to `mixin`, because we might still want to keep universal `#[methods]` blocks in the future for ease of use with WASM. `#[methods(mixin)]` makes a lot more sense for a future auto-registered mixin block than `#[methods(as /* as what? */)]`.

All mix-in blocks have to be manually registered for gdnative v0.11.x. Some difficulty was encountered when trying to make auto-mixins compatible with existing code. It might still be possible with some tricks like autoref-specialization, but that might be too much effort given that we likely want to re-design much of the hierarchy for 0.12.

Close #984.

## Allow `#[register_with]` for `#[monomorphize]`

Enables `#[monomorphize]` to take the same standalone `#[register_with]` attribute as `#[derive(NativeClass)]`. This is chosen for short term consistency, but will probably change in a later version w/ #848, which might not still get implemented for a fair bit of time.


Co-authored-by: Chitose Yuuzaki <chitoyuu@potatoes.gay>
bors bot added a commit that referenced this issue Jan 4, 2023
999: Implement automatic `NativeClass` registration via inventory. Implement mix-ins. r=chitoyuu a=chitoyuu

## Implement automatic NativeClass registration via inventory

This adds the optional `inventory` feature, which allows `NativeClass` types to be automatically registered on supported platforms (everything that OSS Godot currently supports except WASM).

Run-time diagnostic functions are added to help debug missing registration problems that are highly likely to arise when porting `inventory`-enabled projects to WASM.

An internal `cfg_ex` attribute macro is added to help manage cfg conditions.

Close #350. Note that the standalone registration attribute syntax proposed by the original issue isn't implemented, for the limited usefulness -- there are much fewer cases where manual `NativeClass` impls are necessary thanks to all the improvements since the original issue.

## Implement mix-in `#[methods]` blocks

Adds the `#[methods(mixin = "Name")]` syntax for declaring mix-in blocks. Mix-in blocks have a many-to-many relationship with `NativeClass` types. Both `impl Type` and `impl Trait for Type` blocks are accepted.

The argument name is changed from `as` in the original proposal to `mixin`, because we might still want to keep universal `#[methods]` blocks in the future for ease of use with WASM. `#[methods(mixin)]` makes a lot more sense for a future auto-registered mixin block than `#[methods(as /* as what? */)]`.

All mix-in blocks have to be manually registered for gdnative v0.11.x. Some difficulty was encountered when trying to make auto-mixins compatible with existing code. It might still be possible with some tricks like autoref-specialization, but that might be too much effort given that we likely want to re-design much of the hierarchy for 0.12.

Close #984.

## Allow `#[register_with]` for `#[monomorphize]`

Enables `#[monomorphize]` to take the same standalone `#[register_with]` attribute as `#[derive(NativeClass)]`. This is chosen for short term consistency, but will probably change in a later version w/ #848, which might not still get implemented for a fair bit of time.


Co-authored-by: Chitose Yuuzaki <chitoyuu@potatoes.gay>
@bors bors bot closed this as completed in 16e5847 Jan 4, 2023
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