String encoding: change Option<T> to Result<T, StringError>

Author: Bromeon

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

@@ -15,6 +15,7 @@ use sys::{ffi_methods, interface_fn, GodotFfi};
 
 use crate::builtin::string::Encoding;
 use crate::builtin::{inner, NodePath, StringName, Variant};
+use crate::meta::error::StringError;
 use crate::meta::AsArg;
 use crate::{impl_shared_string_api, meta};
 
@@ -88,21 +89,23 @@ impl GString {
     /// - **UTF-8**: The input is validated to be UTF-8.
     ///
     /// Specifying incorrect encoding is safe, but may result in unintended string values.
-    pub fn try_from_bytes(bytes: &[u8], encoding: Encoding) -> Option<Self> {
+    pub fn try_from_bytes(bytes: &[u8], encoding: Encoding) -> Result<Self, StringError> {
         match encoding {
             Encoding::Ascii => {
                 // If the bytes are ASCII, we can fall back to Latin-1, which is always valid (except for NUL).
                 // is_ascii() does *not* check for the NUL byte, so the check in the Latin-1 branch is still necessary.
                 if bytes.is_ascii() {
                     Self::try_from_bytes(bytes, Encoding::Latin1)
+                        .map_err(|_e| StringError::new("intermediate NUL byte in ASCII string"))
                 } else {
-                    None
+                    Err(StringError::new("invalid ASCII"))
                 }
             }
             Encoding::Latin1 => {
                 // Intermediate NUL bytes are not accepted in Godot. Both ASCII + Latin-1 encodings need to explicitly check for this.
                 if bytes.contains(&0) {
-                    return None;
+                    // Error overwritten when called from ASCII branch.
+                    return Err(StringError::new("intermediate NUL byte in Latin-1 string"));
                 }
 
                 let s = unsafe {
@@ -115,20 +118,22 @@ impl GString {
                         );
                     })
                 };
-                Some(s)
+                Ok(s)
             }
             Encoding::Utf8 => {
                 // from_utf8() also checks for intermediate NUL bytes.
                 let utf8 = std::str::from_utf8(bytes);
-                utf8.ok().map(GString::from)
+
+                utf8.map(GString::from)
+                    .map_err(|e| StringError::with_source("invalid UTF-8", e))
             }
         }
     }
 
-    /// Convert string from C-string with given encoding, returning `None` on validation errors.
+    /// Convert string from C-string with given encoding, returning `Err` on validation errors.
     ///
     /// Convenience function for [`try_from_bytes()`](Self::try_from_bytes); see its docs for more information.
-    pub fn try_from_cstr(cstr: &std::ffi::CStr, encoding: Encoding) -> Option<Self> {
+    pub fn try_from_cstr(cstr: &std::ffi::CStr, encoding: Encoding) -> Result<Self, StringError> {
         Self::try_from_bytes(cstr.to_bytes(), encoding)
     }
 

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

@@ -4,14 +4,14 @@
  * 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, GodotFfi};
 
 use crate::builtin::{inner, Encoding, GString, NodePath, Variant};
+use crate::meta::error::StringError;
 use crate::meta::AsArg;
 use crate::{impl_shared_string_api, meta};
 
