From d5d73b5b6e7ff5eafa2ebba84fdf734ed531dfb5 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sun, 5 Feb 2023 10:10:02 +0100 Subject: [PATCH] Remove feature `convenience` This feature was originally designed as an opt-out from panicking methods, which have a Result/Option-based overload, such as from_variant (try_from_variant), instance_id (instance_id_or_none), cast (try_cast), etc. However, it does not really add much in practice besides maintenance overhead. To still warn people to not accidentally use one of these methods, a warning sign is prepended to the brief doc. This should show up first in IDEs. Note that the warning sign is not added to every panicking method, but particularly those which have a "try" overload. To know which methods panic, check out if it contains a `# Panics` section. --- .github/workflows/full-ci.yml | 3 +- .github/workflows/minimal-ci.yml | 3 +- check.sh | 3 +- examples/dodge-the-creeps/rust/Cargo.toml | 2 +- godot-core/Cargo.toml | 1 - godot-core/src/builtin/variant/mod.rs | 3 +- .../src/builtin/variant/variant_traits.rs | 11 ++- godot-core/src/obj/gd.rs | 90 ++++++++----------- godot-core/src/obj/instance_id.rs | 5 +- godot/Cargo.toml | 3 +- godot/src/lib.rs | 28 ++++++ itest/rust/Cargo.toml | 2 +- 12 files changed, 88 insertions(+), 66 deletions(-) diff --git a/.github/workflows/full-ci.yml b/.github/workflows/full-ci.yml index 6650769fe..2952a691c 100644 --- a/.github/workflows/full-ci.yml +++ b/.github/workflows/full-ci.yml @@ -14,7 +14,8 @@ on: - trying env: - GDEXT_FEATURES: '--features godot-core/convenience' + GDEXT_FEATURES: '' +# GDEXT_FEATURES: '--features crate/feature' # GDEXT_CRATE_ARGS: '-p godot-codegen -p godot-ffi -p godot-core -p godot-macros -p godot' defaults: diff --git a/.github/workflows/minimal-ci.yml b/.github/workflows/minimal-ci.yml index 4646248ce..4668382c7 100644 --- a/.github/workflows/minimal-ci.yml +++ b/.github/workflows/minimal-ci.yml @@ -14,7 +14,8 @@ on: - master env: - GDEXT_FEATURES: '--features godot-core/convenience' + GDEXT_FEATURES: '' +# GDEXT_FEATURES: '--features crate/feature' # GDEXT_CRATE_ARGS: '-p godot-codegen -p godot-ffi -p godot-core -p godot-macros -p godot' defaults: diff --git a/check.sh b/check.sh index e7aed976b..fbf48981a 100755 --- a/check.sh +++ b/check.sh @@ -67,7 +67,8 @@ function findGodot() { fi } -features="--features godot-core/convenience" +#features="--features crate/feature" +features="" cmds=() for arg in "${args[@]}"; do diff --git a/examples/dodge-the-creeps/rust/Cargo.toml b/examples/dodge-the-creeps/rust/Cargo.toml index 4797baef7..f85abad06 100644 --- a/examples/dodge-the-creeps/rust/Cargo.toml +++ b/examples/dodge-the-creeps/rust/Cargo.toml @@ -8,5 +8,5 @@ publish = false crate-type = ["cdylib"] [dependencies] -godot = { path = "../../../godot", default-features = false, features = ["formatted", "convenience"] } +godot = { path = "../../../godot", default-features = false, features = ["formatted"] } rand = "0.8" diff --git a/godot-core/Cargo.toml b/godot-core/Cargo.toml index 056011b5d..0642f69da 100644 --- a/godot-core/Cargo.toml +++ b/godot-core/Cargo.toml @@ -12,7 +12,6 @@ default = [] trace = [] codegen-fmt = ["godot-ffi/codegen-fmt"] codegen-full = ["godot-codegen/codegen-full"] -convenience = [] [dependencies] godot-ffi = { path = "../godot-ffi" } diff --git a/godot-core/src/builtin/variant/mod.rs b/godot-core/src/builtin/variant/mod.rs index 8207a4fc4..90ece46ca 100644 --- a/godot-core/src/builtin/variant/mod.rs +++ b/godot-core/src/builtin/variant/mod.rs @@ -40,13 +40,12 @@ impl Variant { value.to_variant() } - /// Convert to type `T`, panicking on failure. + /// ⚠️ Convert to type `T`, panicking on failure. /// /// Equivalent to `T::from_variant(&self)`. /// /// # Panics /// When this variant holds a different type. - #[cfg(feature = "convenience")] pub fn to(&self) -> T { T::from_variant(self) } diff --git a/godot-core/src/builtin/variant/variant_traits.rs b/godot-core/src/builtin/variant/variant_traits.rs index bb2f0d53e..9119727ed 100644 --- a/godot-core/src/builtin/variant/variant_traits.rs +++ b/godot-core/src/builtin/variant/variant_traits.rs @@ -6,10 +6,15 @@ use crate::builtin::Variant; +/// Trait to enable conversions of types _from_ the [`Variant`] type. pub trait FromVariant: Sized { + /// Tries to convert a `Variant` to `Self`, allowing to check the success or failure. fn try_from_variant(variant: &Variant) -> Result; - #[cfg(feature = "convenience")] + /// ⚠️ Converts from `Variant` to `Self`, panicking on error. + /// + /// This method should generally not be overridden by trait impls, even if conversions are infallible. + /// Implementing [`Self::try_from_variant`] suffices. fn from_variant(variant: &Variant) -> Self { Self::try_from_variant(variant).unwrap_or_else(|e| { panic!( @@ -22,6 +27,7 @@ pub trait FromVariant: Sized { } } +/// Trait to enable conversions of types _to_ the [`Variant`] type. pub trait ToVariant { /*fn try_to_variant(&self) -> Result; @@ -35,6 +41,9 @@ pub trait ToVariant { }) }*/ + /// Infallible conversion from `Self` type to `Variant`. + /// + /// This method must not panic. If your conversion is fallible, this trait should not be used. fn to_variant(&self) -> Variant; } diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index 700396cf0..70312cbe5 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -33,7 +33,7 @@ use crate::{callbacks, engine, out}; /// /// * Objects of type [`RefCounted`] or inherited from it are **reference-counted**. This means that every time a smart pointer is /// shared using [`Share::share()`], the reference counter is incremented, and every time one is dropped, it is decremented. -/// This ensures that the last reference (either in Rust or Godot) will deallocate the obj and call `T`'s destructor. +/// This ensures that the last reference (either in Rust or Godot) will deallocate the object and call `T`'s destructor. /// /// * Objects inheriting from [`Object`] which are not `RefCounted` (or inherited) are **manually-managed**. /// Their destructor is not automatically called (unless they are part of the scene tree). Creating a `Gd` means that @@ -49,7 +49,7 @@ pub struct Gd { // Note: `opaque` has the same layout as GDExtensionObjectPtr == Object* in C++, i.e. the bytes represent a pointer // To receive a GDExtensionTypePtr == GDExtensionObjectPtr* == Object**, we need to get the address of this // Hence separate sys() for GDExtensionTypePtr, and obj_sys() for GDExtensionObjectPtr. - // The former is the standard FFI type, while the latter is used in obj-specific GDExtension engines. + // The former is the standard FFI type, while the latter is used in object-specific GDExtension engines. // pub(crate) because accessed in obj::dom pub(crate) opaque: OpaqueObject, _marker: PhantomData<*const T>, @@ -63,24 +63,16 @@ static_assert_eq_size!( ); /// _The methods in this impl block are only available for user-declared `T`, that is, -/// structs with `#[derive(GodotClass)]` but not Godot classes like `Node` or `RefCounted`._ +/// structs with `#[derive(GodotClass)]` but not Godot classes like `Node` or `RefCounted`._

