Remove by-value From conversions between strings

Rationale: there is no "move into" or "reuse buffer" optimization possible when
converting between String, GString, StringName and NodePath. However, consuming
the source string suggests that there is such a benefit.

Implementing From only on references makes this clearer and leaves the original
string intact for further use.

Author: Bromeon

godot-core/src/builtin/callable.rs

@@ -713,7 +713,7 @@ mod custom_callable {
         r_out: sys::GDExtensionStringPtr,
     ) {
         let c: &T = CallableUserdata::inner_from_raw(callable_userdata);
-        let s = crate::builtin::GString::from(c.to_string());
+        let s = GString::from(&c.to_string());
 
         s.move_into_string_ptr(r_out);
         *r_is_valid = sys::conv::SYS_TRUE;

godot-core/src/builtin/collections/array.rs

@@ -1334,7 +1334,7 @@ impl<T: ArrayElement> GodotType for Array<T> {
         // Typed arrays use type hint.
         PropertyHintInfo {
             hint: crate::global::PropertyHint::ARRAY_TYPE,
-            hint_string: GString::from(element_godot_type_name::<T>()),
+            hint_string: GString::from(&element_godot_type_name::<T>()),
         }
     }
 }

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

@@ -359,12 +359,6 @@ impl From<&[char]> for GString {
     }
 }
 
