[WIP] in #[var]s, handle renamed #[func]s
#1019
Conversation
7d0f540 to
6695a62
Compare
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1019 |
6695a62 to
56e5be7
Compare
| }; | ||
|
|
||
| quote! { | ||
| #(#attributes)* |
There was a problem hiding this comment.
at the moment, all attributes are copied over.
Maybe this should probably be changed so only #[cfg] attributes are copied over. Are there any other attributes we should copy?
There was a problem hiding this comment.
can you elaborate?
This would be relevant in, for example, func_test.rs. At first I didn't handle attributes at all, which failed here because it would duplicate the constant. After copying cfg, only one of the constants is active.
There was a problem hiding this comment.
Maybe #[cfg_attr]
Oh I think I know what you meant.
that would only be relevant for the special case #[cfg_attr(/**/, cfg)], right?
There was a problem hiding this comment.
that would only be relevant for the special case
#[cfg_attr(/**/, cfg)], right?
You're right, this probably makes no sense to handle. No one writes code like this 😀
…d with (rename=godot_name)
56e5be7 to
e8a3c98
Compare
| @@ -200,10 +201,14 @@ impl GetterSetterImpl { | |||
| } | |||
| } | |||
|
|
|||
| let constant_declaration = | |||
There was a problem hiding this comment.
| let constant_declaration = | |
| let func_name_constant = |
To make clear that it's not a user-defined #[constant].
| @@ -84,6 +86,8 @@ pub fn transform_inherent_impl( | |||
| let (funcs, signals) = process_godot_fns(&class_name, &mut impl_block, meta.secondary)?; | |||
| let consts = process_godot_constants(&mut impl_block)?; | |||
|
|
|||
| let func_export_name_constants = make_function_registered_name_constants(&funcs, &class_name); | |||
There was a problem hiding this comment.
| let func_export_name_constants = make_function_registered_name_constants(&funcs, &class_name); | |
| let func_name_constants = make_function_registered_name_constants(&funcs, &class_name); |
"export" has a very specific meaning; using it here is probably confusing.
There was a problem hiding this comment.
it was also not consistent with the function name. I'd propose standardizing on "registered" function name, at least that's what it's called in other places in the code.
There was a problem hiding this comment.
It's fine if the local variables are more terse/concise than the function names. Context matters, and if you see the
let func_name_constants = make_function_registered_name_constants(...);once, then you know what func_name_constants refers to. I think using overly long variable names makes expressions harder to read; the "registered" doesn't really add useful information to understand the codegen.
There's also the point of scope: global function names are visible in their entire module, while variable names are limited to the block/function in which they are declared. It's much easier to mentally keep track of a block, than it is of a module; and there's less potential for collisions.
| @@ -184,6 +191,9 @@ pub fn transform_inherent_impl( | |||
| let result = quote! { | |||
| #impl_block | |||
| #fill_storage | |||
| impl #class_name{ | |||
| #( #func_export_name_constants )* | |||
There was a problem hiding this comment.
It's not quite clear to me why individual constants are defined above next to each function, and then all of them again here... Could you elaborate this?
There was a problem hiding this comment.
yep. If you have a struct declaration with #[var(get, set)], then the macro will autogenerate one or two functions. In that case, the constants are placed next to the macro-generated function.
If the user has user-declared functions in an inherent impl block registered with #[func], then I'm placing the constants in a second impl block below. Reason is simple, the #impl_block is currently just copied as-is, I didn't want to have to insert the constants in the middle of it.
Since I can't push any information between macro invocations, the constants need to be declared by the macro that generates or reads the function.
Simple example:
#[derive(GodotClass)]
#[class(base=Node, init)]
struct O {
#[var(get, set)]
int_val: i32,
}
#[godot_api]
impl O {
#[func(rename=f1)]
pub fn foo(&self) -> i32 {
todo!()
}
#[func(rename=f2)]
pub fn bar(&mut self, val: i32) {
todo!()
}
}will generate:
// #[derive(GodotClass)]
impl O {
pub fn get_int_val(&self) -> <i32 as ::godot::meta::GodotConvert>::Via {
<i32 as ::godot::register::property::Var>::get_property(&self.int_val)
}
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const __gdext_func_O_get_int_val: &str = "get_int_val";
pub fn set_int_val(&mut self, int_val: <i32 as ::godot::meta::GodotConvert>::Via) {
<i32 as ::godot::register::property::Var>::set_property(&mut self.int_val, int_val);
}
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const __gdext_func_O_set_int_val: &str = "set_int_val";
}
// #[godot_api]
impl O {
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const __gdext_func_O_foo: &str = "f1";
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const __gdext_func_O_bar: &str = "f2";
}There was a problem hiding this comment.
Thank you very much!
Makes total sense, could you maybe add 1 line of comments next to each substitution?
// Getter/setter generated from struct fields: place constant next to function in impl.
// User-defined functions: collectively place constants in dedicated impl block.
| }; | ||
|
|
||
| quote! { | ||
| #(#attributes)* |
godot-macros/src/util/mod.rs
Outdated
| pub fn format_function_registered_name_constant(class_name: &Ident, func_name: &Ident) -> Ident { | ||
| format_ident!("__gdext_func_{}_{}", class_name, func_name) | ||
| } |
There was a problem hiding this comment.
I'd use consistent naming, that makes it obvious you're referring to the same thing.
Above I suggested func_name_constant, maybe that can be done here, too?
| pub fn format_function_registered_name_constant(class_name: &Ident, func_name: &Ident) -> Ident { | |
| format_ident!("__gdext_func_{}_{}", class_name, func_name) | |
| } | |
| pub fn format_func_name_constant(class_name: &Ident, func_name: &Ident) -> Ident { | |
| format_ident!("__gdext_func_{class_name}_{func_name}") | |
| } |
| // ---------------------------------------------------------------------------------------------------------------------------------------------- | ||
|
|
||
| #[derive(GodotClass)] | ||
| #[class(base=Node, init)] | ||
| struct RenamedFunc { | ||
| #[var(get = get_int_val, set = set_int_val)] | ||
| int_val: i32, | ||
| } | ||
|
|
||
| #[godot_api] | ||
| impl RenamedFunc { | ||
| #[func(rename=f1)] | ||
| pub fn get_int_val(&self) -> i32 { | ||
| self.int_val | ||
| } | ||
|
|
||
| #[func(rename=f2)] | ||
| pub fn set_int_val(&mut self, val: i32) { | ||
| self.int_val = val; | ||
| } | ||
| } |
There was a problem hiding this comment.
Thanks for the test! Could you add an #[itest] that actually invokes the getters + setters by their new name?
You can use Object::call() for dynamic calls.
There was a problem hiding this comment.
sure thing. before merging I'd also like to add a test that reads/writes the property from gdscript
Co-authored-by: Jan Haller <bromeon@gmail.com>
Co-authored-by: Jan Haller <bromeon@gmail.com>
… into dev-handle-func-rename
|
proposal: to not pollute the struct namespace with all these constants, I could create a dummy struct. Currently the following code is generated: // #[derive(GodotClass)]
impl O {
pub fn get_int_val(&self) -> <i32 as ::godot::meta::GodotConvert>::Via {
<i32 as ::godot::register::property::Var>::get_property(&self.int_val)
}
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const __gdext_func_O_get_int_val: &str = "get_int_val";
pub fn set_int_val(&mut self, int_val: <i32 as ::godot::meta::GodotConvert>::Via) {
<i32 as ::godot::register::property::Var>::set_property(&mut self.int_val, int_val);
}
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const __gdext_func_O_set_int_val: &str = "set_int_val";
}
// #[godot_api]
impl O {
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const __gdext_func_O_foo: &str = "f1";
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const __gdext_func_O_bar: &str = "f2";
}I would propose the following: // #[derive(GodotClass)]
impl O {
pub fn get_int_val(&self) -> <i32 as ::godot::meta::GodotConvert>::Via {
<i32 as ::godot::register::property::Var>::get_property(&self.int_val)
}
pub fn set_int_val(&mut self, int_val: <i32 as ::godot::meta::GodotConvert>::Via) {
<i32 as ::godot::register::property::Var>::set_property(&mut self.int_val, int_val);
}
}
#[doc(hidden)]
struct O_Functions { }
impl O_Functions {
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const __gdext_func_O_set_int_val: &str = "set_int_val";
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const __gdext_func_O_get_int_val: &str = "get_int_val";
}
// #[godot_api]
impl O_Functions {
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const __gdext_func_O_foo: &str = "f1";
#[doc(hidden)]
#[allow(non_upper_case_globals)]
pub const __gdext_func_O_bar: &str = "f2";
}The dummy struct is required instead of a |
|
alright no more time for right now, I need to go. I'll look through it one more time when I'm back, then I'll rebase-squash. Todo:
|
|
regarding the memory leak, the memory is allocated here: https://github.com/godotengine/godot/blob/1b7b009674e05b566be11a5377b935f5d3d6c0ee/core/extension/gdextension.cpp#L509 and I could see it being leaked, if it returns with an error here, in case the method name is duplicated: https://github.com/godotengine/godot/blob/master/core/object/class_db.cpp#L1881 why would the method name be duplicated? no idea |
|
After looking into this, I believe it's a bug in godot: godotengine/godot#101870 It's triggered because in mod.rs#244, it doesn't handle pub(crate) fn extract_cfg_attrs(
attrs: &[venial::Attribute],
) -> impl IntoIterator<Item = &venial::Attribute> {
attrs.iter().filter(|attr| {
attr.get_single_path_segment()
.is_some_and(|name| name == "cfg")
})
}and I added a test case that uses cfg_attr, therefore the method is registered multiple times with godot, hitting the memory leak there |
…ue.to_token_stream().to_string().contains("cfg("))
#[var]s, handle renamed #[func]s#[var]s, handle renamed #[func]s
|
Quick tip: instead of renaming the title to |
| @@ -84,6 +86,8 @@ pub fn transform_inherent_impl( | |||
| let (funcs, signals) = process_godot_fns(&class_name, &mut impl_block, meta.secondary)?; | |||
| let consts = process_godot_constants(&mut impl_block)?; | |||
|
|
|||
| let func_export_name_constants = make_function_registered_name_constants(&funcs, &class_name); | |||
There was a problem hiding this comment.
It's fine if the local variables are more terse/concise than the function names. Context matters, and if you see the
let func_name_constants = make_function_registered_name_constants(...);once, then you know what func_name_constants refers to. I think using overly long variable names makes expressions harder to read; the "registered" doesn't really add useful information to understand the codegen.
There's also the point of scope: global function names are visible in their entire module, while variable names are limited to the block/function in which they are declared. It's much easier to mentally keep track of a block, than it is of a module; and there's less potential for collisions.
| @@ -119,6 +125,32 @@ pub fn transform_inherent_impl( | |||
| }); | |||
| }; | |||
|
|
|||
| let class_functions_name = format_function_registered_name_struct_name(&class_name); | |||
| let class_functions_path: Path = match impl_block.self_ty.as_path().unwrap().segments.as_slice() | |||
There was a problem hiding this comment.
The way you use unwrap here indicates that it's infallible. In general, this would need a brief comment documenting why.
Here, it might be better to pass in previously evaluated values via parameters, making the extraction unnecessary. This especially holds true here since as_path() isn't cheap to compute, even less so with some upcoming validations in venial.
Might also make sense to extract the match expression into its own variable 🙂
| @@ -184,6 +191,9 @@ pub fn transform_inherent_impl( | |||
| let result = quote! { | |||
| #impl_block | |||
| #fill_storage | |||
| impl #class_name{ | |||
| #( #func_export_name_constants )* | |||
There was a problem hiding this comment.
Thank you very much!
Makes total sense, could you maybe add 1 line of comments next to each substitution?
// Getter/setter generated from struct fields: place constant next to function in impl.
// User-defined functions: collectively place constants in dedicated impl block.
| let cfg_attributes = extract_cfg_attrs(&func.external_attributes) | ||
| .into_iter() | ||
| .collect(); | ||
| make_function_registered_name_constant( | ||
| class_name, | ||
| &func.signature_info.method_name, | ||
| &func.registered_name, | ||
| &cfg_attributes, | ||
| ) |
There was a problem hiding this comment.
Empty line to separate 2 multi-line statements:
| let cfg_attributes = extract_cfg_attrs(&func.external_attributes) | |
| .into_iter() | |
| .collect(); | |
| make_function_registered_name_constant( | |
| class_name, | |
| &func.signature_info.method_name, | |
| &func.registered_name, | |
| &cfg_attributes, | |
| ) | |
| let cfg_attributes = extract_cfg_attrs(&func.external_attributes) | |
| .into_iter() | |
| .collect(); | |
| make_function_registered_name_constant( | |
| class_name, | |
| &func.signature_info.method_name, | |
| &func.registered_name, | |
| &cfg_attributes, | |
| ) |
| registered_name: &Option<String>, | ||
| attributes: &Vec<&Attribute>, |
There was a problem hiding this comment.
| registered_name: &Option<String>, | |
| attributes: &Vec<&Attribute>, | |
| registered_name: Option<&String>, | |
| attributes: &[&Attribute], |
This signatures are more flexible. The Vec derefs to slices, while Option::as_ref() converts &Option<String> to Option<&String>.
This also makes the call-site less awkward (both &None and &vec[] are unusual).
| }; | ||
|
|
||
| let doc_comment = format!( | ||
| "The rust function `{}` is registered with godot as `{}`.", |
There was a problem hiding this comment.
Capitalize proper names:
| "The rust function `{}` is registered with godot as `{}`.", | |
| "The Rust function `{}` is registered with Godot as `{}`.", |
| }; | ||
|
|
||
| quote! { | ||
| #(#attributes)* |
There was a problem hiding this comment.
that would only be relevant for the special case
#[cfg_attr(/**/, cfg)], right?
You're right, this probably makes no sense to handle. No one writes code like this 😀
| class_name: &Ident, | ||
| func_name: &Ident, | ||
| ) -> Ident { | ||
| format_ident!("__gdext_func_{}_{}", class_name, func_name) |
There was a problem hiding this comment.
| format_ident!("__gdext_func_{}_{}", class_name, func_name) | |
| format_ident!("__gdext_func_{class_name}_{func_name}") |
|
|
||
| # Assert specific names | ||
| assert(gdext_props.has("int_val"), "should have a property named 'int_val'") | ||
| assert(gdext_props.has("RenamedFunc"), "should have a property named 'RenamedFunc'") # godot automatically adds a property of the class name |
There was a problem hiding this comment.
| assert(gdext_props.has("RenamedFunc"), "should have a property named 'RenamedFunc'") # godot automatically adds a property of the class name | |
| # Godot automatically adds a property of the class name (acts as the top-level category in the inspector). | |
| assert(gdext_props.has("RenamedFunc"), "should have a property named 'RenamedFunc'") |
There was a problem hiding this comment.
Godot automatically adds a property of the class name.
Do you know more about this? Some Godot quirk, or why is the class name added as a property? 🤔
There was a problem hiding this comment.
It is category – Godot adds property with PropertyFlagUsage of value 128 to any object as a very first element. Thanks to this one is able to see object class name in the top bar of the inspector.
While undocumented it has been stable since… hmm, Godot 3? and shouldn't be changed until someone do major rewrite/refactor of properties (which is unlikely? This parts works well)
There was a problem hiding this comment.
I have more pressing question – what does this assert check for 🤔? Validity of get_property_list?
There was a problem hiding this comment.
Shape of the object, that it exports the two expected functions (and only them, nothing extra)
There was a problem hiding this comment.
Adjusted my suggestion above to briefly mention this.
|
|
||
|
|
||
| func test_RenamedFunc_shape(): | ||
| # note: RenamedFunc is in property_test.rs |
There was a problem hiding this comment.
Please everywhere start with uppercase and end with period 🙂
| # note: RenamedFunc is in property_test.rs | |
| # Note: RenamedFunc is located in property_test.rs. |
That sounds like a good idea! |
closes #863