Fix bug causing TypeError when converting Rust encoding errors to Python

Author: staticintlucas

guide/src/migration.md

@@ -70,6 +70,37 @@ This should not require migration, nor is there expected to be breakage caused b
 
 Nevertheless, this affects the order of initialization so seemed worth noting in this guide.
 
+### Removed implementations of `From<str::Utf8Error>`, `From<string::FromUtf16Error>`, and `From<char::DecodeUtf16Error>` for `PyErr`
+
+Previously the implementations of `From<string::FromUtf8Error>`, `From<ffi::IntoStringError>`, `From<str::Utf8Error>`, `From<string::FromUtf16Error>`, and `From<char::DecodeUtf16Error>` failed to construct the correct Python exception class, as reported in <https://github.com/PyO3/pyo3/issues/5651>.
+The implementations for `string::FromUtf8Error` and `ffi::IntoStringError` were fixed in this release.
+
+For `str::Utf8Error`, the Rust error does not contain the source bytes required to construct the Python exception.
+Instead, `PyUnicodeDecodeError::new_err_from_utf8` can be used to convert the error to a `PyErr`.
+
+Before:
+
+```rust,ignore
+fn bytes_to_str(bytes: &[u8]) -> PyResult<&str> {
+    Ok(str::from_utf8(bytes)?)
+}
+```
+
+After:
+
+```rust
+# use pyo3::prelude::*;
+use pyo3::exceptions::PyUnicodeDecodeError;
+
+# #[expect(dead_code)]
+fn bytes_to_str(bytes: &[u8]) -> PyResult<&str> {
+    str::from_utf8(bytes).map_err(|e| PyUnicodeDecodeError::new_err_from_utf8(bytes, e))
+}
+```
+
+For `string::FromUtf16Error` and `char::DecodeUtf16Error` the Rust error types do not contain any of the information required to construct a `UnicodeDecodeError`.
+To raise a Python `UnicodeDecodeError` a new error should be manually constructed by calling `PyUnicodeDecodeError::new_err(...)`.
+
 ## from 0.26.* to 0.27
 
 ### `FromPyObject` reworked for flexibility and efficiency
@@ -1132,7 +1163,7 @@ let obj: Py<PyList> = bound.unbind();
 
 > [!WARNING]
 > Dangling pointer trap 💣
-> 
+>
 > Because of the ownership changes, code which uses `.as_ptr()` to convert `&PyAny` and other GIL Refs to a `*mut pyo3_ffi::PyObject` should take care to avoid creating dangling pointers now that `Bound<PyAny>` carries ownership.
 >
 > For example, the following pattern with `Option<&PyAny>` can easily create a dangling pointer when migrating to the `Bound<PyAny>` smart pointer:

newsfragments/5668.fixed.md

@@ -0,0 +1,2 @@
+Fixed the implementations of `From<string::FromUtf8Error>` and `From<ffi::IntoStringError>` for `PyErr`.
+Removed the failing implementations of `From<str::Utf8Error>`, `From<string::FromUtf16Error>`, and `From<char::DecodeUtf16Error>` for `PyErr`.

src/conversions/std/cstring.rs

@@ -1,3 +1,4 @@
+use crate::exceptions::PyUnicodeDecodeError;
 #[cfg(feature = "experimental-inspect")]
 use crate::inspect::TypeHint;
 #[cfg(feature = "experimental-inspect")]
@@ -6,7 +7,6 @@ use crate::types::PyString;
 use crate::{Borrowed, Bound, FromPyObject, IntoPyObject, PyAny, PyErr, Python};
 use std::borrow::Cow;
 use std::ffi::{CStr, CString};
