Add PhantomData<T> support for the GodotConvert derive macro by ProphetOSpam · Pull Request #1619 · godot-rust/gdext

ProphetOSpam · 2026-05-25T17:59:56Z

Fix for #1563

You can now do stuff like:

#[derive(GodotConvert, PartialEq, Debug)]
#[godot(transparent)]
struct NamedPhantomNewtype<T> {
    field1: Vector2,
    _marker: PhantomData<T>,
}

Additionally, as a consequence of the changes, you can now have generics on non-PhantomData fields as well, like:

#[derive(GodotConvert, PartialEq, Debug)]
#[godot(transparent)]
struct NamedPhantomNewtype<T: Element> {
    field1: Array<T>,
    _marker: PhantomData<T>,
}

I'm not sure this is intended behavior, however. I can't see a reason why not, but I'd like clarification. If it isn't I'll go add a check to make sure you can't use generics on non-PhantomData fields.

ProphetOSpam · 2026-05-25T18:06:09Z

Also, right now this only supports PhantomData with structs; enums will still error in the same way.

I think there probably should be support for enums as well; What are your thoughts?

Bromeon

Thanks for your contribution! 🙂

Would be interesting to hear about your use case -- where did you need this? Possibly an example could also be briefly mentioned in docs (no need for code).

I think there probably should be support for enums as well; What are your thoughts?

Let's not add features unless we need them. This PR already adds some complexity for a very niche feature, which also needs to be maintained -- hence my question about use case above 😉

Additionally, as a consequence of the changes, you can now have generics on non-PhantomData fields as well, like:

I don't see something that speaks against it, does the generated code do what's expected? Maybe a short itest would be nice.

Your implementation hardcodes PhantomData as an identifier. Could you elaborate this design choice? AFAIR the original idea was to allow any ZST members, but the hardcoding has multiple problems:

It doesn't allow MyGhostData<T>(PhantomData<T>).
It breaks if PhantomData is renamed on import or type-aliased (edge case).
It breaks if another type is named PhantomData (very edge case).

Not huge deals, but those are inherent limitations of proc-macros operating on the syntax level.

Bromeon · 2026-05-25T19:51:02Z

-        }
-
-        let data = ConvertType::parse_declaration(item)?;
+        let data = ConvertType::parse_declaration(item.clone())?;


Is it possible to avoid this clone by changing the parameter to take a shared-ref? Might propagate to other code that accesses it.

Or would it require an internal clone()? In that case there's no point in refactoring.

Bromeon · 2026-05-25T19:51:54Z

            ty_name: name,
+            where_clause: where_clause.clone(),
+            generic_params: generic_params.clone(),
            convert_type: data,


Also here, if item doesn't need to be cloned, it could just be destructured, and we wouldn't need to needlessly copy around these parts.

It works with some shuffling. This commit also fixed this comment, I forgot to split up the commit. 96ce47f

Bromeon · 2026-05-25T19:53:05Z

    ///
    /// If `None`, then this represents a tuple-struct with one field.
-    pub name: Option<Ident>,
+    pub name: FieldType,


"name" vs. "field type". One of the two is badly named.

Also, unrelated to your changes -- I noticed that we call the type NewtypeStruct, which is a bit confusing given that newtypes typically have shape Struct(InnerType). Could you add an extra /// line for the struct to explain that it allows multiple named or tuple fields, as long as all but one are ZSTs?

This is still open.

Bromeon · 2026-05-25T19:59:06Z

+                fn phantom_predicate(field: &TupleField) -> bool {
+                    // Some types we don't care about are not paths, like references
+                    if let Some(path) = field.ty.as_path() {
+                        // This unwrap only fails if the field had no type specified, which isn't valid code anyways.
+                        return path.segments.last().unwrap().ident
+                            == Ident::new("PhantomData", Span::mixed_site());
+                    }
+                    false
+                }


phantom_predicate is rather unclear. Why not simply is_phantom_data?

Also, can you use this maybe?

gdext/godot-macros/src/util/mod.rs

Lines 247 to 253 in 1a9e41f

/// Gets the right-most type name in the path.

pub(crate) fn extract_typename(ty: &venial::TypeExpr) -> Option<venial::PathSegment> {

match ty.as_path() {

Some(mut path) => path.segments.pop(),

_ => None,

}

}