-impl From<String> for GString {
-    fn from(value: String) -> Self {
-        value.as_str().into()
-    }
-}
-
 impl From<&String> for GString {
     fn from(value: &String) -> Self {
         value.as_str().into()
@@ -424,15 +418,6 @@ impl From<&StringName> for GString {
     }
 }
 
-impl From<StringName> for GString {
-    /// Converts this `StringName` to a `GString`.
-    ///
-    /// This is identical to `GString::from(&string_name)`, and as such there is no performance benefit.
-    fn from(string_name: StringName) -> Self {
-        Self::from(&string_name)
-    }
-}
-
 impl From<&NodePath> for GString {
     fn from(path: &NodePath) -> Self {
         unsafe {
@@ -445,15 +430,6 @@ impl From<&NodePath> for GString {
     }
 }
 
-impl From<NodePath> for GString {
-    /// Converts this `NodePath` to a `GString`.
-    ///
-    /// This is identical to `GString::from(&path)`, and as such there is no performance benefit.
-    fn from(path: NodePath) -> Self {
-        Self::from(&path)
-    }
-}
-
 #[cfg(feature = "serde")]
 mod serialize {
     use std::fmt::Formatter;

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

@@ -233,20 +233,16 @@ impl fmt::Debug for NodePath {
 impl_rust_string_conv!(NodePath);
 
 impl From<&str> for NodePath {
+    // NodePath doesn't offer direct construction from bytes; go via GString.
     fn from(s: &str) -> Self {
-        GString::from(s).into()
-    }
-}
-
-impl From<String> for NodePath {
-    fn from(s: String) -> Self {
-        GString::from(s).into()
+        Self::from(&GString::from(s))
     }
 }
 
 impl From<&String> for NodePath {
     fn from(s: &String) -> Self {
-        GString::from(s).into()
+        // NodePath doesn't offer direct construction from bytes; go via GString.
+        Self::from(&GString::from(s))
     }
 }
 
@@ -262,27 +258,9 @@ impl From<&GString> for NodePath {
     }
 }
 
-impl From<GString> for NodePath {
-    /// Converts this `GString` to a `NodePath`.
-    ///
-    /// This is identical to `NodePath::from(&string)`, and as such there is no performance benefit.
-    fn from(string: GString) -> Self {
-        Self::from(&string)
-    }
-}
-
 impl From<&StringName> for NodePath {
-    fn from(string_name: &StringName) -> Self {
-        Self::from(GString::from(string_name))
-    }
-}
-
-impl From<StringName> for NodePath {
-    /// Converts this `StringName` to a `NodePath`.
-    ///
-    /// This is identical to `NodePath::from(&string_name)`, and as such there is no performance benefit.
-    fn from(string_name: StringName) -> Self {
-        Self::from(GString::from(string_name))
+    fn from(s: &StringName) -> Self {
+        Self::from(&GString::from(s))
     }
 }
 

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

@@ -126,7 +126,7 @@ impl StringName {
                 // This branch is short-circuited if invoked for CStr and Godot 4.2+, which uses `string_name_new_with_latin1_chars`
                 // (requires nul-termination). In general, fall back to GString conversion.
                 GString::try_from_bytes_with_nul_check(bytes, Encoding::Latin1, check_nul)
-                    .map(Self::from)
+                    .map(|s| Self::from(&s))
             }
             Encoding::Utf8 => {
                 // from_utf8() also checks for intermediate NUL bytes.
@@ -344,12 +344,6 @@ impl From<&str> for StringName {
     }
 }
 
-impl From<String> for StringName {
-    fn from(value: String) -> Self {
-        value.as_str().into()
-    }
-}
-
 impl From<&String> for StringName {
     fn from(value: &String) -> Self {
         value.as_str().into()
@@ -369,27 +363,9 @@ impl From<&GString> for StringName {
     }
 }
 
-impl From<GString> for StringName {
-    /// Converts this `GString` to a `StringName`.
-    ///
-    /// This is identical to `StringName::from(&string)`, and as such there is no performance benefit.
-    fn from(string: GString) -> Self {
-        Self::from(&string)
-    }
-}
-
 impl From<&NodePath> for StringName {
     fn from(path: &NodePath) -> Self {
-        Self::from(GString::from(path))
-    }
-}
-
-impl From<NodePath> for StringName {
-    /// Converts this `NodePath` to a `StringName`.
-    ///
-    /// This is identical to `StringName::from(&path)`, and as such there is no performance benefit.
-    fn from(path: NodePath) -> Self {
-        Self::from(GString::from(path))
+        Self::from(&GString::from(path))
     }
 }
 

godot-core/src/meta/property_info.rs

@@ -151,7 +151,7 @@ impl PropertyInfo {
     {
         self.variant_type == VariantType::ARRAY
             && self.hint_info.hint == PropertyHint::ARRAY_TYPE
-            && self.hint_info.hint_string == T::Via::godot_type_name().into()
+            && self.hint_info.hint_string == GString::from(&T::Via::godot_type_name())
     }
 
     // ------------------------------------------------------------------------------------------------------------------------------------------
@@ -291,7 +291,7 @@ impl PropertyHintInfo {
         let hint_string = if sys::GdextBuild::since_api("4.3") {
             GString::new()
         } else {
-            GString::from(type_name)
+            GString::from(&type_name)
         };
 
         Self {
@@ -304,23 +304,23 @@ impl PropertyHintInfo {
     pub fn var_array_element<T: ArrayElement>() -> Self {
         Self {
             hint: PropertyHint::ARRAY_TYPE,
-            hint_string: GString::from(element_godot_type_name::<T>()),
+            hint_string: GString::from(&element_godot_type_name::<T>()),
         }
     }
 
     /// Use for `#[export]` properties -- [`PROPERTY_HINT_TYPE_STRING`](PropertyHint::TYPE_STRING) with the **element** type string as hint string.
     pub fn export_array_element<T: ArrayElement>() -> Self {
         Self {
             hint: PropertyHint::TYPE_STRING,
-            hint_string: GString::from(T::element_type_string()),
+            hint_string: GString::from(&T::element_type_string()),
         }
     }
 
     /// Use for `#[export]` properties -- [`PROPERTY_HINT_TYPE_STRING`](PropertyHint::TYPE_STRING) with the **element** type string as hint string.
     pub fn export_packed_array_element<T: PackedArrayElement>() -> Self {
         Self {
             hint: PropertyHint::TYPE_STRING,
-            hint_string: GString::from(T::element_type_string()),
+            hint_string: GString::from(&T::element_type_string()),
         }
     }
 
@@ -349,7 +349,7 @@ impl PropertyHintInfo {
         D: ?Sized + 'static,
     {
         PropertyHintInfo {
-            hint_string: GString::from(get_dyn_property_hint_string::<T, D>()),
+            hint_string: GString::from(&get_dyn_property_hint_string::<T, D>()),
             ..PropertyHintInfo::export_gd::<T>()
         }
     }

godot-core/src/registry/property/mod.rs

@@ -315,7 +315,7 @@ pub mod export_info_functions {
 
         PropertyHintInfo {
             hint: PropertyHint::RANGE,
-            hint_string: hint_string.into(),
+            hint_string: GString::from(&hint_string),
         }
     }
 
@@ -381,7 +381,7 @@ pub mod export_info_functions {
 
         PropertyHintInfo {
             hint: PropertyHint::ENUM,
-            hint_string: hint_string.into(),
+            hint_string: GString::from(&hint_string),
         }
     }
 
@@ -390,7 +390,7 @@ pub mod export_info_functions {
 
         PropertyHintInfo {
             hint: PropertyHint::EXP_EASING,
-            hint_string: hint_string.into(),
+            hint_string: GString::from(&hint_string),
         }
     }
 
@@ -414,7 +414,7 @@ pub mod export_info_functions {
 
         PropertyHintInfo {
             hint: PropertyHint::FLAGS,
-            hint_string: hint_string.into(),
+            hint_string: GString::from(&hint_string),
         }
     }
 
@@ -493,7 +493,7 @@ pub mod export_info_functions {
 
         PropertyHintInfo {
             hint: PropertyHint::TYPE_STRING,
-            hint_string: format!("{hint_string}:{filter}").into(),
+            hint_string: GString::from(&format!("{hint_string}:{filter}")),
         }
     }
 

godot-core/src/storage/instance_storage.rs

@@ -217,7 +217,7 @@ pub unsafe fn destroy_storage<T: GodotClass>(instance_ptr: sys::GDExtensionClass
         // In Debug mode, crash which may trigger breakpoint.
         // In Release mode, leak player object (Godot philosophy: don't crash if somehow avoidable). Likely leads to follow-up issues.
         if cfg!(debug_assertions) {
-            let error = crate::builtin::GString::from(error);
+            let error = crate::builtin::GString::from(&error);
             crate::classes::Os::singleton().crash(&error);
         } else {
             leak_rust_object = true;

godot-macros/src/derive/data_models/c_style_enum.rs

@@ -115,7 +115,8 @@ impl CStyleEnum {
         }
 
         quote! {
-            format!(#fmt, #(#fmt_args),*)
+            // & because it is passed to GString::from().
+            &format!(#fmt, #(#fmt_args),*)
         }
     }
 

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

@@ -28,11 +28,6 @@ fn string_conversion() {
     let back = String::from(&second);
 
     assert_eq!(string, back);
-
-    let second = GString::from(string.clone());
-    let back = String::from(second);
-
-    assert_eq!(string, back);
 }
 
 #[itest]
@@ -75,7 +70,7 @@ fn string_chars() {
 
     let string = String::from("ö🍎A💡");
     let string_chars: Vec<char> = string.chars().collect();
-    let gstring = GString::from(string);
+    let gstring = GString::from(&string);
 
     assert_eq!(gstring.chars(), string_chars.as_slice());
     assert_eq!(