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

Simplify the InitHandle API #1036

Open
chitoyuu opened this issue Feb 22, 2023 · 4 comments
Open

Simplify the InitHandle API #1036

chitoyuu opened this issue Feb 22, 2023 · 4 comments
Labels
c: export Component: export (mod export, derive) quality-of-life No new functionality, but improves ergonomics/internals
Milestone

Comments

@chitoyuu
Copy link
Contributor

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

  • add_class
  • add_class_with
  • add_tool_class
  • add_tool_class_with
  • add_class_as
  • add_class_as_with
  • add_tool_class_as
  • add_tool_class_as_with

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:

  • This would give us a chance to look at the whole definition of a class at runtime before actually registering it to Godot, which we might be able to use to do some potentially useful things, including but not limited to:
    • Modify certain methods like _ready to insert code, generating one if not provided by the user.
    • 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.
    • Automatically generating get_{} and set_{} methods for properties from their accessors, if not provided by the user.
  • On the other hand, it would obviously increase internal complexity by some amount, and it would probably make init slower -- however unlikely that is to matter.

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).

@chitoyuu chitoyuu added c: export Component: export (mod export, derive) quality-of-life No new functionality, but improves ergonomics/internals labels Feb 22, 2023
@chitoyuu chitoyuu added this to the v0.13.0 milestone Feb 22, 2023
@Bromeon
Copy link
Member

Bromeon commented Feb 22, 2023

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 godot-rust/gdext#4, even if the underlying systems are a bit different at the moment.

@chitoyuu
Copy link
Contributor Author

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

Bromeon commented Feb 23, 2023

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

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.

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) quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

No branches or pull requests

2 participants