From 768020fc17b8cb9e1bf475b2f781c41657b23b04 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sat, 5 Feb 2022 23:36:14 +0100 Subject: [PATCH] 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..4c615dddc 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;