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

Author: Bromeon

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<T>` 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 ➡ <br> Attributes ⬇           | bare `T`                      | `Property<T>`
+/// ------------------------------------------|-------------------------------|-----------------------------
+/// `#[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<T>`.
+///
+/// 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`.<br>
+///   With a `Property<T>` 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<T>`.<br>
+///   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<Reference>, 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<String>,
+/// }
+///
+/// #[methods]
+/// impl MyObject {
+///     fn set_password(&mut self, _owner: TRef<Reference>, 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<T> {
     _marker: PhantomData<T>,
 }

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<T>`. 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<T>`][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))]

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<TokenStream, syn::Error> {
+pub(crate) fn derive_native_class(derive_input: &DeriveInput) -> Result<TokenStream2, syn::Error> {
     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<TokenStr
                         .map(|ty| quote!(::<#ty>)),
                     _ => None,
                 };
-                // #[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)
+
+                // 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<T>`
+                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<TokenStr
     };
 
     // create output token stream
-    Ok(trait_impl.into())
+    Ok(trait_impl)
 }
 
 fn parse_derive_input(input: &DeriveInput) -> Result<DeriveData, syn::Error> {
@@ -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<T> for this table
+        // Columns: #[property] attributes | i32 style fields | Property<i32> 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<i32> }, *allowed_property);
+        }
+    }
+
+    /// Tests whether a certain combination of a `#[property]` attribute (attr) and a field type
+    /// (bare i32 or Property<i32>) 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()
+            );
+        }
     }
 }

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;