Rewrite the derive macros related to `GodotConvert` #599

lilizoey · 2024-02-07T00:28:31Z

Rewrites the GodotConvert + related macros.

Removes ToGodot and FromGodot derive macros, they are now also derived by GodotConvert.

Largely follows what's laid out in this comment #452 (comment), however for implicit discriminants i follow rust's algorithm for calculating them instead as otherwise you can get some unexpected inconsistencies.

As for the questions from this comment :

1. For enums, would the absence of `#[godot(via = ...)]` cause an error or be equivalent to `#[godot(via = GString)]`?

2. Do you think we should consider `StringName` for enumerators, as they are immutable and could be interned?

3. Also, I would consider `#[derive(GodotConvert)]` being enough in this case, as [stated above](https://github.com/godot-rust/gdext/issues/452#issuecomment-1925459754). This could however be implemented separately/later. Possible syntax if I only want one direction:
   ```rust
   #[derive(GodotConvert)]
   #[godot(to, via = i64)] // or 'from' 
   ```

This PR currently does the following:

This causes an error. I could go either way though. Would likely infer transparent for structs and GString for enums in that case.
This is not implemented currently, it shouldn't be hard to support StringName naively. However if we want StringName to be interned or do anything differently than GString that would be a bit more complicated.
GodotConvert also derives ToGodot and FromGodot, but there is no opt-out currently.

closes #452

GodotRust · 2024-02-07T00:34:21Z

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

Bromeon

Great work, very rigorous! 👍

Added some comments, mostly about some naming consistency aspects.

godot-macros/src/derive/data_models/mod.rs

Bromeon · 2024-02-07T20:32:17Z

godot-macros/src/derive/data_models/godot_attribute.rs

+        if parser.handle_alone("transparent")? {
+            parser.finish()?;
+
+            return Ok(Self::Transparent { span });
+        }
+
+        let via_type = parser.handle_ident_required("via")?;
+
+        let via_type = ViaType::parse_ident(via_type)?;
+
+        parser.finish()?;
+
+        Ok(Self::Via { via_type })


Suggested change

if parser.handle_alone("transparent")? {

parser.finish()?;

return Ok(Self::Transparent { span });

}

let via_type = parser.handle_ident_required("via")?;

let via_type = ViaType::parse_ident(via_type)?;

parser.finish()?;

Ok(Self::Via { via_type })

let attr = if parser.handle_alone("transparent")? {

Self::Transparent { span }

} else {

let via_type = parser.handle_ident_required("via")?;

let via_type = ViaType::parse_ident(via_type)?;

Self::Via { via_type }

};

parser.finish()?;

Ok(via)

would only need a single finish() call.

But we may anyway need to rewrite this a bit in the future, as it will be possible to specify multiple keys, not just one. So up to you.

Please avoid an empty line after every single statement though 😉

i rewrote it a bit by adding an extra function that takes the parser so i can just do

pub fn parse_attribute(declaration: &Declaration) -> ParseResult<Self> { let mut parser = KvParser::parse_required(declaration.attributes(), "godot", declaration)?; let attribute = Self::parse(&mut parser)?; parser.finish()?; Ok(attribute) }

also i made via use handle_ident instead cause i just realized handle_ident_required may provide suboptimal error messages. as in it'd complain that via is required, when really either transparent or via is required.

godot-macros/src/derive/data_models/godot_attribute.rs

godot-macros/src/derive/data_models/godot_convert.rs

godot-macros/src/derive/data_models/unit_only_enum.rs

Bromeon · 2024-02-07T21:05:03Z

godot-macros/src/derive/data_models/unit_only_enum.rs

+    fn create_discriminant_mapping(
+        variants: Vec<UnitEnumVariant>,
+    ) -> ParseResult<(Vec<Ident>, Vec<Literal>)> {
+        // See here for how implicit discriminants are decided
+        // https://doc.rust-lang.org/reference/items/enumerations.html#implicit-discriminants
+        let mut names = Vec::new();
+        let mut discriminants = Vec::new();
+
+        let mut last_discriminant = None;
+        for variant in variants.into_iter() {
+            let discriminant_span = variant.discriminant_span();
+
+            let discriminant = match variant.discriminant_as_i64()? {
+                Some(discriminant) => discriminant,
+                None => last_discriminant.unwrap_or(0) + 1,
+            };
+            last_discriminant = Some(discriminant);
+
+            let mut discriminant = Literal::i64_unsuffixed(discriminant);
+            discriminant.set_span(discriminant_span);
+
+            names.push(variant.name);
+            discriminants.push(discriminant)
+        }
+
+        Ok((names, discriminants))
+    }


It's probably OK to talk about "discriminants" here, as they refer to the syntactic part at this stage. For example, they can be implicit (SomeName) or explicit (SomeName = 12).

Calling them "ords" afterwards would then help to separate this syntactic from the semantic concept -- tokens vs. numbers.

godot-macros/src/derive/data_models/unit_only_enum.rs

itest/rust/src/register_tests/derive_variant_test.rs

astrale-sharp · 2024-02-07T22:39:42Z

Like, this is very cool , it will be much nicer to work with c like enums from godot

but we do lose the possibility to derive to/from godot for structs with more than one field and enum with variants containing data
We still plan to support that right?

Bromeon · 2024-02-07T23:03:02Z

@lilizoey

This PR currently does the following:

This causes an error. I could go either way though. Would likely infer transparent for structs and GString for enums in that case.

This is not implemented currently, it shouldn't be hard to support StringName naively. However if we want StringName to be interned or do anything differently than GString that would be a bit more complicated.

GodotConvert also derives ToGodot and FromGodot, but there is no opt-out currently.

All good for me, thanks! I think the defaults we can discuss in detail when it comes to the follow-up PRs 👍

As for StringName, Godot will intern it automatically. Even if we need to reconstruct it, that's probably fine; the situation might be mitigated with #531.

Bromeon · 2024-02-07T23:03:13Z

@astrale-sharp

but we do lose the possibility to derive to/from godot for structs with more than one field and enum with variants containing data
We still plan to support that right?

Yes, we are currently in step 1 according to this comment.

Bromeon · 2024-02-09T09:19:20Z

godot-macros/src/derive/data_models/c_style_enum.rs

+    }
+
+    fn create_discriminant_mapping(
+        variants: Vec<CStyleEnumVariant>,


Probably also here:

Suggested change

variants: Vec<CStyleEnumVariant>,

enumerators: Vec<CStyleEnumerator>,

Especially since in our case Variant has a prominent different meaning, this helps keep terminology apart.

Bromeon · 2024-02-09T09:28:03Z

godot-macros/src/derive/derive_to_godot.rs

+/// Derives `ToGodot` for newtype structs.
+fn to_newtype(name: &Ident, field: &NewtypeStruct) -> ParseResult<TokenStream> {
+    let field_name = field.field_name();
+    let via_type = &field.ty;
+
+    Ok(quote! {
+        impl ::godot::builtin::meta::ToGodot for #name {
+            fn to_godot(&self) -> #via_type {
+                ::godot::builtin::meta::ToGodot::to_godot(&self.#field_name)
+            }
+
+            fn into_godot(self) -> #via_type {
+                ::godot::builtin::meta::ToGodot::into_godot(self.#field_name)
+            }
+        }
+    })
+}
+
+/// Derives `ToGodot` for enums with a via type of integers.
+fn to_enum_int(name: &Ident, enum_: &CStyleEnum, int: &Ident) -> ParseResult<TokenStream> {
+    let discriminants = enum_.discriminants();
+    let names = enum_.names();
+
+    Ok(quote! {
+        impl ::godot::builtin::meta::ToGodot for #name {
+            fn to_godot(&self) -> #int {
+                match self {
+                    #(
+                        #name::#names => #discriminants,
+                    )*
+                }
+            }
+        }
+    })
+}
+
+/// Derives `ToGodot` for enums with a via type of `GString`.
+fn to_enum_string(name: &Ident, enum_: &CStyleEnum) -> ParseResult<TokenStream> {
+    let names = enum_.names();
+    let names_str = names.iter().map(ToString::to_string).collect::<Vec<_>>();
+
+    Ok(quote! {
+        impl ::godot::builtin::meta::ToGodot for #name {
+            fn to_godot(&self) -> ::godot::builtin::GString {
+                match self {
+                    #(
+                        #name::#names => #names_str.into(),
+                    )*
+                }
+            }
+        }
+    })
+}