@@ -72,16 +72,18 @@ impl StringName {
     /// - **UTF-8**: The input is validated to be UTF-8.
     ///
     /// Specifying incorrect encoding is safe, but may result in unintended string values.
-    pub fn try_from_bytes(bytes: &[u8], encoding: Encoding) -> Option<Self> {
+    pub fn try_from_bytes(bytes: &[u8], encoding: Encoding) -> Result<Self, StringError> {
         match encoding {
             Encoding::Ascii => {
                 // ASCII is a subset of UTF-8, and UTF-8 has a more direct implementation than Latin-1; thus use UTF-8 via `From<&str>`.
-                if bytes.is_ascii() && !bytes.contains(&0) {
-                    // SAFETY: ASCII is a subset of UTF-8 and was just verified.
-                    let ascii = unsafe { std::str::from_utf8_unchecked(bytes) };
-                    Some(Self::from(ascii))
+                if !bytes.is_ascii() {
+                    Err(StringError::new("invalid ASCII"))
+                } else if bytes.contains(&0) {
+                    Err(StringError::new("intermediate NUL byte in ASCII string"))
                 } else {
-                    None
+                    // SAFETY: ASCII is a subset of UTF-8 and was verified above.
+                    let ascii = unsafe { std::str::from_utf8_unchecked(bytes) };
+                    Ok(Self::from(ascii))
                 }
             }
             Encoding::Latin1 => {
@@ -92,17 +94,19 @@ impl StringName {
             Encoding::Utf8 => {
                 // from_utf8() also checks for intermediate NUL bytes.
                 let utf8 = std::str::from_utf8(bytes);
-                utf8.ok().map(StringName::from)
+
+                utf8.map(StringName::from)
+                    .map_err(|e| StringError::with_source("invalid UTF-8", e))
             }
         }
     }
 
-    /// Convert string from bytes with given encoding, returning `None` on validation errors.
+    /// Convert string from bytes with given encoding, returning `Err` on validation errors.
     ///
     /// Convenience function for [`try_from_bytes()`](Self::try_from_bytes); see its docs for more information.
     ///
     /// When called with `Encoding::Latin1`, this can be slightly more efficient than `try_from_bytes()`.
-    pub fn try_from_cstr(cstr: &std::ffi::CStr, encoding: Encoding) -> Option<Self> {
+    pub fn try_from_cstr(cstr: &std::ffi::CStr, encoding: Encoding) -> Result<Self, StringError> {
         // Short-circuit the direct Godot 4.2 function for Latin-1, which takes a null-terminated C string.
         #[cfg(since_api = "4.2")]
         if encoding == Encoding::Latin1 {
@@ -119,7 +123,7 @@ impl StringName {
                     );
                 })
             };
-            return Some(s);
+            return Ok(s);
         }
 
         Self::try_from_bytes(cstr.to_bytes(), encoding)

godot-core/src/meta/error/mod.rs

@@ -10,7 +10,9 @@
 mod call_error;
 mod convert_error;
 mod io_error;
+mod string_error;
 
 pub use call_error::*;
 pub use convert_error::*;
 pub use io_error::*;
+pub use string_error::*;

godot-core/src/meta/error/string_error.rs

@@ -0,0 +1,51 @@
+/*
+ * Copyright (c) godot-rust; Bromeon and contributors.
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * 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::error::Error;
+use std::fmt;
+
+/// Error related to string encoding/decoding.
+#[derive(Debug)]
+pub struct StringError {
+    message: String,
+    source: Option<Box<(dyn Error + 'static)>>,
+}
+
+impl fmt::Display for StringError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        if let Some(source) = self.source() {
+            write!(f, "{}: {}", self.message, source)
+        } else {
+            write!(f, "{}", self.message)
+        }
+    }
+}
+
+impl Error for StringError {
+    fn source(&self) -> Option<&(dyn Error + 'static)> {
+        self.source.as_deref()
+    }
+}
+
+impl StringError {
+    pub(crate) fn new(message: impl Into<String>) -> Self {
+        Self {
+            message: message.into(),
+            source: None,
+        }
+    }
+
+    pub(crate) fn with_source(
+        message: impl Into<String>,
+        source: impl Into<Box<(dyn Error + 'static)>>,
+    ) -> Self {
+        Self {
+            message: message.into(),
+            source: Some(source.into()),
+        }
+    }
+}

itest/rust/src/builtin_tests/mod.rs

@@ -35,10 +35,10 @@ mod containers {
 }
 
 mod string {
-    mod string_test_macros;
     mod gstring_test;
     mod node_path_test;
     mod string_name_test;
+    mod string_test_macros;
 }
 
 mod script {

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

@@ -27,14 +27,20 @@ macro_rules! generate_string_bytes_and_cstr_tests {
             assert_eq!(ascii.len(), 5);
 
             let ascii_nul = <$T>::try_from_bytes(b"Hello\0", Encoding::Ascii);
-            assert_eq!(ascii_nul, None, "intermediate NUL byte is not valid ASCII"); // at end, but still not NUL terminator.
+            let ascii_nul = ascii_nul.expect_err("intermediate NUL byte is not valid ASCII"); // at end, but still not NUL terminator.
+            assert_eq!(
+                ascii_nul.to_string(),
+                "intermediate NUL byte in ASCII string"
+            );
 
             let latin1 = <$T>::try_from_bytes(b"/\xF0\xF5\xBE", Encoding::Ascii);
-            assert_eq!(latin1, None, "Latin-1 is *not* valid ASCII");
+            let latin1 = latin1.expect_err("Latin-1 is *not* valid ASCII");
+            assert_eq!(latin1.to_string(), "invalid ASCII");
 
             let utf8 =
                 <$T>::try_from_bytes(b"\xF6\xF0\x9F\x8D\x8E\xF0\x9F\x92\xA1", Encoding::Ascii);
-            assert_eq!(utf8, None, "UTF-8 is *not* valid ASCII");
+            let utf8 = utf8.expect_err("UTF-8 is *not* valid ASCII");
+            assert_eq!(utf8.to_string(), "invalid ASCII");
         }
 
         #[itest]
@@ -45,10 +51,12 @@ macro_rules! generate_string_bytes_and_cstr_tests {
             assert_eq!(ascii.len(), 5);
 
             let latin1 = <$T>::try_from_cstr(c"/ðõ¾", Encoding::Ascii);
-            assert_eq!(latin1, None, "Latin-1 is *not* valid ASCII");
+            let latin1 = latin1.expect_err("Latin-1 is *not* valid ASCII");
+            assert_eq!(latin1.to_string(), "invalid ASCII");
 
             let utf8 = <$T>::try_from_cstr(c"ö🍎A💡", Encoding::Ascii);
-            assert_eq!(utf8, None, "UTF-8 is *not* valid ASCII");
+            let utf8 = utf8.expect_err("UTF-8 is *not* valid ASCII");
+            assert_eq!(utf8.to_string(), "invalid ASCII");
         }
 
         #[itest]
@@ -64,9 +72,10 @@ macro_rules! generate_string_bytes_and_cstr_tests {
             assert_eq!(latin1.len(), 4);
 
             let latin1_nul = <$T>::try_from_bytes(b"/\0\xF0\xF5\xBE", Encoding::Latin1);
+            let latin1_nul = latin1_nul.expect_err("intermediate NUL byte is not valid Latin-1");
             assert_eq!(
-                latin1_nul, None,
-                "intermediate NUL byte is not valid Latin-1"
+                latin1_nul.to_string(),
+                "intermediate NUL byte in Latin-1 string"
             );
 
             // UTF-8 -> Latin-1: always succeeds, even if result is garbage, since every byte is a valid Latin-1 character.
@@ -107,7 +116,12 @@ macro_rules! generate_string_bytes_and_cstr_tests {
             assert_eq!(ascii.len(), 5);
 
             let latin1 = <$T>::try_from_bytes(b"/\xF0\xF5\xBE", Encoding::Utf8);
-            assert_eq!(latin1, None, "Latin-1 is *not* valid UTF-8");
+            let latin1 = latin1.expect_err("Latin-1 is *not* valid UTF-8");
+            // Note: depends on exact output of std's Utf8Error; might need format!() if that changes.
+            assert_eq!(
+                latin1.to_string(),
+                "invalid UTF-8: invalid utf-8 sequence of 1 bytes from index 1"
+            );
 
             let utf8 = <$T>::try_from_bytes(
                 b"\xC3\xB6\xF0\x9F\x8D\x8E\x41\xF0\x9F\x92\xA1",
@@ -118,7 +132,11 @@ macro_rules! generate_string_bytes_and_cstr_tests {
             assert_eq!(utf8.len(), 4);
 
             let utf8_nul = <$T>::try_from_bytes(b"\xC3\0A", Encoding::Utf8);
-            assert_eq!(utf8_nul, None, "intermediate NUL byte is not valid UTF-8");
+            let utf8_nul = utf8_nul.expect_err("intermediate NUL byte is not valid UTF-8");
+            assert_eq!(
+                utf8_nul.to_string(),
+                "invalid UTF-8: invalid utf-8 sequence of 1 bytes from index 0"
+            );
         }
 
         #[itest]