Unify string parameters across the repo

The types of string parameters across the repo has been regularized
according to the following rules that shall be followed from now on:

- Methods during the init/registration process should take `&str` for
  things like identifiers. They may specify explicit lifetimes in their
  types if necessary, but *not* `'static`.
- Manually written methods that abstract over, or mimic API methods should
  take `impl Into<GodotString>` or `impl Into<NodePath>` depending on the
  usage, to be consistent with their generated counterparts.

Regarding the usage of `&str` in init instead of more specific conversions
required by each method:

The GDNative API isn't very consistent when it comes to the string types
it wants during init. Sometimes, functions may want different types of
strings even when they are concepturally similar, like for signal and
method registration. This gets more complicated when we add our own
library-side logic into the mix, like for the `InitHandle::add_class`
family, where allocation is current inevitable even when they are given
`&'static str`s.

It still makes sense to avoid excessive allocation, but that should not
get in the way of future development. `'static` in particular is
extremely limiting, and there are very few cases of its usage throughout
the library's history that we have not since regretted. It shouldn't be
the norm but the rare exception.

In any case, class registration is something that only run once in the
lifecycle of a game, and for the amount of classes most people are using
with `gdnative`, we can afford to be slightly inefficient with strings
to make our APIs more consistent, less leaky, and easier to stablize and
maintain.

Author: chitoyuu

gdnative-async/src/rt.rs

@@ -3,7 +3,7 @@ use std::marker::PhantomData;
 
 use func_state::FuncState;
 use gdnative_bindings::Object;
-use gdnative_core::core_types::{GodotError, Variant};
+use gdnative_core::core_types::{GodotError, Variant, GodotString};
 use gdnative_core::init::InitHandle;
 use gdnative_core::object::{Instance, SubClass, TInstance, TRef};
 
@@ -77,13 +77,13 @@ impl Context {
     pub fn signal<C>(
         &self,
         obj: TRef<'_, C>,
-        signal: &str,
+        signal: impl Into<GodotString>,
     ) -> Result<future::Yield<Vec<Variant>>, GodotError>
     where
         C: SubClass<Object>,
     {
         let (future, resume) = future::make();
-        bridge::SignalBridge::connect(obj.upcast(), signal, resume)?;
+        bridge::SignalBridge::connect(obj.upcast(), signal.into(), resume)?;
         Ok(future)
     }
 }
@@ -107,8 +107,8 @@ pub fn register_runtime_with_prefix<S>(handle: &InitHandle, prefix: S)
 where
     S: Display,
 {
-    handle.add_class_as::<bridge::SignalBridge>(format!("{prefix}SignalBridge"));
-    handle.add_class_as::<func_state::FuncState>(format!("{prefix}FuncState"));
+    handle.add_class_as::<bridge::SignalBridge>(&format!("{prefix}SignalBridge"));
+    handle.add_class_as::<func_state::FuncState>(&format!("{prefix}FuncState"));
 }
 
 /// Releases all observers still in use. This should be called in the

gdnative-async/src/rt/bridge.rs

@@ -4,7 +4,7 @@ use once_cell::sync::OnceCell;
 use parking_lot::Mutex;
 
 use gdnative_bindings::{Object, Reference};
-use gdnative_core::core_types::{GodotError, Variant, VariantArray};
+use gdnative_core::core_types::{GodotError, Variant, VariantArray, GodotString};
 use gdnative_core::export::user_data::{ArcData, Map};
 use gdnative_core::export::{ClassBuilder, Method, NativeClass, NativeClassMethods, Varargs};
 use gdnative_core::godot_site;
