Add type-checking to #[var(get, set)]

godot-core/src/private.rs

@@ -16,6 +16,7 @@ use crate::global::godot_error;
 use crate::meta::error::{CallError, CallResult};
 use crate::meta::CallContext;
 use crate::obj::Gd;
+use crate::registry::property::Var;
 use crate::{classes, sys};
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------
@@ -188,6 +189,14 @@ pub struct ClassConfig {
     pub is_tool: bool,
 }
 
+// ----------------------------------------------------------------------------------------------------------------------------------------------
+// Type-checkers for user-defined getters/setters in Var
+
+// These functions are used to generate nice error messages if a #[var(get)], [var(get = my_getter)] etc. mismatches types.
+// Don't modify without thorough UX testing; the use of `impl Fn` vs. `fn` is deliberate.
+pub fn typecheck_getter<C, T: Var>(_getter: impl Fn(&C) -> T::PubType) {}
+pub fn typecheck_setter<C, T: Var>(_setter: fn(&mut C, T::PubType)) {}
+
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Capability queries and internal access
 

godot-core/src/registry/property/mod.rs

@@ -182,6 +182,42 @@ pub trait BuiltinExport {}
 ///     field: i32,
 /// }
 /// ```
+///
+/// Custom getter must return the correct type (matching the field's `PubType`):
+///
+/// ```compile_fail
+/// use godot::prelude::*;
+///
+/// #[derive(GodotClass)]
+/// #[class(init)]
+/// struct Foo {
+///     #[var(get = my_getter)]
+///     field: GString,
+/// }
+///
+/// #[godot_api]
+/// impl Foo {
+///     fn my_getter(&self) -> i32 { 42 }
+/// }
+/// ```
+///
+/// Custom setter must accept the correct type (matching the field's `PubType`):
+///
+/// ```compile_fail
+/// use godot::prelude::*;
+///
+/// #[derive(GodotClass)]
+/// #[class(init)]
+/// struct Foo {
+///     #[var(set = my_setter)]
+///     field: GString,
+/// }
+///
+/// #[godot_api]
+/// impl Foo {
+///     fn my_setter(&mut self, value: i32) {}
+/// }
+/// ```
 fn __var_doctests() {}
 
 /// This function only exists as a place to add doc-tests for the `Export` trait.

godot-core/src/registry/property/phantom_var.rs

@@ -64,7 +64,9 @@ impl<T: GodotConvert + Var> GodotConvert for PhantomVar<T> {
 // `PhantomVar` supports only part of `Var`, but it has to implement it, otherwise we cannot implement `Export` either.
 // The `GodotClass` derive macro should ensure that the `Var` implementation is not used.
 impl<T: GodotConvert + Var> Var for PhantomVar<T> {
-    type PubType = std::convert::Infallible;
+    // Needs to be the inner type, because type-checking on user-defined getters/setters is based on this associated type.
+    // In practice, #[var(pub)] cannot be used with PhantomVar.
+    type PubType = T;
 
     fn var_get(_field: &Self) -> Self::Via {
         unreachable!("PhantomVar requires custom getter")

godot-macros/src/class/data_models/field_var.rs

@@ -9,7 +9,7 @@ use proc_macro2::{Ident, Span, TokenStream};
 use quote::{format_ident, quote, quote_spanned};
 
 use crate::class::{
-    into_signature_info, make_existence_check, make_method_registration, Field, FieldHint,
+    into_signature_info, make_accessor_type_check, make_method_registration, Field, FieldHint,
     FuncDefinition,
 };
 use crate::util::{make_funcs_collection_constant, KvParser};
@@ -165,13 +165,11 @@ impl GetterSetter {
                 rust_public,
             )),
             GetterSetter::Custom => Some(GetterSetterImpl::from_custom_impl(
-                kind,
-                &field.name,
-                rename,
+                class_name, kind, field, rename,
             )),
-            GetterSetter::CustomRenamed(function_name) => {
-                Some(GetterSetterImpl::from_custom_impl_renamed(function_name))
-            }
+            GetterSetter::CustomRenamed(function_name) => Some(
+                GetterSetterImpl::from_custom_impl_renamed(class_name, kind, field, function_name),
+            ),
         }
     }
 }
@@ -364,19 +362,31 @@ impl GetterSetterImpl {
     }
 
     /// User-defined name.
