Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 35 additions & 32 deletions godot-macros/src/derive/data_models/godot_convert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

use proc_macro2::{Ident, TokenStream};
use quote::ToTokens;
use venial::{GenericParamList, WhereClause};

use super::c_style_enum::CStyleEnum;
use super::godot_attribute::{GodotAttribute, ViaType};
Expand All @@ -18,23 +19,41 @@ use crate::util::bail;
pub struct GodotConvert {
/// The name of the type we're deriving for.
pub ty_name: Ident,
pub where_clause: Option<WhereClause>,
pub generic_params: Option<GenericParamList>,
/// The data from the type and `godot` attribute.
pub convert_type: ConvertType,
}

impl GodotConvert {
pub fn parse_declaration(item: venial::Item) -> ParseResult<Self> {
let (name, where_clause, generic_params) = match &item {
venial::Item::Struct(struct_) => (
struct_.name.clone(),
&struct_.where_clause,
&struct_.generic_params,
),
venial::Item::Enum(enum_) => (
enum_.name.clone(),
&enum_.where_clause,
&enum_.generic_params,
),
let data = ConvertType::parse_declaration(&item)?;

let (name, where_clause, generic_params) = match item {
venial::Item::Struct(struct_) => {
(struct_.name, struct_.where_clause, struct_.generic_params)
}
venial::Item::Enum(enum_) => {
// We only have C-style enums, so Rust would already complain that generics are unused.
// This provides clearer error messages though.
if let Some(generic_params) = &enum_.generic_params {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return bail!(
generic_params,
"#[derive(GodotConvert)] does not support lifetimes or generic parameters on enums"
);
}

// Is this check even necessary? What's the use case of where clauses without generics?
// For traits, one can imagine `Self: SomeBound`, but for structs/enums?
if let Some(where_clause) = &enum_.where_clause {
return bail!(
where_clause,
"#[derive(GodotConvert)] does not support where clauses"
);
}

(enum_.name, enum_.where_clause, enum_.generic_params)
}
other => {
return bail!(
other,
Expand All @@ -43,26 +62,10 @@ impl GodotConvert {
}
};

if let Some(generic_params) = generic_params {
return bail!(
generic_params,
"#[derive(GodotConvert)] does not support lifetimes or generic parameters"
);
}

// Is this check even necessary? What's the use case of where clauses without generics?
// For traits, one can imagine `Self: SomeBound`, but for structs/enums?
if let Some(where_clause) = where_clause {
return bail!(
where_clause,
"#[derive(GodotConvert)] does not support where clauses"
);
}

let data = ConvertType::parse_declaration(item)?;

Ok(Self {
ty_name: name,
where_clause: where_clause,
generic_params: generic_params,
convert_type: data,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

})
}
Expand All @@ -77,10 +80,10 @@ pub enum ConvertType {
}

impl ConvertType {
pub fn parse_declaration(item: venial::Item) -> ParseResult<Self> {
let attribute = GodotAttribute::parse_attribute(&item)?;
pub fn parse_declaration(item: &venial::Item) -> ParseResult<Self> {
let attribute = GodotAttribute::parse_attribute(item)?;

match &item {
match item {
venial::Item::Struct(struct_) => {
let GodotAttribute::Transparent { .. } = attribute else {
return bail!(
Expand Down
123 changes: 107 additions & 16 deletions godot-macros/src/derive/data_models/newtype.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,34 @@
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
*/

use proc_macro2::{Ident, TokenStream};
use proc_macro2::{Ident, Literal, Span, TokenStream};
use quote::quote;
use venial::{NamedField, TupleField};

use crate::ParseResult;
use crate::util::bail;

pub enum FieldType {
Tuple(Literal),
Named(Ident),
}

pub enum FieldsType {
Tuple(Vec<Literal>),
Named(Vec<Ident>),
}

/// Stores info from the field of a newtype struct for use in deriving `GodotConvert` and other related traits.
///
/// Here, a newtype struct must have exactly 1 non-ZST field, and can have an arbitrary amount of ZST fields.
pub struct NewtypeStruct {
/// The name of the field.
///
/// If `None`, then this represents a tuple-struct with one field.
pub name: Option<Ident>,
pub name: FieldType,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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?


/// The names of the phantom fields.
pub phantom_names: FieldsType,

/// The type of the field.
pub ty: venial::TypeExpr,
Expand All @@ -33,34 +49,99 @@ impl NewtypeStruct {
"GodotConvert expects a struct with a single field, unit structs are currently not supported"
),
venial::Fields::Tuple(fields) => {
if fields.fields.len() != 1 {
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
}
Comment on lines +52 to +60

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

phantom_predicate is rather unclear. Why not simply is_phantom_data?

Also, can you use this maybe?

/// 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?


let mut non_phantom_fields = fields
.fields
.items()
.enumerate()
.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
);
}

let (field, _) = fields.fields[0].clone();
let (field_num, field) = maybe_field.unwrap();

let phantom_nums = (0..field_num)
.chain(field_num + 1..fields.fields.len())
.map(Literal::usize_unsuffixed)
.collect();

Ok(NewtypeStruct {
name: None,
ty: field.ty,
name: FieldType::Tuple(Literal::usize_unsuffixed(field_num)),
phantom_names: FieldsType::Tuple(phantom_nums),
ty: field.ty.clone(),
})
}
venial::Fields::Named(fields) => {
if fields.fields.len() != 1 {
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
);
}
Comment on lines +98 to 127

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code duplication...


let (field, _) = fields.fields[0].clone();
let field = maybe_field.unwrap().clone();

let phantom_names = fields
.fields
.items()
.filter_map(|field| {
if phantom_predicate(field) {
return Some(field.name.clone());
}
None
})
.collect();

Ok(NewtypeStruct {
name: Some(field.name),
name: FieldType::Named(field.name),
phantom_names: FieldsType::Named(phantom_names),
ty: field.ty,
})
}
Expand All @@ -69,7 +150,7 @@ impl NewtypeStruct {

/// Gets the field name.
///
/// If this represents a tuple-struct, then it will return `0`. This can be used just like it was a named field with the name `0`.
/// If this represents a tuple-struct, then it will return a number. This can be used just like it was a named field.
/// For instance:
/// ```
/// struct Foo(i64);
Expand All @@ -80,8 +161,18 @@ impl NewtypeStruct {
/// ```
pub fn field_name(&self) -> TokenStream {
match &self.name {
Some(name) => quote! { #name },
None => quote! { 0 },
FieldType::Named(name) => quote! { #name },
FieldType::Tuple(num) => quote! { #num },
}
}

/// Gets the phantom field names.
///
/// If this represents a tuple-struct, then it will return numbers. See `Self::field_name`
pub fn phantom_field_names(&self) -> Vec<TokenStream> {
match &self.phantom_names {
FieldsType::Named(vec) => vec.iter().map(|ident| quote! {#ident}).collect(),
FieldsType::Tuple(vec) => vec.iter().map(|ident| quote! {#ident}).collect(),
}
}
}
22 changes: 18 additions & 4 deletions godot-macros/src/derive/derive_from_godot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,11 @@ pub fn make_fromgodot(convert: &GodotConvert, cache: &mut EnumeratorExprCache) -
let GodotConvert {
ty_name: name,
convert_type: data,
..
} = convert;

match data {
ConvertType::NewType { field } => make_fromgodot_for_newtype_struct(name, field),
ConvertType::NewType { field } => make_fromgodot_for_newtype_struct(convert, field),

ConvertType::Enum {
variants,
Expand All @@ -37,16 +38,29 @@ pub fn make_fromgodot(convert: &GodotConvert, cache: &mut EnumeratorExprCache) -
}

/// Derives `FromGodot` for newtype structs.
fn make_fromgodot_for_newtype_struct(name: &Ident, field: &NewtypeStruct) -> TokenStream {
fn make_fromgodot_for_newtype_struct(convert: &GodotConvert, field: &NewtypeStruct) -> TokenStream {
// For tuple structs this ends up using the alternate tuple-struct constructor syntax of
// TupleStruct { 0: value }
let GodotConvert {
ty_name: name,
generic_params,
where_clause,
..
} = convert;
let generic_args = generic_params
.as_ref()
.map(|params| params.as_inline_args());
let field_name = field.field_name();
let phantom_field_names = field.phantom_field_names();
let via_type = &field.ty;

quote! {
impl ::godot::meta::FromGodot for #name {
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 })
Ok(Self {
#field_name: via,
#(#phantom_field_names: ::std::marker::PhantomData),*
})
}
}
}
Expand Down
Loading
Loading