These methods never return Err.

Also, typically the methods that generate token streams start with make_. I would probably also include the generated item in the name; it doesn't hurt if they become a bit longer. For example:

make_togodot_for_newtype_struct make_togodot_for_int_enum make_togodot_for_gstring_enum

Bromeon · 2024-02-09T09:33:17Z

itest/rust/src/object_tests/property_test.rs

@@ -147,18 +147,18 @@ impl INode for HasProperty {

 #[derive(Default, Copy, Clone)]
 #[repr(i64)]
-enum SomeCStyleEnum {
+enum SomeUnitEnum {


This renaming can probably be undone now 🙂

Bromeon · 2024-02-09T18:34:02Z

godot-macros/src/lib.rs

+/// This expects a derived [`GodotConvert`](../builtin/meta/trait.GodotConvert.html) implementation, using a manual
+/// implementation of `GodotConvert` may lead to incorrect values being displayed in Godot.


Have you encountered this? I think it's good advice, but I wonder what concretely you thought of here?

if you did say

#[derive(Var, Export)] #[godot(via = i64)] pub enum MyEnum { A, B, } impl GodotConvert for MyEnum { type Via = i64; } impl FromGodot for MyEnum { fn try_from_Godot(via: Self::Via) -> Result<..> { match via { 1 => Ok(Self::A), 0 => Ok(Self::B), _ => Err(..) } } } // And corresponding ToGodot

Then the Var and Export impl would map A and B to each other, since the assumption is that A = 0 and B = 1 since that's their rust discriminants.

We could probably fix this by making the property hint impls instead do something like

let mut s = Vec::new(); s.push(format!("{}:{}", "A", Self::A.to_godot()); // repeat for each variant s.join(",").into()

Though this would add a runtime cost to using it in the editor, but also that's maybe not a big deal? rust might be able to optimize it fairly well anyway.

This could still lead to inconsistencies if FromGodot and ToGodot arent inverses. But that's just a general issue with implementing those two traits.

Thanks for elaborating!

Your suggestion sounds good, but would require that any ToGodot implementation by the user here also support Display -- and that Display then match the property-hint format. It's mixing responsibilities a bit. Either a dedicated method/trait, or conversion to Variant and using its Display might be an option...

But for now, the limitation is OK I think, especially since it's documented.

…t` derive macros

Document things properly Other cleanup

Bromeon

Thanks a lot, looks much cleaner now!

Bromeon · 2024-02-11T11:17:59Z

godot-macros/src/lib.rs

+/// This expects a derived [`GodotConvert`](../builtin/meta/trait.GodotConvert.html) implementation, using a manual
+/// implementation of `GodotConvert` may lead to incorrect values being displayed in Godot.


Thanks for elaborating!

Your suggestion sounds good, but would require that any ToGodot implementation by the user here also support Display -- and that Display then match the property-hint format. It's mixing responsibilities a bit. Either a dedicated method/trait, or conversion to Variant and using its Display might be an option...

But for now, the limitation is OK I think, especially since it's documented.

lilizoey added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels Feb 7, 2024

lilizoey force-pushed the refactor/rewrite-convert-derives branch from 5a90282 to c7838f5 Compare February 7, 2024 19:24

lilizoey marked this pull request as ready for review February 7, 2024 19:32

Bromeon reviewed Feb 7, 2024

View reviewed changes

lilizoey force-pushed the refactor/rewrite-convert-derives branch from c7838f5 to 28cd3c1 Compare February 8, 2024 17:14

Bromeon reviewed Feb 9, 2024

View reviewed changes

lilizoey added 2 commits February 10, 2024 17:00

Rewrite the GodotConvert, ToGodot, FromGodot, Var, and `Expor…

ff11540

…t` derive macros

Make a data_models module for all the data models in derive

4b445c6

Document things properly Other cleanup

lilizoey force-pushed the refactor/rewrite-convert-derives branch from 28cd3c1 to 4b445c6 Compare February 10, 2024 16:00

Bromeon approved these changes Feb 11, 2024

View reviewed changes

Bromeon added this pull request to the merge queue Feb 11, 2024

Merged via the queue into godot-rust:master with commit 535ec61 Feb 11, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite the derive macros related to `GodotConvert` #599

Rewrite the derive macros related to `GodotConvert` #599

lilizoey commented Feb 7, 2024 •

edited

Loading

GodotRust commented Feb 7, 2024

Bromeon left a comment

Bromeon Feb 7, 2024

lilizoey Feb 8, 2024

Bromeon Feb 7, 2024

astrale-sharp commented Feb 7, 2024

Bromeon commented Feb 7, 2024

Bromeon commented Feb 7, 2024

Bromeon Feb 9, 2024

Bromeon Feb 9, 2024

Bromeon Feb 9, 2024

Bromeon Feb 9, 2024

lilizoey Feb 10, 2024 •

edited

Loading

Bromeon Feb 11, 2024

Bromeon left a comment

Bromeon Feb 11, 2024

	variants: Vec<CStyleEnumVariant>,
	enumerators: Vec<CStyleEnumerator>,

		/// This expects a derived [`GodotConvert`](../builtin/meta/trait.GodotConvert.html) implementation, using a manual
		/// implementation of `GodotConvert` may lead to incorrect values being displayed in Godot.

Rewrite the derive macros related to GodotConvert #599

Rewrite the derive macros related to GodotConvert #599

Conversation

lilizoey commented Feb 7, 2024 • edited Loading

GodotRust commented Feb 7, 2024

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrale-sharp commented Feb 7, 2024

Bromeon commented Feb 7, 2024

Bromeon commented Feb 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilizoey Feb 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rewrite the derive macros related to `GodotConvert` #599

Rewrite the derive macros related to `GodotConvert` #599

lilizoey commented Feb 7, 2024 •

edited

Loading

lilizoey Feb 10, 2024 •

edited

Loading