-    fn from_custom_impl_renamed(function_name: &Ident) -> Self {
+    fn from_custom_impl_renamed(
+        class_name: &Ident,
+        kind: GetSet,
+        field: &Field,
+        function_name: &Ident,
+    ) -> Self {
+        let export_token = make_accessor_type_check(class_name, function_name, &field.ty, kind);
+
         Self {
             rust_accessor: function_name.clone(),
             function_impl: TokenStream::new(),
-            export_token: make_existence_check(function_name),
+            export_token,
             funcs_collection_constant: TokenStream::new(),
         }
     }
 
     /// Default name for property.
-    fn from_custom_impl(kind: GetSet, field_name: &Ident, rename: &Option<Ident>) -> Self {
-        let function_name = kind.make_pub_fn_name(field_name, rename);
-        let export_token = make_existence_check(&function_name);
+    fn from_custom_impl(
+        class_name: &Ident,
+        kind: GetSet,
+        field: &Field,
+        rename: &Option<Ident>,
+    ) -> Self {
+        let function_name = kind.make_pub_fn_name(&field.name, rename);
+        let export_token = make_accessor_type_check(class_name, &function_name, &field.ty, kind);
 
         Self {
             rust_accessor: function_name,

godot-macros/src/class/derive_godot_class.rs

@@ -16,7 +16,8 @@ use crate::class::{
     FieldExport, FieldVar, GetterSetter, SignatureInfo,
 };
 use crate::util::{
-    bail, error, format_funcs_collection_struct, ident, path_ends_with_complex, KvParser,
+    bail, error, format_funcs_collection_struct, ident, ident_respan, path_ends_with_complex,
+    KvParser,
 };
 use crate::{handle_mutually_exclusive_keys, util, ParseResult};
 
@@ -312,12 +313,38 @@ fn make_deny_manual_init_macro(class_name: &Ident, init_strategy: InitStrategy)
     }
 }
 
-/// Checks at compile time that a function with the given name exists on `Self`.
-#[must_use]
-pub fn make_existence_check(ident: &Ident) -> TokenStream {
-    quote! {
-        #[allow(path_statements)]
-        Self::#ident;
+/// Checks at compile time that a custom (user-defined) getter or setter has the correct signature.
+///
+/// The following signature is expected, with `T = Var::PubType`.
+/// - Getter: `fn(&self) -> T`
+/// - Setter: `fn(&mut self, T)`
+pub fn make_accessor_type_check(
+    class_name: &Ident,
+    accessor_name: &Ident,
+    field_type: &venial::TypeExpr,
+    kind: crate::class::GetSet,
+) -> TokenStream {
+    use crate::class::GetSet;
+
+    // Makes sure the span points to the ident in the macro:
+    //
+    // 76 |     #[var(pub, get = my_custom_get)]
+    //    |                      ^^^^^^^^^^^^^ expected fn pointer, found fn item
+    let accessor_span = accessor_name.span();
+    let class_name = ident_respan(class_name, accessor_span);
+
+    match kind {
+        GetSet::Get => quote_spanned! { accessor_span=>
+            ::godot::private::typecheck_getter::<#class_name, #field_type>(
+                #class_name::#accessor_name
+            )
+        },
+        GetSet::Set => quote_spanned! { accessor_span=>
+            ::godot::private::typecheck_setter::<#class_name, #field_type>(
+                #[allow(clippy::redundant_closure)] // Passing fn ref instead of closure deteriorates error message.
+                |this, val| #class_name::#accessor_name(this, val)
+            )
+        },
     }
 }
 

godot-macros/src/util/mod.rs

@@ -30,6 +30,12 @@ pub fn ident(s: &str) -> Ident {
     format_ident!("{}", s)
 }
 
+pub fn ident_respan(existing_ident: &Ident, span: Span) -> Ident {
+    let mut ident = existing_ident.clone();
+    ident.set_span(span);
+    ident
+}
+
 pub fn c_str(string: &str) -> Literal {
     let c_string = std::ffi::CString::new(string).expect("CString::new() failed");
     Literal::c_string(&c_string)

itest/rust/src/object_tests/phantom_var_test.rs

@@ -14,13 +14,13 @@ struct HasPhantomVar {
     #[var(get = get_read_only, no_set)]
     read_only: PhantomVar<i64>,
 
-    #[var(get = get_read_write, set = set_read_write)]
+    #[var(get, set)]
     read_write: PhantomVar<i64>,
 
     #[var(get = get_engine_enum, set = set_engine_enum)]
     read_write_engine_enum: PhantomVar<godot::global::VerticalAlignment>,
 
-    #[var(get = get_bit_enum,set = set_bit_enum)]
+    #[var(get = get_bit_enum, set = set_bit_enum)]
     read_write_bit_enum: PhantomVar<godot::global::KeyModifierMask>,
 
     value: i64,

itest/rust/src/object_tests/property_test.rs

@@ -41,31 +41,23 @@ struct HasProperty {
 #[godot_api]
 impl HasProperty {
     #[func]
-    pub fn get_object_val(&self) -> Variant {
-        if let Some(object_val) = self.object_val.as_ref() {
-            object_val.to_variant()
-        } else {
-            Variant::nil()
-        }
+    pub fn get_object_val(&self) -> Option<Gd<Object>> {
+        self.object_val.clone()
     }
 
     #[func]
-    pub fn set_object_val(&mut self, val: Gd<Object>) {
-        self.object_val = Some(val);
+    pub fn set_object_val(&mut self, val: Option<Gd<Object>>) {
+        self.object_val = val;
     }
 
     #[func]
-    pub fn get_resource_rw(&self) -> Variant {
-        if let Some(resource) = self.resource_rw.as_ref() {
-            resource.to_variant()
-        } else {
-            Variant::nil()
-        }
+    pub fn get_resource_rw(&self) -> Option<Gd<Resource>> {
+        self.resource_rw.clone()
     }
 
     #[func]
-    pub fn set_resource_rw(&mut self, val: Gd<Resource>) {
-        self.resource_rw = Some(val);
+    pub fn set_resource_rw(&mut self, val: Option<Gd<Resource>>) {
+        self.resource_rw = val;
     }
 }