And maybe more generally, checking on the syntax level against types is brittle, and we can't easily cover edge cases. Do we have to do it?

Bromeon · 2026-05-25T20:01:25Z

+                fn phantom_predicate(field: &NamedField) -> bool {
+                    // Some types we don't care about are not paths, like references
+                    if let Some(path) = field.ty.as_path() {
+                        // This unwrap only fails if the field had no type specified, which isn't valid code anyways.
+                        return path.segments.last().unwrap().ident
+                            == Ident::new("PhantomData", Span::mixed_site());
+                    }
+                    false
+                }
+
+                let mut non_phantom_fields = fields
+                    .fields
+                    .items()
+                    .filter(|field| !phantom_predicate(field));
+
+                let maybe_field = non_phantom_fields.next();
+
+                let total_count = if maybe_field.is_none() {
+                    0
+                } else {
+                    non_phantom_fields.count() + 1
+                };
+
+                if total_count != 1 {
                    return bail!(
                        &fields.fields,
-                        "GodotConvert expects a struct with a single field, not {} fields",
-                        fields.fields.len()
+                        "GodotConvert expects a struct with a single non-PhantomData field, not {} fields",
+                        total_count
                    );
                }


Code duplication...

Bromeon · 2026-05-25T20:04:08Z

+                        // If we're in here we have at least 1 bound, and rust doesn't error if the
+                        // bound is already 'static, i.e. `T: 'static + 'static` works. It's a
+                        // little hacky, but there's not really a reason to inspect all of the
+                        // bound's tokens if we really don't care what it is.
+                        bound
+                            .tokens
+                            .append(&mut quote! {+ 'static}.into_iter().collect());


You explain the hack but not why it's necessary.

Also nitpick, please use available 120-145 chars per line.

here's the fix. Is this good? 84c620f

GodotRust · 2026-05-25T20:14:07Z

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

ProphetOSpam · 2026-05-25T21:12:25Z

Would be interesting to hear about your use case -- where did you need this? Possibly an example could also be briefly mentioned in docs (no need for code).

In my project I have an Id<T> that is a key for a Registry<T>; internally its just a uuid. I would make them from rust and godot, and pass them between each other. Without this I just have a separate GId with no generics that I convert Id<T> to and from when I need to. With this I don't have to write nearly as much boilerplate.

This:

pub struct Id<T>(u32, Phantom<T>);

#[derive(GodotConvert)]
pub struct GId(u32);

impl<T> From<Id<T>> for GId {
    fn from(value: Id<T>) -> GId {
        Self(value.0)
    }
}

impl<T> From<GId> for Id<T> {
    fn from(value: GId) -> Id<T> {
        Self(value.0, PhantomData)
    }
}

becomes this:

#[derive(GodotConvert)]
pub struct Id<T>(u32, Phantom<T>);

Additionally, I use Id<T> in some other structs that have to be translated to godot. For example, I have a Card struct which contains a field def_id: Id<CardDef> for its definition, that way functionality can be shared among multiple instances.

pub struct CardDef {
   ...
}

pub struct Card {
   pub def_id: Id<CardDef>,
   ...
}

These cards can be created in rust or through godot as well (I make these GodotClasses), and it would have to be a lot more boilerplate in order to create a GCard and for them as well, which instead of an Id contains a GId.

#[derive(GodotClass)]
#[class(no_init)]
pub struct GCard {
   #[var]
   pub def_id: GId,
   ...
}

// More `From` impls
...

Something I just realized is that in order to do what I was saying above there needs to be PhantomData handling for Var as well. Woops. Should I add this functionality as well?

I'll add a summary of the above as an example for the relevant documentation.

Let's not add features unless we need them. This PR already adds some complexity for a very niche feature, which also needs to be maintained -- hence my question about use case above

Oki doki, sounds good. I don't think there's an alternative to the above solution (if there is, I'd be happy to know!).

I don't see something that speaks against it, does the generated code do what's expected? Maybe a short itest would be nice.

I believe it does. If you look here, there's an example with Array<T>

Your implementation hardcodes PhantomData as an identifier. Could you elaborate this design choice? AFAIR the original idea was to allow any ZST members, but the hardcoding has multiple problems:

You are very correct. It should not be checking for the name PhantomData. I think rewriting it to have compile time assertions that only 1 type is not a ZST with size_of::<T>() should work.

ProphetOSpam · 2026-05-25T21:26:59Z

Actually, we need to know what field is the target of the newtype for creating the struct in from_godot. How about a #[target] attribute that tells you the one target field of the newtype, and then having const assertions that every other field is a ZST?

like:

type Ghost<T> = PhantomData<T>;

struct MyStruct<T, U>(
   PhantomData<T>,
   #[target]
   GString,
   Ghost<T>,
   // The following field would error because its not ZST
   bool
);

ProphetOSpam · 2026-05-25T22:23:18Z

I'm going to rewrite it to include const assertions instead of looking at PhantomData directly, so I left the comments pertaining to that unaddressed, as they'll be irrelevant by then.

ProphetOSpam · 2026-05-27T20:59:09Z

Actually, instead of a #[target] on the target field, there should instead be an #[ignore] on the zst fields. Otherwise it's not backwards compatible unless you have a special case where you don't need #[target] on a struct with one field, which is less intuitive.

like:

type Ghost<T> = PhantomData<T>;

struct MyStruct<T, U>(
   #[ignore]
   PhantomData<T>,
   GString,
   #[ignore]
   Ghost<T>,
)

ProphetOSpam · 2026-05-27T21:02:24Z

Also, I won't be able to work much on this for the next 2 weeks, but I will see it through

Bromeon · 2026-05-28T07:08:39Z

Actually, instead of a #[target] on the target field, there should instead be an #[ignore] on the zst fields. Otherwise it's not backwards compatible

Technically it's still backwards-compatible, because we didn't support more than 1 field so far.

But it's usually nicer when adding a field, to also add the attribute along with it, rather than needing to change a different field. I would recommend #[skip] rather than #[ignore].

Another thought I had whether we should scope this to #[godot(skip)]. We don't do this anywhere else though, except on the #[godot(transparent)] of the class itself. But I definitely don't want to adopt this convention on GodotClass like #[godot(func)] 🙂

ProphetOSpam · 2026-06-20T20:00:59Z

This would be a really helpful feature, but alas.
rust-lang/rust#95383

Bromeon

@ProphetOSpam do you have any update on this? 🙂

CI is currently red, too.

Bromeon · 2026-06-23T17:26:16Z

    ///
    /// If `None`, then this represents a tuple-struct with one field.
-    pub name: Option<Ident>,
+    pub name: FieldType,


This is still open.

Bromeon · 2026-06-23T17:26:34Z

+                fn phantom_predicate(field: &TupleField) -> bool {
+                    // Some types we don't care about are not paths, like references
+                    if let Some(path) = field.ty.as_path() {
+                        // This unwrap only fails if the field had no type specified, which isn't valid code anyways.
+                        return path.segments.last().unwrap().ident
+                            == Ident::new("PhantomData", Span::mixed_site());
+                    }
+                    false
+                }


Bromeon · 2026-06-23T17:26:43Z

+                fn phantom_predicate(field: &NamedField) -> bool {
+                    // Some types we don't care about are not paths, like references
+                    if let Some(path) = field.ty.as_path() {
+                        // This unwrap only fails if the field had no type specified, which isn't valid code anyways.
+                        return path.segments.last().unwrap().ident
+                            == Ident::new("PhantomData", Span::mixed_site());
+                    }
+                    false
+                }
+
+                let mut non_phantom_fields = fields
+                    .fields
+                    .items()
+                    .filter(|field| !phantom_predicate(field));
+
+                let maybe_field = non_phantom_fields.next();
+
+                let total_count = if maybe_field.is_none() {
+                    0
+                } else {
+                    non_phantom_fields.count() + 1
+                };
+
+                if total_count != 1 {
                    return bail!(
                        &fields.fields,
-                        "GodotConvert expects a struct with a single field, not {} fields",
-                        fields.fields.len()
+                        "GodotConvert expects a struct with a single non-PhantomData field, not {} fields",
+                        total_count
                    );
                }


Bromeon · 2026-06-23T17:27:40Z

 /// ```
 ///
-/// However, it will not work for structs with more than one field, even if that field is zero sized:
+/// It will not work for structs with more than one field, unless the extra fields are `PhantomData`


Suggested change

/// It will not work for structs with more than one field, unless the extra fields are `PhantomData`

/// It will not work for structs with more than one field, unless the extra fields are zero-sized:

But you should then mention the attribute to make it work, no?

Bromeon · 2026-06-23T21:31:30Z

+        if let Some(params) = &mut maybe_params {
+            for (param, _) in params.params.iter_mut() {
+                if param.is_ty() || param.is_lifetime() {
+                    if let Some(bound) = &mut param.bound {
+                        // We have at least 1 bound, and rust doesn't error if the bound is already 'static, i.e. `T: 'static + 'static` works.
+                        // If it were to error, we would have to inspect the bounds to make sure the tokens `+ 'static` are valid. It feels a
+                        // little hacky, but there's not really a reason to inspect all of the bound's tokens if rust doesn't really care what
+                        // they are.
+                        bound
+                            .tokens
+                            .append(&mut quote! {+ 'static}.into_iter().collect());
+                    } else {
+                        param.bound = Some(GenericBound {
+                            tk_colon: Punct::new(':', Spacing::Alone),
+                            tokens: quote! {'static}.into_iter().collect(),
+                        });
+                    }
+                }
+            }
+        }


This is indeed pretty hacky. It seems we need this for Element: 'static?

Can this not be derived by the compiler, and we not do something like this? (untested)

// where-clause that adds `Self: 'static`, keeping any user clause. let element_where = match where_clause { Some(w) => quote! { #w, Self: 'static }, None => quote! { where Self: 'static }, }; impl #generic_params ::godot::meta::Element for #name #generic_args #element_where {}

This is still unaddressed.

ProphetOSpam · 2026-06-23T22:13:25Z

I've got some local changes I haven't been able to finish lately, I'm actually working on it right now.

ProphetOSpam · 2026-06-24T03:31:30Z

Also, is there a reason the Var derive macro docs say that it is specifically for enums? It seems that it can be used for any type that is GodotConvert (if you threw on the extra information from convert so generics work).

relevant docs:
https://godot-rust.github.io/docs/gdext/master/godot/prelude/derive.Var.html

source:

gdext/godot-macros/src/derive/derive_var.rs

Lines 14 to 44 in 477501f

    
           /// Derives `Var` for the given declaration. 
        
           /// 
        
           /// This uses `ToGodot` and `FromGodot` for the `var_get` and `var_set` implementations. 
        
           /// Property hints are derived from `GodotConvert::shape()`. 
        
           pub fn derive_var(item: venial::Item) -> ParseResult<TokenStream> { 
        
               let convert = GodotConvert::parse_declaration(item)?; 
        
               let name = convert.ty_name; 
        
               Ok(quote! { 
        
                   impl ::godot::register::property::Var for #name { 
        
                       type PubType = Self; 
        
                       fn var_get(field: &Self) -> <Self as ::godot::meta::GodotConvert>::Via { 
        
                           ::godot::meta::ToGodot::to_godot(field) 
        
                       } 
        
                       fn var_set(field: &mut Self, value: <Self as ::godot::meta::GodotConvert>::Via) { 
        
                           *field = ::godot::meta::FromGodot::from_godot(value); 
        
                       } 
        
                       fn var_pub_get(field: &Self) -> Self::PubType { 
        
                           field.clone() 
        
                       } 
        
                       fn var_pub_set(field: &mut Self, value: Self::PubType) { 
        
                           *field = value; 
        
                       } 
        
                   } 
        
               }) 
        
           }

I'm asking because of this comment earlier,

Something I just realized is that in order to do what I was saying above there needs to be PhantomData handling for Var as well. Woops. Should I add this functionality as well?

I hadn't yet looked at the implementation of var, and thought it was more complicated than this. Like I said above, it seems like the Var derive macro should support anything that implements GodotConvert. Should this be in a separate issue for clarity?

Bromeon · 2026-06-27T14:14:33Z

Regarding Var derive, we should probably update the docs to accurately reflect the current state (C-style enums + transparent/newtype-like structs).

If Var blocks your generics, you could consider opening a separate PR to add support for this 🙂 but I wouldn't want to add it without a real use case, or if it comes with massive complexity.

ProphetOSpam · 2026-06-27T21:24:16Z

If Var blocks your generics, you could consider opening a separate PR to add support for this 🙂 but I wouldn't want to add it without a real use case, or if it comes with massive complexity.

I'll go ahead and do that. The actual fix is really simple, just adding the generics provided from GodotConvert::parse_declaration to the impl.

ProphetOSpam · 2026-06-27T23:31:57Z

I realized that I had added the generic functionality to parse_declaration in this PR.

Correct me if I'm wrong, but I don't think Github supports something like a child PR. Should I add the changes to make Var generic in this PR instead?

Bromeon · 2026-06-28T08:08:03Z

Should I add the changes to make Var generic in this PR instead?

If they're simple, yes. Please consider separate commit -- total 2 commits, one for ZST support in GodotConvert, one for generics in Var.

There's no "child PR" but you can target another PR's branch when opening it, so diff is relative to that. But 2 commits in 1 PR is probably easier.

ProphetOSpam · 2026-07-01T01:58:39Z

I didn't change the documentation for Var or Export (see below), which both still say that they are for enums, and I noticed that GodotConvert's docs are also wrong.

Here's all of them that I could find:

(GodotConvert works on enums too)

gdext/godot-macros/src/lib.rs

Line 1352 in 770cf1a

    
           /// Derive macro for [`GodotConvert`](../meta/trait.GodotConvert.html) on structs.

(These both work on structs too)

gdext/godot-macros/src/lib.rs

Line 1484 in 770cf1a

/// Derive macro for [`Var`](../register/property/trait.Var.html) on enums.

gdext/godot-macros/src/lib.rs

Line 1493 in 770cf1a

    
           /// Derive macro for [`Export`](../register/property/trait.Export.html) on enums.

I feel like this should contextually belong to a separate issue? This doesn't really have anything to do with GodotConvert ZSTs. What are your thoughts? Or is it really even worth fixing.

Bromeon · 2026-07-01T06:54:20Z

Ah, I somehow understood it's related to this. If not, it can be separate issue/PR 🙂

ProphetOSpam · 2026-07-13T01:16:30Z

Sorry, I forgot to mark this as ready for review, it should all be done

Bromeon

Thanks! Several comments from last time are still unaddressed, e.g. the 'static hack you didn't respond to. There are also new soundness holes with ZST conjuring.

Apart from that, there's quite a bit of premature abstraction (Field trait, plural versions of types, etc). Some ideas on simplification -- we can reduce both FieldIdentifier/FieldIdentifiers, and the parallel zst_fields/zst_tys as well:

Removetrait Field -- it only exposes get_attributes() and the call sites diverge right after. Instead:

// returns (sized_index, zst_indices)
fn partition_fields<'a>(
    attrs: impl Iterator<Item = &'a [venial::Attribute]>,
    context: impl ToTokens,
) -> ParseResult<(usize, Vec<usize>)> {
    let mut sized = None;
    let mut zsts = vec![];
    for (i, a) in attrs.enumerate() {
        match KvParser::parse(a, "godot")? {
            Some(mut p) => {
                if p.handle_alone("skip")? { zsts.push(i); }
                p.finish()?; // also rejects typos like `#[godot(skpi)]`
            }
            None if sized.is_none() => sized = Some(i),
            None => return bail!(&context, "expected a single unskipped field, found multiple"),
        }
    }
    Ok((sized.ok_or_else(|| /* bail: found none */)?, zsts))
}

Caller indexes back into its own fields.fields.

FieldIdentifier::Named/Tuple both evaluate to their ToTokens impl, so the enum's tag is not really needed. Keep the "only ident or index" guarantee in one type:

pub struct FieldIdent(TokenStream);
impl FieldIdent {
    fn named(id: Ident) -> Self { Self(quote!{ #id }) }
    fn tuple(i: usize) -> Self { Self(Literal::usize_unsuffixed(i).into_token_stream()) }
}
impl ToTokens for FieldIdent { /* forward to self.0 */ }

Bundle id + type, remove FieldIdentifiers, zst_tys, field_name(), zst_field_names():

pub struct NewtypeField { pub ident: FieldIdent, pub ty: venial::TypeExpr }
pub struct NewtypeStruct {
    pub field: NewtypeField,     // single sized field
    pub zsts: Vec<NewtypeField>, // skipped ZSTs
}

Construction (named arm identical with FieldIdent::named(..name.clone())):

let (sized, zsts) = Self::partition_fields(/* attrs */, fields)?;
let mk = |i: usize| NewtypeField {
    ident: FieldIdent::tuple(i),
    ty: fields.fields[i].0.ty.clone(),
};
Ok(NewtypeStruct { field: mk(sized), zsts: zsts.into_iter().map(mk).collect() })

Consumers use &f.ident / &f.ty directly.

Since id and type are now stored together, each const block can assert only its own type instead of all of them:

#(assert!(size_of::<#zst_tys>() == 0, "field is not a ZST");)*

Bromeon · 2026-07-16T17:05:49Z

+    // This is basically copy-paste of the unstable feature for creating arbitrary ZSTs.
+    // https://github.com/rust-lang/rust/issues/95383
+    let create_zst = quote! {
+        const {
+            #(assert!(size_of::<#field_zst_tys>() == 0, "Type is not a ZST");)*
+
+            // SAFETY: because the caller must guarantee that it's inhabited and zero-sized,
+            // there's nothing in the representation that needs to be set.
+            // `assume_init` calls `assert_inhabited`, so we don't need to here.
+            unsafe {
+                // #[allow(clippy::uninit_assumed_init)]
+                ::std::mem::MaybeUninit::uninit().assume_init()
+            }
+        }
+    };


Several issues here:

In general I'm not a fan of using rustc-internal hacks to emulate not-yet-stable features. Do we truly need this? Or can we just request Default for ZSTs? Who guarantees that the type is inhabitable at all, i.e. not an enum without variants?

size_of in generated code, but not qualified with modules.

Dead code // #[allow(clippy::uninit_assumed_init)] -- remove.

#(assert!(size_of::<#field_zst_tys>() == 0, "Type is not a ZST");)* -- this repeats the assertion for all fields, but the function is already called for all fields. Keep per-field.

possibly fixed with

99d00e3

uses Default::default instead; I was adverse to using it before because there may be some ZSTs provided by external crates that don't implement default, and so rusts orphan rules may require you to wrap a newtype around it.

Thinking on it now that's probably a better solution than emulating unstable features anyways, so I took your advice.

Bromeon · 2026-07-16T17:07:38Z

+    fn partition_fields<T: Field>(
+        fields: impl Iterator<Item = T>,
+        context: impl ToTokens,
+    ) -> ParseResult<(T, Vec<T>)> {
+        let mut sized_field = None;
+        let mut zst_fields = vec![];
+
+        for field in fields {
+            match KvParser::parse(field.get_attributes(), "godot")? {
+                Some(mut parser) => {
+                    if parser.handle_alone("skip")? {
+                        zst_fields.push(field)
+                    }
+                    // If we don't see "skip", assume its meant for someone else to handle
+                }
+                None => {
+                    if sized_field.is_none() {
+                        sized_field = Some(field);
+                    } else {
+                        bail!(
+                            &context,
+                            "GodotConvert expects a struct with a single unskipped field, found multple",
+                        )?;
+                    }
+                }
+            }
+        }
+
+        if sized_field.is_none() {
+            bail!(
+                &context,
+                "GodotConvert expects a struct with a single sized field, found none",
+            )?;
+        }
+
+        Ok((sized_field.unwrap(), zst_fields))
+    }


Also here a few issues:

Parser's finish() method must be called to error on unknown inputs.

"// If we don't see "skip", assume its meant for someone else to handle"
What does this mean? Unrecognized attributes must be errors, otherwise we invite typos and other things.

I was thinking that in the future there may be some attributes that also utilize the godot attribute unrelated to GodotConvert, and so this macro should ignore them. Is this correct?

Good thinking, but maybe for now we should still limit it to avoid potential errors today. If we see collision potential in the future, might also be an option to rename to #[convert] or so...

Bromeon · 2026-07-16T17:13:23Z

+                    } else {
+                        bail!(
+                            &context,
+                            "GodotConvert expects a struct with a single unskipped field, found multple",


Suggested change

"GodotConvert expects a struct with a single unskipped field, found multple",

"GodotConvert expects a struct with a single unskipped field, found multiple",

Also, convention is return bail!(...) rather than bail(...)? for explicit control flow -- always returns immediately.

fixed in 6d33d16

Bromeon · 2026-07-16T17:15:57Z

 #[itest]
 fn newtype_tuple_struct() {
    roundtrip(TupleNewtype(GString::from("hello!")));
+    roundtrip(TuplePhantomNewtype::<u32, String>(array![], PhantomData))


Suggested change

roundtrip(TuplePhantomNewtype::<u32, String>(array![], PhantomData))

roundtrip(TuplePhantomNewtype::<u32, String>(array![], PhantomData));

fixed in 528b6bb

Bromeon · 2026-07-16T17:16:55Z

    field1: Vector2,
 }

+type ZSTType<T> = PhantomData<T>;


Suggested change

type ZSTType<T> = PhantomData<T>;

type ZstType<T> = PhantomData<T>;

fixed in 528b6bb

Bromeon · 2026-07-16T17:26:03Z

+// Helper trait to abstract over NamedField and TupleField.
+trait Field {
+    fn get_attributes(&self) -> &[venial::Attribute];
+}
+
+impl Field for (usize, &venial::TupleField) {
+    fn get_attributes(&self) -> &[venial::Attribute] {
+        self.1.attributes.as_slice()
+    }
+}
+
+impl Field for &venial::NamedField {
+    fn get_attributes(&self) -> &[venial::Attribute] {
+        self.attributes.as_slice()
+    }
 }


I'm not sure if abstracting here via trait really pulls its weight. There's one method, used in one place, and afterwards there's again case differentiation.

Could you not do something like:

// returns (sized_index, zst_indices) fn partition_fields<'a>( attrs: impl Iterator<Item = &'a [venial::Attribute]>, context: impl ToTokens, ) -> ParseResult<(usize, Vec<usize>)> { let mut sized = None; let mut zsts = vec![]; for (i, a) in attrs.enumerate() { match KvParser::parse(a, "godot")? { Some(mut p) => { if p.handle_alone("skip")? { zsts.push(i); } p.finish()?; // see my other comment } None if sized.is_none() => sized = Some(i), None => return bail!(&context, "GodotConvert expects a struct with a single unskipped field, found multple"), } } let sized = sized.ok_or_else(|| /* bail found none */)?; Ok((sized, zsts)) }

Thanks for the suggestion, this is a much better way to handle it!

Here's the implementation 6d33d16

Bromeon · 2026-07-16T17:47:52Z

The second part of the PR, Var/Export generics, is not really tested. I think we've reached a scale where it can't hurt to extract them to their own PR, will also help reviews.

Bromeon

Thanks a lot! Some nice progress.

The 'static issue is still there, and I noted a few other changes. But we're slowly getting there 🙂

Bromeon · 2026-07-17T22:41:19Z

+        impl #generic_params ::godot::meta::FromGodot for #name #generic_args #where_clause {
            fn try_from_godot(via: #via_type) -> ::std::result::Result<Self, ::godot::meta::error::ConvertError> {
-                Ok(Self { #field_name: via })
+                #(assert_eq!(::std::mem::size_of::<#field_zst_tys>(), 0);)*


This is now a runtime check instead of a compile-time one...
Couldn't we still do this with const { ... }?

You're right, woops

Fixed by 6006f67

Bromeon · 2026-07-17T22:41:55Z

+                        zsts.push(i)
+                    }
+                    parser.finish()?;
+                    // If we don't see "skip", assume its meant for someone else to handle


Stale comment now

Bromeon · 2026-07-17T22:43:29Z

This was a "negative test", the compile_fail explicitly made sure that some code would not compile. Your changes remove it, meaning we lose test coverage.

I added some compile_fail tests back here: 71b2868, is this what you were talking about?

Bromeon · 2026-07-17T22:45:10Z

+                let (sized, zsts) = Self::partition_fields(
+                    fields
+                        .fields
+                        .iter()
+                        .map(|(field, _)| field.attributes.as_slice()),
+                    fields,
+                )?;


This code is still duplicated for Fields::Tuple + Fields::Named...

I'm not certain there is a way to remove the duplication for this part, barring writing a janky macro that explicitly references the field names.

Because TupleFeilds, and NamedFields don't have some shared interface to their attributes, only way you can get at them is through their respective attributes field.

Bromeon · 2026-07-18T07:03:10Z

 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // General FromGodot/ToGodot derive tests

+// #[derive(PartialEq, Debug)]


Bromeon · 2026-07-18T07:05:39Z

+                None => {
+                    return bail!(
+                        &context,
+                        "GodotConvert expects a struct with a single unskipped field, found multple"


Typo is still there.

We should also align terminology -- currently "sized" vs "unskipped" vs "none". Maybe let's agree on "sized" everywhere?

Bromeon · 2026-07-23T18:22:32Z

@ProphetOSpam do you think you'll have some time to look at this until end of the month? The PR is open since May, would be cool to merge it soon 🙂

If not, also no problem, just let us know.

ProphetOSpam · 2026-07-25T19:07:49Z

@ProphetOSpam do you think you'll have some time to look at this until end of the month? The PR is open since May, would be cool to merge it soon 🙂

If not, also no problem, just let us know.

I'm good to work on it more.

Sorry for the sparse timing, I've just had a busy few months

Bromeon reviewed May 25, 2026

View reviewed changes

Bromeon added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels May 25, 2026

ProphetOSpam force-pushed the qol/GodotConvert-derive-with-PhantomData branch from aaed9be to 60d1a3e Compare June 14, 2026 20:49

Bromeon reviewed Jun 23, 2026

View reviewed changes

ProphetOSpam force-pushed the qol/GodotConvert-derive-with-PhantomData branch from 189a836 to fc465b6 Compare July 1, 2026 01:29

ProphetOSpam marked this pull request as ready for review July 13, 2026 01:16

Bromeon reviewed Jul 16, 2026

View reviewed changes

Bromeon reviewed Jul 17, 2026

View reviewed changes

Bromeon reviewed Jul 18, 2026

View reviewed changes

ProphetOSpam added 10 commits July 25, 2026 15:03

Add ZST support for GodotConvert derive macro with newtype structs

c5d04d0

Add generics to Var and Export derive macros

4acf6e0

Rework godot_convert as per Bromeon's suggestion

cd8f179

Replace unsafe create_zst code with Default::default

6a152e8

Cleanup integration tests

5297892

Make zst asserts constant again

fcaf02c

Simplify Element 'static requirement

dd4aea3

Add compile_fail doctests

2e22d4a

Use sized instead of unskipped in errors

4460de4

Remove stale code/comments

2b6973d

ProphetOSpam force-pushed the qol/GodotConvert-derive-with-PhantomData branch from d2a2305 to 2b6973d Compare July 25, 2026 19:03

	/// Gets the right-most type name in the path.
	pub(crate) fn extract_typename(ty: &venial::TypeExpr) -> Option<venial::PathSegment> {
	match ty.as_path() {
	Some(mut path) => path.segments.pop(),
	_ => None,
	}
	}

	/// It will not work for structs with more than one field, unless the extra fields are `PhantomData`
	/// It will not work for structs with more than one field, unless the extra fields are zero-sized:

	"GodotConvert expects a struct with a single unskipped field, found multple",
	"GodotConvert expects a struct with a single unskipped field, found multiple",

	roundtrip(TuplePhantomNewtype::<u32, String>(array![], PhantomData))
	roundtrip(TuplePhantomNewtype::<u32, String>(array![], PhantomData));

	type ZSTType<T> = PhantomData<T>;
	type ZstType<T> = PhantomData<T>;

Uh oh!

Uh oh!

Conversation

ProphetOSpam commented May 25, 2026

Uh oh!

ProphetOSpam commented May 25, 2026

Uh oh!

Bromeon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GodotRust commented May 25, 2026

Uh oh!

ProphetOSpam commented May 25, 2026

Uh oh!

ProphetOSpam commented May 25, 2026

Uh oh!

ProphetOSpam commented May 25, 2026

Uh oh!

ProphetOSpam commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ProphetOSpam commented May 27, 2026

Uh oh!

Bromeon commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ProphetOSpam commented Jun 20, 2026

Uh oh!

Bromeon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ProphetOSpam commented Jun 23, 2026

Uh oh!

ProphetOSpam commented Jun 24, 2026

Uh oh!

Bromeon commented Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ProphetOSpam commented Jun 27, 2026

Uh oh!

ProphetOSpam commented Jun 27, 2026

Uh oh!

Bromeon commented Jun 28, 2026

Uh oh!

Bromeon left a comment •

edited

Loading

ProphetOSpam commented May 27, 2026 •

edited

Loading

Bromeon commented May 28, 2026 •

edited

Loading

Bromeon commented Jun 27, 2026 •

edited

Loading