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.

…pe structs

…onality

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:47:57Z

+                // being used. These error messages are a bit more clear with our intentions,
+                // though.
+
+                if let Some(generic_params) = &enum_.generic_params {


Shorter, no empty line:

Suggested change

if let Some(generic_params) = &enum_.generic_params {

// We only have C-style enums, so Rust would already complain that generics are unused.

// This provides clearer error messages though.

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?

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
);

… hack

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 added 2 commits May 25, 2026 13:55

Add PhantomData<T> support for the GodotConvert derive macro on newty…

08c22d8

…pe structs

Update GodotConvert derive macro docs to match new PhantomData functi…

ad0855f

…onality

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 added 3 commits May 25, 2026 18:06

Refactor to remove unnecessary clones

96ce47f

Add comment claryifying NewtypeStruct can have multiple fields

085bb97

Reword and correctly format comment describing T: 'static + 'static…

84c620f

… hack

Add example to GodotConvert derive macro docs

aaed9be

	if let Some(generic_params) = &enum_.generic_params {
	// We only have C-style enums, so Rust would already complain that generics are unused.
	// This provides clearer error messages though.

	/// 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,
	}
	}

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Bromeon left a comment •

edited

Loading

ProphetOSpam commented May 27, 2026 •

edited

Loading

Bromeon commented May 28, 2026 •

edited

Loading