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

Implement derive macro for ToVariant and FromVariant #189

Merged
merged 3 commits into from Aug 31, 2019
Merged

Implement derive macro for ToVariant and FromVariant #189

merged 3 commits into from Aug 31, 2019

Conversation

ghost
Copy link

@ghost ghost commented Aug 29, 2019

The derive macro does the following mapping between Rust structures and Godot types:

  • Newtype(inner) is unwrapped to inner
  • Tuple(a, b, c) is represented as a VariantArray ([a, b, c])
  • Struct { a, b, c } is represented as a Dictionary ({ "a": a, "b": b, "c": c })
  • Unit is represented as an empty Dictionary ({})
  • Enum::Variant(a, b, c) is represented as an externally tagged Dictionary ({ "Variant": [a, b, c] })

For ease of use, ToVariant and FromVariant are also implemented for Option<T>, Result<T, E> and Vec<T>. Vec<T> silently drops values for which from_variant failed, but the composition with Option<T> works nicely, in case it's necessary to know which items failed.

Support for more collections should probably come in the form of iterator support for Godot types, as there is no real way to return or take a BTreeMap from Godot, and maps in general can probably have surprising behaviors when their keys are non-primitive (value equality in Rust vs possibly reference equality in other languages).

Fixes #186.

@karroffel
Copy link
Member

This is really cool, I like it! Apart from the code for the generic bounds this is also relatively simple to follow, it's nice :)

@ghost
Copy link
Author

ghost commented Aug 30, 2019

The code for the generic bounds are initially adapted from serde_derive and is still going through changes. I'll refactor it to make it easier to read!

@karroffel
Copy link
Member

Yes I saw the comment that it was taken from serde, so no worries. I think this approach is fine and once this is out of draft phase I would have no problem merging this.

@ghost ghost changed the title [WIP] Implement derive macro for ToVariant Implement derive macro for ToVariant and FromVariant Aug 30, 2019
@ghost ghost marked this pull request as ready for review August 30, 2019 13:23
@ghost
Copy link
Author

ghost commented Aug 30, 2019

Added derive macro for FromVariant, and implemented the conversion traits on some common types. I think it's ready now.

Rebased on master because of merge conflicts.

@ghost
Copy link
Author

ghost commented Aug 30, 2019

If we want Option<T> to be stricter we can write this instead for FromVariant:

fn from_variant(variant: &Variant) -> Option<Self> {
    if variant.is_nil() {
        Some(None)
    }
    else {
        T::from_variant(variant).map(Some)
    }
}

...so it returns None for non-Nil values that can't be converted to T.

But then we can no longer write Vec<Option<T>> for an array that may contain things that are not T. Maybe a new wrapper type needed here?


Added one.

gdnative-core/src/variant.rs Outdated Show resolved Hide resolved
gdnative-core/src/variant.rs Outdated Show resolved Hide resolved
gdnative-derive/src/derive_conv_variant.rs Outdated Show resolved Hide resolved
@ghost ghost mentioned this pull request Aug 31, 2019
toasteater added 3 commits August 31, 2019 14:25
The macro does the following mapping between Rust structures and Godot types:

- `Newtype(inner)` are mapped to `inner`
- `Tuple(a, b, c)` are mapped to `[a, b, c]`
- `Struct { a, b, c }` are mapped to `{ "a": a, "b": b, "c": c }`
- `Unit` are mapped to `{}`
- `Enum::Variant(a, b, c)` are mapped to `{ "Variant": [a, b, c] }`
Allows taking and returning `Option<T>`, `Result<T, E>`, and `Vec<T>` directly.
Added `FromVariant` wrapper `MaybeNot<T>` with looser semantic than `Option`.

Fixes #186.
With Variant conversion traits becoming derivable, type errors are no longer
limited to `VariantType` mismatches. The error message is expanded to be more
informative for non-primitive types.
@karroffel karroffel merged commit cf730c0 into godot-rust:master Aug 31, 2019
@ghost ghost deleted the feature/derive-tovariant-fromvariant branch September 1, 2019 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allow Option<T> as parameter types
1 participant