-use std::str::Utf8Error;
 #[cfg(any(Py_3_10, not(Py_LIMITED_API)))]
 use {
     crate::{exceptions::PyValueError, ffi},
@@ -16,21 +16,24 @@ use {
 impl<'py> IntoPyObject<'py> for &CStr {
     type Target = PyString;
     type Output = Bound<'py, Self::Target>;
-    type Error = Utf8Error;
+    type Error = PyErr;
 
     #[cfg(feature = "experimental-inspect")]
     const OUTPUT_TYPE: TypeHint = <&str>::OUTPUT_TYPE;
 
     #[inline]
     fn into_pyobject(self, py: Python<'py>) -> Result<Self::Output, Self::Error> {
-        self.to_str()?.into_pyobject(py).map_err(|err| match err {})
+        self.to_str()
+            .map_err(|e| PyUnicodeDecodeError::new_err_from_utf8(self.to_bytes(), e))?
+            .into_pyobject(py)
+            .map_err(|err| match err {})
     }
 }
 
 impl<'py> IntoPyObject<'py> for CString {
     type Target = PyString;
     type Output = Bound<'py, Self::Target>;
-    type Error = Utf8Error;
+    type Error = PyErr;
 
     #[cfg(feature = "experimental-inspect")]
     const OUTPUT_TYPE: TypeHint = <&CStr>::OUTPUT_TYPE;
@@ -44,7 +47,7 @@ impl<'py> IntoPyObject<'py> for CString {
 impl<'py> IntoPyObject<'py> for &CString {
     type Target = PyString;
     type Output = Bound<'py, Self::Target>;
-    type Error = Utf8Error;
+    type Error = PyErr;
 
     #[cfg(feature = "experimental-inspect")]
     const OUTPUT_TYPE: TypeHint = <&CStr>::OUTPUT_TYPE;
@@ -58,7 +61,7 @@ impl<'py> IntoPyObject<'py> for &CString {
 impl<'py> IntoPyObject<'py> for Cow<'_, CStr> {
     type Target = PyString;
     type Output = Bound<'py, Self::Target>;
-    type Error = Utf8Error;
+    type Error = PyErr;
 
     #[cfg(feature = "experimental-inspect")]
     const OUTPUT_TYPE: TypeHint = <&CStr>::OUTPUT_TYPE;
@@ -71,7 +74,7 @@ impl<'py> IntoPyObject<'py> for Cow<'_, CStr> {
 impl<'py> IntoPyObject<'py> for &Cow<'_, CStr> {
     type Target = PyString;
     type Output = Bound<'py, Self::Target>;
-    type Error = Utf8Error;
+    type Error = PyErr;
 
     #[cfg(feature = "experimental-inspect")]
     const OUTPUT_TYPE: TypeHint = <&CStr>::OUTPUT_TYPE;

src/conversions/std/osstr.rs

@@ -1,4 +1,6 @@
 use crate::conversion::IntoPyObject;
+#[cfg(target_os = "wasi")]
+use crate::exceptions::PyUnicodeDecodeError;
 use crate::ffi_ptr_ext::FfiPtrExt;
 #[cfg(feature = "experimental-inspect")]
 use crate::inspect::TypeHint;
@@ -103,7 +105,11 @@ impl FromPyObject<'_, '_> for OsString {
             //
             // For WASI: OS strings are UTF-8 by definition.
             #[cfg(target_os = "wasi")]
-            let os_str: &OsStr = OsStr::new(std::str::from_utf8(fs_encoded_bytes.as_bytes())?);
+            let os_str: &OsStr = OsStr::new(
+                std::str::from_utf8(fs_encoded_bytes.as_bytes()).map_err(|e| {
+                    PyUnicodeDecodeError::new_err_from_utf8(fs_encoded_bytes.as_bytes(), e)
+                })?,
+            );
             #[cfg(not(target_os = "wasi"))]
             let os_str: &OsStr =
                 std::os::unix::ffi::OsStrExt::from_bytes(fs_encoded_bytes.as_bytes());

src/err/impls.rs

@@ -1,4 +1,4 @@
-use crate::{err::PyErrArguments, exceptions, PyErr, Python};
+use crate::{err::PyErrArguments, exceptions, types, PyErr, Python};
 use crate::{IntoPyObject, Py, PyAny};
 use std::io;
 
@@ -118,23 +118,70 @@ macro_rules! impl_to_pyerr {
     };
 }
 
+impl PyErrArguments for exceptions::Utf8ErrorWithBytes {
+    fn arguments(self, py: Python<'_>) -> Py<PyAny> {
+        let Self { err, bytes } = self;
+        let start = err.valid_up_to();
+        let end = err.error_len().map_or(bytes.len(), |l| start + l);
+
+        let encoding = types::PyString::new(py, "utf-8").into_any();
+        let bytes = types::PyBytes::new(py, &bytes).into_any();
+        let start = types::PyInt::new(py, start).into_any();
+        let end = types::PyInt::new(py, end).into_any();
+        let reason = types::PyString::new(py, "invalid utf-8").into_any();
+
+        // FIXME(icxolu) remove unwrap
+        types::PyTuple::new(py, &[encoding, bytes, start, end, reason])
+            .unwrap()
+            .into_any()
+            .unbind()
+    }
+}
+
+impl std::convert::From<exceptions::Utf8ErrorWithBytes> for PyErr {
+    fn from(err: exceptions::Utf8ErrorWithBytes) -> PyErr {
+        exceptions::PyUnicodeDecodeError::new_err(err)
+    }
+}
+
+impl PyErrArguments for std::string::FromUtf8Error {
+    fn arguments(self, py: Python<'_>) -> Py<PyAny> {
+        exceptions::Utf8ErrorWithBytes {
+            err: self.utf8_error(),
+            bytes: self.into_bytes(),
+        }
+        .arguments(py)
+    }
+}
+
+impl std::convert::From<std::string::FromUtf8Error> for PyErr {
+    fn from(err: std::string::FromUtf8Error) -> PyErr {
+        exceptions::PyUnicodeDecodeError::new_err(err)
+    }
+}
+
+impl PyErrArguments for std::ffi::IntoStringError {
+    fn arguments(self, py: Python<'_>) -> Py<PyAny> {
+        exceptions::Utf8ErrorWithBytes {
+            err: self.utf8_error(),
+            bytes: self.into_cstring().into_bytes(),
+        }
+        .arguments(py)
+    }
+}
+
+impl std::convert::From<std::ffi::IntoStringError> for PyErr {
+    fn from(err: std::ffi::IntoStringError) -> PyErr {
+        exceptions::PyUnicodeDecodeError::new_err(err)
+    }
+}
+
 impl_to_pyerr!(std::array::TryFromSliceError, exceptions::PyValueError);
 impl_to_pyerr!(std::num::ParseIntError, exceptions::PyValueError);
 impl_to_pyerr!(std::num::ParseFloatError, exceptions::PyValueError);
 impl_to_pyerr!(std::num::TryFromIntError, exceptions::PyValueError);
 impl_to_pyerr!(std::str::ParseBoolError, exceptions::PyValueError);
-impl_to_pyerr!(std::ffi::IntoStringError, exceptions::PyUnicodeDecodeError);
 impl_to_pyerr!(std::ffi::NulError, exceptions::PyValueError);
-impl_to_pyerr!(std::str::Utf8Error, exceptions::PyUnicodeDecodeError);
-impl_to_pyerr!(std::string::FromUtf8Error, exceptions::PyUnicodeDecodeError);
-impl_to_pyerr!(
-    std::string::FromUtf16Error,
-    exceptions::PyUnicodeDecodeError
-);
-impl_to_pyerr!(
-    std::char::DecodeUtf16Error,
-    exceptions::PyUnicodeDecodeError
-);
 impl_to_pyerr!(std::net::AddrParseError, exceptions::PyValueError);
 
 #[cfg(test)]

src/exceptions.rs

@@ -703,6 +703,11 @@ impl_windows_native_exception!(
     native_doc!("WindowsError")
 );
 
+pub(crate) struct Utf8ErrorWithBytes {
+    pub(crate) err: std::str::Utf8Error,
+    pub(crate) bytes: Vec<u8>,
+}
+
 impl PyUnicodeDecodeError {
     /// Creates a Python `UnicodeDecodeError`.
     pub fn new<'py>(
@@ -754,8 +759,30 @@ impl PyUnicodeDecodeError {
         input: &[u8],
         err: std::str::Utf8Error,
     ) -> PyResult<Bound<'py, PyUnicodeDecodeError>> {
-        let pos = err.valid_up_to();
-        PyUnicodeDecodeError::new(py, c"utf-8", input, pos..(pos + 1), c"invalid utf-8")
+        let start = err.valid_up_to();
+        let end = err.error_len().map_or(input.len(), |l| start + l);
+        PyUnicodeDecodeError::new(py, c"utf-8", input, start..end, c"invalid utf-8")
+    }
+
+    /// Create a new [`crate::PyErr`] of this type from a Rust UTF-8 decoding error.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// use pyo3::prelude::*;
+    /// use pyo3::exceptions::PyUnicodeDecodeError;
+    ///
+    /// let invalid_utf8 = b"fo\xd8o";
+    /// # #[expect(invalid_from_utf8)]
+    /// let err = std::str::from_utf8(invalid_utf8).expect_err("should be invalid utf8");
+    /// let py_err = PyUnicodeDecodeError::new_err_from_utf8(invalid_utf8, err);
+    /// ```
+    pub fn new_err_from_utf8(bytes: &[u8], err: std::str::Utf8Error) -> crate::PyErr {
+        Utf8ErrorWithBytes {
+            err,
+            bytes: bytes.to_vec(),
+        }
+        .into()
     }
 }
 

src/impl_/pymodule.rs

@@ -24,6 +24,7 @@ use std::sync::atomic::{AtomicI64, Ordering};
 
 #[cfg(not(any(PyPy, GraalPy)))]
 use crate::exceptions::PyImportError;
+use crate::exceptions::PyUnicodeDecodeError;
 use crate::prelude::PyTypeMethods;
 use crate::{
     ffi,
@@ -152,7 +153,11 @@ impl ModuleDef {
 
         let ffi_def = self.ffi_def.get();
 
-        let name = unsafe { CStr::from_ptr((*ffi_def).m_name).to_str()? }.to_string();
+        let m_name = unsafe { CStr::from_ptr((*ffi_def).m_name) };
+        let name = m_name
+            .to_str()
+            .map_err(|e| PyUnicodeDecodeError::new_err_from_utf8(m_name.to_bytes(), e))?
+            .to_string();
         let kwargs = PyDict::new(py);
         kwargs.set_item("name", name)?;
         let spec = simple_ns.call((), Some(&kwargs))?;

tests/ui/invalid_result_conversion.stderr

@@ -8,10 +8,10 @@ error[E0277]: the trait bound `PyErr: From<MyError>` is not satisfied
              `PyErr` implements `From<AddrParseError>`
              `PyErr` implements `From<CastError<'_, '_>>`
              `PyErr` implements `From<CastIntoError<'_>>`
-             `PyErr` implements `From<DecodeUtf16Error>`
              `PyErr` implements `From<DowncastError<'_, '_>>`
              `PyErr` implements `From<DowncastIntoError<'_>>`
-             `PyErr` implements `From<FromUtf16Error>`
              `PyErr` implements `From<FromUtf8Error>`
+             `PyErr` implements `From<Infallible>`
+             `PyErr` implements `From<IntoInnerError<W>>`
            and $N others
    = note: required for `MyError` to implement `Into<PyErr>`