diff --git a/godot-core/src/private.rs b/godot-core/src/private.rs index faeafe548..3e6f85e07 100644 --- a/godot-core/src/private.rs +++ b/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(_getter: impl Fn(&C) -> T::PubType) {} +pub fn typecheck_setter(_setter: fn(&mut C, T::PubType)) {} + // ---------------------------------------------------------------------------------------------------------------------------------------------- // Capability queries and internal access diff --git a/godot-core/src/registry/property/mod.rs b/godot-core/src/registry/property/mod.rs index ec3b1617d..0ab8bd26a 100644 --- a/godot-core/src/registry/property/mod.rs +++ b/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. diff --git a/godot-core/src/registry/property/phantom_var.rs b/godot-core/src/registry/property/phantom_var.rs index 107f8ebb9..bda5b1f95 100644 --- a/godot-core/src/registry/property/phantom_var.rs +++ b/godot-core/src/registry/property/phantom_var.rs @@ -64,7 +64,9 @@ impl GodotConvert for PhantomVar { // `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 Var for PhantomVar { - 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") diff --git a/godot-macros/src/class/data_models/field_var.rs b/godot-macros/src/class/data_models/field_var.rs index 723b4351a..3b18465b6 100644 --- a/godot-macros/src/class/data_models/field_var.rs +++ b/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) -> 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, + ) -> 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, diff --git a/godot-macros/src/class/derive_godot_class.rs b/godot-macros/src/class/derive_godot_class.rs index e55652cd1..c716258ad 100644 --- a/godot-macros/src/class/derive_godot_class.rs +++ b/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) + ) + }, } } diff --git a/godot-macros/src/util/mod.rs b/godot-macros/src/util/mod.rs index 0b4f5f248..eda7a5399 100644 --- a/godot-macros/src/util/mod.rs +++ b/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) diff --git a/itest/rust/src/object_tests/phantom_var_test.rs b/itest/rust/src/object_tests/phantom_var_test.rs index 217d164b4..2409bdcfc 100644 --- a/itest/rust/src/object_tests/phantom_var_test.rs +++ b/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, - #[var(get = get_read_write, set = set_read_write)] + #[var(get, set)] read_write: PhantomVar, #[var(get = get_engine_enum, set = set_engine_enum)] read_write_engine_enum: PhantomVar, - #[var(get = get_bit_enum,set = set_bit_enum)] + #[var(get = get_bit_enum, set = set_bit_enum)] read_write_bit_enum: PhantomVar, value: i64, diff --git a/itest/rust/src/object_tests/property_test.rs b/itest/rust/src/object_tests/property_test.rs index 5832e8e4c..7c77be4ce 100644 --- a/itest/rust/src/object_tests/property_test.rs +++ b/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> { + self.object_val.clone() } #[func] - pub fn set_object_val(&mut self, val: Gd) { - self.object_val = Some(val); + pub fn set_object_val(&mut self, val: Option>) { + 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> { + self.resource_rw.clone() } #[func] - pub fn set_resource_rw(&mut self, val: Gd) { - self.resource_rw = Some(val); + pub fn set_resource_rw(&mut self, val: Option>) { + self.resource_rw = val; } }