From 7c22d8f8c2a35b2a6bc10653288cc8ba3127ad01 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Wed, 3 Jun 2026 14:46:04 +0200 Subject: [PATCH] Fix `#[var(pub)]` panic during `OnEditor>` reload The accessor registered with Godot now always goes through `GodotConvert::Via` instead of `Var::PubType`. `Via` is nullable for wrappers like `OnEditor>`, so Godot-driven serialization (e.g. hot reload) reads an uninitialized field as `null` rather than panicking when unwrapping `PubType`. The Rust-facing `#[var(pub)]` accessor still uses `PubType` (may panic on uninit, by design). --- .../src/class/data_models/field_var.rs | 114 +++++++++++------- itest/rust/src/register_tests/var_test.rs | 60 +++++++++ 2 files changed, 129 insertions(+), 45 deletions(-) diff --git a/godot-macros/src/class/data_models/field_var.rs b/godot-macros/src/class/data_models/field_var.rs index da94ac7b6..7ec1f823a 100644 --- a/godot-macros/src/class/data_models/field_var.rs +++ b/godot-macros/src/class/data_models/field_var.rs @@ -337,56 +337,21 @@ impl GetterSetterImpl { &rust_accessor, tool_button_fn, ); - } else if rust_public { - rust_accessor = godot_function_name.clone(); - doc_hidden = TokenStream::new(); - deprecated_function = TokenStream::new(); - - match kind { - GetSet::Get => { - signature = quote_spanned! { field_ty_span=> - fn #rust_accessor(&self) -> <#field_type as ::godot::register::property::Var>::PubType - }; - function_body = quote_spanned! { field_ty_span=> - <#field_type as ::godot::register::property::Var>::var_pub_get(&self.#field_name) - }; - } - GetSet::Set => { - signature = quote_spanned! { field_ty_span=> - fn #rust_accessor(&mut self, #field_name: <#field_type as ::godot::register::property::Var>::PubType) - }; - function_body = quote_spanned! { field_ty_span=> - <#field_type as ::godot::register::property::Var>::var_pub_set(&mut self.#field_name, #field_name) - }; - } - } } else { + // Godot accessor always uses `GodotConvert::Via` (not `PubType`): it satisfies the `GodotType` bound and is nullable for + // `OnEditor>` so hot-reload reads uninit fields as null rather than panicking. `make_named_accessor()` then adds + // an ergonomic `PubType` accessor for `#[var(pub)]`, or a deprecated `Via` forwarder for plain `#[var]`. let prefix = kind.prefix(); rust_accessor = format_ident!("__godot_{prefix}{field_name}", span = field_span); doc_hidden = quote! { #[doc(hidden)] }; - // Uses GodotConvert::Via as opposed to Var::PubType, because the latter has no ToGodot/FromGodot bound, while Via always - // has GodotType (and thus EngineToGodot). This is needed for the Signature machinery in method registration. - let deprecated_fn = match kind { - GetSet::Get => quote_spanned! { field_ty_span=> - pub fn #godot_function_name(&self) -> <#field_type as ::godot::meta::GodotConvert>::Via { - self.#rust_accessor() - } - }, - GetSet::Set => quote_spanned! { field_ty_span=> - pub fn #godot_function_name(&mut self, #field_name: <#field_type as ::godot::meta::GodotConvert>::Via) { - self.#rust_accessor(#field_name) - } - }, - }; - - deprecated_function = quote! { - // Deprecated forwarding function with the nice name. - #[deprecated = "Auto-generated Rust getters/setters for `#[var]` are being phased out until v0.6.\n\ - If you need them, opt in with #[var(pub)]."] - #[allow(dead_code)] // These functions are not used for registration; pub fns in private modules remain unused. - #deprecated_fn - }; + deprecated_function = Self::make_named_accessor( + kind, + rust_public, + field, + &godot_function_name, + &rust_accessor, + ); match kind { GetSet::Get => { @@ -513,6 +478,65 @@ impl GetterSetterImpl { (funcs_collection_constant, export_token) } + // Nice-named function emitted alongside the registered (`Via`) accessor. For `#[var(pub)]`, an ergonomic `PubType` accessor; + // otherwise a deprecated `Via` forwarder that delegates to the mangled `rust_accessor`. + fn make_named_accessor( + kind: GetSet, + rust_public: bool, + field: &Field, + godot_function_name: &Ident, + rust_accessor: &Ident, + ) -> TokenStream { + let Field { + name: field_name, + ty: field_type, + .. + } = field; + let field_ty_span = field_type.span(); + + if rust_public { + let pub_fn = match kind { + GetSet::Get => quote_spanned! { field_ty_span=> + pub fn #godot_function_name(&self) -> <#field_type as ::godot::register::property::Var>::PubType { + <#field_type as ::godot::register::property::Var>::var_pub_get(&self.#field_name) + } + }, + GetSet::Set => quote_spanned! { field_ty_span=> + pub fn #godot_function_name(&mut self, #field_name: <#field_type as ::godot::register::property::Var>::PubType) { + <#field_type as ::godot::register::property::Var>::var_pub_set(&mut self.#field_name, #field_name) + } + }, + }; + + quote! { + #[allow(dead_code)] // Public accessor may be unused within the defining crate (e.g. cdylib with no Rust callers). + #pub_fn + } + } else { + // Uses GodotConvert::Via as opposed to Var::PubType, because the latter has no ToGodot/FromGodot bound, while Via always + // has GodotType (and thus EngineToGodot). This is needed for the Signature machinery in method registration. + let deprecated_fn = match kind { + GetSet::Get => quote_spanned! { field_ty_span=> + pub fn #godot_function_name(&self) -> <#field_type as ::godot::meta::GodotConvert>::Via { + self.#rust_accessor() + } + }, + GetSet::Set => quote_spanned! { field_ty_span=> + pub fn #godot_function_name(&mut self, #field_name: <#field_type as ::godot::meta::GodotConvert>::Via) { + self.#rust_accessor(#field_name) + } + }, + }; + + quote! { + #[deprecated = "Auto-generated Rust getters/setters for `#[var]` are being phased out until v0.6.\n\ + If you need them, opt in with #[var(pub)]."] + #[allow(dead_code)] // These functions are not used for registration; pub fns in private modules remain unused. + #deprecated_fn + } + } + } + fn generate_tool_button_impl( field_name: &Ident, field_type: &TypeExpr, diff --git a/itest/rust/src/register_tests/var_test.rs b/itest/rust/src/register_tests/var_test.rs index 19e956cce..70af46695 100644 --- a/itest/rust/src/register_tests/var_test.rs +++ b/itest/rust/src/register_tests/var_test.rs @@ -335,6 +335,66 @@ fn var_pub_access_on_editor_builtin() { obj.free(); } +/// Runs `f` and returns whether it panicked *internally* (not reaching the call site). +/// +/// godot-rust catches panics at the FFI boundary, so a Godot-side accessor returns normally even when its body panicked, and the return value +/// alone can't reveal it. A temporary panic hook records the panic, as the hook runs before the `catch_unwind`. +fn detect_internal_panic(f: impl FnOnce()) -> bool { + use std::sync::Arc; + use std::sync::atomic::{AtomicBool, Ordering}; + + // Per-call flag (not a `static`), so concurrent tests (if implemented in the future) don't interfere. + let panicked = Arc::new(AtomicBool::new(false)); + let flag = Arc::clone(&panicked); + let prev_hook = std::panic::take_hook(); + std::panic::set_hook(Box::new(move |_| flag.store(true, Ordering::SeqCst))); + f(); + std::panic::set_hook(prev_hook); + panicked.load(Ordering::SeqCst) +} + +// The getter/setter registered with Godot (used during property serialization, e.g. hot reload) must not panic on an uninitialized +// `OnEditor>`, even though the Rust-facing `#[var(pub)]` accessors do -- they go through nullable `Var::Via`. Regression test for +// https://github.com/godot-rust/gdext/issues/1566. +#[itest] +fn var_pub_godot_get_uninit() { + let obj = VarPubEdgeCases::new_alloc(); + + let mut value = Variant::nil(); + let panicked = + detect_internal_panic(|| value = obj.upcast_ref::().get("on_editor_node")); + + assert!( + !panicked, + "Godot-side getter panicked on uninitialized OnEditor>" + ); + assert_eq!(value, Variant::nil()); + + obj.free(); +} + +#[itest] +fn var_pub_godot_set_null() { + let mut obj = VarPubEdgeCases::new_alloc(); + let node = Node::new_alloc(); + + // Godot writes null back to fields when restoring serialized state. The setter must accept it through nullable `Var::Via`; a `PubType` + // setter would reject the conversion (logged error, no panic), silently leaving the previous value in place. + obj.upcast_mut::() + .set("on_editor_node", &node.to_variant()); + obj.upcast_mut::() + .set("on_editor_node", &Variant::nil()); + + assert_eq!( + obj.upcast_ref::().get("on_editor_node"), + Variant::nil(), + "Godot-side setter failed to write null to OnEditor>" + ); + + node.free(); + obj.free(); +} + #[itest] fn var_pub_access_on_editor_gd() { let mut obj = VarPubEdgeCases::new_alloc();