@@ -58,7 +58,7 @@ impl NativeClass for SignalBridge {
 impl SignalBridge {
     pub(crate) fn connect(
         source: TRef<Object>,
-        signal: &str,
+        signal: GodotString,
         resume: Resume<Vec<Variant>>,
     ) -> Result<(), GodotError> {
         let mut pool = BRIDGES.get_or_init(Mutex::default).lock();

gdnative-bindings/src/utils.rs

@@ -18,7 +18,7 @@ use super::generated::{Engine, Node, SceneTree};
 /// invariants must be observed for the resulting node during `'a`, if any.
 ///
 /// [thread-safety]: https://docs.godotengine.org/en/stable/tutorials/threads/thread_safe_apis.html
-pub unsafe fn autoload<'a, T>(name: &str) -> Option<TRef<'a, T>>
+pub unsafe fn autoload<'a, T>(name: impl Into<NodePath>) -> Option<TRef<'a, T>>
 where
     T: SubClass<Node>,
 {

gdnative-core/src/export/class_builder.rs

@@ -10,12 +10,6 @@ use crate::export::*;
 use crate::object::NewRef;
 use crate::private::get_api;
 
-// TODO(#996): unify string parameters across all buiders
-// Potential candidates:
-// * &str
-// * impl Into<GodotString>
-// * impl Into<Cow<'a, str>>
-
 /// Allows registration of exported properties, methods and signals.
 ///
 /// See member functions of this class for usage examples.

gdnative-core/src/export/class_registry.rs

@@ -1,5 +1,4 @@
 use std::any::TypeId;
-use std::borrow::Cow;
 use std::collections::hash_map::Entry;
 use std::collections::HashMap;
 use std::fmt;
@@ -15,7 +14,7 @@ static CLASS_REGISTRY: Lazy<RwLock<HashMap<TypeId, ClassInfo>>> =
 
 #[derive(Clone, Debug)]
 pub(crate) struct ClassInfo {
-    pub name: Cow<'static, str>,
+    pub name: String,
     pub init_level: InitLevel,
 }
 
@@ -31,7 +30,7 @@ where
 /// Returns the NativeScript name of the class `C` if it is registered.
 /// Can also be used to validate whether or not `C` has been added using `InitHandle::add_class<C>()`
 #[inline]
-pub(crate) fn class_name<C: NativeClass>() -> Option<Cow<'static, str>> {
+pub(crate) fn class_name<C: NativeClass>() -> Option<String> {
     with_class_info::<C, _, _>(|i| i.name.clone())
 }
 
@@ -41,8 +40,8 @@ pub(crate) fn class_name<C: NativeClass>() -> Option<Cow<'static, str>> {
 /// The returned string should only be used for diagnostic purposes, not for types that the user
 /// has to name explicitly. The format is not guaranteed.
 #[inline]
-pub(crate) fn class_name_or_default<C: NativeClass>() -> Cow<'static, str> {
-    class_name::<C>().unwrap_or_else(|| Cow::Borrowed(std::any::type_name::<C>()))
+pub(crate) fn class_name_or_default<C: NativeClass>() -> String {
+    class_name::<C>().unwrap_or_else(|| std::any::type_name::<C>().into())
 }
 
 /// Registers the class `C` in the class registry, using a custom name at the given level.
@@ -51,14 +50,17 @@ pub(crate) fn class_name_or_default<C: NativeClass>() -> Cow<'static, str> {
 /// Returns an error with the old `ClassInfo` if a conflicting entry for `C` was already added.
 #[inline]
 pub(crate) fn register_class_as<C: NativeClass>(
-    name: Cow<'static, str>,
+    name: &str,
     init_level: InitLevel,
 ) -> Result<bool, RegisterError> {
     let type_id = TypeId::of::<C>();
     let mut registry = CLASS_REGISTRY.write();
     match registry.entry(type_id) {
         Entry::Vacant(entry) => {
-            entry.insert(ClassInfo { name, init_level });
+            entry.insert(ClassInfo {
+                name: name.into(),
+                init_level,
+            });
             Ok(true)
         }
         Entry::Occupied(entry) => {
@@ -86,7 +88,7 @@ pub(crate) fn register_class_as<C: NativeClass>(
 
 #[inline]
 #[allow(dead_code)] // Currently unused on platforms with inventory support
-pub(crate) fn types_with_init_level(allow: InitLevel, deny: InitLevel) -> Vec<Cow<'static, str>> {
+pub(crate) fn types_with_init_level(allow: InitLevel, deny: InitLevel) -> Vec<String> {
     let registry = CLASS_REGISTRY.read();
     let mut list = registry
         .values()

gdnative-core/src/export/property/invalid_accessor.rs

@@ -84,7 +84,7 @@ unsafe impl<'l, C: NativeClass, T> RawSetter<C, T> for InvalidSetter<'l> {
         let mut set = sys::godot_property_set_func::default();
 
         let data = Box::new(InvalidAccessorData {
-            class_name: class_registry::class_name_or_default::<C>().into_owned(),
+            class_name: class_registry::class_name_or_default::<C>(),
             property_name: self.property_name.to_string(),
         });
 
@@ -101,7 +101,7 @@ unsafe impl<'l, C: NativeClass, T> RawGetter<C, T> for InvalidGetter<'l> {
         let mut get = sys::godot_property_get_func::default();
 
         let data = Box::new(InvalidAccessorData {
-            class_name: class_registry::class_name_or_default::<C>().into_owned(),
+            class_name: class_registry::class_name_or_default::<C>(),
             property_name: self.property_name.to_string(),
         });
 

gdnative-core/src/init/init_handle.rs

@@ -4,7 +4,6 @@ use crate::export::{
 };
 use crate::object::{GodotObject, RawObject, TRef};
 use crate::private::get_api;
-use std::borrow::Cow;
 use std::ffi::CString;
 use std::ptr;
 
@@ -43,7 +42,7 @@ impl InitHandle {
     where
         C: NativeClassMethods + StaticallyNamed,
     {
-        self.add_maybe_tool_class_as_with::<C>(Cow::Borrowed(C::CLASS_NAME), false, |builder| {
+        self.add_maybe_tool_class_as_with::<C>(C::CLASS_NAME, false, |builder| {
             C::nativeclass_register_monomorphized(builder);
             f(builder);
         })
@@ -64,7 +63,7 @@ impl InitHandle {
     where
         C: NativeClassMethods + StaticallyNamed,
     {
-        self.add_maybe_tool_class_as_with::<C>(Cow::Borrowed(C::CLASS_NAME), true, |builder| {
+        self.add_maybe_tool_class_as_with::<C>(C::CLASS_NAME, true, |builder| {
             C::nativeclass_register_monomorphized(builder);
             f(builder);
         })
@@ -75,7 +74,7 @@ impl InitHandle {
     /// If the type implements [`StaticallyTyped`], that name is ignored in favor of the
     /// name provided at registration.
     #[inline]
-    pub fn add_class_as<C>(self, name: String)
+    pub fn add_class_as<C>(self, name: &str)
     where
         C: NativeClassMethods,
     {
@@ -87,19 +86,19 @@ impl InitHandle {
     /// If the type implements [`StaticallyTyped`], that name is ignored in favor of the
     /// name provided at registration.
     #[inline]
-    pub fn add_class_as_with<C>(self, name: String, f: impl FnOnce(&ClassBuilder<C>))
+    pub fn add_class_as_with<C>(self, name: &str, f: impl FnOnce(&ClassBuilder<C>))
     where
         C: NativeClassMethods,
     {
-        self.add_maybe_tool_class_as_with::<C>(Cow::Owned(name), false, f)
+        self.add_maybe_tool_class_as_with::<C>(name, false, f)
     }
 
     /// Registers a new tool class to the engine
     ///
     /// If the type implements [`StaticallyTyped`], that name is ignored in favor of the
     /// name provided at registration.
     #[inline]
-    pub fn add_tool_class_as<C>(self, name: String)
+    pub fn add_tool_class_as<C>(self, name: &str)
     where
         C: NativeClassMethods,
     {
@@ -111,23 +110,23 @@ impl InitHandle {
     /// If the type implements [`StaticallyTyped`], that name is ignored in favor of the
     /// name provided at registration.
     #[inline]
-    pub fn add_tool_class_as_with<C>(self, name: String, f: impl FnOnce(&ClassBuilder<C>))
+    pub fn add_tool_class_as_with<C>(self, name: &str, f: impl FnOnce(&ClassBuilder<C>))
     where
         C: NativeClassMethods,
     {
-        self.add_maybe_tool_class_as_with::<C>(Cow::Owned(name), true, f)
+        self.add_maybe_tool_class_as_with::<C>(name, true, f)
     }
 
     #[inline]
     fn add_maybe_tool_class_as_with<C>(
         self,
-        name: Cow<'static, str>,
+        name: &str,
         is_tool: bool,
         f: impl FnOnce(&ClassBuilder<C>),
     ) where
         C: NativeClassMethods,
     {
-        let c_class_name = CString::new(&*name).unwrap();
+        let c_class_name = CString::new(name).unwrap();
 
         match class_registry::register_class_as::<C>(name, self.init_level) {
             Ok(true) => {}

gdnative-core/src/object/instance.rs

@@ -1,3 +1,4 @@
+use std::borrow::Cow;
 use std::fmt::Debug;
 use std::ptr::NonNull;
 
@@ -599,7 +600,7 @@ where
     fn from_variant(variant: &Variant) -> Result<Self, FromVariantError> {
         let owner = Ref::<T::Base, Shared>::from_variant(variant)?;
         Self::from_base(owner).ok_or(FromVariantError::InvalidInstance {
-            expected: class_registry::class_name_or_default::<T>(),
+            expected: Cow::Owned(class_registry::class_name_or_default::<T>()),
         })
     }
 }

gdnative/src/globalscope.rs

@@ -20,8 +20,8 @@
 //! [@GDScript]: https://docs.godotengine.org/en/stable/classes/[email protected]
 
 use crate::api::{Resource, ResourceLoader};
-use crate::core_types::NodePath;
 use crate::object::{memory::RefCounted, GodotObject, Ref, SubClass};
+use crate::core_types::GodotString;
 
 #[doc(inline)]
 pub use gdnative_core::globalscope::*;
@@ -52,7 +52,7 @@ pub use gdnative_core::globalscope::*;
 /// let scene = load::<PackedScene>("res://path/to/Main.tscn").unwrap();
 /// ```
 #[inline]
-pub fn load<T>(path: impl Into<NodePath>) -> Option<Ref<T>>
+pub fn load<T>(path: impl Into<GodotString>) -> Option<Ref<T>>
 where
     T: SubClass<Resource> + GodotObject<Memory = RefCounted>,
 {