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

Can't Derive ToVariant on generic type with constraint #957

Open
Tracked by #958
astrale-sharp opened this issue Oct 9, 2022 · 10 comments
Open
Tracked by #958

Can't Derive ToVariant on generic type with constraint #957

astrale-sharp opened this issue Oct 9, 2022 · 10 comments
Labels
bug c: export Component: export (mod export, derive)
Milestone

Comments

@astrale-sharp
Copy link

Hi !
I'm trying to get this to compile :

#[derive(Debug, Clone, ToVariant)]
pub enum WorldChange<T: Entity> {
    EntityMoved {
        id: EntityId,
        from: Position,
        to: Position,
    },
    EntitySentMessage {
        from: EntityId,
        to: EntityId,
        msg: T::Message,
    },
    EntityStateChanged {
        id: EntityId,
        change: T::EntityChange,
    },
    EntityPlaced(EntityId, Position),
    EntityUnplaced(EntityId),
}

but it complains this enum takes 1 generic argument but 0 generic arguments were supplied expected 1 generic argument.

Note that it doesn't complain for MyStruct, but it complains for any constraint MyStruct or MyStruct
it doesn't matter as well if T::Message impl ToVariant or not (which it does)

It would be nice to be able to constrain the type because my only other option is to impl ToVariant by myself which is not ideal.

@astrale-sharp astrale-sharp added the feature Adds functionality to the library label Oct 9, 2022
@Bromeon
Copy link
Member

Bromeon commented Oct 9, 2022

Thanks for reporting. I'm not 100% sure how good the support for generics is in the derives.

Did you already have a look with cargo expand to see the code generated by the derive-proc-macro? That often helps pinpoint where exactly the error message occurs.

@Bromeon Bromeon added the c: export Component: export (mod export, derive) label Oct 9, 2022
@astrale-sharp
Copy link
Author

Okay, I learned a bit I installed nightly and i'm running through that code to try and understand what's up :
this :

#[derive(Debug, Clone, ToVariant)]
pub enum WorldChange<T: Entity> {
    EntityMoved {
        id: EntityId,
        from: Position,
        to: Position,
        cost: f32,
    },
    EntitySentMessage {
        from: EntityId,
        to: EntityId,
        msg: T::Message,
    },
    EntityStateChanged {
        id: EntityId,
        change: T::EntityChange,
    },
    EntityPlaced(EntityId, Position),
    EntityUnplaced(EntityId),
    EntityPassedTurn(EntityId),
}

expands the bloc of code at the end of this comment:

It was incredibly useful to pinpoint the bug !
notice that if you replace the first line
impl<T: Entity> ::gdnative::core_types::ToVariant for WorldChange<T: Entity>
by
impl<T: Entity> ::gdnative::core_types::ToVariant for WorldChange<T>
There is no more problems ! I've never messed with proc macros yet but that doesn't look so hard (the error was really confusing tho, maybe we should report it to rust-lang
cause this enum takes 1 generic argument but 0 generic arguments were supplied expected 1 generic argument is really not the problem here ...

expansion

 impl<T: Entity> ::gdnative::core_types::ToVariant for WorldChange<T: Entity>
    where
        T::Message: ::gdnative::core_types::ToVariant,
        T::EntityChange: ::gdnative::core_types::ToVariant,
    {
        fn to_variant(&self) -> ::gdnative::core_types::Variant {
            use ::gdnative::core_types::ToVariant;
            use ::gdnative::core_types::FromVariant;
            match &self {
                WorldChange::EntityMoved { id, from, to, cost } => {
                    let __dict = ::gdnative::core_types::Dictionary::new();
                    let __key = ::gdnative::core_types::ToVariant::to_variant(
                        &::gdnative::core_types::GodotString::from("EntityMoved"),
                    );
                    let __value = {
                        let __dict = ::gdnative::core_types::Dictionary::new();
                        {
                            let __key = ::gdnative::core_types::GodotString::from("id")
                                .to_variant();
                            __dict.insert(&__key, &(id).to_variant());
                        }
                        {
                            let __key = ::gdnative::core_types::GodotString::from("from")
                                .to_variant();
                            __dict.insert(&__key, &(from).to_variant());
                        }
                        {
                            let __key = ::gdnative::core_types::GodotString::from("to")
                                .to_variant();
                            __dict.insert(&__key, &(to).to_variant());
                        }
                        {
                            let __key = ::gdnative::core_types::GodotString::from("cost")
                                .to_variant();
                            __dict.insert(&__key, &(cost).to_variant());
                        }
                        __dict.into_shared().to_variant()
                    };
                    __dict.insert(&__key, &__value);
                    ::gdnative::core_types::ToVariant::to_variant(&__dict.into_shared())
                }
                WorldChange::EntitySentMessage { from, to, msg } => {
                    let __dict = ::gdnative::core_types::Dictionary::new();
                    let __key = ::gdnative::core_types::ToVariant::to_variant(
                        &::gdnative::core_types::GodotString::from("EntitySentMessage"),
                    );
                    let __value = {
                        let __dict = ::gdnative::core_types::Dictionary::new();
                        {
                            let __key = ::gdnative::core_types::GodotString::from("from")
                                .to_variant();
                            __dict.insert(&__key, &(from).to_variant());
                        }
                        {
                            let __key = ::gdnative::core_types::GodotString::from("to")
                                .to_variant();
                            __dict.insert(&__key, &(to).to_variant());
                        }
                        {
                            let __key = ::gdnative::core_types::GodotString::from("msg")
                                .to_variant();
                            __dict.insert(&__key, &(msg).to_variant());
                        }
                        __dict.into_shared().to_variant()
                    };
                    __dict.insert(&__key, &__value);
                    ::gdnative::core_types::ToVariant::to_variant(&__dict.into_shared())
                }
                WorldChange::EntityStateChanged { id, change } => {
                    let __dict = ::gdnative::core_types::Dictionary::new();
                    let __key = ::gdnative::core_types::ToVariant::to_variant(
                        &::gdnative::core_types::GodotString::from("EntityStateChanged"),
                    );
                    let __value = {
                        let __dict = ::gdnative::core_types::Dictionary::new();
                        {
                            let __key = ::gdnative::core_types::GodotString::from("id")
                                .to_variant();
                            __dict.insert(&__key, &(id).to_variant());
                        }
                        {
                            let __key = ::gdnative::core_types::GodotString::from(
                                    "change",
                                )
                                .to_variant();
                            __dict.insert(&__key, &(change).to_variant());
                        }
                        __dict.into_shared().to_variant()
                    };
                    __dict.insert(&__key, &__value);
                    ::gdnative::core_types::ToVariant::to_variant(&__dict.into_shared())
                }
                WorldChange::EntityPlaced(__field_0, __field_1) => {
                    let __dict = ::gdnative::core_types::Dictionary::new();
                    let __key = ::gdnative::core_types::ToVariant::to_variant(
                        &::gdnative::core_types::GodotString::from("EntityPlaced"),
                    );
                    let __value = {
                        let __array = ::gdnative::core_types::VariantArray::new();
                        __array.push(&(__field_0).to_variant());
                        __array.push(&(__field_1).to_variant());
                        __array.into_shared().to_variant()
                    };
                    __dict.insert(&__key, &__value);
                    ::gdnative::core_types::ToVariant::to_variant(&__dict.into_shared())
                }
                WorldChange::EntityUnplaced(__field_0) => {
                    let __dict = ::gdnative::core_types::Dictionary::new();
                    let __key = ::gdnative::core_types::ToVariant::to_variant(
                        &::gdnative::core_types::GodotString::from("EntityUnplaced"),
                    );
                    let __value = (__field_0).to_variant();
                    __dict.insert(&__key, &__value);
                    ::gdnative::core_types::ToVariant::to_variant(&__dict.into_shared())
                }
                WorldChange::EntityPassedTurn(__field_0) => {
                    let __dict = ::gdnative::core_types::Dictionary::new();
                    let __key = ::gdnative::core_types::ToVariant::to_variant(
                        &::gdnative::core_types::GodotString::from("EntityPassedTurn"),
                    );
                    let __value = (__field_0).to_variant();
                    __dict.insert(&__key, &__value);
                    ::gdnative::core_types::ToVariant::to_variant(&__dict.into_shared())
                }
            }
        }
    }

@Bromeon
Copy link
Member

Bromeon commented Oct 12, 2022

Thanks for expanding, that helped me narrow down the problem!

A fix is available in #961, could you see if it works for you? You can add a Git dependency in Cargo.toml with a branch name.

@Bromeon Bromeon added bug and removed feature Adds functionality to the library labels Oct 12, 2022
@Bromeon Bromeon added this to the v0.11.x milestone Oct 12, 2022
@astrale-sharp
Copy link
Author

astrale-sharp commented Oct 16, 2022

Amazing thanks !
No it doesn't work at all ! But the errors i get are very very weird !

cargo expand generate code that impl ToVariant Correctly but #[method] now complains that all the struct deriving ToVariant don't satisfy the trait bound , I'm extremly puzzled to be honest ...
(The implementation of the trait is there !)

What's extremely odd is that your test seems to be passing and i dont do anything differently, maybe i messed it up in Cargo.toml ? I used :
gdnative = {git = "https://github.com/Bromeon/godot-rust.git", branch = "bugfix/to-variant-bounds"}
EDIT: or maybe it's not compatible with 0.11.0 which i was using until now but that would be surprising
EDIT 2 : I've been investigating and the bug(?) seems to be there before the fix (which is reassuring because the fix only slightly modified the derive macro), let me check some more stuff before reporting

@astrale-sharp
Copy link
Author

astrale-sharp commented Oct 16, 2022

I found it ! Oh my it was subtle (for me)!!
I temporarily had to add #![feature(associated_type_bounds)] but it is not needed once i fixed the impl

Okay so a struct like this

#[derive(Debug, Clone,ToVariant)]
pub struct Intent<T: Entity> {
    pub emitter: EntityId,
    pub action: Action<T>,
    pub priority: i32,
}

has a generated cargo expand like this

impl<T: Entity> ::gdnative::core_types::ToVariant for Intent<T>
 where
     T: ::gdnative::core_types::ToVariant,
{
    fn to_variant(&self) -> ::gdnative::core_types::Variant {
        use ::gdnative::core_types::FromVariant;
        use ::gdnative::core_types::ToVariant;
        {
            let Intent {
                emitter,
                action,
                priority,
            } = self;
            {
                let __dict = ::gdnative::core_types::Dictionary::new();
                {
                    let __key = ::gdnative::core_types::GodotString::from("emitter").to_variant();
                    __dict.insert(
                        &__key,
                        &(emitter).to_variant(),
                    );
                }
                {
                    let __key = ::gdnative::core_types::GodotString::from("action").to_variant();
                    __dict.insert(
                        &__key,
                        &(action).to_variant(),
                    );
                }
                {
                    let __key = ::gdnative::core_types::GodotString::from("priority").to_variant();
                    __dict.insert(
                        &__key,
                        &(priority).to_variant(),
                    );
                }
                __dict.into_shared().to_variant()
            }
        }
    }
}

Note that where T: ::gdnative::core_types::ToVariant, is not needed (and not implemented so that's why it wouldn't work with me) commenting out these line suffice as a fix for me, so i'm not stuck
Fixing the macros probably won't be easy tho because we would need to know if we need T to impl ToVariant and that doesn't seem trivial to me?

Edit : maybe it wouldnt be so hard, the user would just have to write

#[derive(ToVariant)]
MyStruct<T : ToVariant> /* ... */

if needed, but i guess we'd have to see if the error msg is nice, maybe leave that to me i'd like to help and try to improve the macros (plus i have a code base to test it right after so)
Is that okay if I try to help?

@Bromeon
Copy link
Member

Bromeon commented Oct 16, 2022

Yeah, gladly! 😊

Maybe I'll merge the existing PR but keep this issue open; then you can use it as a base to address the where problem (in a separate PR). Would be good if you could cover it in tests as well!

bors bot added a commit that referenced this issue Oct 16, 2022
961: Fixed ToVariant/FromVariant derive for generic types with bounds r=Bromeon a=Bromeon

Fixes part of #957; but there's a separate problem with `where` clauses (see description).

Co-authored-by: Jan Haller <[email protected]>
@astrale-sharp
Copy link
Author

astrale-sharp commented Oct 16, 2022

Hey, I'm having trouble launching the tests in test_derive.rs with cargo test and other cargo test stuff i tried (-p, etc)
A little help? ^^
I found ./check.sh itest but it fails with > cp target/debug/gdnative_test* test/project/lib/ cp: impossible d'évaluer 'target/debug/gdnative_test*': Aucun fichier ou dossier de ce type

@Bromeon
Copy link
Member

Bromeon commented Oct 17, 2022

Did you run sh check.sh itest from the repository root?

It should create some dynamic library files (for example .dll or .so) in target/debug/, can you check they are there?

Otherwise feel free to use the CI for testing, just amend your commits and keep force-pushing (git push --force-with-lease) 🙂

@Bogay
Copy link
Contributor

Bogay commented Oct 18, 2022

Hey, I'm having trouble launching the tests in test_derive.rs with cargo test and other cargo test stuff i tried (-p, etc) A little help? ^^ I found ./check.sh itest but it fails with > cp target/debug/gdnative_test* test/project/lib/ cp: impossible d'évaluer 'target/debug/gdnative_test*': Aucun fichier ou dossier de ce type

Maybe related to #965

@chitoyuu
Copy link
Contributor

chitoyuu commented Jan 9, 2023

Fixing the macros probably won't be easy tho because we would need to know if we need T to impl ToVariant and that doesn't seem trivial to me?

This is correct. Macros operate only at the token level, so any automatic bounds are at most guesses. serde handles this problem by allowing users to override any automatic bounds with a item-level attribute parameter.

Now that we have ItemAttr we can probably do this as well, perhaps with a similar syntax.

@chitoyuu chitoyuu modified the milestones: v0.11.x, v0.12.0 Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: export Component: export (mod export, derive)
Projects
None yet
Development

No branches or pull requests

4 participants