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

#[class(no_init)] to explicitly disable constructor #593

Merged
merged 7 commits into from
Feb 4, 2024
Merged

Conversation

Bromeon
Copy link
Member

@Bromeon Bromeon commented Feb 4, 2024

Adds #[class(no_init)], which is now required to disable constructor.
This is a breaking change for cases where the constructor was implicitly disabled -- but this didn't work properly anyway, see below.

Forgetting a constructor is no longer possible: if there is no #[class(init)] attribute and no overridden init() function, then the library will emit a compile error. In many cases, constructors were accidentally disabled due to forgotten #[class(init)], which also broke hot reloading.

In addition, the no_init key now registers a class properly as non-instantiable (in Godot terms "abstract", which is confusing). This means that you get a helpful error message in the editor in case you call MyClass.new() from GDScript:

ERROR: Class 'MyClass' or its base class cannot be instantiated.

Fixes #539.

Solves the issue where constructors were accidentally disabled due to forgotten #[class(init)], which also broke hot reloading.
Not providing #[class(init)] and not overriding init() now causes a compile error.
…ass as "abstract"

"Abstract" is Godot terminology; this does not make it an abstract base class.
@Bromeon Bromeon added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels Feb 4, 2024
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-593

@Bromeon Bromeon added this pull request to the merge queue Feb 4, 2024
Merged via the queue into master with commit cd652b7 Feb 4, 2024
16 checks passed
@Bromeon Bromeon deleted the feature/no-init branch February 4, 2024 18:34
@bluenote10
Copy link
Contributor

Forgetting a constructor is no longer possible: if there is no #[class(init)] attribute and no overridden init() function, then the library will emit a compile error.

Is it actually by design that this compiler error is a little bit cryptic? Or is that rather an accidental regression?

For me the compiler would currently fail with something like:

error[E0277]: the trait bound `SomeClass: GodotDefault` is not satisfied
   --> src/lib.rs:190:12
    |
190 | pub struct SomeClass {
    |            ^^^^^ the trait `GodotDefault` is not implemented for `SomeClass`
    |
    = help: the following other types implement trait `GodotDefault`:
              TreeRoot
              ClassicCounter
              Counter
              AesContext
              AStar2D
              AStar3D
              AStarGrid2D
              AcceptDialog
            and 710 others
note: required by a bound in `class_SomeClass_must_have_an_init_method::__type_check`
   --> src/lib.rs:188:10
    |
188 | #[derive(GodotClass)]
    |          ^^^^^^^^^^ required by this bound in `__type_check`
    = note: this error originates in the derive macro `GodotClass` (in Nightly builds, run with -Z macro-backtrace for more info)

I vaguely remembered seeing this PR, which is why I fortunately made the right guess that I had to add no_init. I'd imagine that without prior knowledge this error could be a bit misleading, because the intuitive solution would be to try implementing GodotDefault manually, i.e., the no_init switch is hard to discover.

@Bromeon
Copy link
Member Author

Bromeon commented Mar 18, 2024

Is it actually by design that this compiler error is a little bit cryptic? Or is that rather an accidental regression?

Do you have a suggestion how to improve errors here?

Some options:

  • Renaming GodotDefault to MustHaveInit is theoretically possible, but that makes other uses of the traits (e.g. in bounds) awkward. I'd rather not make the public API weird just for this.
  • Renaming __type_check to __must_have_init is also possible, but it's only slightly less cryptic, and this appears at the end of the message. At least it's not interfering with public symbols.

@Bromeon
Copy link
Member Author

Bromeon commented Mar 18, 2024

On another note, I did add a dedicated chapter about constructors just yesterday, which also elaborates #[class(no_init)].

That doesn't dismiss the importance of having good error messages, especially since this one can happen accidentally, but it provides additional knowledge.

@bluenote10
Copy link
Contributor

Thanks for clarifying! I somehow guessed that at the time this was introduced the message was better, and it had degraded accidentally.

Do you have a suggestion how to improve errors here?

The problem with "must have init" is that it may also be misleading, because it sounds like having an init in one way or another is always mandatory. I was hoping we can somehow create a custom error in the macro. But if that is not possible what about renaming __type_check to a more explicit __class_without_init_must_be_marked_with_no_init. This would at least give a hint that there are two options: either add an init or go for "no init". What "marking with no init" exactly means is of course hard to explain via a symbol.

On another note, I did add a dedicated chapter about constructors just yesterday, which also elaborates #[class(no_init)].

We could also mention that error there, but it still may be hard for users to discover.

@Bromeon
Copy link
Member Author

Bromeon commented Mar 19, 2024

I was hoping we can somehow create a custom error in the macro.

The error doesn't happen at macro evaluation time, but only when the expanded code is compiled.

It is allowed to have neither #[class(init)] nor #[class(no_init)], namely when a user-defined init() function is provided. This is however declared inside a #[godot_api] impl block, which is a different macro from #[derive(GodotClass)]. The two don't know about each other.

This all sucks quite a bit, but unfortunately our hope at ever getting proper compile-time introspection (rather than these proc-macro hacks) was shattered by very professional Rust leadership.


But if that is not possible what about renaming __type_check to a more explicit __class_without_init_must_be_marked_with_no_init.

Something like this should be possible 👍


We could also mention that error there, but it still may be hard for users to discover.

There is already a mention of this:

Not providing/generating an init method and forgetting to use #[class(no_init)] will result in a compile-time error.

I'd rather not mention the concrete error wording -- it's one more thing that goes out of sync, and it's very unlikely that people find that page in the book when enountering the error.

@bluenote10
Copy link
Contributor

I'd rather not mention the concrete error wording -- it's one more thing that goes out of sync, and it's very unlikely that people find that page in the book when enountering the error.

For me it simply would have helped, because the error "the trait bound SomeClass: GodotDefault is not satisfied" sounded not at all like that error message, but something completely different. A small info box a la

Note that the error manifest itself in the form of an unsatisfied trait bound like the trait bound SomeClass: GodotDefault is not satisfied.

would have clarified it for me. Perhaps it would also make the error googlable...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Classes without init break hot reloading
3 participants