godot-rust / gdnative

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

Simplify the `InitHandle` API #1036

Open chitoyuu opened 1 year ago

chitoyuu commented 1 year ago

As of writing (7d4ab54), the InitHandle type now contains a staggering amount of publicly visible variants of the add_class method:

It would look much nicer if we can somehow reverse the control flow and reduce this to just a single entry point returning ClassBuilder, which then expose methods to rename and customize the type being registered naturally. It would however mean a departure from our current model of forwarding calls one-by-one to Godot as they happen, which has consequences that could be deemed good or bad depending on how one looks at it:

Depending on how we do this the final implementation does not necessarily have to be breaking, although the implementation complexity would probably be lower if we remove the ability to use ClassBuilder with a shared reference (and thus break any registration functions that are manually written).

Bromeon commented 1 year ago

A refactoring in this area would make the builder API more intuitive and accessible. As such I'd support it, even if it means breaking changes and/or slightly different workflows. I would estimate the number of users that rely on the builder API to be already not too large (mainly for parts not exposed via proc macros), and the number among those that need either a) speed, or b) the immediate forwarding of calls to Godot, even lower.

  • Remove the need for #[no_constructor] completely, since the add_class call would no longer need to know immediately whether there is a default constructor or not.

This is an issue I also faced in GDExtension, and each approach has its trade-offs. I'm still not sure if opt-in or opt-out is better:

  1. Detecting whether a constructor is provided and having implicit #[no_constructor] otherwise is more automatic, but allows users to forget it, causing runtime errors.
  2. Generating one via proc-macro as a fallback is nice, but if it cannot initialize the fields, it will cause compiler errors seemingly out of nowhere.

Rethinking the builder API might also benefit https://github.com/godot-rust/gdextension/issues/4, even if the underlying systems are a bit different at the moment.

chitoyuu commented 1 year ago

Detecting whether a constructor is provided and having implicit #[no_constructor] otherwise is more automatic, but allows users to forget it, causing runtime errors.

Indeed. I'm not sure which is the way that leads to least confusion either, but even with the way we currently require Self::new by default, we've had confusion over compiler errors and concerns about the signature and using the new identifier.

Specialization would help a lot here but that's unlikely to be coming to stable any time soon... Perhaps this is just one of those cases where the complexity is necessary.

Generating one via proc-macro as a fallback is nice, but if it cannot initialize the fields, it will cause compiler errors seemingly out of nowhere.

I'm not sure what you mean. That would basically be writing our own derive(Default) wouldn't it?

Bromeon commented 1 year ago

Specialization would help a lot here but that's unlikely to be coming to stable any time soon... Perhaps this is just one of those cases where the complexity is necessary.

I know, and it's quite sad. This and the orphan rule make Rust traits feel really underpowered. There are a lot of generic programming things you can currently do in C++ but still not in Rust.

I'm not sure what you mean. That would basically be writing our own derive(Default) wouldn't it?

Yes, in GDExtension this is quite useful because the #[base] field can be directly initialized by the macro. Every other field is initialized to Default::default(). In GDNative we don't have the base field, so I don't think it would add much over just having an impl Default.

chitoyuu commented 1 year ago

Yes, in GDExtension this is quite useful because the #[base] field can be directly initialized by the macro. Every other field is initialized to Default::default(). In GDNative we don't have the base field, so I don't think it would add much over just having an impl Default.

Ah, I see. For GDNative I think I'll keep the current approach of separating the user type from the base object, which as a side effect also allows tools like the default derive or derivative to work.