From 5dc638209543491034fcb7b54f6289281022e759 Mon Sep 17 00:00:00 2001 From: bogay Date: Fri, 4 Feb 2022 18:37:02 +0000 Subject: [PATCH 1/3] feat(property): add `get`, `get_ref` and `set` attributes these attributes are used to set custom getter and setter. --- gdnative-derive/src/native_script.rs | 112 ++++++++++--- .../src/native_script/property_args.rs | 157 ++++++++++++++++-- 2 files changed, 234 insertions(+), 35 deletions(-) diff --git a/gdnative-derive/src/native_script.rs b/gdnative-derive/src/native_script.rs index 733499954..c8d412a8c 100644 --- a/gdnative-derive/src/native_script.rs +++ b/gdnative-derive/src/native_script.rs @@ -2,10 +2,10 @@ use proc_macro::TokenStream; use proc_macro2::TokenStream as TokenStream2; use syn::spanned::Spanned; -use syn::{Data, DeriveInput, Fields, Ident, Meta, MetaList, NestedMeta, Path, Stmt, Type}; +use syn::{Data, DeriveInput, Expr, Fields, Ident, Meta, MetaList, NestedMeta, Path, Stmt, Type}; mod property_args; -use property_args::{PropertyAttrArgs, PropertyAttrArgsBuilder}; +use property_args::{PropertyAttrArgs, PropertyAttrArgsBuilder, PropertyGet, PropertySet}; pub(crate) struct DeriveData { pub(crate) name: Ident, @@ -66,28 +66,61 @@ pub(crate) fn derive_native_class(derive_input: &DeriveInput) -> Result = config .before_get .map(|path_expr| parse_quote!(#path_expr(this, _owner);)); - let after_get: Option = config .after_get .map(|path_expr| parse_quote!(#path_expr(this, _owner);)); - + let with_getter = get.map(|get| { + let register_fn = match get { + PropertyGet::Owned(_) => quote!(with_getter), + _ => quote!(with_ref_getter), + }; + let get: Expr = match get { + PropertyGet::Default => parse_quote!(&this.#ident), + PropertyGet::Owned(path_expr) | PropertyGet::Ref(path_expr) => { + parse_quote!(#path_expr(this, _owner)) + } + }; + quote!( + .#register_fn(|this: &#name, _owner: ::gdnative::object::TRef| { + #before_get + let res = #get; + #after_get + res + }) + ) + }); let before_set: Option = config .before_set .map(|path_expr| parse_quote!(#path_expr(this, _owner);)); - let after_set: Option = config .after_set .map(|path_expr| parse_quote!(#path_expr(this, _owner);)); + let with_setter = set.map(|set| { + let set: Stmt = match set { + PropertySet::Default => parse_quote!(this.#ident = v;), + PropertySet::WithPath(path_expr) => parse_quote!(#path_expr(this, _owner, v);), + }; + quote!( + .with_setter(|this: &mut #name, _owner: ::gdnative::object::TRef, v| { + #before_set + #set + #after_set + })) + }); let label = config.path.unwrap_or_else(|| format!("{}", ident)); quote!({ @@ -95,17 +128,8 @@ pub(crate) fn derive_native_class(derive_input: &DeriveInput) -> Result| { - #before_get - let res = &this.#ident; - #after_get - res - }) - .with_setter(|this: &mut #name, _owner: ::gdnative::object::TRef, v| { - #before_set - this.#ident = v; - #after_set - }) + #with_getter + #with_setter .done(); }) }); @@ -221,8 +245,8 @@ fn parse_derive_input(input: &DeriveInput) -> Result { match meta { Meta::List(MetaList { nested, .. }) => { - let attr_args_builder = - property_args.get_or_insert_with(PropertyAttrArgsBuilder::default); + let attr_args_builder = property_args + .get_or_insert_with(|| PropertyAttrArgsBuilder::new(&field.ty)); for arg in &nested { if let NestedMeta::Meta(Meta::NameValue(ref pair)) = arg { @@ -236,7 +260,8 @@ fn parse_derive_input(input: &DeriveInput) -> Result { } } Meta::Path(_) => { - property_args.get_or_insert_with(PropertyAttrArgsBuilder::default); + property_args + .get_or_insert_with(|| PropertyAttrArgsBuilder::new(&field.ty)); } m => { let msg = format!("Unexpected meta variant: {:?}", m); @@ -337,4 +362,49 @@ mod tests { parse_derive_input(&input).unwrap(); } + + #[test] + fn derive_property_get_set() { + let input: TokenStream2 = syn::parse_str( + r#" + #[inherit(Node)] + struct Foo { + #[property(get = "get_bar", set = "set_bar")] + bar: i64, + }"#, + ) + .unwrap(); + let input: DeriveInput = syn::parse2(input).unwrap(); + parse_derive_input(&input).unwrap(); + } + + #[test] + fn derive_property_default_get_set() { + let input: TokenStream2 = syn::parse_str( + r#" + #[inherit(Node)] + struct Foo { + #[property(get, set)] + bar: i64, + }"#, + ) + .unwrap(); + let input: DeriveInput = syn::parse2(input).unwrap(); + parse_derive_input(&input).unwrap(); + } + + #[test] + fn derive_property_default_get_ref() { + let input: TokenStream2 = syn::parse_str( + r#" + #[inherit(Node)] + struct Foo { + #[property(get_ref = "Self::get_bar")] + bar: i64, + }"#, + ) + .unwrap(); + let input: DeriveInput = syn::parse2(input).unwrap(); + parse_derive_input(&input).unwrap(); + } } diff --git a/gdnative-derive/src/native_script/property_args.rs b/gdnative-derive/src/native_script/property_args.rs index be784496a..bd679d2a6 100644 --- a/gdnative-derive/src/native_script/property_args.rs +++ b/gdnative-derive/src/native_script/property_args.rs @@ -1,29 +1,63 @@ use syn::spanned::Spanned; +#[derive(Debug)] +pub enum PropertyGet { + Default, + Owned(syn::Path), + Ref(syn::Path), +} + +#[derive(Debug)] +pub enum PropertySet { + Default, + WithPath(syn::Path), +} + pub struct PropertyAttrArgs { + pub ty: syn::Type, pub path: Option, pub default: Option, pub hint: Option, pub before_get: Option, + pub get: Option, pub after_get: Option, pub before_set: Option, + pub set: Option, pub after_set: Option, pub no_editor: bool, } -#[derive(Default)] pub struct PropertyAttrArgsBuilder { + ty: syn::Type, path: Option, default: Option, hint: Option, before_get: Option, + get: Option, after_get: Option, before_set: Option, + set: Option, after_set: Option, no_editor: bool, } impl PropertyAttrArgsBuilder { + pub fn new(ty: &syn::Type) -> Self { + Self { + ty: ty.clone(), + path: None, + default: None, + hint: None, + before_get: None, + get: None, + after_get: None, + before_set: None, + set: None, + after_set: None, + no_editor: false, + } + } + pub fn add_pair(&mut self, pair: &syn::MetaNameValue) -> Result<(), syn::Error> { let path_span = pair.lit.span(); let invalid_value_path = |_| { @@ -43,7 +77,10 @@ impl PropertyAttrArgsBuilder { if let Some(old) = self.default.replace(pair.lit.clone()) { return Err(syn::Error::new( pair.span(), - format!("there is already a default value set: {:?}", old), + format!( + "there is already a 'default' attribute with value: {:?}", + old + ), )); } } @@ -53,14 +90,14 @@ impl PropertyAttrArgsBuilder { } else { return Err(syn::Error::new( pair.span(), - "path value is not a string literal".to_string(), + "'path' value is not a string literal", )); }; if let Some(old) = self.path.replace(string) { return Err(syn::Error::new( pair.span(), - format!("there is already a path set: {:?}", old), + format!("there is already a 'path' attribute with value: {:?}", old), )); } } @@ -70,7 +107,7 @@ impl PropertyAttrArgsBuilder { } else { return Err(syn::Error::new( pair.span(), - "hint value is not a string literal".to_string(), + "'hint' value is not a string literal", )); }; @@ -79,7 +116,7 @@ impl PropertyAttrArgsBuilder { if let Some(old) = self.hint.replace(path) { return Err(syn::Error::new( pair.span(), - format!("there is already a hint value set: {:?}", old), + format!("there is already a 'hint' attribute with value: {:?}", old), )); } } @@ -89,7 +126,7 @@ impl PropertyAttrArgsBuilder { } else { return Err(syn::Error::new( pair.span(), - "before_get value is not a string literal".to_string(), + "'before_get' value is not a string literal", )); }; @@ -98,7 +135,53 @@ impl PropertyAttrArgsBuilder { if let Some(old) = self.before_get.replace(path) { return Err(syn::Error::new( pair.span(), - format!("there is already a before_get value set: {:?}", old), + format!( + "there is already a 'before_get' attribute with value: {:?}", + old + ), + )); + } + } + "get" => { + let string = if let syn::Lit::Str(lit_str) = &pair.lit { + lit_str.value() + } else { + return Err(syn::Error::new( + pair.span(), + "'get' value is not a string literal", + )); + }; + + let path = + syn::parse_str::(string.as_str()).map_err(invalid_value_path)?; + let get = PropertyGet::Owned(path); + if let Some(old) = self.get.replace(get) { + return Err(syn::Error::new( + pair.span(), + format!("there is already a 'get' attribute with value: {:?}", old), + )); + } + } + "get_ref" => { + let string = if let syn::Lit::Str(lit_str) = &pair.lit { + lit_str.value() + } else { + return Err(syn::Error::new( + pair.span(), + "'get_ref' value is not a string literal", + )); + }; + + let path = + syn::parse_str::(string.as_str()).map_err(invalid_value_path)?; + let get_ref = PropertyGet::Ref(path); + if let Some(old) = self.get.replace(get_ref) { + return Err(syn::Error::new( + pair.span(), + format!( + "there is already a 'get_ref' attribute with value: {:?}", + old + ), )); } } @@ -108,7 +191,7 @@ impl PropertyAttrArgsBuilder { } else { return Err(syn::Error::new( pair.span(), - "after_get value is not a string literal".to_string(), + "'after_get' value is not a string literal", )); }; @@ -117,7 +200,10 @@ impl PropertyAttrArgsBuilder { if let Some(old) = self.after_get.replace(path) { return Err(syn::Error::new( pair.span(), - format!("there is already a after_get value set: {:?}", old), + format!( + "there is already a 'after_get' attribute with value: {:?}", + old + ), )); } } @@ -127,7 +213,7 @@ impl PropertyAttrArgsBuilder { } else { return Err(syn::Error::new( pair.span(), - "before_set value is not a string literal".to_string(), + "'before_set' value is not a string literal", )); }; @@ -136,7 +222,30 @@ impl PropertyAttrArgsBuilder { if let Some(old) = self.before_set.replace(path) { return Err(syn::Error::new( pair.span(), - format!("there is already a before_set value set: {:?}", old), + format!( + "there is already a 'before_set' attribute with value: {:?}", + old + ), + )); + } + } + "set" => { + let string = if let syn::Lit::Str(lit_str) = &pair.lit { + lit_str.value() + } else { + return Err(syn::Error::new( + pair.span(), + "'set' value is not a string literal", + )); + }; + + let path = + syn::parse_str::(string.as_str()).map_err(invalid_value_path)?; + let set = PropertySet::WithPath(path); + if let Some(old) = self.set.replace(set) { + return Err(syn::Error::new( + pair.span(), + format!("there is already a 'set' attribute with value: {:?}", old), )); } } @@ -146,7 +255,7 @@ impl PropertyAttrArgsBuilder { } else { return Err(syn::Error::new( pair.span(), - "after_set value is not a string literal".to_string(), + "'after_set' value is not a string literal", )); }; @@ -155,7 +264,10 @@ impl PropertyAttrArgsBuilder { if let Some(old) = self.after_set.replace(path) { return Err(syn::Error::new( pair.span(), - format!("there is already a after_set value set: {:?}", old), + format!( + "there is already a 'after_set' attribute with value: {:?}", + old + ), )); } } @@ -173,6 +285,20 @@ impl PropertyAttrArgsBuilder { pub fn add_path(&mut self, path: &syn::Path) -> Result<(), syn::Error> { if path.is_ident("no_editor") { self.no_editor = true; + } else if path.is_ident("get") { + if let Some(get) = self.get.replace(PropertyGet::Default) { + return Err(syn::Error::new( + path.span(), + format!("there is already a 'get' attribute with value: {:?}", get), + )); + } + } else if path.is_ident("set") { + if let Some(set) = self.set.replace(PropertySet::Default) { + return Err(syn::Error::new( + path.span(), + format!("there is already a 'set' attribute with value: {:?}", set), + )); + } } else { return Err(syn::Error::new( path.span(), @@ -187,12 +313,15 @@ impl PropertyAttrArgsBuilder { impl PropertyAttrArgsBuilder { pub fn done(self) -> PropertyAttrArgs { PropertyAttrArgs { + ty: self.ty, path: self.path, default: self.default, hint: self.hint, before_get: self.before_get, + get: self.get, after_get: self.after_get, before_set: self.before_set, + set: self.set, after_set: self.after_set, no_editor: self.no_editor, } From de96485d1e1b645edef828c5f183dcff6c9ad014 Mon Sep 17 00:00:00 2001 From: bogay Date: Fri, 4 Feb 2022 18:44:32 +0000 Subject: [PATCH 2/3] feat(property): enforce it should be used on `Property` if both get/set are provided --- gdnative-core/src/export/property.rs | 7 + gdnative-derive/src/lib.rs | 15 +++ gdnative-derive/src/native_script.rs | 187 +++++++++++++++++---------- test/src/test_derive.rs | 123 ++++++++++++++++++ 4 files changed, 265 insertions(+), 67 deletions(-) diff --git a/gdnative-core/src/export/property.rs b/gdnative-core/src/export/property.rs index 5e465a303..573cf8f60 100644 --- a/gdnative-core/src/export/property.rs +++ b/gdnative-core/src/export/property.rs @@ -1,4 +1,5 @@ //! Property registration. +use std::marker::PhantomData; use accessor::{Getter, RawGetter, RawSetter, Setter}; use invalid_accessor::{InvalidGetter, InvalidSetter}; @@ -322,6 +323,12 @@ impl PropertyUsage { } } +/// A ZST used to register a property with no backing field for it. +#[derive(Default)] +pub struct Property { + _marker: PhantomData, +} + mod impl_export { use super::*; diff --git a/gdnative-derive/src/lib.rs b/gdnative-derive/src/lib.rs index 3996714fa..5ff938d3e 100644 --- a/gdnative-derive/src/lib.rs +++ b/gdnative-derive/src/lib.rs @@ -218,6 +218,21 @@ pub fn profiled(meta: TokenStream, input: TokenStream) -> TokenStream { /// Call hook methods with `self` and `owner` before and/or after the generated property /// accessors. /// +/// - `get` / `get_ref` / `set` +/// +/// Configure getter/setter for property. All of them can accept a path to specify a custom +/// property accessor. For example, `#[property(get = "Self::my_getter")]` will use +/// `Self::my_getter` as the getter. +/// +/// The difference of `get` and `get_ref` is that `get` will register the getter with +/// `with_getter` function, which means your getter should return an owned value `T`, but +/// `get_ref` use `with_ref_getter` to register getter. In this case, your custom getter +/// should return a shared reference `&T`. +/// +/// `get` and `set` can be used without specifying a path, as long as the field type is not +/// `Property`. In this case, godot-rust generates an accessor function for the field. +/// For example, `#[property(get)]` will generate a read-only property. +/// /// - `no_editor` /// /// Hides the property from the editor. Does not prevent it from being sent over network or saved in storage. diff --git a/gdnative-derive/src/native_script.rs b/gdnative-derive/src/native_script.rs index c8d412a8c..e9e528380 100644 --- a/gdnative-derive/src/native_script.rs +++ b/gdnative-derive/src/native_script.rs @@ -61,78 +61,112 @@ pub(crate) fn derive_native_class(derive_input: &DeriveInput) -> Result = config - .before_get - .map(|path_expr| parse_quote!(#path_expr(this, _owner);)); - let after_get: Option = config - .after_get - .map(|path_expr| parse_quote!(#path_expr(this, _owner);)); - let with_getter = get.map(|get| { - let register_fn = match get { - PropertyGet::Owned(_) => quote!(with_getter), - _ => quote!(with_ref_getter), + let properties = data + .properties + .into_iter() + .map(|(ident, config)| { + let with_default = config + .default + .map(|default_value| quote!(.with_default(#default_value))); + let with_hint = config.hint.map(|hint_fn| quote!(.with_hint(#hint_fn()))); + let with_usage = if config.no_editor { + Some(quote!(.with_usage(::gdnative::export::PropertyUsage::NOEDITOR))) + } else { + None }; - let get: Expr = match get { - PropertyGet::Default => parse_quote!(&this.#ident), - PropertyGet::Owned(path_expr) | PropertyGet::Ref(path_expr) => { - parse_quote!(#path_expr(this, _owner)) - } + // check whether this property type is `Property`. if so, extract T from it. + let property_ty = match config.ty { + Type::Path(ref path) => path + .path + .segments + .iter() + .last() + .filter(|seg| seg.ident == "Property") + .and_then(|seg| match seg.arguments { + syn::PathArguments::AngleBracketed(ref params) => params.args.first(), + _ => None, + }) + .and_then(|arg| match arg { + syn::GenericArgument::Type(ref ty) => Some(ty), + _ => None, + }) + .map(|ty| quote!(::<#ty>)), + _ => None, }; - quote!( - .#register_fn(|this: &#name, _owner: ::gdnative::object::TRef| { - #before_get - let res = #get; - #after_get - res - }) - ) - }); - let before_set: Option = config - .before_set - .map(|path_expr| parse_quote!(#path_expr(this, _owner);)); - let after_set: Option = config - .after_set - .map(|path_expr| parse_quote!(#path_expr(this, _owner);)); - let with_setter = set.map(|set| { - let set: Stmt = match set { - PropertySet::Default => parse_quote!(this.#ident = v;), - PropertySet::WithPath(path_expr) => parse_quote!(#path_expr(this, _owner, v);), + // #[property] is not attached on `Property` + if property_ty.is_none() + // custom getter used + && config.get.as_ref().map(|get| !matches!(get, PropertyGet::Default)).unwrap_or(false) + // custom setter used + && config.set.as_ref().map(|set| !matches!(set, PropertySet::Default)).unwrap_or(false) + { + return Err(syn::Error::new( + ident.span(), + "The `#[property]` attribute can only be used on a field of type `Property`, \ + if a path is provided for both get/set method(s)." + )); + } + // if both of them are not set, i.e. `#[property]`. implicitly use both getter/setter + let (get, set) = if config.get.is_none() && config.set.is_none() { + (Some(PropertyGet::Default), Some(PropertySet::Default)) + } else { + (config.get, config.set) }; - quote!( - .with_setter(|this: &mut #name, _owner: ::gdnative::object::TRef, v| { - #before_set - #set - #after_set + let before_get: Option = config + .before_get + .map(|path_expr| parse_quote!(#path_expr(this, _owner);)); + let after_get: Option = config + .after_get + .map(|path_expr| parse_quote!(#path_expr(this, _owner);)); + let with_getter = get.map(|get| { + let register_fn = match get { + PropertyGet::Owned(_) => quote!(with_getter), + _ => quote!(with_ref_getter), + }; + let get: Expr = match get { + PropertyGet::Default => parse_quote!(&this.#ident), + PropertyGet::Owned(path_expr) | PropertyGet::Ref(path_expr) => parse_quote!(#path_expr(this, _owner)) + }; + quote!( + .#register_fn(|this: &#name, _owner: ::gdnative::object::TRef| { + #before_get + let res = #get; + #after_get + res + }) + ) + }); + let before_set: Option = config + .before_set + .map(|path_expr| parse_quote!(#path_expr(this, _owner);)); + let after_set: Option = config + .after_set + .map(|path_expr| parse_quote!(#path_expr(this, _owner);)); + let with_setter = set.map(|set| { + let set: Stmt = match set { + PropertySet::Default => parse_quote!(this.#ident = v;), + PropertySet::WithPath(path_expr) => parse_quote!(#path_expr(this, _owner, v);), + }; + quote!( + .with_setter(|this: &mut #name, _owner: ::gdnative::object::TRef, v| { + #before_set + #set + #after_set + })) + }); + + let label = config.path.unwrap_or_else(|| format!("{}", ident)); + Ok(quote!({ + builder.property#property_ty(#label) + #with_default + #with_hint + #with_usage + #with_getter + #with_setter + .done(); })) - }); - - let label = config.path.unwrap_or_else(|| format!("{}", ident)); - quote!({ - builder.property(#label) - #with_default - #with_hint - #with_usage - #with_getter - #with_setter - .done(); }) - }); + .collect::, _>>()?; let maybe_statically_named = data.godot_name.map(|name_str| { quote! { @@ -407,4 +441,23 @@ mod tests { let input: DeriveInput = syn::parse2(input).unwrap(); parse_derive_input(&input).unwrap(); } + + #[test] + fn derive_property_require_to_be_used_on_property_without_default_accessor() { + let input: TokenStream2 = syn::parse_str( + r#" + #[inherit(Node)] + struct Foo { + #[property(get = "Self::get_bar", set = "Self::set_bar")] + bar: i64, + }"#, + ) + .unwrap(); + let input: DeriveInput = syn::parse2(input).unwrap(); + assert_eq!( + derive_native_class(&input).unwrap_err().to_string(), + "The `#[property]` attribute can only be used on a field of type `Property`, \ + if a path is provided for both get/set method(s).", + ); + } } diff --git a/test/src/test_derive.rs b/test/src/test_derive.rs index ad92f8547..f47e173ba 100644 --- a/test/src/test_derive.rs +++ b/test/src/test_derive.rs @@ -1,5 +1,6 @@ use std::cell::Cell; +use gdnative::export::Property; use gdnative::prelude::*; pub(crate) fn run_tests() -> bool { @@ -9,6 +10,8 @@ pub(crate) fn run_tests() -> bool { status &= test_derive_owned_to_variant(); status &= test_derive_nativeclass_with_property_hooks(); status &= test_derive_nativeclass_without_constructor(); + status &= test_derive_nativeclass_with_property_get_set(); + status &= test_derive_nativeclass_property_with_only_getter(); status } @@ -16,6 +19,8 @@ pub(crate) fn run_tests() -> bool { pub(crate) fn register(handle: InitHandle) { handle.add_class::(); handle.add_class::(); + handle.add_class::(); + handle.add_class::(); } fn test_derive_to_variant() -> bool { @@ -322,3 +327,121 @@ fn test_derive_nativeclass_without_constructor() -> bool { ok } + +#[derive(NativeClass)] +#[inherit(Node)] +struct CustomGetSet { + pub get_called: Cell, + pub set_called: Cell, + #[allow(dead_code)] + #[property(get_ref = "Self::get_foo", set = "Self::set_foo")] + pub foo: Property, + pub _foo: i32, +} + +#[methods] +impl CustomGetSet { + fn new(_onwer: &Node) -> Self { + Self { + get_called: Cell::new(0), + set_called: Cell::new(0), + foo: Property::default(), + _foo: 0, + } + } + + fn get_foo(&self, _owner: TRef) -> &i32 { + self.get_called.set(self.get_called.get() + 1); + &self._foo + } + + fn set_foo(&mut self, _owner: TRef, value: i32) { + self.set_called.set(self.set_called.get() + 1); + self._foo = value; + } +} + +fn test_derive_nativeclass_with_property_get_set() -> bool { + println!(" -- test_derive_nativeclass_with_property_get_set"); + let ok = std::panic::catch_unwind(|| { + use gdnative::export::user_data::Map; + let (owner, script) = CustomGetSet::new_instance().decouple(); + script + .map(|script| { + assert_eq!(0, script.get_called.get()); + assert_eq!(0, script.set_called.get()); + }) + .unwrap(); + owner.set("foo", 1); + script + .map(|script| { + assert_eq!(0, script.get_called.get()); + assert_eq!(1, script.set_called.get()); + assert_eq!(1, script._foo); + }) + .unwrap(); + assert_eq!(1, i32::from_variant(&owner.get("foo")).unwrap()); + script + .map(|script| { + assert_eq!(1, script.get_called.get()); + assert_eq!(1, script.set_called.get()); + }) + .unwrap(); + owner.free(); + }) + .is_ok(); + + if !ok { + godot_error!(" !! Test test_derive_nativeclass_with_property_get_set failed"); + } + + ok +} + +#[derive(NativeClass)] +struct MyVec { + vec: Vec, + #[allow(dead_code)] + #[property(get = "Self::get_size")] + size: Property, +} + +#[methods] +impl MyVec { + fn new(_owner: TRef) -> Self { + Self { + vec: Vec::new(), + size: Property::default(), + } + } + + fn add(&mut self, val: i32) { + self.vec.push(val); + } + + fn get_size(&self, _owner: TRef) -> u32 { + self.vec.len() as u32 + } +} + +fn test_derive_nativeclass_property_with_only_getter() -> bool { + println!(" -- test_derive_nativeclass_property_with_only_getter"); + + let ok = std::panic::catch_unwind(|| { + use gdnative::export::user_data::MapMut; + let (owner, script) = MyVec::new_instance().decouple(); + assert_eq!(u32::from_variant(&owner.get("size")).unwrap(), 0); + script.map_mut(|script| script.add(42)).unwrap(); + assert_eq!(u32::from_variant(&owner.get("size")).unwrap(), 1); + // check the setter doesn't work for `size` + let _ = std::panic::catch_unwind(|| owner.set("size", 3)); + assert_eq!(u32::from_variant(&owner.get("size")).unwrap(), 1); + }) + .is_ok(); + + if !ok { + godot_error!(" !! Test test_derive_nativeclass_property_with_only_getter failed"); + } + + ok +} From 44bb3f93d0b5b54afa19d91b319d42dd80ad3f6f Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sat, 5 Feb 2022 23:36:14 +0100 Subject: [PATCH 3/3] Handle all #[property(get, set)] combinations + test Changes: * Logic that allows/disallows property <-> field type combinations * Unit tests * Create internal proc_macro2 derive that can be used in tests * Add Property to prelude * Extensive documentation for Property --- gdnative-core/src/export/property.rs | 107 +++++++++++++++++++++++++- gdnative-derive/src/lib.rs | 16 ++-- gdnative-derive/src/native_script.rs | 111 ++++++++++++++++++++------- gdnative/src/prelude.rs | 2 +- 4 files changed, 198 insertions(+), 38 deletions(-) diff --git a/gdnative-core/src/export/property.rs b/gdnative-core/src/export/property.rs index 573cf8f60..bc98b5662 100644 --- a/gdnative-core/src/export/property.rs +++ b/gdnative-core/src/export/property.rs @@ -323,8 +323,111 @@ impl PropertyUsage { } } -/// A ZST used to register a property with no backing field for it. -#[derive(Default)] +/// Placeholder type for exported properties with no backing field. +/// +/// This is the go-to type whenever you want to expose a getter/setter to GDScript, which +/// does not directly map to a field in your struct. Instead of adding a useless field +/// of the corresponding type (which needs initialization, extra space, etc.), you can use +/// an instance of this type as a placeholder. +/// +/// `Property` is a zero-sized type (ZST) which has exactly one value: `Property::default()`. +/// It implements most of the basic traits, which allows its enclosing struct to remain +/// composable and derive those traits itself. +/// +/// ## When to use `Property` instead of `T` +/// +/// The following table shows which combinations of `#[property]` attributes and field types are allowed. +/// In this context, `get` and `set` behave symmetrically, so only one of the combinations is listed. +/// Furthermore, `get_ref` can be used in place of `get`, when it appears with a path. +/// +/// Field type ➡
Attributes ⬇ | bare `T` | `Property` +/// ------------------------------------------|-------------------------------|----------------------------- +/// `#[property]` | ✔️ default get + set | ❌️ +/// `#[property(get, set)]` _(same as above)_ | ✔️ default get + set | ❌️ +/// `#[property(get)]` | ✔️ default get (no set) | ❌️ +/// `#[property(get="path")]` | ⚠️ custom get (no set) | ✔️ custom get (no set) +/// `#[property(get="path", set)]` | ✔️ custom get, default set | ❌️ +/// `#[property(get="path", set="path")]` | ⚠️ custom get + set | ✔️ custom get + set +/// +/// "⚠️" means that this attribute combination is allowed for bare `T`, but you should consider +/// using `Property`. +/// +/// Since there is no default `get` or `set` in these cases, godot-rust will never access the field +/// directly. In other words, you are not really exporting _that field_, but linking its name and type +/// (but not its value) to the specified get/set methods. +/// +/// To decide when to use which: +/// * If you access your field as-is on the Rust side, use bare `T`.
+/// With a `Property` field on the other hand, you would need to _additionally_ add a `T` backing field. +/// * If you don't need a backing field, use `Property`.
+/// This is the case whenever you compute a result dynamically, or map values between Rust and GDScript +/// representations. +/// +/// ## Examples +/// +/// Read/write accessible: +/// ```no_run +/// # use gdnative::prelude::*; +/// #[derive(NativeClass)] +/// # #[no_constructor] +/// struct MyObject { +/// #[property] +/// color: Color, +/// } +/// ``` +/// +/// Read-only: +/// ```no_run +/// # use gdnative::prelude::*; +/// #[derive(NativeClass)] +/// # #[no_constructor] +/// struct MyObject { +/// #[property(get)] +/// hitpoints: f32, +/// } +/// ``` +/// +/// Read-write, with validating setter: +/// ```no_run +/// # use gdnative::prelude::*; +/// # fn validate(s: &String) -> bool { true } +/// #[derive(NativeClass)] +/// # #[no_constructor] +/// struct MyObject { +/// #[property(get, set = "Self::set_name")] +/// player_name: String, +/// } +/// +/// #[methods] +/// impl MyObject { +/// fn set_name(&mut self, _owner: TRef, name: String) { +/// if validate(&name) { +/// self.player_name = name; +/// } +/// } +/// } +/// ``` +/// +/// Write-only, no backing field, custom setter: +/// ```no_run +/// # use gdnative::prelude::*; +/// #[derive(NativeClass)] +/// # #[no_constructor] +/// struct MyObject { +/// #[property(set = "Self::set_password")] +/// password: Property, +/// } +/// +/// #[methods] +/// impl MyObject { +/// fn set_password(&mut self, _owner: TRef, password: String) { +/// // securely hash and store password +/// } +/// } +/// ``` + +// Note: traits are mostly implemented to enable deriving the same traits on the enclosing struct. +#[derive(Copy, Clone, Debug, Default, Ord, PartialOrd, Eq, PartialEq, Hash)] pub struct Property { _marker: PhantomData, } diff --git a/gdnative-derive/src/lib.rs b/gdnative-derive/src/lib.rs index 5ff938d3e..612a0df54 100644 --- a/gdnative-derive/src/lib.rs +++ b/gdnative-derive/src/lib.rs @@ -229,9 +229,9 @@ pub fn profiled(meta: TokenStream, input: TokenStream) -> TokenStream { /// `get_ref` use `with_ref_getter` to register getter. In this case, your custom getter /// should return a shared reference `&T`. /// -/// `get` and `set` can be used without specifying a path, as long as the field type is not -/// `Property`. In this case, godot-rust generates an accessor function for the field. -/// For example, `#[property(get)]` will generate a read-only property. +/// Situations with custom getters/setters and no backing fields require the use of the +/// type [`Property`][gdnative::export::Property]. Consult its documentation for +/// a deeper elaboration of property exporting. /// /// - `no_editor` /// @@ -394,19 +394,21 @@ pub fn derive_native_class(input: TokenStream) -> TokenStream { let derive_input = syn::parse_macro_input!(input as DeriveInput); // Implement NativeClass for the input - native_script::derive_native_class(&derive_input).map_or_else( + let derived = native_script::derive_native_class(&derive_input).map_or_else( |err| { // Silence the other errors that happen because NativeClass is not implemented let empty_nativeclass = native_script::impl_empty_nativeclass(&derive_input); let err = err.to_compile_error(); - TokenStream::from(quote! { + quote! { #empty_nativeclass #err - }) + } }, std::convert::identity, - ) + ); + + TokenStream::from(derived) } #[proc_macro_derive(ToVariant, attributes(variant))] diff --git a/gdnative-derive/src/native_script.rs b/gdnative-derive/src/native_script.rs index e9e528380..d10aa088f 100644 --- a/gdnative-derive/src/native_script.rs +++ b/gdnative-derive/src/native_script.rs @@ -1,4 +1,3 @@ -use proc_macro::TokenStream; use proc_macro2::TokenStream as TokenStream2; use syn::spanned::Spanned; @@ -48,7 +47,7 @@ pub(crate) fn impl_empty_nativeclass(derive_input: &DeriveInput) -> TokenStream2 } } -pub(crate) fn derive_native_class(derive_input: &DeriveInput) -> Result { +pub(crate) fn derive_native_class(derive_input: &DeriveInput) -> Result { let derived = crate::automatically_derived(); let data = parse_derive_input(derive_input)?; @@ -93,21 +92,27 @@ pub(crate) fn derive_native_class(derive_input: &DeriveInput) -> Result)), _ => None, }; - // #[property] is not attached on `Property` - if property_ty.is_none() - // custom getter used - && config.get.as_ref().map(|get| !matches!(get, PropertyGet::Default)).unwrap_or(false) - // custom setter used - && config.set.as_ref().map(|set| !matches!(set, PropertySet::Default)).unwrap_or(false) + + // Attribute is #[property] (or has other arguments which are not relevant here) + let is_standalone_attribute = config.get.is_none() && config.set.is_none(); + // Attribute is #[property(get)] or #[property(get, set="path")] + let has_default_getter = matches!(config.get, Some(PropertyGet::Default)); + // Attribute is #[property(set)] or #[property(get="path", set)] + let has_default_setter = matches!(config.set, Some(PropertySet::Default)); + + // Field type is `Property` + if property_ty.is_some() + && (is_standalone_attribute || has_default_getter || has_default_setter) { return Err(syn::Error::new( ident.span(), - "The `#[property]` attribute can only be used on a field of type `Property`, \ - if a path is provided for both get/set method(s)." + "The `#[property]` attribute requires explicit paths for `get` and `set` argument; \ + the defaults #[property], #[property(get)] and #[property(set)] are not allowed." )); } + // if both of them are not set, i.e. `#[property]`. implicitly use both getter/setter - let (get, set) = if config.get.is_none() && config.set.is_none() { + let (get, set) = if is_standalone_attribute { (Some(PropertyGet::Default), Some(PropertySet::Default)) } else { (config.get, config.set) @@ -206,7 +211,7 @@ pub(crate) fn derive_native_class(derive_input: &DeriveInput) -> Result Result { @@ -443,21 +448,71 @@ mod tests { } #[test] - fn derive_property_require_to_be_used_on_property_without_default_accessor() { - let input: TokenStream2 = syn::parse_str( - r#" - #[inherit(Node)] - struct Foo { - #[property(get = "Self::get_bar", set = "Self::set_bar")] - bar: i64, - }"#, - ) - .unwrap(); - let input: DeriveInput = syn::parse2(input).unwrap(); - assert_eq!( - derive_native_class(&input).unwrap_err().to_string(), - "The `#[property]` attribute can only be used on a field of type `Property`, \ - if a path is provided for both get/set method(s).", - ); + fn derive_property_combinations() { + let attr_none = quote! { #[property] }; + let attr_get = quote! { #[property(get )] }; + let attr_getp = quote! { #[property(get="path" )] }; + let attr_set = quote! { #[property( set )] }; + let attr_setp = quote! { #[property( set="path")] }; + let attr_get_set = quote! { #[property(get, set )] }; + let attr_get_setp = quote! { #[property(get, set="path")] }; + let attr_getp_set = quote! { #[property(get="path", set )] }; + let attr_getp_setp = quote! { #[property(get="path", set="path")] }; + + // See documentation of Property for this table + // Columns: #[property] attributes | i32 style fields | Property style fields + let combinations = [ + (attr_none, true, false), + (attr_get, true, false), + (attr_getp, true, true), + (attr_set, true, false), + (attr_setp, true, true), + (attr_get_set, true, false), + (attr_get_setp, true, false), + (attr_getp_set, true, false), + (attr_getp_setp, true, true), + ]; + + for (attr, allowed_bare, allowed_property) in combinations { + check_property_combination(&attr, quote! { i32 }, allowed_bare); + check_property_combination(&attr, quote! { Property }, allowed_property); + } + } + + /// Tests whether a certain combination of a #[property] attribute (attr) and a field type + /// (bare i32 or Property) should compile successfully + fn check_property_combination( + attr: &TokenStream2, + field_type: TokenStream2, + should_succeed: bool, + ) { + // Lazy because of formatting in error message + let input = || { + quote! { + #[inherit(Node)] + struct Foo { + #attr + field: #field_type + } + } + }; + + let derive_input: DeriveInput = syn::parse2(input()).unwrap(); + let derived = derive_native_class(&derive_input); + + if should_succeed { + assert!( + derived.is_ok(), + "Valid derive expression fails to compile:\n{}", + input().to_string() + ); + } else { + assert_eq!( + derived.unwrap_err().to_string(), + "The `#[property]` attribute requires explicit paths for `get` and `set` argument; \ + the defaults #[property], #[property(get)] and #[property(set)] are not allowed.", + "Invalid derive expression compiles by mistake:\n{}", input().to_string() + ); + } } } diff --git a/gdnative/src/prelude.rs b/gdnative/src/prelude.rs index 0b8f87794..3fdf5a625 100644 --- a/gdnative/src/prelude.rs +++ b/gdnative/src/prelude.rs @@ -14,7 +14,7 @@ pub use gdnative_core::core_types::{ FromVariant, FromVariantError, OwnedToVariant, ToVariant, ToVariantEq, }; pub use gdnative_core::export::{ - ClassBuilder, ExportInfo, Method, MethodBuilder, NativeClass, NativeClassMethods, + ClassBuilder, ExportInfo, Method, MethodBuilder, NativeClass, NativeClassMethods, Property, PropertyUsage, SignalBuilder, SignalParam, }; pub use gdnative_core::init::InitHandle;