From 9a5b2135edf915f86df1f857676d5b2067cd943b Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Fri, 2 Jun 2023 11:16:49 +0200 Subject: [PATCH 1/3] Gd: replace Deref/DerefMut with base() + base_mut(); rework InstanceStorage No more implicit access to base; avoids runtime-borrowing (Godot objects don't have Rust borrow limits) and makes access more explicit. InstanceStorage is now only accessed via shared-ref, which should fix some soundness issues with unbounded &mut. All fields are immutable or have interior mutability. --- .../dodge-the-creeps/rust/src/main_scene.rs | 21 ++-- examples/dodge-the-creeps/rust/src/player.rs | 2 - godot-core/src/obj/gd.rs | 83 ++++++++++--- godot-core/src/obj/traits.rs | 21 +++- godot-core/src/registry.rs | 17 ++- godot-core/src/storage.rs | 111 +++++++++--------- godot-macros/src/derive_godot_class/mod.rs | 26 ---- itest/rust/src/base_test.rs | 53 ++++++--- itest/rust/src/callable_test.rs | 7 +- itest/rust/src/codegen_test.rs | 6 +- itest/rust/src/native_structures_test.rs | 5 +- itest/rust/src/object_test.rs | 28 +---- itest/rust/src/signal_test.rs | 4 +- 13 files changed, 205 insertions(+), 179 deletions(-) diff --git a/examples/dodge-the-creeps/rust/src/main_scene.rs b/examples/dodge-the-creeps/rust/src/main_scene.rs index 7abbfed65..6e1c0f803 100644 --- a/examples/dodge-the-creeps/rust/src/main_scene.rs +++ b/examples/dodge-the-creeps/rust/src/main_scene.rs @@ -94,18 +94,19 @@ impl Main { self.base.add_child(mob_scene.share().upcast()); let mut mob = mob_scene.cast::(); - { - // Local scope to bind `mob` - let mut mob = mob.bind_mut(); - let range = rng.gen_range(mob.min_speed..mob.max_speed); - - mob.set_linear_velocity(Vector2::new(range, 0.0)); - let lin_vel = mob.get_linear_velocity().rotated(real::from_f32(direction)); - mob.set_linear_velocity(lin_vel); - } + let range = { + // Local scope to bind `mob` user object + let mob = mob.bind(); + rng.gen_range(mob.min_speed..mob.max_speed) + }; + + let mut mob = mob.base_mut(); + mob.set_linear_velocity(Vector2::new(range, 0.0)); + let lin_vel = mob.get_linear_velocity().rotated(real::from_f32(direction)); + mob.set_linear_velocity(lin_vel); let mut hud = self.base.get_node_as::("Hud"); - hud.bind_mut().connect( + hud.base_mut().connect( "start_game".into(), Callable::from_object_method(mob, "on_start_game"), ); diff --git a/examples/dodge-the-creeps/rust/src/player.rs b/examples/dodge-the-creeps/rust/src/player.rs index 1d1111b1e..8680bc287 100644 --- a/examples/dodge-the-creeps/rust/src/player.rs +++ b/examples/dodge-the-creeps/rust/src/player.rs @@ -58,8 +58,6 @@ impl Area2DVirtual for Player { } fn process(&mut self, delta: f64) { - println!("process"); - let mut animated_sprite = self .base .get_node_as::("AnimatedSprite2D"); diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index 325c918f9..2d6ccd14e 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -160,25 +160,68 @@ where GdMut::from_cell(self.storage().get_mut()) } - /// Storage object associated with the extension instance - // FIXME proper + safe interior mutability, also that Clippy is happy - #[allow(clippy::mut_from_ref)] - pub(crate) fn storage(&self) -> &mut InstanceStorage { - let callbacks = crate::storage::nop_instance_callbacks(); + /// Access base without locking the user object. + /// + /// If you need mutations, use [`base_mut()`][Self::base_mut]. If you need a value copy, use [`base().share()`][Share::share]. + pub fn base(&self) -> &Gd { + self.storage().base().deref() + } + + /// Access base mutably without locking the user object. + /// + /// The idea is to support the `gd.base_mut().mutating_method()` pattern, which is not allowed with `gd.base()`. + /// If you need a copy of a `Gd` object, use `base().share()`. + /// + /// The return type is currently `Gd` and not `&mut Gd` because of `&mut` aliasing restrictions. This may change in the future. + pub fn base_mut(&mut self) -> Gd { + // Note: we cannot give safe mutable access to a base without RefCell, because multiple Gd pointers could call base_mut(), + // leading to aliased &mut. And we don't want RefCell, as C++ objects (nodes etc.) don't underly Rust's exclusive-ref limitations. + // The whole point of the Gd::base*() methods are to not require runtime-exclusive access to the Rust object. + // + // This is not a problem when accessing the `#[base]` field of a user struct directly, because `self` is guarded by the + // RefCell/RwLock in the InstanceStorage. + // + // Here, we instead return a copy (for now), which for the user looks mostly the same. The idea is that: + // - gd.base().mutating_method() fails + // - gd.base_mut().mutating_method() works. + // + // Downside: small ref-counting overhead for `RefCounted` types. If this is an issue in a real game (highly unlikely), + // the user could always cache Gd base pointers. + self.storage().base().share() + } + /// Storage object associated with the extension instance. + pub(crate) fn storage(&self) -> &InstanceStorage { + // SAFETY: instance pointer belongs to this instance. We only get a shared reference, no exclusive access, so even + // calling this from multiple Gd pointers is safe. + // Potential issue is a concurrent free() in multi-threaded access; but that would need to be guarded against inside free(). unsafe { - let token = sys::get_library() as *mut std::ffi::c_void; - let binding = - interface_fn!(object_get_instance_binding)(self.obj_sys(), token, &callbacks); - - debug_assert!( - !binding.is_null(), - "Class {} -- null instance; does the class have a Godot creator function?", - std::any::type_name::() - ); - crate::private::as_storage::(binding as sys::GDExtensionClassInstancePtr) + let binding = self.resolve_instance_ptr(); + crate::private::as_storage::(binding) } } + + /// Storage object associated with the extension instance. + // pub(crate) fn storage_mut(&mut self) -> &mut InstanceStorage { + // // SAFETY: + // unsafe { + // let binding = self.resolve_instance_ptr(); + // crate::private::as_storage_mut::(binding) + // } + // } + + unsafe fn resolve_instance_ptr(&self) -> sys::GDExtensionClassInstancePtr { + let callbacks = crate::storage::nop_instance_callbacks(); + let token = sys::get_library() as *mut std::ffi::c_void; + let binding = interface_fn!(object_get_instance_binding)(self.obj_sys(), token, &callbacks); + + debug_assert!( + !binding.is_null(), + "Class {} -- null instance; does the class have a Godot creator function?", + std::any::type_name::() + ); + binding as sys::GDExtensionClassInstancePtr + } } /// _The methods in this impl block are available for any `T`._

@@ -506,12 +549,13 @@ where fn deref(&self) -> &Self::Target { // SAFETY: - // This relies on Gd having the layout as Node3D (as an example), + // + // This relies on `Gd` having the layout as `Node3D` (as an example), // which also needs #[repr(transparent)]: // // struct Gd { - // opaque: OpaqueObject, <- size of GDExtensionObjectPtr - // _marker: PhantomData, <- ZST + // opaque: OpaqueObject, // <- size of GDExtensionObjectPtr + // _marker: PhantomData, // <- ZST // } // struct Node3D { // object_ptr: sys::GDExtensionObjectPtr, @@ -527,13 +571,14 @@ where fn deref_mut(&mut self) -> &mut T { // SAFETY: see also Deref // - // The resulting &mut T is transmuted from &mut OpaqueObject, i.e. a *pointer* to the `opaque` field. + // The resulting `&mut T` is transmuted from `&mut OpaqueObject`, i.e. a *pointer* to the `opaque` field. // `opaque` itself has a different *address* for each Gd instance, meaning that two simultaneous // DerefMut borrows on two Gd instances will not alias, *even if* the underlying Godot object is the // same (i.e. `opaque` has the same value, but not address). unsafe { std::mem::transmute::<&mut OpaqueObject, &mut T>(&mut self.opaque) } } } + // SAFETY: // - `move_return_ptr` // When the `call_type` is `PtrcallType::Virtual`, and the current type is known to inherit from `RefCounted` diff --git a/godot-core/src/obj/traits.rs b/godot-core/src/obj/traits.rs index cd0556066..89e28445f 100644 --- a/godot-core/src/obj/traits.rs +++ b/godot-core/src/obj/traits.rs @@ -122,17 +122,18 @@ pub trait Inherits: GodotClass {} impl Inherits for T {} -/// Trait implemented for all objects that inherit from `Resource` or `Node`. As those are the only objects -/// you can export to the editor. +/// Trait implemented for all objects that inherit from `Resource` or `Node`. +/// +/// Those are the only objects you can export to the editor. pub trait ExportableObject: GodotClass {} -/// Auto-implemented for all engine-provided classes +/// Auto-implemented for all engine-provided classes. pub trait EngineClass: GodotClass { fn as_object_ptr(&self) -> sys::GDExtensionObjectPtr; fn as_type_ptr(&self) -> sys::GDExtensionTypePtr; } -/// Auto-implemented for all engine-provided enums +/// Auto-implemented for all engine-provided enums. pub trait EngineEnum: Copy { fn try_from_ord(ord: i32) -> Option; @@ -176,6 +177,7 @@ pub trait IndexEnum: EngineEnum { /// Capability traits, providing dedicated functionalities for Godot classes pub mod cap { use super::*; + use crate::obj::Gd; /// Trait for all classes that are constructible from the Godot engine. /// @@ -192,6 +194,17 @@ pub mod cap { fn __godot_init(base: Base) -> Self; } + /// Trait that's implemented for user-defined classes that provide a `#[base]` field. + /// + /// Gives direct access to the base pointer without going through upcast FFI. + pub trait WithBaseField: GodotClass { + #[doc(hidden)] + fn __godot_base(&self) -> &Gd; + + #[doc(hidden)] + fn __godot_base_mut(&mut self) -> &mut Gd; + } + // TODO Evaluate whether we want this public or not #[doc(hidden)] pub trait GodotToString: GodotClass { diff --git a/godot-core/src/registry.rs b/godot-core/src/registry.rs index 6959773e2..fc3d8ca82 100644 --- a/godot-core/src/registry.rs +++ b/godot-core/src/registry.rs @@ -307,10 +307,14 @@ pub mod callbacks { let base_ptr = unsafe { interface_fn!(classdb_construct_object)(base_class_name.string_sys()) }; - let base = unsafe { Base::from_sys(base_ptr) }; + let base = unsafe { Base::from_sys(base_ptr) }; let user_instance = make_user_instance(base); - let instance = InstanceStorage::::construct(user_instance); + + // Create 2nd base to cache inside storage. This one will remain weak forever (no action on destruction). + let cached_base = unsafe { Base::from_sys(base_ptr) }; + + let instance = InstanceStorage::::construct(user_instance, cached_base); let instance_ptr = instance.into_raw(); let instance_ptr = instance_ptr as sys::GDExtensionClassInstancePtr; @@ -334,9 +338,12 @@ pub mod callbacks { _class_user_data: *mut std::ffi::c_void, instance: sys::GDExtensionClassInstancePtr, ) { - let storage = as_storage::(instance); - storage.mark_destroyed_by_godot(); - let _drop = Box::from_raw(storage as *mut InstanceStorage<_>); + { + let storage = as_storage::(instance); + storage.mark_destroyed_by_godot(); + } // Ref no longer valid once next statement is executed. + + crate::storage::destroy_storage::(instance); } pub unsafe extern "C" fn get_virtual( diff --git a/godot-core/src/storage.rs b/godot-core/src/storage.rs index 2eac95187..b34de73ae 100644 --- a/godot-core/src/storage.rs +++ b/godot-core/src/storage.rs @@ -28,7 +28,7 @@ mod single_thread { use std::any::type_name; use std::cell; - use crate::obj::GodotClass; + use crate::obj::{Base, GodotClass}; use crate::out; use super::Lifecycle; @@ -36,57 +36,50 @@ mod single_thread { /// Manages storage and lifecycle of user's extension class instances. pub struct InstanceStorage { user_instance: cell::RefCell, + cached_base: Base, // Declared after `user_instance`, is dropped last - pub lifecycle: Lifecycle, - godot_ref_count: u32, + pub lifecycle: cell::Cell, + godot_ref_count: cell::Cell, } /// For all Godot extension classes impl InstanceStorage { - pub fn construct(user_instance: T) -> Self { + pub fn construct(user_instance: T, cached_base: Base) -> Self { out!(" Storage::construct <{}>", type_name::()); Self { user_instance: cell::RefCell::new(user_instance), - lifecycle: Lifecycle::Alive, - godot_ref_count: 1, + cached_base, + lifecycle: cell::Cell::new(Lifecycle::Alive), + godot_ref_count: cell::Cell::new(1), } } - pub(crate) fn on_inc_ref(&mut self) { - self.godot_ref_count += 1; + pub(crate) fn on_inc_ref(&self) { + let refc = self.godot_ref_count.get() + 1; + self.godot_ref_count.set(refc); + out!( " Storage::on_inc_ref (rc={}) <{}>", // -- {:?}", - self.godot_ref_count(), + refc, type_name::(), //self.user_instance ); } - pub(crate) fn on_dec_ref(&mut self) { - self.godot_ref_count -= 1; + pub(crate) fn on_dec_ref(&self) { + let refc = self.godot_ref_count.get() - 1; + self.godot_ref_count.set(refc); + out!( " | Storage::on_dec_ref (rc={}) <{}>", // -- {:?}", - self.godot_ref_count(), + refc, type_name::(), //self.user_instance ); } - /* pub fn destroy(&mut self) { - assert!( - self.user_instance.is_some(), - "Cannot destroy user instance which is not yet initialized" - ); - assert!( - !self.destroyed, - "Cannot destroy user instance multiple times" - ); - self.user_instance = None; // drops T - // TODO drop entire Storage - }*/ - pub fn get(&self) -> cell::Ref { self.user_instance.try_borrow().unwrap_or_else(|_e| { panic!( @@ -98,7 +91,7 @@ mod single_thread { }) } - pub fn get_mut(&mut self) -> cell::RefMut { + pub fn get_mut(&self) -> cell::RefMut { self.user_instance.try_borrow_mut().unwrap_or_else(|_e| { panic!( "Gd::bind_mut() failed, already bound; T = {}.\n \ @@ -109,8 +102,12 @@ mod single_thread { }) } + pub fn base(&self) -> &Base { + &self.cached_base + } + pub(super) fn godot_ref_count(&self) -> u32 { - self.godot_ref_count + self.godot_ref_count.get() } } } @@ -118,31 +115,33 @@ mod single_thread { #[cfg(feature = "threads")] mod multi_thread { use std::any::type_name; - use std::sync; use std::sync::atomic::{AtomicU32, Ordering}; + use std::{cell, sync}; - use crate::obj::GodotClass; - use crate::out; + use crate::obj::{Base, GodotClass}; + use crate::{out, sys}; use super::Lifecycle; /// Manages storage and lifecycle of user's extension class instances. pub struct InstanceStorage { user_instance: sync::RwLock, + cached_base: Base, // Declared after `user_instance`, is dropped last - pub lifecycle: Lifecycle, + pub lifecycle: cell::Cell, godot_ref_count: AtomicU32, } /// For all Godot extension classes impl InstanceStorage { - pub fn construct(user_instance: T) -> Self { + pub fn construct(user_instance: T, cached_base: Base) -> Self { out!(" Storage::construct <{}>", type_name::()); Self { user_instance: sync::RwLock::new(user_instance), - lifecycle: Lifecycle::Alive, + cached_base, + lifecycle: cell::Cell::new(Lifecycle::Alive), godot_ref_count: AtomicU32::new(1), } } @@ -167,19 +166,6 @@ mod multi_thread { ); } - /* pub fn destroy(&mut self) { - assert!( - self.user_instance.is_some(), - "Cannot destroy user instance which is not yet initialized" - ); - assert!( - !self.destroyed, - "Cannot destroy user instance multiple times" - ); - self.user_instance = None; // drops T - // TODO drop entire Storage - }*/ - pub fn get(&self) -> sync::RwLockReadGuard { self.user_instance.read().unwrap_or_else(|_e| { panic!( @@ -191,7 +177,7 @@ mod multi_thread { }) } - pub fn get_mut(&mut self) -> sync::RwLockWriteGuard { + pub fn get_mut(&self) -> sync::RwLockWriteGuard { self.user_instance.write().unwrap_or_else(|_e| { panic!( "Gd::bind_mut() failed, already bound; T = {}.\n \ @@ -202,6 +188,10 @@ mod multi_thread { }) } + pub fn base(&self) -> &Base { + &self.cached_base + } + pub(super) fn godot_ref_count(&self) -> u32 { self.godot_ref_count.load(Ordering::Relaxed) } @@ -214,16 +204,16 @@ impl InstanceStorage { Box::into_raw(Box::new(self)) } - pub fn mark_destroyed_by_godot(&mut self) { + pub fn mark_destroyed_by_godot(&self) { out!( " Storage::mark_destroyed_by_godot", // -- {:?}", //self.user_instance ); - self.lifecycle = Lifecycle::Destroying; + self.lifecycle.set(Lifecycle::Destroying); out!( " mark; self={:?}, val={:?}", - self as *mut _, - self.lifecycle + self as *const _, + self.lifecycle.get() ); } @@ -232,9 +222,12 @@ impl InstanceStorage { out!( " is_d; self={:?}, val={:?}", self as *const _, - self.lifecycle + self.lifecycle.get() ); - matches!(self.lifecycle, Lifecycle::Destroying | Lifecycle::Dead) + matches!( + self.lifecycle.get(), + Lifecycle::Destroying | Lifecycle::Dead + ) } } @@ -261,11 +254,17 @@ impl Drop for InstanceStorage { /// /// # Safety /// `instance_ptr` is assumed to point to a valid instance. -// FIXME unbounded ref AND &mut out of thin air is a huge hazard -- consider using with_storage(ptr, closure) and drop_storage(ptr) +// Note: unbounded ref AND &mut out of thin air is not very beautiful, but it's -- consider using with_storage(ptr, closure) and drop_storage(ptr) pub unsafe fn as_storage<'u, T: GodotClass>( instance_ptr: sys::GDExtensionClassInstancePtr, -) -> &'u mut InstanceStorage { - &mut *(instance_ptr as *mut InstanceStorage) +) -> &'u InstanceStorage { + &*(instance_ptr as *mut InstanceStorage) +} + +/// # Safety +/// `instance_ptr` is assumed to point to a valid instance. This function must only be invoked once for a pointer. +pub unsafe fn destroy_storage(instance_ptr: sys::GDExtensionClassInstancePtr) { + let _drop = Box::from_raw(instance_ptr as *mut InstanceStorage); } pub fn nop_instance_callbacks() -> sys::GDExtensionInstanceBindingCallbacks { diff --git a/godot-macros/src/derive_godot_class/mod.rs b/godot-macros/src/derive_godot_class/mod.rs index 7807a4f13..78bddff6b 100644 --- a/godot-macros/src/derive_godot_class/mod.rs +++ b/godot-macros/src/derive_godot_class/mod.rs @@ -35,8 +35,6 @@ pub fn transform(decl: Declaration) -> ParseResult { let inherits_macro = format_ident!("inherits_transitive_{}", base_ty); let prv = quote! { ::godot::private }; - let deref_impl = make_deref_impl(class_name, &fields); - let godot_exports_impl = make_property_impl(class_name, &fields); let (godot_init_impl, create_fn); @@ -61,7 +59,6 @@ pub fn transform(decl: Declaration) -> ParseResult { #godot_init_impl #godot_exports_impl - #deref_impl ::godot::sys::plugin_add!(__GODOT_PLUGIN_REGISTRY in #prv; #prv::ClassPlugin { class_name: #class_name_obj, @@ -231,29 +228,6 @@ fn make_godot_init_impl(class_name: &Ident, fields: Fields) -> TokenStream { } } -fn make_deref_impl(class_name: &Ident, fields: &Fields) -> TokenStream { - let base_field = if let Some(Field { name, .. }) = &fields.base_field { - name - } else { - return TokenStream::new(); - }; - - quote! { - impl std::ops::Deref for #class_name { - type Target = ::Base; - - fn deref(&self) -> &Self::Target { - &*self.#base_field - } - } - impl std::ops::DerefMut for #class_name { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut *self.#base_field - } - } - } -} - /// Checks at compile time that a function with the given name exists on `Self`. #[must_use] fn make_existence_check(ident: &Ident) -> TokenStream { diff --git a/itest/rust/src/base_test.rs b/itest/rust/src/base_test.rs index fce5f1b79..52e644458 100644 --- a/itest/rust/src/base_test.rs +++ b/itest/rust/src/base_test.rs @@ -15,7 +15,7 @@ fn base_test_is_weak() { #[itest] fn base_instance_id() { - let obj = Gd::::new_default(); + let obj = Gd::::new_default(); let obj_id = obj.instance_id(); let base_id = obj.bind().base.instance_id(); @@ -24,15 +24,29 @@ fn base_instance_id() { } #[itest] -fn base_deref() { - let mut obj = Gd::::new_default(); +fn base_access_unbound() { + let mut obj = Gd::::new_default(); { - let mut guard = obj.bind_mut(); let pos = Vector2::new(-5.5, 7.0); - guard.set_position(pos); // GdMut as DerefMut + obj.base_mut().set_position(pos); - assert_eq!(guard.base.get_position(), pos); + assert_eq!(obj.base().get_position(), pos); + } + + obj.free(); +} + +// Tests whether access to base is possible from outside the Gd, even if there is no `#[base]` field. +#[itest] +fn base_access_unbound_no_field() { + let mut obj = Gd::::new_default(); + + { + let pos = Vector2::new(-5.5, 7.0); + obj.base_mut().set_position(pos); + + assert_eq!(obj.base().get_position(), pos); } obj.free(); @@ -40,14 +54,14 @@ fn base_deref() { #[itest] fn base_display() { - let obj = Gd::::new_default(); + let obj = Gd::::new_default(); { let guard = obj.bind(); let id = guard.base.instance_id(); - // We expect the dynamic type to be part of Godot's to_string(), so BaseHolder and not Node2D + // We expect the dynamic type to be part of Godot's to_string(), so Based and not Node2D let actual = format!(".:{}:.", guard.base); - let expected = format!(".::."); + let expected = format!(".::."); assert_eq!(actual, expected); } @@ -56,14 +70,14 @@ fn base_display() { #[itest] fn base_debug() { - let obj = Gd::::new_default(); + let obj = Gd::::new_default(); { let guard = obj.bind(); let id = guard.base.instance_id(); - // We expect the dynamic type to be part of Godot's to_string(), so BaseHolder and not Node2D + // We expect the dynamic type to be part of Godot's to_string(), so Based and not Node2D let actual = format!(".:{:?}:.", guard.base); - let expected = format!(".:Base {{ id: {id}, class: BaseHolder }}:."); + let expected = format!(".:Base {{ id: {id}, class: Based }}:."); assert_eq!(actual, expected); } @@ -72,23 +86,30 @@ fn base_debug() { #[itest] fn base_with_init() { - let obj = Gd::::with_base(|mut base| { + let obj = Gd::::with_base(|mut base| { base.set_rotation(11.0); - BaseHolder { base, i: 732 } + Based { base, i: 732 } }); { let guard = obj.bind(); assert_eq!(guard.i, 732); - assert_eq!(guard.get_rotation(), 11.0); + assert_eq!(guard.base.get_rotation(), 11.0); } obj.free(); } #[derive(GodotClass)] #[class(init, base=Node2D)] -struct BaseHolder { +struct Based { #[base] base: Base, + i: i32, } + +#[derive(GodotClass)] +#[class(init, base=Node2D)] +struct Baseless { + // No need for fields, we just test if we can access this as Gd. +} diff --git a/itest/rust/src/callable_test.rs b/itest/rust/src/callable_test.rs index 7086bac3e..e3a612076 100644 --- a/itest/rust/src/callable_test.rs +++ b/itest/rust/src/callable_test.rs @@ -6,17 +6,14 @@ use godot::bind::{godot_api, GodotClass}; use godot::builtin::{varray, Callable, ToVariant, Variant}; -use godot::engine::{Object, RefCounted}; -use godot::obj::{Base, Gd, Share}; +use godot::engine::Object; +use godot::obj::{Gd, Share}; use godot::prelude::GodotString; use godot::test::itest; #[derive(GodotClass)] #[class(init, base=RefCounted)] struct CallableTestObj { - #[base] - base: Base, - value: i32, } diff --git a/itest/rust/src/codegen_test.rs b/itest/rust/src/codegen_test.rs index 812511cd8..ca9c1175c 100644 --- a/itest/rust/src/codegen_test.rs +++ b/itest/rust/src/codegen_test.rs @@ -24,7 +24,7 @@ fn codegen_base_renamed() { // The registration is done at startup time, so it may already fail during GDExtension init. // Nevertheless, try to instantiate an object with base HttpRequest here. - let obj = Gd::with_base(|base| TestBaseRenamed { base }); + let obj = Gd::with_base(|base| TestBaseRenamed { _base: base }); let _id = obj.instance_id(); obj.free(); @@ -62,7 +62,7 @@ fn codegen_constants() { #[class(base=HttpRequest)] pub struct TestBaseRenamed { #[base] - base: Base, + _base: Base, } #[allow(unused)] @@ -81,7 +81,7 @@ impl TestBaseRenamed { #[godot_api] impl HttpRequestVirtual for TestBaseRenamed { fn init(base: Base) -> Self { - TestBaseRenamed { base } + TestBaseRenamed { _base: base } } // Test unnamed parameter in virtual function diff --git a/itest/rust/src/native_structures_test.rs b/itest/rust/src/native_structures_test.rs index 0cf389519..ec2d12be9 100644 --- a/itest/rust/src/native_structures_test.rs +++ b/itest/rust/src/native_structures_test.rs @@ -15,8 +15,6 @@ use std::cell::Cell; #[derive(GodotClass)] #[class(base=TextServerExtension)] pub struct TestTextServer { - #[base] - base: Base, glyphs: [Glyph; 2], cell: Cell>, } @@ -40,9 +38,8 @@ fn sample_glyph(start: i32) -> Glyph { #[godot_api] impl TextServerExtensionVirtual for TestTextServer { - fn init(base: Base) -> Self { + fn init(_base: Base) -> Self { TestTextServer { - base, glyphs: [sample_glyph(99), sample_glyph(700)], cell: Cell::new(None), } diff --git a/itest/rust/src/object_test.rs b/itest/rust/src/object_test.rs index c9d9349d2..c10cccba0 100644 --- a/itest/rust/src/object_test.rs +++ b/itest/rust/src/object_test.rs @@ -5,7 +5,6 @@ */ use std::cell::{Cell, RefCell}; -use std::mem; use std::rc::Rc; use godot::bind::{godot_api, GodotClass}; @@ -56,7 +55,7 @@ fn object_subtype_swap() { println!("..swap.."); */ - mem::swap(&mut *a, &mut *b); + std::mem::swap(&mut *a, &mut *b); /* dbg!(a_id); @@ -792,9 +791,6 @@ fn custom_constructor_works() { #[derive(GodotClass)] #[class(init, base=Object)] struct DoubleUse { - #[base] - base: Base, - used: Cell, } @@ -850,25 +846,3 @@ fn double_use_reference() { double_use.free(); emitter.free(); } - -#[derive(GodotClass)] -#[class(init, base=Object)] -struct GodotApiTest { - #[base] - base: Base, -} - -#[godot_api] -impl GodotApiTest { - #[func] - fn func_only_mut(&mut self, mut _a: Gd, mut _b: Gd) {} - #[func] - fn func_mut_and_not_mut(&mut self, _a: Gd, mut _b: Gd, _c: Gd) {} - // #[func] - // fn func_optional(&mut self, _a: Gd, mut _b: Gd, #[opt(987)] _c: i32) {} - // #[func] // waiting https://github.com/godotengine/godot/pull/75415 - // /// Godot Docs - // fn func_docs(&mut self, _a: Gd, mut _b: Gd) {} - // #[func] - // fn func_lifetime<'a>(&'a mut self) {} -} diff --git a/itest/rust/src/signal_test.rs b/itest/rust/src/signal_test.rs index 36e6c5c5d..c0f834c6d 100644 --- a/itest/rust/src/signal_test.rs +++ b/itest/rust/src/signal_test.rs @@ -82,10 +82,10 @@ fn signals() { let receiver_name = format!("receive_{i}_arg"); emitter - .bind_mut() + .base_mut() .connect(signal_name.clone().into(), receiver.callable(receiver_name)); - emitter.bind_mut().emit_signal(signal_name.into(), arg); + emitter.base_mut().emit_signal(signal_name.into(), arg); assert!(receiver.bind().used[i].get()); } From c7561e72277cbaba1fbe274eae3dc145142a22f1 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Wed, 2 Aug 2023 23:52:54 +0200 Subject: [PATCH 2/3] Enable godot-itest CI for draft PRs --- .github/workflows/minimal-ci.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/minimal-ci.yml b/.github/workflows/minimal-ci.yml index 1e97560e1..712781a19 100644 --- a/.github/workflows/minimal-ci.yml +++ b/.github/workflows/minimal-ci.yml @@ -90,7 +90,6 @@ jobs: godot-itest: name: godot-itest (${{ matrix.name }}) runs-on: ${{ matrix.os }} - if: github.event.pull_request.draft != true continue-on-error: false timeout-minutes: 15 strategy: From 4dd712c4c2f78e1d138b6b7c32c6db5af9c5ea5c Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Wed, 2 Aug 2023 23:56:51 +0200 Subject: [PATCH 3/3] Allow Deref/DerefMut for all Gd; remove Gd::base/base_mut() again More uniform and should be safe. --- .../dodge-the-creeps/rust/src/main_scene.rs | 3 +- godot-core/src/obj/gd.rs | 55 +++--------- godot-core/src/obj/traits.rs | 6 ++ godot-core/src/registry.rs | 5 +- godot-core/src/storage.rs | 84 +++++++++++++------ itest/rust/src/base_test.rs | 18 ++-- itest/rust/src/object_test.rs | 10 +-- itest/rust/src/property_test.rs | 14 ++-- itest/rust/src/signal_test.rs | 12 +-- 9 files changed, 98 insertions(+), 109 deletions(-) diff --git a/examples/dodge-the-creeps/rust/src/main_scene.rs b/examples/dodge-the-creeps/rust/src/main_scene.rs index 6e1c0f803..901cb3790 100644 --- a/examples/dodge-the-creeps/rust/src/main_scene.rs +++ b/examples/dodge-the-creeps/rust/src/main_scene.rs @@ -100,13 +100,12 @@ impl Main { rng.gen_range(mob.min_speed..mob.max_speed) }; - let mut mob = mob.base_mut(); mob.set_linear_velocity(Vector2::new(range, 0.0)); let lin_vel = mob.get_linear_velocity().rotated(real::from_f32(direction)); mob.set_linear_velocity(lin_vel); let mut hud = self.base.get_node_as::("Hud"); - hud.base_mut().connect( + hud.connect( "start_game".into(), Callable::from_object_method(mob, "on_start_game"), ); diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index 2d6ccd14e..48cc6c04b 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -160,36 +160,6 @@ where GdMut::from_cell(self.storage().get_mut()) } - /// Access base without locking the user object. - /// - /// If you need mutations, use [`base_mut()`][Self::base_mut]. If you need a value copy, use [`base().share()`][Share::share]. - pub fn base(&self) -> &Gd { - self.storage().base().deref() - } - - /// Access base mutably without locking the user object. - /// - /// The idea is to support the `gd.base_mut().mutating_method()` pattern, which is not allowed with `gd.base()`. - /// If you need a copy of a `Gd` object, use `base().share()`. - /// - /// The return type is currently `Gd` and not `&mut Gd` because of `&mut` aliasing restrictions. This may change in the future. - pub fn base_mut(&mut self) -> Gd { - // Note: we cannot give safe mutable access to a base without RefCell, because multiple Gd pointers could call base_mut(), - // leading to aliased &mut. And we don't want RefCell, as C++ objects (nodes etc.) don't underly Rust's exclusive-ref limitations. - // The whole point of the Gd::base*() methods are to not require runtime-exclusive access to the Rust object. - // - // This is not a problem when accessing the `#[base]` field of a user struct directly, because `self` is guarded by the - // RefCell/RwLock in the InstanceStorage. - // - // Here, we instead return a copy (for now), which for the user looks mostly the same. The idea is that: - // - gd.base().mutating_method() fails - // - gd.base_mut().mutating_method() works. - // - // Downside: small ref-counting overhead for `RefCounted` types. If this is an issue in a real game (highly unlikely), - // the user could always cache Gd base pointers. - self.storage().base().share() - } - /// Storage object associated with the extension instance. pub(crate) fn storage(&self) -> &InstanceStorage { // SAFETY: instance pointer belongs to this instance. We only get a shared reference, no exclusive access, so even @@ -541,16 +511,16 @@ where } } -impl Deref for Gd -where - T: GodotClass, -{ - type Target = T; +impl Deref for Gd { + // Target is always an engine class: + // * if T is an engine class => T + // * if T is a user class => T::Base + type Target = <::Declarer as dom::Domain>::DerefTarget; fn deref(&self) -> &Self::Target { // SAFETY: // - // This relies on `Gd` having the layout as `Node3D` (as an example), + // This relies on `Gd.opaque` having the layout as `Node3D` (as an example), // which also needs #[repr(transparent)]: // // struct Gd { @@ -560,22 +530,21 @@ where // struct Node3D { // object_ptr: sys::GDExtensionObjectPtr, // } - unsafe { std::mem::transmute::<&OpaqueObject, &T>(&self.opaque) } + unsafe { std::mem::transmute::<&OpaqueObject, &Self::Target>(&self.opaque) } } } -impl DerefMut for Gd -where - T: GodotClass, -{ - fn deref_mut(&mut self) -> &mut T { +impl DerefMut for Gd { + fn deref_mut(&mut self) -> &mut Self::Target { // SAFETY: see also Deref // // The resulting `&mut T` is transmuted from `&mut OpaqueObject`, i.e. a *pointer* to the `opaque` field. // `opaque` itself has a different *address* for each Gd instance, meaning that two simultaneous // DerefMut borrows on two Gd instances will not alias, *even if* the underlying Godot object is the // same (i.e. `opaque` has the same value, but not address). - unsafe { std::mem::transmute::<&mut OpaqueObject, &mut T>(&mut self.opaque) } + // + // The `&mut self` guarantees that no other base access can take place for *the same Gd instance* (access to other Gds is OK). + unsafe { std::mem::transmute::<&mut OpaqueObject, &mut Self::Target>(&mut self.opaque) } } } diff --git a/godot-core/src/obj/traits.rs b/godot-core/src/obj/traits.rs index 89e28445f..cd16b73c9 100644 --- a/godot-core/src/obj/traits.rs +++ b/godot-core/src/obj/traits.rs @@ -260,6 +260,8 @@ pub mod dom { /// Trait that specifies who declares a given `GodotClass`. pub trait Domain: Sealed { + type DerefTarget; + #[doc(hidden)] fn scoped_mut(obj: &mut Gd, closure: F) -> R where @@ -271,6 +273,8 @@ pub mod dom { pub enum EngineDomain {} impl Sealed for EngineDomain {} impl Domain for EngineDomain { + type DerefTarget = T; + fn scoped_mut(obj: &mut Gd, closure: F) -> R where T: GodotClass, @@ -284,6 +288,8 @@ pub mod dom { pub enum UserDomain {} impl Sealed for UserDomain {} impl Domain for UserDomain { + type DerefTarget = T::Base; + fn scoped_mut(obj: &mut Gd, closure: F) -> R where T: GodotClass, diff --git a/godot-core/src/registry.rs b/godot-core/src/registry.rs index fc3d8ca82..0ae362e10 100644 --- a/godot-core/src/registry.rs +++ b/godot-core/src/registry.rs @@ -311,10 +311,7 @@ pub mod callbacks { let base = unsafe { Base::from_sys(base_ptr) }; let user_instance = make_user_instance(base); - // Create 2nd base to cache inside storage. This one will remain weak forever (no action on destruction). - let cached_base = unsafe { Base::from_sys(base_ptr) }; - - let instance = InstanceStorage::::construct(user_instance, cached_base); + let instance = InstanceStorage::::construct(user_instance); let instance_ptr = instance.into_raw(); let instance_ptr = instance_ptr as sys::GDExtensionClassInstancePtr; diff --git a/godot-core/src/storage.rs b/godot-core/src/storage.rs index b34de73ae..0baf5e278 100644 --- a/godot-core/src/storage.rs +++ b/godot-core/src/storage.rs @@ -12,23 +12,24 @@ use std::any::type_name; #[derive(Copy, Clone, Debug)] pub enum Lifecycle { + // Warning: when reordering/changing enumerators, update match in AtomicLifecycle below Alive, Destroying, Dead, // reading this would typically already be too late, only best-effort in case of UB } #[cfg(not(feature = "threads"))] -pub use single_thread::*; +pub(crate) use single_threaded::*; #[cfg(feature = "threads")] -pub use multi_thread::*; +pub(crate) use multi_threaded::*; #[cfg(not(feature = "threads"))] -mod single_thread { +mod single_threaded { use std::any::type_name; use std::cell; - use crate::obj::{Base, GodotClass}; + use crate::obj::GodotClass; use crate::out; use super::Lifecycle; @@ -36,7 +37,6 @@ mod single_thread { /// Manages storage and lifecycle of user's extension class instances. pub struct InstanceStorage { user_instance: cell::RefCell, - cached_base: Base, // Declared after `user_instance`, is dropped last pub lifecycle: cell::Cell, @@ -45,12 +45,11 @@ mod single_thread { /// For all Godot extension classes impl InstanceStorage { - pub fn construct(user_instance: T, cached_base: Base) -> Self { + pub fn construct(user_instance: T) -> Self { out!(" Storage::construct <{}>", type_name::()); Self { user_instance: cell::RefCell::new(user_instance), - cached_base, lifecycle: cell::Cell::new(Lifecycle::Alive), godot_ref_count: cell::Cell::new(1), } @@ -102,10 +101,6 @@ mod single_thread { }) } - pub fn base(&self) -> &Base { - &self.cached_base - } - pub(super) fn godot_ref_count(&self) -> u32 { self.godot_ref_count.get() } @@ -113,40 +108,63 @@ mod single_thread { } #[cfg(feature = "threads")] -mod multi_thread { +mod multi_threaded { use std::any::type_name; + use std::sync; use std::sync::atomic::{AtomicU32, Ordering}; - use std::{cell, sync}; - use crate::obj::{Base, GodotClass}; - use crate::{out, sys}; + use crate::obj::GodotClass; + use crate::out; use super::Lifecycle; + pub struct AtomicLifecycle { + atomic: AtomicU32, + } + + impl AtomicLifecycle { + pub fn new(value: Lifecycle) -> Self { + Self { + atomic: AtomicU32::new(value as u32), + } + } + + pub fn get(&self) -> Lifecycle { + match self.atomic.load(Ordering::Relaxed) { + 0 => Lifecycle::Alive, + 1 => Lifecycle::Dead, + 2 => Lifecycle::Destroying, + other => panic!("Invalid lifecycle {other}"), + } + } + + pub fn set(&self, value: Lifecycle) { + self.atomic.store(value as u32, Ordering::Relaxed); + } + } + /// Manages storage and lifecycle of user's extension class instances. pub struct InstanceStorage { user_instance: sync::RwLock, - cached_base: Base, // Declared after `user_instance`, is dropped last - pub lifecycle: cell::Cell, + pub lifecycle: AtomicLifecycle, godot_ref_count: AtomicU32, } /// For all Godot extension classes impl InstanceStorage { - pub fn construct(user_instance: T, cached_base: Base) -> Self { + pub fn construct(user_instance: T) -> Self { out!(" Storage::construct <{}>", type_name::()); Self { user_instance: sync::RwLock::new(user_instance), - cached_base, - lifecycle: cell::Cell::new(Lifecycle::Alive), + lifecycle: AtomicLifecycle::new(Lifecycle::Alive), godot_ref_count: AtomicU32::new(1), } } - pub(crate) fn on_inc_ref(&mut self) { + pub(crate) fn on_inc_ref(&self) { self.godot_ref_count.fetch_add(1, Ordering::Relaxed); out!( " Storage::on_inc_ref (rc={}) <{}>", // -- {:?}", @@ -156,7 +174,7 @@ mod multi_thread { ); } - pub(crate) fn on_dec_ref(&mut self) { + pub(crate) fn on_dec_ref(&self) { self.godot_ref_count.fetch_sub(1, Ordering::Relaxed); out!( " | Storage::on_dec_ref (rc={}) <{}>", // -- {:?}", @@ -188,14 +206,25 @@ mod multi_thread { }) } - pub fn base(&self) -> &Base { - &self.cached_base - } - pub(super) fn godot_ref_count(&self) -> u32 { self.godot_ref_count.load(Ordering::Relaxed) } + + // fn __static_type_check() { + // enforce_sync::>(); + // } } + + // TODO make InstanceStorage Sync + // This type can be accessed concurrently from multiple threads, so it should be Sync. That implies however that T must be Sync too + // (and possibly Send, because with `&mut` access, a `T` can be extracted as a value using mem::take() etc.). + // Which again means that we need to infest half the codebase with T: Sync + Send bounds, *and* make it all conditional on + // `#[cfg(feature = "threads")]`. Until the multi-threading design is clarified, we'll thus leave it as is. + // + // The following code + __static_type_check() above would make sure that InstanceStorage is Sync. + + // Make sure storage is Sync in multi-threaded case, as it can be concurrently accessed through aliased Gd pointers. + // fn enforce_sync() {} } impl InstanceStorage { @@ -267,6 +296,9 @@ pub unsafe fn destroy_storage(instance_ptr: sys::GDExtensionClass let _drop = Box::from_raw(instance_ptr as *mut InstanceStorage); } +// ---------------------------------------------------------------------------------------------------------------------------------------------- +// Callbacks + pub fn nop_instance_callbacks() -> sys::GDExtensionInstanceBindingCallbacks { // These could also be null pointers, if they are definitely not invoked (e.g. create_callback only passed to object_get_instance_binding(), // when there is already a binding). Current "empty but not null" impl corresponds to godot-cpp (wrapped.hpp). diff --git a/itest/rust/src/base_test.rs b/itest/rust/src/base_test.rs index 52e644458..494ae475b 100644 --- a/itest/rust/src/base_test.rs +++ b/itest/rust/src/base_test.rs @@ -27,12 +27,9 @@ fn base_instance_id() { fn base_access_unbound() { let mut obj = Gd::::new_default(); - { - let pos = Vector2::new(-5.5, 7.0); - obj.base_mut().set_position(pos); - - assert_eq!(obj.base().get_position(), pos); - } + let pos = Vector2::new(-5.5, 7.0); + obj.set_position(pos); + assert_eq!(obj.get_position(), pos); obj.free(); } @@ -42,12 +39,9 @@ fn base_access_unbound() { fn base_access_unbound_no_field() { let mut obj = Gd::::new_default(); - { - let pos = Vector2::new(-5.5, 7.0); - obj.base_mut().set_position(pos); - - assert_eq!(obj.base().get_position(), pos); - } + let pos = Vector2::new(-5.5, 7.0); + obj.set_position(pos); + assert_eq!(obj.get_position(), pos); obj.free(); } diff --git a/itest/rust/src/object_test.rs b/itest/rust/src/object_test.rs index c10cccba0..b9d3b4aa1 100644 --- a/itest/rust/src/object_test.rs +++ b/itest/rust/src/object_test.rs @@ -763,9 +763,6 @@ pub mod object_test_gd { #[derive(GodotClass)] #[class(base=Object)] pub struct CustomConstructor { - #[base] - base: Base, - #[var] pub val: i64, } @@ -774,7 +771,7 @@ pub mod object_test_gd { impl CustomConstructor { #[func] pub fn construct_object(val: i64) -> Gd { - Gd::with_base(|base| Self { base, val }) + Gd::with_base(|_base| Self { val }) } } } @@ -804,10 +801,7 @@ impl DoubleUse { #[derive(GodotClass)] #[class(init, base=Object)] -struct SignalEmitter { - #[base] - base: Base, -} +struct SignalEmitter {} #[godot_api] impl SignalEmitter { diff --git a/itest/rust/src/property_test.rs b/itest/rust/src/property_test.rs index 9c24617e3..1d401628e 100644 --- a/itest/rust/src/property_test.rs +++ b/itest/rust/src/property_test.rs @@ -15,28 +15,33 @@ use godot::{ #[derive(GodotClass)] #[class(base=Node)] struct HasProperty { - #[base] - base: Base, - #[var] int_val: i32, + #[var(get = get_int_val_read)] int_val_read: i32, + #[var(set = set_int_val_write)] int_val_write: i32, + #[var(get = get_int_val_rw, set = set_int_val_rw)] int_val_rw: i32, + #[var(get = get_int_val_getter, set)] int_val_getter: i32, + #[var(get, set = set_int_val_setter)] int_val_setter: i32, #[var(get = get_string_val, set = set_string_val)] string_val: GodotString, + #[var(get = get_object_val, set = set_object_val)] object_val: Option>, + #[var] texture_val: Gd, + #[var(get = get_texture_val, set = set_texture_val, hint = PROPERTY_HINT_RESOURCE_TYPE, hint_string = "Texture")] texture_val_rw: Option>, } @@ -120,7 +125,7 @@ impl HasProperty { #[godot_api] impl NodeVirtual for HasProperty { - fn init(base: Base) -> Self { + fn init(_base: Base) -> Self { HasProperty { int_val: 0, int_val_read: 2, @@ -132,7 +137,6 @@ impl NodeVirtual for HasProperty { string_val: GodotString::new(), texture_val: Texture::new(), texture_val_rw: None, - base, } } } diff --git a/itest/rust/src/signal_test.rs b/itest/rust/src/signal_test.rs index c0f834c6d..08f8c3c27 100644 --- a/itest/rust/src/signal_test.rs +++ b/itest/rust/src/signal_test.rs @@ -17,10 +17,7 @@ use crate::itest; #[derive(GodotClass)] #[class(init, base=Object)] -struct Emitter { - #[base] - base: Base, -} +struct Emitter {} #[godot_api] impl Emitter { @@ -81,11 +78,8 @@ fn signals() { let signal_name = format!("signal_{i}_arg"); let receiver_name = format!("receive_{i}_arg"); - emitter - .base_mut() - .connect(signal_name.clone().into(), receiver.callable(receiver_name)); - - emitter.base_mut().emit_signal(signal_name.into(), arg); + emitter.connect(signal_name.clone().into(), receiver.callable(receiver_name)); + emitter.emit_signal(signal_name.into(), arg); assert!(receiver.bind().used[i].get()); }