Merge pull request #1316 from godot-rust/qol/remove-static-stringname

`StringName`: remove `From` impl for `&'static CStr`

Author: Bromeon

godot-codegen/src/util.rs

@@ -41,7 +41,7 @@ pub fn c_str(string: &str) -> Literal {
 pub fn make_string_name(identifier: &str) -> TokenStream {
     let lit = c_str(identifier);
 
-    quote! { StringName::from(#lit) }
+    quote! { StringName::__cstr(#lit) }
 }
 
 pub fn make_sname_ptr(identifier: &str) -> TokenStream {

godot-core/src/builtin/mod.rs

@@ -41,7 +41,7 @@ pub mod __prelude_reexport {
     pub use rect2i::*;
     pub use rid::*;
     pub use signal::*;
-    pub use string::{static_name, Encoding, GString, NodePath, StringName};
+    pub use string::{Encoding, GString, NodePath, StringName};
     pub use transform2d::*;
     pub use transform3d::*;
     pub use variant::*;
@@ -54,6 +54,9 @@ pub mod __prelude_reexport {
     #[allow(deprecated)]
     #[rustfmt::skip] // Do not reorder.
     pub use crate::dict;
+
+    #[cfg(feature = "trace")] // Test only.
+    pub use crate::static_sname;
 }
 
 pub use __prelude_reexport::*;

godot-core/src/builtin/string/mod.rs

@@ -20,7 +20,6 @@ pub use string_name::*;
 use crate::meta;
 use crate::meta::error::ConvertError;
 use crate::meta::{FromGodot, GodotConvert, ToGodot};
-pub use crate::static_name;
 
 impl GodotConvert for &str {
     type Via = GString;

godot-core/src/builtin/string/string_name.rs

@@ -4,10 +4,10 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
+
 use std::fmt;
 
 use godot_ffi as sys;
-use godot_ffi::interface_fn;
 use sys::{ffi_methods, ExtVariantType, GodotFfi};
 
 use crate::builtin::{inner, Encoding, GString, NodePath, Variant};
@@ -33,11 +33,6 @@ use crate::{impl_shared_string_api, meta};
 /// Note that Godot ignores any bytes after a null-byte. This means that for instance `"hello, world!"` and  \
 /// `"hello, world!\0 ignored by Godot"` will be treated as the same string if converted to a `StringName`.
 ///
-/// # Performance
-///
-/// The fastest way to create string names is using [`static_name!`][crate::builtin::static_name], which creates a static cached `StringName` from null-terminated C-string literals such as `c"MyClass"`. These can be used directly by Godot without conversion. The encoding is limited to Latin-1, however. See the corresponding
-/// [`From<&CStr>` impl](#impl-From<%26CStr>-for-StringName).
-///
 /// # All string types
 ///
 /// | Intended use case | String type                                |
@@ -88,7 +83,7 @@ impl StringName {
             let is_static = sys::conv::SYS_FALSE;
             let s = unsafe {
                 Self::new_with_string_uninit(|string_ptr| {
-                    let ctor = interface_fn!(string_name_new_with_latin1_chars);
+                    let ctor = sys::interface_fn!(string_name_new_with_latin1_chars);
                     ctor(
                         string_ptr,
                         cstr.as_ptr() as *const std::ffi::c_char,
@@ -248,6 +243,48 @@ impl StringName {
     pub fn as_inner(&self) -> inner::InnerStringName<'_> {
         inner::InnerStringName::from_outer(self)
     }
+
+    #[doc(hidden)] // Private for now. Needs API discussion, also regarding overlap with try_from_cstr().
+    pub fn __cstr(c_str: &'static std::ffi::CStr) -> Self {
+        // This used to be set to true, but `p_is_static` parameter in Godot should only be enabled if the result is indeed stored
+        // in a static. See discussion in https://github.com/godot-rust/gdext/pull/1316. We may unify this into a regular constructor,
+        // or provide a dedicated StringName cache (similar to ClassId cache) in the future, which would be freed on shutdown.
+        let is_static = false;
+
+        Self::__cstr_with_static(c_str, is_static)
+    }
+
+    /// Creates a `StringName` from a static ASCII/Latin-1 `c"string"`.
+    ///
+    /// If `is_static` is true, avoids unnecessary copies and allocations and directly uses the backing buffer. However, this must
+    /// be stored in an actual `static` to not cause leaks/error messages with Godot. For literals, use `is_static=false`.
+    ///
+    /// Note that while Latin-1 encoding is the most common encoding for c-strings, it isn't a requirement. So if your c-string
+    /// uses a different encoding (e.g. UTF-8), it is possible that some characters will not show up as expected.
+    ///
+    /// # Safety
+    /// `c_str` must be a static c-string that remains valid for the entire program duration.
+    ///
+    /// # Example
+    /// ```no_run
+    /// use godot::builtin::StringName;
+    ///
+    /// // '±' is a Latin-1 character with codepoint 0xB1. Note that this is not UTF-8, where it would need two bytes.
+    /// let sname = StringName::__cstr(c"\xb1 Latin-1 string");
+    /// ```
+    #[doc(hidden)] // Private for now. Needs API discussion, also regarding overlap with try_from_cstr().
+    pub fn __cstr_with_static(c_str: &'static std::ffi::CStr, is_static: bool) -> Self {
+        // SAFETY: c_str is nul-terminated and remains valid for entire program duration.
+        unsafe {
+            Self::new_with_string_uninit(|ptr| {
+                sys::interface_fn!(string_name_new_with_latin1_chars)(
+                    ptr,
+                    c_str.as_ptr(),
+                    sys::conv::bool_to_sys(is_static),
+                )
+            })
+        }
+    }
 }
 
 // SAFETY:
@@ -354,35 +391,6 @@ impl From<&NodePath> for StringName {
     }
 }
 
-impl From<&std::ffi::CStr> for StringName {
-    /// Creates a `StringName` from a ASCII/Latin-1 `c"string"`.
-    ///
-    /// This avoids unnecessary copies and allocations and directly uses the backing buffer. Useful for literals.
-    ///
-    /// Note that while Latin-1 encoding is the most common encoding for c-strings, it isn't a requirement. So if your c-string
-    /// uses a different encoding (e.g. UTF-8), it is possible that some characters will not show up as expected.
-    ///
-    /// # Example
-    /// ```no_run
-    /// use godot::builtin::StringName;
-    ///
-    /// // '±' is a Latin-1 character with codepoint 0xB1. Note that this is not UTF-8, where it would need two bytes.
-    /// let sname = StringName::from(c"\xb1 Latin-1 string");
-    /// ```
-    fn from(c_str: &std::ffi::CStr) -> Self {
-        // SAFETY: c_str is nul-terminated and remains valid for entire program duration.
-        unsafe {
-            Self::new_with_string_uninit(|ptr| {
-                sys::interface_fn!(string_name_new_with_latin1_chars)(
-                    ptr,
-                    c_str.as_ptr(),
-                    sys::conv::SYS_FALSE, // p_is_static
-                )
-            })
-        }
-    }
-}
-
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Ordering
 
@@ -489,29 +497,18 @@ mod serialize {
     }
 }
 
+// TODO(v0.4.x): consider re-exposing in public API. Open questions: thread-safety, performance, memory leaks, global overhead.
+// Possibly in a more general StringName cache, similar to ClassId. See https://github.com/godot-rust/gdext/pull/1316.
 /// Creates and gets a reference to a static `StringName` from a ASCII/Latin-1 `c"string"`.
 ///
 /// This is the fastest way to create a StringName repeatedly, with the result being cached and never released, like `SNAME` in Godot source code. Suitable for scenarios where high performance is required.
 #[macro_export]
-macro_rules! static_name {
+macro_rules! static_sname {
     ($str:literal) => {{
         use std::sync::OnceLock;
 
-        use godot::sys;
-
         let c_str: &'static std::ffi::CStr = $str;
         static SNAME: OnceLock<StringName> = OnceLock::new();
-        SNAME.get_or_init(|| {
-            // SAFETY: c_str is nul-terminated and remains valid for entire program duration.
-            unsafe {
-                StringName::new_with_string_uninit(|ptr| {
-                    sys::interface_fn!(string_name_new_with_latin1_chars)(
-                        ptr,
-                        c_str.as_ptr(),
-                        sys::conv::SYS_TRUE, // p_is_static
-                    )
-                })
-            }
-        })
+        SNAME.get_or_init(|| StringName::__cstr_with_static(c_str, true))
     }};
 }

godot-core/src/meta/args/as_arg.rs

@@ -5,8 +5,6 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
 
-use std::ffi::CStr;
-
 use crate::builtin::{GString, NodePath, StringName, Variant};
 use crate::meta::sealed::Sealed;
 use crate::meta::traits::{GodotFfiVariant, GodotNullableFfi};
@@ -562,12 +560,6 @@ impl AsArg<StringName> for &String {
     }
 }
 
-impl AsArg<StringName> for &'static CStr {
-    fn into_arg<'arg>(self) -> CowArg<'arg, StringName> {
-        CowArg::Owned(StringName::from(self))
-    }
-}
-
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // NodePath
 

godot-core/src/meta/class_id.rs

@@ -263,7 +263,7 @@ impl ClassIdSource {
     fn to_string_name(&self) -> StringName {
         match self {
             ClassIdSource::Owned(s) => StringName::from(s),
-            ClassIdSource::Borrowed(cstr) => StringName::from(*cstr),
+            ClassIdSource::Borrowed(cstr) => StringName::__cstr(cstr),
         }
     }
 

godot-macros/src/class/data_models/inherent_impl.rs

@@ -459,7 +459,7 @@ fn add_virtual_script_call(
 
     let code = quote! {
         let object_ptr = #object_ptr;
-        let method_sname = ::godot::builtin::StringName::from(#method_name_cstr);
+        let method_sname = ::godot::builtin::StringName::__cstr(#method_name_cstr);
         let method_sname_ptr = method_sname.string_sys();
         let has_virtual_override = unsafe { ::godot::private::has_virtual_script_method(object_ptr, method_sname_ptr) };
 

itest/rust/src/builtin_tests/convert_test.rs

@@ -344,7 +344,6 @@ fn strings_as_arg() {
     // Note: CowArg is an internal type.
 
     let str = "GodotRocks";
-    let cstr = c"GodotRocks";
     let gstring = GString::from("GodotRocks");
     let sname = StringName::from("GodotRocks");
     let npath = NodePath::from("GodotRocks");
@@ -355,7 +354,6 @@ fn strings_as_arg() {
     assert_eq!(as_gstr_arg(npath.arg()), CowArg::Owned(gstring.clone()));
 
     assert_eq!(as_sname_arg(str), CowArg::Owned(sname.clone()));
-    assert_eq!(as_sname_arg(cstr), CowArg::Owned(sname.clone()));
     assert_eq!(as_sname_arg(&sname), CowArg::Borrowed(&sname));
     assert_eq!(as_sname_arg(gstring.arg()), CowArg::Owned(sname.clone()));
     assert_eq!(as_sname_arg(npath.arg()), CowArg::Owned(sname.clone()));

itest/rust/src/builtin_tests/string/string_name_test.rs

@@ -7,7 +7,7 @@
 
 use std::collections::HashSet;
 
-use godot::builtin::{static_name, Encoding, GString, NodePath, StringName};
+use godot::builtin::{static_sname, Encoding, GString, NodePath, StringName};
 
 use crate::framework::{assert_eq_self, itest};
 
@@ -127,32 +127,32 @@ fn string_name_from_cstr() {
     ];
 
     for (bytes, string) in cases.into_iter() {
-        let a = StringName::from(bytes);
+        let a = StringName::__cstr(bytes);
         let b = StringName::from(string);
 
         assert_eq!(a, b);
     }
 }
 
 #[itest]
-fn string_name_static_name() {
-    let a = static_name!(c"pure ASCII\t[~]").clone();
+fn string_name_static_sname() {
+    let a = static_sname!(c"pure ASCII\t[~]").clone();
     let b = StringName::from("pure ASCII\t[~]");
 
     assert_eq!(a, b);
 
     let a1 = a.clone();
-    let a2 = static_name!(c"pure ASCII\t[~]").clone();
+    let a2 = static_sname!(c"pure ASCII\t[~]").clone();
 
     assert_eq!(a, a1);
     assert_eq!(a1, a2);
 
-    let a = static_name!(c"\xB1").clone();
+    let a = static_sname!(c"\xB1").clone();
     let b = StringName::from("±");
 
     assert_eq!(a, b);
 
-    let a = static_name!(c"Latin-1 \xA3 \xB1 text \xBE").clone();
+    let a = static_sname!(c"Latin-1 \xA3 \xB1 text \xBE").clone();
     let b = StringName::from("Latin-1 £ ± text ¾");
 
     assert_eq!(a, b);