Allow omitting getter/setter from #[export]

Also:

- Remove unused `property` argument.
- Rename arguments `getter` and `setter` to `get` and `set` to stay
  closer to GDScript and save keystrokes.
- Check at compilation time that the referenced getter and setter
  actually exist (otherwise Godot gives a cryptic "invalid get/set
  index" error).

godot-macros/src/derive_godot_class.rs

@@ -4,7 +4,7 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
 
-use crate::util::{bail, ident, KvParser};
+use crate::util::{bail, ident, strip_quotes, KvParser};
 use crate::ParseResult;
 use proc_macro2::{Ident, Punct, TokenStream};
 use quote::{format_ident, quote};
@@ -68,7 +68,7 @@ fn parse_struct_attributes(class: &Struct) -> ParseResult<ClassAttributes> {
     let mut base_ty = ident("RefCounted");
     let mut has_generated_init = false;
 
-    // #[func] attribute on struct
+    // #[class] attribute on struct
     if let Some(mut parser) = KvParser::parse(&class.attributes, "class")? {
         if let Some(base) = parser.handle_ident("base")? {
             base_ty = base;
@@ -174,8 +174,8 @@ impl Field {
 
 struct ExportedField {
     field: Field,
-    getter: String,
-    setter: String,
+    getter: Option<String>,
+    setter: Option<String>,
     hint: Option<ExportHint>,
 }
 
@@ -196,8 +196,8 @@ impl ExportHint {
 
 impl ExportedField {
     pub fn new_from_kv(field: Field, parser: &mut KvParser) -> ParseResult<ExportedField> {
-        let getter = parser.handle_lit_required("getter")?;
-        let setter = parser.handle_lit_required("setter")?;
+        let getter = parser.handle_lit("get")?;
+        let setter = parser.handle_lit("set")?;
 
         let hint = parser
             .handle_ident("hint")?
@@ -265,51 +265,109 @@ fn make_deref_impl(class_name: &Ident, fields: &Fields) -> TokenStream {
 }
 
 fn make_exports_impl(class_name: &Ident, fields: &Fields) -> TokenStream {
-    let export_tokens = fields
-        .exported_fields
-        .iter()
-        .map(|exported_field: &ExportedField| {
-            use std::str::FromStr;
-            let name = exported_field.field.name.to_string();
-            let getter = proc_macro2::Literal::from_str(&exported_field.getter).unwrap();
-            let setter = proc_macro2::Literal::from_str(&exported_field.setter).unwrap();
-            let field_type = exported_field.field.ty.clone();
-
-            let ExportHint {
-                hint_type,
-                description,
-            } = exported_field.hint.clone().unwrap_or_else(ExportHint::none);
-
-            // trims '"' and '\' from both ends of the hint description.
-            let description = description.trim_matches(|c| c == '\\' || c == '"');
-
-            quote! {
-                use ::godot::builtin::meta::VariantMetadata;
-
-                let class_name = ::godot::builtin::StringName::from(#class_name::CLASS_NAME);
-                let property_info = ::godot::builtin::meta::PropertyInfo::new(
-                    <#field_type>::variant_type(),
-                    ::godot::builtin::meta::ClassName::of::<#class_name>(),
-                    ::godot::builtin::StringName::from(#name),
-                    ::godot::engine::global::PropertyHint::#hint_type,
-                    GodotString::from(#description),
-                );
-                let property_info_sys = property_info.property_sys();
-
-                let getter_string_name = ::godot::builtin::StringName::from(#getter);
-                let setter_string_name = ::godot::builtin::StringName::from(#setter);
-                unsafe {
-                    ::godot::sys::interface_fn!(classdb_register_extension_class_property)(
-                        ::godot::sys::get_library(),
-                        class_name.string_sys(),
-                        std::ptr::addr_of!(property_info_sys),
-                        setter_string_name.string_sys(),
-                        getter_string_name.string_sys(),
-                    );
+    use std::str::FromStr;
+
+    let mut getter_setter_impls = Vec::new();
+    let mut export_tokens = Vec::with_capacity(fields.exported_fields.len());
+
+    for exported_field in &fields.exported_fields {
+        let field_name = exported_field.field.name.to_string();
+        let field_ident = proc_macro2::Ident::new(&field_name, proc_macro2::Span::call_site());
+        let field_type = exported_field.field.ty.clone();
+
+        let getter_name = exported_field
+            .getter
+            .as_ref()
+            .map(String::to_owned)
+            .unwrap_or_else(|| format!("\"get_{}\"", field_name));
+        let setter_name = exported_field
+            .setter
+            .as_ref()
+            .map(String::to_owned)
+            .unwrap_or_else(|| format!("\"set_{}\"", field_name));
+        let getter_lit = proc_macro2::Literal::from_str(&getter_name).unwrap();
+        let setter_lit = proc_macro2::Literal::from_str(&setter_name).unwrap();
+        let getter_ident =
+            proc_macro2::Ident::new(&strip_quotes(&getter_name), proc_macro2::Span::call_site());
+        let setter_ident =
+            proc_macro2::Ident::new(&strip_quotes(&setter_name), proc_macro2::Span::call_site());
+
+        let ExportHint {
+            hint_type,
+            description,
+        } = exported_field.hint.clone().unwrap_or_else(ExportHint::none);
+
+        // trims '"' and '\' from both ends of the hint description.
+        let description = description.trim_matches(|c| c == '\\' || c == '"');
+
+        if exported_field.getter.is_none() {
+            let signature = quote! {
+                fn #getter_ident(&self) -> #field_type
+            };
+            getter_setter_impls.push(quote! {
+                #signature {
+                    self.#field_ident
+                }
+            });
+            export_tokens.push(quote! {
+                ::godot::private::gdext_register_method!(#class_name, #signature);
+            });
+        };
+
+        if exported_field.setter.is_none() {
+            let signature = quote! {
+                fn #setter_ident(&mut self, #field_ident: #field_type)
+            };
+            getter_setter_impls.push(quote! {
+                #signature {
+                    self.#field_ident = #field_ident;
                 }
+            });
+            export_tokens.push(quote! {
+                ::godot::private::gdext_register_method!(#class_name, #signature);
+            });
+        };
+
+        export_tokens.push(quote! {
+            use ::godot::builtin::meta::VariantMetadata;
+
+            let class_name = ::godot::builtin::StringName::from(#class_name::CLASS_NAME);
+
+            let property_info = ::godot::builtin::meta::PropertyInfo::new(
+                <#field_type>::variant_type(),
+                ::godot::builtin::meta::ClassName::of::<#class_name>(),
+                ::godot::builtin::StringName::from(#field_name),
+                ::godot::engine::global::PropertyHint::#hint_type,
+                GodotString::from(#description),
+            );
+            let property_info_sys = property_info.property_sys();
+
+            // Check at compile time that these functions actually exist.
+            // TODO add a compile test (e.g. `trybuild` crate) for this
+            #[allow(path_statements)]
+            Self::#getter_ident;
+            #[allow(path_statements)]
+            Self::#setter_ident;
+
+            let getter_string_name = ::godot::builtin::StringName::from(#getter_lit);
+            let setter_string_name = ::godot::builtin::StringName::from(#setter_lit);
+            unsafe {
+                ::godot::sys::interface_fn!(classdb_register_extension_class_property)(
+                    ::godot::sys::get_library(),
+                    class_name.string_sys(),
+                    std::ptr::addr_of!(property_info_sys),
+                    setter_string_name.string_sys(),
+                    getter_string_name.string_sys(),
+                );
             }
         });
+    }
+
     quote! {
+        impl #class_name {
+            #(#getter_setter_impls)*
+        }
+
         impl ::godot::obj::cap::ImplementsGodotExports for #class_name {
             fn __register_exports() {
                 #(
@@ -321,3 +379,23 @@ fn make_exports_impl(class_name: &Ident, fields: &Fields) -> TokenStream {
         }
     }
 }
+
+/// Tests that the proc-macro doesn't panic. This is helpful because `RUST_BACKTRACE=1` does not
+/// affect panics in macro invocations (e.g. in our integration tests), but it works fine in unit
+/// tests.
+#[test]
+fn does_not_panic() {
+    let decl = venial::parse_declaration(quote! {
+        #[class(base=Node)]
+        struct HasProperty {
+            #[base]
+            base: Base<Node>,
+            #[export]
+            int_val_default: i32,
+            #[export(get = "get_int_val", set = "set_int_val")]
+            int_val: i32,
+        }
+    })
+    .unwrap();
+    transform(decl).unwrap();
+}

godot-macros/src/lib.rs

@@ -16,7 +16,7 @@ mod godot_api;
 mod itest;
 mod util;
 
-#[proc_macro_derive(GodotClass, attributes(class, property, export, base, signal))]
+#[proc_macro_derive(GodotClass, attributes(class, export, base, signal))]
 pub fn derive_native_class(input: TokenStream) -> TokenStream {
     translate(input, derive_godot_class::transform)
 }

godot-macros/src/util.rs

@@ -63,8 +63,6 @@ pub(crate) struct KvParser {
     attr_name: String,
     map: KvMap,
     span: Span,
-    #[cfg(debug_assertions)]
-    finished: bool,
 }
 
 #[allow(dead_code)] // some functions will be used later
@@ -89,7 +87,7 @@ impl KvParser {
 
     /// Create a new parser which checks for presence of an `#[expected]` attribute.
     pub fn parse(attributes: &[Attribute], expected: &str) -> ParseResult<Option<Self>> {
-        let mut found_attr = None;
+        let mut found_attr: Option<Self> = None;
 
         for attr in attributes.iter() {
             let path = &attr.path;
@@ -105,8 +103,6 @@ impl KvParser {
                     attr_name: expected.to_string(),
                     span: attr.__span(),
                     map: parse_kv_group(&attr.value)?,
-                    #[cfg(debug_assertions)]
-                    finished: false,
                 });
             }
         }
@@ -169,14 +165,12 @@ impl KvParser {
         }
     }
 
-    /// Explicit "pre-destructor" that must be called, because Drop cannot propagate errors.
-    // Note: this could possibly be modeled using a closure: KvParser::parse(...) .with(|parser| ...)?
-    pub fn finish(mut self) -> ParseResult<()> {
-        #[cfg(debug_assertions)]
-        {
-            self.finished = true; // disarm destructor
-        }
-
+    /// Explicit "pre-destructor" that must be called, and checks that all map entries have been
+    /// consumed.
+    // We used to check in a `Drop` impl that `finish` has actually been called, but that turns out
+    // to be overzealous: it panics if the calling function just wants to return an error and drops
+    // a partially-consumed parser.
+    pub fn finish(self) -> ParseResult<()> {
         if self.map.is_empty() {
             Ok(())
         } else {
@@ -202,16 +196,6 @@ impl KvParser {
     }
 }
 
-#[cfg(debug_assertions)]
-impl Drop for KvParser {
-    fn drop(&mut self) {
-        assert!(
-            self.finished,
-            "proc-macro did not check for remaining elements; this is a bug in the library"
-        );
-    }
-}
-
 // parses (a="hey", b=342)
 pub(crate) fn parse_kv_group(value: &venial::AttributeValue) -> ParseResult<KvMap> {
     // FSM with possible flows:
@@ -506,3 +490,28 @@ pub(crate) fn path_ends_with(path: &[TokenTree], expected: &str) -> bool {
         .map(|last| last.to_string() == expected)
         .unwrap_or(false)
 }
+
+/// Strips surrounding double quotes from a string. Panics if there aren't any.
+pub(crate) fn strip_quotes(s: &str) -> String {
+    let bytes = s.as_bytes();
+    let len = bytes.len();
+    assert!(
+        len >= 2,
+        "string {:?} is not surrounded by double quotes",
+        s
+    );
+    assert_eq!(
+        bytes[0], b'"',
+        "string {:?} does not start with a double quote",
+        s
+    );
+    assert_eq!(
+        bytes[len - 1],
+        b'"',
+        "string {:?} does not end with a double quote",
+        s
+    );
+    // SAFETY: " is an ASCII character, so guaranteed to be one byte. Stripping it out from a
+    // valid UTF-8 string results in a valid UTF-8 string.
+    unsafe { String::from_utf8_unchecked(bytes[1..len - 1].to_owned()) }
+}

itest/godot/ManualFfiTests.gd

@@ -29,6 +29,15 @@ func test_export():
 	obj.int_val = 5
 	assert_eq(obj.int_val, 5)
 
+	obj.int_val_get = 5
+	assert_eq(obj.int_val_get, 10)
+
+	obj.int_val_set = 5
+	assert_eq(obj.int_val_set, 10)
+
+	obj.int_val_get_set = 5
+	assert_eq(obj.int_val_get_set, 5)
+
 	obj.string_val = "test val"
 	assert_eq(obj.string_val, "test val")
 

itest/rust/src/export_test.rs

@@ -13,26 +13,42 @@ use godot::{engine::Texture, prelude::*};
 struct HasProperty {
     #[base]
     base: Base<Node>,
-    #[export(getter = "get_int_val", setter = "set_int_val")]
+    #[export]
     int_val: i32,
-    #[export(getter = "get_string_val", setter = "set_string_val")]
+    #[export(get = "get_int_val_get")]
+    int_val_get: i32,
+    #[export(set = "set_int_val_set")]
+    int_val_set: i32,
+    #[export(get = "get_int_val_get_set", set = "set_int_val_get_set")]
+    int_val_get_set: i32,
+    #[export(get = "get_string_val", set = "set_string_val")]
     string_val: GodotString,
-    #[export(getter = "get_object_val", setter = "set_object_val")]
+    #[export(get = "get_object_val", set = "set_object_val")]
     object_val: Option<Gd<Object>>,
-    #[export(getter = "get_texture_val", setter = "set_texture_val", hint = PROPERTY_HINT_RESOURCE_TYPE, hint_desc = "Texture")]
+    #[export(get = "get_texture_val", set = "set_texture_val", hint = PROPERTY_HINT_RESOURCE_TYPE, hint_desc = "Texture")]
     texture_val: Option<Gd<Texture>>,
 }
 
 #[godot_api]
 impl HasProperty {
     #[func]
-    pub fn get_int_val(&self) -> i32 {
-        self.int_val
+    pub fn get_int_val_get(&self) -> i32 {
+        self.int_val_get * 2
     }
 
     #[func]
-    pub fn set_int_val(&mut self, val: i32) {
-        self.int_val = val;
+    pub fn set_int_val_set(&mut self, val: i32) {
+        self.int_val_set = val * 2;
+    }
+
+    #[func]
+    pub fn get_int_val_get_set(&self) -> i32 {
+        self.int_val_get_set
+    }
+
+    #[func]
+    pub fn set_int_val_get_set(&mut self, val: i32) {
+        self.int_val_get_set = val;
     }
 
     #[func]
@@ -79,6 +95,9 @@ impl GodotExt for HasProperty {
     fn init(base: Base<Node>) -> Self {
         HasProperty {
             int_val: 0,
+            int_val_get: 0,
+            int_val_set: 0,
+            int_val_get_set: 0,
             object_val: None,
             string_val: GodotString::new(),
             texture_val: None,