impl Gd where T: GodotClass, { - /// Moves a user-created obj into this smart pointer, submitting ownership to the Godot engine. + /// Moves a user-created object into this smart pointer, submitting ownership to the Godot engine. /// /// This is only useful for types `T` which do not store their base objects (if they have a base, /// you cannot construct them standalone). pub fn new(user_object: T) -> Self { - /*let result = unsafe { - //let ptr = interface_fn!(classdb_construct_object)(class_name.c_str()); - let ptr = callbacks::create::(ptr::null_mut()); - Obj::from_obj_sys(ptr) - }; - - result.storage().initialize(user_object);*/ - Self::with_base(move |_base| user_object) } @@ -91,23 +83,12 @@ where where T: cap::GodotInit, { - /*let class_name = ClassName::new::(); - let result = unsafe { - let ptr = interface_fn!(classdb_construct_object)(class_name.c_str()); - Obj::from_obj_sys(ptr) - }; - - result.storage().initialize_default(); - T::Mem::maybe_init_ref(&result); - result*/ - unsafe { let object_ptr = callbacks::create::(ptr::null_mut()); Gd::from_obj_sys(object_ptr) } } - // FIXME use ```no_run instead of ```ignore, as soon as unit test #[cfg] mess is cleaned up /// Creates a `Gd` using a function that constructs a `T` from a provided base. /// /// Imagine you have a type `T`, which has a `#[base]` field that you cannot default-initialize. @@ -115,7 +96,7 @@ where /// is then wrapped in a `Gd`. /// /// Example: - /// ```ignore + /// ```no_run /// # use godot::prelude::*; /// #[derive(GodotClass)] /// #[class(init, base=Node2D)] @@ -169,7 +150,7 @@ where GdMut::from_cell(self.storage().get_mut()) } - /// Storage obj associated with the extension instance + /// 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 { @@ -190,11 +171,11 @@ where } } -/// _The methods in this impl block are available for any `T`._ +/// _The methods in this impl block are available for any `T`._

impl Gd { /// Looks up the given instance ID and returns the associated obj, if possible. /// - /// If no such instance ID is registered, or if the dynamic type of the obj behind that instance ID + /// If no such instance ID is registered, or if the dynamic type of the object behind that instance ID /// is not compatible with `T`, then `None` is returned. pub fn try_from_instance_id(instance_id: InstanceId) -> Option { // SAFETY: Godot looks up ID in ObjectDB and returns null if not found @@ -209,16 +190,15 @@ impl Gd { } } - /// Looks up the given instance ID and returns the associated obj. + /// ⚠️ Looks up the given instance ID and returns the associated object. /// /// # Panics - /// If no such instance ID is registered, or if the dynamic type of the obj behind that instance ID + /// If no such instance ID is registered, or if the dynamic type of the object behind that instance ID /// is not compatible with `T`. - #[cfg(feature = "convenience")] pub fn from_instance_id(instance_id: InstanceId) -> Self { Self::try_from_instance_id(instance_id).unwrap_or_else(|| { panic!( - "Instance ID {} does not belong to a valid obj of class '{}'", + "Instance ID {} does not belong to a valid object of class '{}'", instance_id, T::CLASS_NAME ) @@ -232,7 +212,7 @@ impl Gd { } } - /// Returns the instance ID of this obj, or `None` if the obj is dead. + /// Returns the instance ID of this obj, or `None` if the object is dead. /// pub fn instance_id_or_none(&self) -> Option { // Note: bit 'id & (1 << 63)' determines if the instance is ref-counted @@ -240,11 +220,10 @@ impl Gd { InstanceId::try_from_u64(id) } - /// Returns the instance ID of this obj (panics when dead). + /// ⚠️ Returns the instance ID of this object (panics when dead). /// /// # Panics - /// If this obj is no longer alive (registered in Godot's obj database). - #[cfg(feature = "convenience")] + /// If this object is no longer alive (registered in Godot's object database). pub fn instance_id(&self) -> InstanceId { self.instance_id_or_none().unwrap_or_else(|| { panic!( @@ -254,12 +233,12 @@ impl Gd { }) } - /// Checks if this smart pointer points to a live obj (read description!). + /// Checks if this smart pointer points to a live object (read description!). /// - /// Using this method is often indicative of bad design -- you should dispose of your pointers once an obj is + /// Using this method is often indicative of bad design -- you should dispose of your pointers once an object is /// destroyed. However, this method exists because GDScript offers it and there may be **rare** use cases. /// - /// Do not use this method to check if you can safely access an obj. Accessing dead objects is generally safe + /// Do not use this method to check if you can safely access an object. Accessing dead objects is generally safe /// and will panic in a defined manner. Encountering such panics is almost always a bug you should fix, and not a /// runtime condition to check against. pub fn is_instance_valid(&self) -> bool { @@ -275,9 +254,15 @@ impl Gd { /// /// Moves out of this value. If you want to create _another_ smart pointer instance, /// use this idiom: - /// ```ignore - /// let obj: Gd = ...; - /// let base = obj.share().upcast::(); + /// ```no_run + /// # use godot::prelude::*; + /// + /// #[derive(GodotClass)] + /// #[class(init, base=Node2D)] + /// struct MyClass {} + /// + /// let obj: Gd = Gd::new_default(); + /// let base = obj.share().upcast::(); /// ``` pub fn upcast(self) -> Gd where @@ -293,7 +278,7 @@ impl Gd { /// If `T`'s dynamic type is not `Derived` or one of its subclasses, `None` is returned /// and the reference is dropped. Otherwise, `Some` is returned and the ownership is moved /// to the returned value. - // TODO consider Result, Self> so that user can still use original obj (e.g. to free if manual) + // TODO consider Result, Self> so that user can still use original object (e.g. to free if manual) pub fn try_cast(self) -> Option> where Derived: GodotClass + Inherits, @@ -301,11 +286,10 @@ impl Gd { self.owned_cast().ok() } - /// **Downcast:** convert into a smart pointer to a derived class. Panics on error. + /// ⚠️ **Downcast:** convert into a smart pointer to a derived class. Panics on error. /// /// # Panics /// If the class' dynamic type is not `Derived` or one of its subclasses. Use [`Self::try_cast()`] if you want to check the result. - #[cfg(feature = "convenience")] pub fn cast(self) -> Gd where Derived: GodotClass + Inherits, @@ -328,7 +312,7 @@ impl Gd { // to return the same pointer, however in theory those may yield a different pointer (VTable offset, // virtual inheritance etc.). It *seems* to work so far, but this is no indication it's not UB. // - // The Deref/DerefMut impls for T implement an "implicit upcast" on the obj (not Gd) level and + // The Deref/DerefMut impls for T implement an "implicit upcast" on the object (not Gd) level and // rely on this (e.g. &Node3D -> &Node). let result = unsafe { self.ffi_cast::() }; @@ -363,7 +347,7 @@ impl Gd { ); let tmp = unsafe { self.ffi_cast::() }; - let mut tmp = tmp.expect("obj expected to inherit RefCounted"); + let mut tmp = tmp.expect("object expected to inherit RefCounted"); let return_val = ::Declarer::scoped_mut(&mut tmp, |obj| apply(obj)); @@ -375,7 +359,7 @@ impl Gd { // Note: no validity check; this could be called by to_string(), which can be called on dead instances let tmp = unsafe { self.ffi_cast::() }; - let mut tmp = tmp.expect("obj expected to inherit Object; should never fail"); + let mut tmp = tmp.expect("object expected to inherit Object; should never fail"); // let return_val = apply(tmp.inner_mut()); let return_val = ::Declarer::scoped_mut(&mut tmp, |obj| apply(obj)); @@ -415,15 +399,15 @@ impl Gd { } /// _The methods in this impl block are only available for objects `T` that are manually managed, -/// i.e. anything that is not `RefCounted` or inherited from it._ +/// i.e. anything that is not `RefCounted` or inherited from it._

impl Gd where T: GodotClass, M: mem::PossiblyManual + mem::Memory, { - /// Destroy the manually-managed Godot obj. + /// Destroy the manually-managed Godot object. /// - /// Consumes this smart pointer and renders all other `Gd` smart pointers (as well as any GDScript references) to the same obj + /// Consumes this smart pointer and renders all other `Gd` smart pointers (as well as any GDScript references) to the same object /// immediately invalid. Using those `Gd` instances will lead to panics, but not undefined behavior. /// /// This operation is **safe** and effectively prevents double-free. @@ -432,7 +416,7 @@ where /// example to the node tree in case of nodes. /// /// # Panics - /// * When the referred-to obj has already been destroyed. + /// * When the referred-to object has already been destroyed. /// * When this is invoked on an upcast `Gd` that dynamically points to a reference-counted type (i.e. operation not supported). pub fn free(self) { // TODO disallow for singletons, either only at runtime or both at compile time (new memory policy) and runtime @@ -490,7 +474,7 @@ where // // 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 obj is the + // 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) } } @@ -540,7 +524,7 @@ pub unsafe fn raw_object_init( /// * If this `Gd` smart pointer holds a reference-counted type, this will decrement the reference counter. /// If this was the last remaining reference, dropping it will invoke `T`'s destructor. /// -/// * If the held obj is manually-managed, **nothing happens**. +/// * If the held object is manually-managed, **nothing happens**. /// To destroy manually-managed `Gd` pointers, you need to call [`Self::free()`]. impl Drop for Gd { fn drop(&mut self) { diff --git a/godot-core/src/obj/instance_id.rs b/godot-core/src/obj/instance_id.rs index 20832a9e0..e7daa2c49 100644 --- a/godot-core/src/obj/instance_id.rs +++ b/godot-core/src/obj/instance_id.rs @@ -26,17 +26,18 @@ pub struct InstanceId { impl InstanceId { /// Constructs an instance ID from an integer, or `None` if the integer is zero. + /// + /// This does *not* check if the instance is valid. pub fn try_from_i64(id: i64) -> Option { Self::try_from_u64(id as u64) } - /// Constructs an instance ID from a non-zero integer, or panics. + /// ⚠️ Constructs an instance ID from a non-zero integer, or panics. /// /// This does *not* check if the instance is valid. /// /// # Panics /// If `id` is zero. - #[cfg(feature = "convenience")] pub fn from_nonzero(id: i64) -> Self { Self::try_from_i64(id).expect("expected non-zero instance ID") } diff --git a/godot/Cargo.toml b/godot/Cargo.toml index eebda8353..bf85f1147 100644 --- a/godot/Cargo.toml +++ b/godot/Cargo.toml @@ -8,8 +8,7 @@ keywords = ["gamedev", "godot", "engine", "2d", "3d"] # possibly: "ffi" categories = ["game-engines", "graphics"] [features] -default = ["convenience", "codegen-full"] -convenience = ["godot-core/convenience"] +default = ["codegen-full"] formatted = ["godot-core/codegen-fmt"] trace = ["godot-core/trace"] diff --git a/godot/src/lib.rs b/godot/src/lib.rs index 16207cf68..be4555353 100644 --- a/godot/src/lib.rs +++ b/godot/src/lib.rs @@ -57,6 +57,34 @@ //! you must either hand over ownership to Godot (e.g. by adding a node to the scene tree) or //! free them manually using [`Gd::free()`][crate::obj::Gd::free].

//! +//! # Ergonomics and panics +//! +//! The GDExtension Rust bindings are designed with usage ergonomics in mind, making them viable +//! for fast prototyping. Part of this design means that users should not constantly be forced +//! to write code such as `obj.cast::().unwrap()`. Instead, they can just write `obj.cast::()`, +//! which may panic at runtime. +//! +//! This approach has several advantages: +//! * The code is more concise and less cluttered. +//! * Methods like `cast()` provide very sophisticated panic messages when they fail (e.g. involved +//! classes), immediately giving you the necessary context for debugging. This is certainly +//! preferable over a generic `unwrap()`, and in most cases also over a `expect("literal")`. +//! * Usually, such methods panicking indicate bugs in the application. For example, you have a static +//! scene tree, and you _know_ that a node of certain type and name exists. `get_node_as::("name")` +//! thus _must_ succeed, or your mental concept is wrong. In other words, there is not much you can +//! do at runtime to recover from such errors anyway; the code needs to be fixed. +//! +//! Now, there are of course cases where you _do_ want to check certain assumptions dynamically. +//! Imagine a scene tree that is constructed at runtime, e.g. in a game editor. +//! This is why the library provides "overloads" for most of these methods that return `Option` or `Result`. +//! Such methods have more verbose names and highlight the attempt, e.g. `try_cast()`. +//! +//! To help you identify panicking methods, we use the symbol "⚠️" at the beginning of the documentation; +//! this should also appear immediately in the auto-completion of your IDE. Note that this warning sign is +//! not used as a general panic indicator, but particularly for methods which have a `Option`/`Result`-based +//! overload. If you want to know whether and how a method can panic, check if its documentation has a +//! _Panics_ section. +//! //! # Thread safety //! //! [Godot's own thread safety diff --git a/itest/rust/Cargo.toml b/itest/rust/Cargo.toml index f22fdfdb5..0e610b266 100644 --- a/itest/rust/Cargo.toml +++ b/itest/rust/Cargo.toml @@ -13,7 +13,7 @@ default = [] trace = ["godot/trace"] [dependencies] -godot = { path = "../../godot", default-features = false, features = ["formatted", "convenience"] } +godot = { path = "../../godot", default-features = false, features = ["formatted"] } [build-dependencies] quote = "1"