feat(property): enforce it should be used on Property if both get/s…

…et are provided

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<T> {
+    _marker: PhantomData<T>,
+}
+
 mod impl_export {
     use super::*;
 

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<T>`. 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.

gdnative-derive/src/native_script.rs

@@ -61,78 +61,112 @@ pub(crate) fn derive_native_class(derive_input: &DeriveInput) -> Result<TokenStr
             .register_callback
             .map(|function_path| quote!(#function_path(builder);))
             .unwrap_or(quote!({}));
-        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
-            };
-            // 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)
-            };
-            let before_get: Option<Stmt> = config
-                .before_get
-                .map(|path_expr| parse_quote!(#path_expr(this, _owner);));
-            let after_get: Option<Stmt> = 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<T>`. 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<Self::Base>| {
-                        #before_get
-                        let res = #get;
-                        #after_get
-                        res
-                    })
-                )
-            });
-            let before_set: Option<Stmt> = config
-                .before_set
-                .map(|path_expr| parse_quote!(#path_expr(this, _owner);));
-            let after_set: Option<Stmt> = 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<T>`
+                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<Self::Base>, v| {
-                    #before_set
-                    #set
-                    #after_set
+                let before_get: Option<Stmt> = config
+                    .before_get
+                    .map(|path_expr| parse_quote!(#path_expr(this, _owner);));
+                let after_get: Option<Stmt> = 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<Self::Base>| {
+                            #before_get
+                            let res = #get;
+                            #after_get
+                            res
+                        })
+                    )
+                });
+                let before_set: Option<Stmt> = config
+                    .before_set
+                    .map(|path_expr| parse_quote!(#path_expr(this, _owner);));
+                let after_set: Option<Stmt> = 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<Self::Base>, 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::<Result<Vec<_>, _>>()?;
 
         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).",
+        );
+    }
 }

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,13 +10,17 @@ 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
 }
 
 pub(crate) fn register(handle: InitHandle) {
     handle.add_class::<PropertyHooks>();
     handle.add_class::<EmplacementOnly>();
+    handle.add_class::<CustomGetSet>();
+    handle.add_class::<MyVec>();
 }
 
 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<i32>,
+    pub set_called: Cell<i32>,
+    #[allow(dead_code)]
+    #[property(get_ref = "Self::get_foo", set = "Self::set_foo")]
+    pub foo: Property<i32>,
+    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<Node>) -> &i32 {
+        self.get_called.set(self.get_called.get() + 1);
+        &self._foo
+    }
+
+    fn set_foo(&mut self, _owner: TRef<Node>, 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<i32>,
+    #[allow(dead_code)]
+    #[property(get = "Self::get_size")]
+    size: Property<u32>,
+}
+
+#[methods]
+impl MyVec {
+    fn new(_owner: TRef<Reference>) -> Self {
+        Self {
+            vec: Vec::new(),
+            size: Property::default(),
+        }
+    }
+
+    fn add(&mut self, val: i32) {
+        self.vec.push(val);
+    }
+
+    fn get_size(&self, _owner: TRef<Reference>) -> 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
+}