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

Handle generics in NativeScript derive and #[methods] #983

Merged
merged 2 commits into from
Dec 6, 2022

Conversation

chitoyuu
Copy link
Contributor

@chitoyuu chitoyuu commented Dec 4, 2022

The derive macros are now aware of both lifetime and type parameters. 'static bounds are added to both kinds, which improves the error message shown when a user tries to put a lifetime into a NativeClass.

This behavior is chosen because unlike type parameters, lifetime parameters aren't very useful in this context: only 'static would be supported anyway, and any attempted usage is hightly likely to have originated from user error.

Also added the #[skip] attribute for skipping fields in FromVarargs. This is useful mainly for skipping marker fields in generic code.

Close #601

Error message

The error message that appears when lifetime parameters are declared for NativeClass types now clearly indicate where the problem lies and what the expectations are:

error[E0478]: lifetime bound not satisfied
 --> tests/ui/derive_fail_lifetime.rs:3:10
  |
3 | #[derive(NativeClass)]
  |          ^^^^^^^^^^^
  |
note: lifetime parameter instantiated with the lifetime `'a` as defined here
 --> tests/ui/derive_fail_lifetime.rs:4:12
  |
4 | struct Foo<'a> {
  |            ^^
  = note: but lifetime parameter must outlive the static lifetime
  = note: this error originates in the derive macro `NativeClass` (in Nightly builds, run with -Z macro-backtrace for more info)

Compares this with the old error message:

error[E0726]: implicit elided lifetime not allowed here
 --> tests/ui/derive_fail_lifetime.rs:4:8
  |
4 | struct Foo<'a> {
  |        ^^^ expected lifetime parameter
  |
  = note: assuming a `'static` lifetime...
help: indicate the anonymous lifetime
  |
4 | struct Foo<'_><'a> {
  |           ++++

error[E0106]: missing lifetime specifier
 --> tests/ui/derive_fail_lifetime.rs:4:8
  |
4 | struct Foo<'a> {
  |        ^^^ expected named lifetime parameter
  |
help: consider introducing a named lifetime parameter
  |
3 ~ #[derive(NativeClass<'a>)]
4 ~ struct Foo<'a><'a> {
  |

error[E0277]: the trait bound `&gdnative::prelude::Node: OwnerArg<'_, Reference, Shared
>` is not satisfied
 --> tests/ui/derive_fail_lifetime.rs:3:10
  |
3 | #[derive(NativeClass)]
  |          ^^^^^^^^^^^ the trait `OwnerArg<'_, Reference, Shared>` is not implemented
 for `&gdnative::prelude::Node`
  |
  = help: the following other types implement trait `OwnerArg<'a, T, Own>`:
            &'a T
            TRef<'a, T, Own>
  = note: this error originates in the derive macro `NativeClass` (in Nightly builds, r
un with -Z macro-backtrace for more info)

Close #174

This is useful mainly for skipping marker fields in generic code.
@chitoyuu chitoyuu added feature Adds functionality to the library c: export Component: export (mod export, derive) labels Dec 4, 2022
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice addition! I especially like how you found a way to use the natural type-aliases to register monomorphizations:

#[gdnative::derive::monomorphize]
type IntPair = Pair<i32>;

I was first under the impression this would need to happen in the scope of the NativeClass macro itself.

Proc-macro code for To/FromVariant has reached quite a complexity already, but I think that's in the nature of proc-macros. Those two traits have become very powerful in the meantime, with lots of serde-like features! 💪

Would you add monomorphize to the prelude, or do you think it's rare enough to deserve a qualified import/path?

if !status:
printerr(" !! _test_generic_class failed")

return status
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add newline at end of file?

The derive macros are now aware of both lifetime and type parameters.
`'static` bounds are added to both kinds, which improves the error
message shown when a user tries to put a lifetime into a NativeClass.

This behavior is chosen because unlike type parameters, lifetime
parameters aren't very useful in this context: only `'static` would be
supported anyway, and any attempted usage is hightly likely to have
originated from user error.
@chitoyuu
Copy link
Contributor Author

chitoyuu commented Dec 4, 2022

Proc-macro code for To/FromVariant has reached quite a complexity already, but I think that's in the nature of proc-macros. Those two traits have become very powerful in the meantime, with lots of serde-like features! muscle

Admittedly, there is indeed a lot of complexity in those two macros. It's more of an unfortunate consequence of Godot having a richer data model than what serde allows (Rids, pointers, etc.). The changes this PR made are mainly to the NativeScript and #[methods] macros though.

Would you add monomorphize to the prelude, or do you think it's rare enough to deserve a qualified import/path?

I don't think it's prelude-worthy. It isn't something you'd expect to use in every other file in the average project.

@chitoyuu
Copy link
Contributor Author

chitoyuu commented Dec 5, 2022

I'll be merging this in a day, in case there are no further comments.

@chitoyuu
Copy link
Contributor Author

chitoyuu commented Dec 5, 2022

bors try

bors bot added a commit that referenced this pull request Dec 5, 2022
@bors
Copy link
Contributor

bors bot commented Dec 5, 2022

try

Build succeeded:

@chitoyuu
Copy link
Contributor Author

chitoyuu commented Dec 6, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 6, 2022

Build succeeded:

@bors bors bot merged commit 7eafdd7 into godot-rust:master Dec 6, 2022
@chitoyuu chitoyuu deleted the feature/nativeclass-generics branch December 6, 2022 06:37
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
2 participants