From c456dcf358662ac13a40f9ddf0e8b716243b3f2e Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Wed, 20 May 2026 16:04:46 +0200 Subject: [PATCH 1/5] fix type confusion when different `#[pyclass]` types returned from `#[new]` --- pyo3-macros-backend/src/method.rs | 1 + pyo3-macros-backend/src/pymethod.rs | 19 +-- pyo3-macros-backend/src/utils.rs | 1 + src/impl_/pyclass.rs | 6 +- src/impl_/pyclass_init.rs | 174 +------------------------- src/impl_/pymethods.rs | 27 ++-- src/impl_/wrap.rs | 47 +++++++ src/internal.rs | 1 + src/internal/pyclass_init.rs | 184 ++++++++++++++++++++++++++++ src/pyclass_init.rs | 3 +- tests/test_class_init.rs | 24 ++++ tests/test_compile_error.rs | 1 + tests/ui/invalid_pyclass_new.rs | 16 +++ tests/ui/invalid_pyclass_new.stderr | 32 +++++ 14 files changed, 340 insertions(+), 196 deletions(-) create mode 100644 src/internal/pyclass_init.rs create mode 100644 tests/ui/invalid_pyclass_new.rs create mode 100644 tests/ui/invalid_pyclass_new.stderr diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index 3ec89dc08ca..87574c74b8a 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -449,6 +449,7 @@ pub struct FnSpec<'a> { pub asyncness: Option, pub unsafety: Option, pub warnings: Vec, + #[cfg_attr(not(feature = "experimental-inspect"), expect(dead_code))] pub output: syn::ReturnType, } diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index d82bc2241bc..7b62341b456 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -16,8 +16,8 @@ use crate::{ use crate::{quotes, utils}; use proc_macro2::{Span, TokenStream}; use quote::{format_ident, quote, quote_spanned, ToTokens}; +use syn::LitCStr; use syn::{ext::IdentExt, spanned::Spanned, Field, Ident, Result}; -use syn::{parse_quote, LitCStr}; /// Generated code for a single pymethod item. pub struct MethodAndMethodDef { @@ -1473,25 +1473,16 @@ fn generate_method_body( } }); - let output = if let syn::ReturnType::Type(_, ty) = &spec.output { - let mut ty = ty.clone(); - utils::elide_lifetimes(&mut ty); - ty - } else { - parse_quote!(()) - }; - let body = quote! { + let body = quote_spanned! { *output_span => #text_signature_impl use #pyo3_path::impl_::pyclass::Probe as _; #warnings #arg_convert let result = #call; - #pyo3_path::impl_::pymethods::tp_new_impl::< - _, - { #pyo3_path::impl_::pyclass::IsPyClass::<#output>::VALUE }, - { #pyo3_path::impl_::pyclass::IsInitializerTuple::<#output>::VALUE } - >(py, result, _slf) + let value = #pyo3_path::impl_::wrap::OkWrapper::new(&result).ok_wrap(result)?; + let initializer = #pyo3_path::impl_::pymethods::tp_new_resolver::<#cls, _>(&value).resolve(value); + #pyo3_path::impl_::pymethods::tp_new_impl::<_, #cls>(py, initializer, _slf) }; (arg_idents, arg_types, body) } diff --git a/pyo3-macros-backend/src/utils.rs b/pyo3-macros-backend/src/utils.rs index 56e07aaf8d7..eacc9f71c0f 100644 --- a/pyo3-macros-backend/src/utils.rs +++ b/pyo3-macros-backend/src/utils.rs @@ -380,6 +380,7 @@ pub(crate) fn locate_tokens_at(tokens: TokenStream, span: Span) -> TokenStream { /// /// This is useful if `Self` is used in `const` context, where explicit /// lifetimes are not allowed (yet). +#[cfg(feature = "experimental-inspect")] pub(crate) fn elide_lifetimes(ty: &mut syn::Type) { struct ElideLifetimesVisitor; diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index 253064f27d9..f6475d7766e 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -5,7 +5,6 @@ use crate::{ impl_::{ freelist::PyObjectFreeList, pycell::{GetBorrowChecker, PyClassMutability, PyClassObjectBaseLayout}, - pyclass_init::PyObjectInit, pymethods::{PyGetterDef, PyMethodDefType}, }, pycell::{impl_::PyClassObjectLayout, PyBorrowError}, @@ -1096,6 +1095,11 @@ pub trait PyClassBaseType: Sized { type Layout; } +/// Constraint on base types available within PyO3 +#[expect(private_bounds, reason = "internal trait implementation")] +pub trait PyObjectInit: crate::internal::pyclass_init::PyObjectInit {} +impl PyObjectInit for I where I: crate::internal::pyclass_init::PyObjectInit {} + /// Implementation of tp_dealloc for pyclasses without gc pub(crate) unsafe extern "C" fn tp_dealloc(obj: *mut ffi::PyObject) { unsafe { crate::impl_::trampoline::dealloc(obj, ::Layout::tp_dealloc) } diff --git a/src/impl_/pyclass_init.rs b/src/impl_/pyclass_init.rs index ce2845f9013..eb6bec379d1 100644 --- a/src/impl_/pyclass_init.rs +++ b/src/impl_/pyclass_init.rs @@ -1,172 +1,2 @@ -//! Contains initialization utilities for `#[pyclass]`. -use crate::exceptions::PyTypeError; -use crate::ffi_ptr_ext::FfiPtrExt; -use crate::impl_::pyclass::PyClassBaseType; -use crate::internal::get_slot::TP_NEW; -use crate::types::{PyTuple, PyType}; -use crate::{ffi, PyClass, PyClassInitializer, PyErr, PyResult, Python}; -use crate::{ffi::PyTypeObject, sealed::Sealed, type_object::PyTypeInfo}; -use core::marker::PhantomData; - -/// Initializer for Python types. -/// -/// This trait is intended to use internally for distinguishing `#[pyclass]` and -/// Python native types. -pub trait PyObjectInit: Sized + Sealed { - /// # Safety - /// - `subtype` must be a valid pointer to a type object of T or a subclass. - unsafe fn into_new_object( - self, - py: Python<'_>, - subtype: *mut PyTypeObject, - ) -> PyResult<*mut ffi::PyObject>; -} - -/// Initializer for Python native types, like `PyDict`. -pub struct PyNativeTypeInitializer(pub PhantomData); - -impl PyObjectInit for PyNativeTypeInitializer { - unsafe fn into_new_object( - self, - py: Python<'_>, - subtype: *mut PyTypeObject, - ) -> PyResult<*mut ffi::PyObject> { - unsafe fn inner( - py: Python<'_>, - type_ptr: *mut PyTypeObject, - subtype: *mut PyTypeObject, - ) -> PyResult<*mut ffi::PyObject> { - let tp_new = unsafe { - type_ptr - .cast::() - .assume_borrowed_unchecked(py) - .cast_unchecked::() - .get_slot(TP_NEW) - .ok_or_else(|| PyTypeError::new_err("base type without tp_new"))? - }; - - // TODO: make it possible to provide real arguments to the base tp_new - let obj = - unsafe { tp_new(subtype, PyTuple::empty(py).as_ptr(), core::ptr::null_mut()) }; - if obj.is_null() { - Err(PyErr::fetch(py)) - } else { - Ok(obj) - } - } - unsafe { inner(py, T::type_object_raw(py), subtype) } - } -} - -pub trait PyClassInit<'py, const IS_PYCLASS: bool, const IS_INITIALIZER_TUPLE: bool>: - seal_pyclass_init::Sealed<'py, IS_PYCLASS, IS_INITIALIZER_TUPLE> -{ - fn init( - self, - cls: crate::Borrowed<'_, 'py, crate::types::PyType>, - ) -> PyResult>; -} - -mod seal_pyclass_init { - use crate::impl_::pyclass::{self, PyClassBaseType}; - use crate::impl_::pyclass_init::{PyClassInit, PyNativeTypeInitializer}; - use crate::{PyClass, PyClassInitializer, PyErr}; - - pub trait Sealed<'py, const IS_PYCLASS: bool, const IS_INITIALIZER_TUPLE: bool> {} - - impl<'py, T> Sealed<'py, false, false> for T where T: crate::IntoPyObject<'py> {} - impl<'py, T> Sealed<'py, true, false> for T - where - T: crate::PyClass, - T::BaseType: pyclass::PyClassBaseType>, - { - } - impl<'py, T, E, const IS_PYCLASS: bool, const IS_INITIALIZER_TUPLE: bool> - Sealed<'py, IS_PYCLASS, IS_INITIALIZER_TUPLE> for Result - where - T: PyClassInit<'py, IS_PYCLASS, IS_INITIALIZER_TUPLE>, - E: Into, - { - } - impl<'py, T> Sealed<'py, false, false> for PyClassInitializer where T: PyClass {} - impl<'py, S, B> Sealed<'py, false, true> for (S, B) - where - S: PyClass, - B: PyClass + PyClassBaseType>, - B::BaseType: PyClassBaseType>, - { - } -} - -impl<'py, T> PyClassInit<'py, false, false> for T -where - T: crate::IntoPyObject<'py>, -{ - fn init( - self, - cls: crate::Borrowed<'_, 'py, crate::types::PyType>, - ) -> PyResult> { - self.into_pyobject(cls.py()) - .map(crate::BoundObject::into_any) - .map(crate::BoundObject::into_bound) - .map_err(Into::into) - } -} - -impl<'py, T> PyClassInit<'py, true, false> for T -where - T: crate::PyClass, - T::BaseType: - super::pyclass::PyClassBaseType>, -{ - fn init( - self, - cls: crate::Borrowed<'_, 'py, crate::types::PyType>, - ) -> PyResult> { - PyClassInitializer::from(self).init(cls) - } -} - -impl<'py, T, E, const IS_PYCLASS: bool, const IS_INITIALIZER_TUPLE: bool> - PyClassInit<'py, IS_PYCLASS, IS_INITIALIZER_TUPLE> for Result -where - T: PyClassInit<'py, IS_PYCLASS, IS_INITIALIZER_TUPLE>, - E: Into, -{ - fn init( - self, - cls: crate::Borrowed<'_, 'py, crate::types::PyType>, - ) -> PyResult> { - self.map_err(Into::into)?.init(cls) - } -} - -impl<'py, T> PyClassInit<'py, false, false> for PyClassInitializer -where - T: PyClass, -{ - fn init( - self, - cls: crate::Borrowed<'_, 'py, crate::types::PyType>, - ) -> PyResult> { - unsafe { - self.create_class_object_of_type(cls.py(), cls.as_ptr().cast()) - .map(crate::Bound::into_any) - } - } -} - -impl<'py, S, B> PyClassInit<'py, false, true> for (S, B) -where - S: PyClass, - B: PyClass + PyClassBaseType>, - B::BaseType: PyClassBaseType>, -{ - fn init( - self, - cls: crate::Borrowed<'_, 'py, crate::types::PyType>, - ) -> PyResult> { - let (sub, base) = self; - PyClassInitializer::from(base).add_subclass(sub).init(cls) - } -} +/// Re-exported so that macros can name this internal type. +pub use crate::internal::pyclass_init::PyNativeTypeInitializer; diff --git a/src/impl_/pymethods.rs b/src/impl_/pymethods.rs index 9bbb6fde063..d73d80cb335 100644 --- a/src/impl_/pymethods.rs +++ b/src/impl_/pymethods.rs @@ -3,10 +3,13 @@ use crate::impl_::callback::IntoPyCallbackOutput; use crate::impl_::panic::PanicTrap; use crate::impl_::pycell::PyClassObjectBaseLayout; use crate::internal::get_slot::{get_slot, TP_BASE, TP_CLEAR, TP_TRAVERSE}; +use crate::internal::pyclass_init::PyClassInit; use crate::internal::state::ForbidAttaching; use crate::pycell::impl_::{PyClassBorrowChecker as _, PyClassObjectLayout}; use crate::types::PyType; -use crate::{ffi, Bound, Py, PyAny, PyClass, PyErr, PyResult, PyTraverseError, PyVisit, Python}; +use crate::{ + ffi, Borrowed, Bound, Py, PyAny, PyClass, PyErr, PyResult, PyTraverseError, PyVisit, Python, +}; use core::ffi::CStr; use core::ffi::{c_int, c_void}; use core::fmt; @@ -688,18 +691,26 @@ pub trait AsyncIterResultOptionKind { impl AsyncIterResultOptionKind for Result, Error> {} -pub unsafe fn tp_new_impl<'py, T, const IS_PYCLASS: bool, const IS_INITIALIZER_TUPLE: bool>( +/// Re-exported so that `#[new]` generated code can resolve the type tag for `tp_new_impl` +pub use crate::internal::pyclass_init::tp_new_resolver; + +#[expect( + private_bounds, + reason = "`PyClassInit` is not a public trait, bound exist for diagnostics" +)] +/// # SAFETY +/// - `cls` must be the type object for `ClassT` (or a subclass) +pub unsafe fn tp_new_impl<'py, InitializerT, ClassT>( py: Python<'py>, - obj: T, + initializer: InitializerT, cls: *mut ffi::PyTypeObject, ) -> PyResult<*mut ffi::PyObject> where - T: super::pyclass_init::PyClassInit<'py, IS_PYCLASS, IS_INITIALIZER_TUPLE>, + InitializerT: PyClassInit<'py, ClassT>, { - unsafe { - obj.init(crate::Borrowed::from_ptr_unchecked(py, cls.cast()).cast_unchecked()) - .map(Bound::into_ptr) - } + // SAFETY: caller has guaranteed `cls` is the correct object + unsafe { initializer.init(Borrowed::from_ptr_unchecked(py, cls.cast()).cast_unchecked()) } + .map(Bound::into_ptr) } #[cfg(test)] diff --git a/src/impl_/wrap.rs b/src/impl_/wrap.rs index 49ae60b3d8d..d5c5afd76b2 100644 --- a/src/impl_/wrap.rs +++ b/src/impl_/wrap.rs @@ -23,6 +23,34 @@ impl SomeWrap for Option { } } +pub struct OkWrapper(OkWrapperInner); +pub struct OkWrapperInner(PhantomData); + +impl OkWrapper { + pub fn new(_: &T) -> Self { + Self(OkWrapperInner(PhantomData)) + } +} + +impl Deref for OkWrapper { + type Target = OkWrapperInner; + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl OkWrapper> { + pub fn ok_wrap(&self, value: Result) -> Result { + value + } +} + +impl OkWrapperInner { + pub fn ok_wrap(&self, value: T) -> Result { + Ok(value) + } +} + // Hierarchy of conversions used in the function return type machinery pub struct Converter(EmptyTupleConverter); pub struct EmptyTupleConverter(IntoPyObjectConverter); @@ -155,4 +183,23 @@ mod tests { let b: Option = SomeWrap::wrap(None); assert_eq!(b, None); } + + #[test] + fn wrap_result() { + let a = 42; + let Ok(a) = OkWrapper::new(&a).ok_wrap(a); + assert_eq!(a, 42); + + let b = OkWrap::wrap(Result::<_, String>::Ok(42)); + assert_eq!(b, Ok(42)); + + let c = (|| -> PyResult { + let value = OkWrap::wrap(Result::<_, Infallible>::Ok(42))?; + let value = OkWrap::wrap(value)?; + Ok(value) + })() + .unwrap(); + + assert_eq!(c, 42); + } } diff --git a/src/internal.rs b/src/internal.rs index 7299f90ed03..b8f841ac0ae 100644 --- a/src/internal.rs +++ b/src/internal.rs @@ -1,4 +1,5 @@ //! Holding place for code which is not intended to be reachable from outside of PyO3. pub(crate) mod get_slot; +pub(crate) mod pyclass_init; pub(crate) mod state; diff --git a/src/internal/pyclass_init.rs b/src/internal/pyclass_init.rs new file mode 100644 index 00000000000..21114c50e49 --- /dev/null +++ b/src/internal/pyclass_init.rs @@ -0,0 +1,184 @@ +//! Contains initialization utilities for `#[pyclass]`. +use crate::exceptions::PyTypeError; +use crate::ffi_ptr_ext::FfiPtrExt; +use crate::impl_::pyclass::PyClassBaseType; +use crate::internal::get_slot::TP_NEW; +use crate::types::{PyTuple, PyType, PyTypeMethods}; +use crate::{ + ffi, IntoPyObject, IntoPyObjectExt, PyClass, PyClassInitializer, PyErr, PyResult, Python, +}; +use crate::{ffi::PyTypeObject, type_object::PyTypeInfo}; +use core::marker::PhantomData; + +/// Initializer for Python types. +/// +/// This trait is used internally for distinguishing `#[pyclass]` and +/// Python native types. +pub(crate) trait PyObjectInit: Sized { + /// # Safety + /// - `subtype` must be a valid pointer to a type object of T or a subclass. + unsafe fn into_new_object( + self, + py: Python<'_>, + subtype: *mut PyTypeObject, + ) -> PyResult<*mut ffi::PyObject>; +} + +/// Initializer for Python native types, like `PyDict`. +pub struct PyNativeTypeInitializer(pub PhantomData); + +impl PyObjectInit for PyNativeTypeInitializer { + unsafe fn into_new_object( + self, + py: Python<'_>, + subtype: *mut PyTypeObject, + ) -> PyResult<*mut ffi::PyObject> { + unsafe fn inner( + py: Python<'_>, + type_ptr: *mut PyTypeObject, + subtype: *mut PyTypeObject, + ) -> PyResult<*mut ffi::PyObject> { + let tp_new = unsafe { + type_ptr + .cast::() + .assume_borrowed_unchecked(py) + .cast_unchecked::() + .get_slot(TP_NEW) + .ok_or_else(|| PyTypeError::new_err("base type without tp_new"))? + }; + + // TODO: make it possible to provide real arguments to the base tp_new + let obj = + unsafe { tp_new(subtype, PyTuple::empty(py).as_ptr(), core::ptr::null_mut()) }; + if obj.is_null() { + Err(PyErr::fetch(py)) + } else { + Ok(obj) + } + } + unsafe { inner(py, T::type_object_raw(py), subtype) } + } +} + +pub struct TpNewValueTypeResolver( + ResolveToArbitraryObject, + PhantomData<(ClassT, ValueT)>, +); +pub struct ResolveToArbitraryObject(()); + +/// First step of machinery for resolving the type of `#[new]` return values, which is used to +/// implement specialization for the various cases. +/// +/// Call `.resolve()` on the returned tag to get the final tag type. +/// +/// This resolution step is necessary in order to encode the preference for `PyClassInitializer` +/// to use proper machinery instead of `IntoPyObject`; if we removed the implementation of +/// `IntoPyObject` for `PyClassInitializer` then this machinery could probably collapse away to +/// type inference. +pub fn tp_new_resolver(_: &ValueT) -> TpNewValueTypeResolver { + TpNewValueTypeResolver(ResolveToArbitraryObject(()), PhantomData) +} + +impl std::ops::Deref for TpNewValueTypeResolver { + type Target = ResolveToArbitraryObject; + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +/// Second stage of machinery for resolving the type of `#[new]` return values. This +/// is the specialized implementation for the case where the return type should be +/// used to build a new class object (`T`, `PyClassInitializer`, `(S, B)` tuples). +#[expect(private_bounds, reason = "internal trait implementation")] +impl TpNewValueTypeResolver +where + ClassT: PyClass, + ValueT: IntoPyClassInitializer, +{ + pub fn resolve(&self, value: ValueT) -> PyClassInitializer { + value.into_pyclass_initializer() + } +} + +/// All other conversions fall back to IntoPyObject via the deref implementation +impl ResolveToArbitraryObject { + pub fn resolve(&self, value: ValueT) -> ValueT { + value + } +} + +#[diagnostic::on_unimplemented( + message = "`{Self}` cannot be used as the return value for `#[new]` methods", + note = "all types which implement `IntoPyObject` are suitable return values for `#[new]` methods", + note = "`PyClassInitializer` may also be used and is necessary for `#[pyclass(extends = '...')]` types", + label = "must be a type which implements `IntoPyObject`, a `PyClassInitializer<{Self}>`, or a `Result` wrapping such types" +)] +pub(crate) trait PyClassInit<'py, T> { + /// # Safety + /// - `cls` must be the type object for `T` (or a subclass) + unsafe fn init( + self, + cls: crate::Borrowed<'_, 'py, crate::types::PyType>, + ) -> PyResult>; +} + +impl<'py, ClassT, T> PyClassInit<'py, ClassT> for T +where + T: IntoPyObject<'py>, +{ + unsafe fn init( + self, + cls: crate::Borrowed<'_, 'py, crate::types::PyType>, + ) -> PyResult> { + self.into_bound_py_any(cls.py()) + } +} + +impl<'py, T> PyClassInit<'py, T> for PyClassInitializer +where + T: PyClass, +{ + unsafe fn init( + self, + cls: crate::Borrowed<'_, 'py, crate::types::PyType>, + ) -> PyResult> { + // SAFETY: caller has guaranteed that `cls` is correct object + unsafe { + self.create_class_object_of_type(cls.py(), cls.as_type_ptr()) + .map(crate::Bound::into_any) + } + } +} + +/// Analagous to `Into>`, but just an internal equivalent +/// to avoid allowing user code to define custom return types from `#[new]`. +trait IntoPyClassInitializer: Sized { + fn into_pyclass_initializer(self) -> PyClassInitializer; +} + +impl IntoPyClassInitializer for PyClassInitializer { + fn into_pyclass_initializer(self) -> PyClassInitializer { + self + } +} + +impl IntoPyClassInitializer for T +where + T: PyClass, + T::BaseType: PyClassBaseType>, +{ + fn into_pyclass_initializer(self) -> PyClassInitializer { + self.into() + } +} + +impl IntoPyClassInitializer for (S, B) +where + S: PyClass, + B: PyClass + PyClassBaseType>, + B::BaseType: PyClassBaseType>, +{ + fn into_pyclass_initializer(self) -> PyClassInitializer { + self.into() + } +} diff --git a/src/pyclass_init.rs b/src/pyclass_init.rs index b744b2765ec..a3116500d86 100644 --- a/src/pyclass_init.rs +++ b/src/pyclass_init.rs @@ -1,7 +1,8 @@ //! Contains initialization utilities for `#[pyclass]`. use crate::ffi_ptr_ext::FfiPtrExt; use crate::impl_::pyclass::{PyClassBaseType, PyClassImpl}; -use crate::impl_::pyclass_init::{PyNativeTypeInitializer, PyObjectInit}; +use crate::impl_::pyclass_init::PyNativeTypeInitializer; +use crate::internal::pyclass_init::PyObjectInit; use crate::pycell::impl_::PyClassObjectLayout; use crate::{ffi, Bound, PyClass, PyResult, Python}; use crate::{ffi::PyTypeObject, pycell::impl_::PyClassObjectContents}; diff --git a/tests/test_class_init.rs b/tests/test_class_init.rs index 58dd8a4e22f..40b46b5d56c 100644 --- a/tests/test_class_init.rs +++ b/tests/test_class_init.rs @@ -84,3 +84,27 @@ fn test_subclass_with_init() { assert_eq!(obj.as_super().borrow().num, 43); }); } + +#[test] +fn test_arbitrary_object() { + #[pyclass] + struct A {} + + #[pyclass] + struct B {} + + #[pymethods] + impl A { + #[new] + fn new() -> B { + B {} + } + } + + Python::attach(|py| { + let typeobj = py.get_type::(); + let obj = typeobj.call((), None).unwrap(); + + assert!(obj.is_instance_of::()); + }); +} diff --git a/tests/test_compile_error.rs b/tests/test_compile_error.rs index e5b646239d7..02efeea78d2 100644 --- a/tests/test_compile_error.rs +++ b/tests/test_compile_error.rs @@ -16,6 +16,7 @@ fn test_compile_errors() { t.compile_fail("tests/ui/invalid_pyclass_enum.rs"); t.compile_fail("tests/ui/invalid_pyclass_init.rs"); t.compile_fail("tests/ui/invalid_pyclass_item.rs"); + t.compile_fail("tests/ui/invalid_pyclass_new.rs"); #[cfg(Py_3_9)] t.compile_fail("tests/ui/invalid_pyclass_generic.rs"); #[cfg(Py_3_9)] diff --git a/tests/ui/invalid_pyclass_new.rs b/tests/ui/invalid_pyclass_new.rs new file mode 100644 index 00000000000..a04408547e3 --- /dev/null +++ b/tests/ui/invalid_pyclass_new.rs @@ -0,0 +1,16 @@ +use pyo3::prelude::*; + +#[pyclass] +struct InvalidNewReturn; + +struct Invalid; + +#[pymethods] +impl InvalidNewReturn { + #[new] + fn new() -> Invalid { + Invalid + } +} + +fn main() {} diff --git a/tests/ui/invalid_pyclass_new.stderr b/tests/ui/invalid_pyclass_new.stderr new file mode 100644 index 00000000000..bc2fe5fc772 --- /dev/null +++ b/tests/ui/invalid_pyclass_new.stderr @@ -0,0 +1,32 @@ +error[E0277]: `Invalid` cannot be used as the return value for `#[new]` methods + --> tests/ui/invalid_pyclass_new.rs:11:17 + | +11 | fn new() -> Invalid { + | ^^^^^^^ must be a type which implements `IntoPyObject`, a `PyClassInitializer`, or a `Result` wrapping such types + | +help: the trait `IntoPyObject<'_>` is not implemented for `Invalid` + --> tests/ui/invalid_pyclass_new.rs:6:1 + | + 6 | struct Invalid; + | ^^^^^^^^^^^^^^ + = note: all types which implement `IntoPyObject` are suitable return values for `#[new]` methods + = note: `PyClassInitializer` may also be used and is necessary for `#[pyclass(extends = '...')]` types + = help: the following other types implement trait `IntoPyObject<'py>`: + &&'a T + &&OsStr + &&Path + &&str + &'a (T0, T1) + &'a (T0, T1, T2) + &'a (T0, T1, T2, T3) + &'a (T0, T1, T2, T3, T4) + and $N others + = note: required for `Invalid` to implement `pyo3::internal::pyclass_init::PyClassInit<'_, InvalidNewReturn>` +note: required by a bound in `pyo3::impl_::pymethods::tp_new_impl` + --> src/impl_/pymethods.rs + | + | pub unsafe fn tp_new_impl<'py, InitializerT, ClassT>( + | ----------- required by a bound in this function +... + | InitializerT: PyClassInit<'py, ClassT>, + | ^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `tp_new_impl` From 4d39ec5cd7fcfdcc23b58f460ac93b86d95276e3 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Wed, 20 May 2026 22:13:05 +0200 Subject: [PATCH 2/5] fixup --- pyo3-macros-backend/src/pymethod.rs | 15 +++++++++++---- src/impl_/wrap.rs | 12 ++---------- src/internal/pyclass_init.rs | 8 +++----- tests/test_class_init.rs | 2 +- 4 files changed, 17 insertions(+), 20 deletions(-) diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index 7b62341b456..90b1ecbf497 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -1473,16 +1473,23 @@ fn generate_method_body( } }); + let py = syn::Ident::new("py", Span::call_site()); + let initializer = syn::Ident::new("initializer", Span::call_site()); + let slf = syn::Ident::new("_slf", Span::call_site()); + + let conversion = quote_spanned! { *output_span => + #pyo3_path::impl_::pymethods::tp_new_impl::<_, #cls>(#py, #initializer, #slf) + }; + let body = quote_spanned! { *output_span => #text_signature_impl - - use #pyo3_path::impl_::pyclass::Probe as _; #warnings #arg_convert + let result = #call; let value = #pyo3_path::impl_::wrap::OkWrapper::new(&result).ok_wrap(result)?; - let initializer = #pyo3_path::impl_::pymethods::tp_new_resolver::<#cls, _>(&value).resolve(value); - #pyo3_path::impl_::pymethods::tp_new_impl::<_, #cls>(py, initializer, _slf) + let #initializer = #pyo3_path::impl_::pymethods::tp_new_resolver::<#cls, _>(&value).resolve(value); + unsafe { #conversion } }; (arg_idents, arg_types, body) } diff --git a/src/impl_/wrap.rs b/src/impl_/wrap.rs index d5c5afd76b2..7b423b216ff 100644 --- a/src/impl_/wrap.rs +++ b/src/impl_/wrap.rs @@ -190,16 +190,8 @@ mod tests { let Ok(a) = OkWrapper::new(&a).ok_wrap(a); assert_eq!(a, 42); - let b = OkWrap::wrap(Result::<_, String>::Ok(42)); + let b = Result::<_, String>::Ok(42); + let b = OkWrapper::new(&b).ok_wrap(b); assert_eq!(b, Ok(42)); - - let c = (|| -> PyResult { - let value = OkWrap::wrap(Result::<_, Infallible>::Ok(42))?; - let value = OkWrap::wrap(value)?; - Ok(value) - })() - .unwrap(); - - assert_eq!(c, 42); } } diff --git a/src/internal/pyclass_init.rs b/src/internal/pyclass_init.rs index 21114c50e49..f0ddfd43a80 100644 --- a/src/internal/pyclass_init.rs +++ b/src/internal/pyclass_init.rs @@ -79,7 +79,7 @@ pub fn tp_new_resolver(_: &ValueT) -> TpNewValueTypeResolver std::ops::Deref for TpNewValueTypeResolver { +impl core::ops::Deref for TpNewValueTypeResolver { type Target = ResolveToArbitraryObject; fn deref(&self) -> &Self::Target { &self.0 @@ -143,10 +143,8 @@ where cls: crate::Borrowed<'_, 'py, crate::types::PyType>, ) -> PyResult> { // SAFETY: caller has guaranteed that `cls` is correct object - unsafe { - self.create_class_object_of_type(cls.py(), cls.as_type_ptr()) - .map(crate::Bound::into_any) - } + unsafe { self.create_class_object_of_type(cls.py(), cls.as_type_ptr()) } + .map(crate::Bound::into_any) } } diff --git a/tests/test_class_init.rs b/tests/test_class_init.rs index 40b46b5d56c..3b7f5e36eb8 100644 --- a/tests/test_class_init.rs +++ b/tests/test_class_init.rs @@ -96,7 +96,7 @@ fn test_arbitrary_object() { #[pymethods] impl A { #[new] - fn new() -> B { + fn __new__() -> B { B {} } } From d9354df4225e98c012582e9fce8c3de050bdf25b Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Wed, 20 May 2026 22:14:16 +0200 Subject: [PATCH 3/5] newsfragment --- newsfragments/6062.fixed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/6062.fixed.md diff --git a/newsfragments/6062.fixed.md b/newsfragments/6062.fixed.md new file mode 100644 index 00000000000..a77dc6f01bc --- /dev/null +++ b/newsfragments/6062.fixed.md @@ -0,0 +1 @@ +Fix type confusion when returning a `#[pyclass]` from a different pyclass' `#[new]` method. From 66a7ac971e3d92a50ae84ff273fc45c6c73f4ad9 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Wed, 20 May 2026 22:16:00 +0200 Subject: [PATCH 4/5] don't quote `unsafe` at user code --- pyo3-macros-backend/src/pymethod.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index 90b1ecbf497..a697a330601 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -1477,11 +1477,13 @@ fn generate_method_body( let initializer = syn::Ident::new("initializer", Span::call_site()); let slf = syn::Ident::new("_slf", Span::call_site()); + // Having just this call emitted at the span of the return value helps surface errors + // if the user passed an invalid return type. let conversion = quote_spanned! { *output_span => #pyo3_path::impl_::pymethods::tp_new_impl::<_, #cls>(#py, #initializer, #slf) }; - let body = quote_spanned! { *output_span => + let body = quote! { #text_signature_impl #warnings #arg_convert From 1160174c22418644f9fbcee21d5fb234751c6507 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Sun, 24 May 2026 11:39:36 +0200 Subject: [PATCH 5/5] review feedback --- src/impl_/pyclass.rs | 10 +++++----- src/internal/pyclass_init.rs | 8 ++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index f6475d7766e..a0fec366677 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -7,6 +7,7 @@ use crate::{ pycell::{GetBorrowChecker, PyClassMutability, PyClassObjectBaseLayout}, pymethods::{PyGetterDef, PyMethodDefType}, }, + internal::pyclass_init::PyObjectInit, pycell::{impl_::PyClassObjectLayout, PyBorrowError}, types::{any::PyAnyMethods, PyBool}, Borrowed, IntoPyObject, IntoPyObjectExt, Py, PyAny, PyClass, PyClassGuard, PyErr, PyResult, @@ -1086,6 +1087,10 @@ impl PyClassThreadChecker for ThreadCheckerImpl { note = "subclassing native types requires Python >= 3.12 when using the `abi3` feature", ) )] +#[expect( + private_bounds, + reason = "`PyObjectInit` is an internal trait implementation" +)] pub trait PyClassBaseType: Sized { type LayoutAsBase: PyClassObjectBaseLayout; type BaseNativeType; @@ -1095,11 +1100,6 @@ pub trait PyClassBaseType: Sized { type Layout; } -/// Constraint on base types available within PyO3 -#[expect(private_bounds, reason = "internal trait implementation")] -pub trait PyObjectInit: crate::internal::pyclass_init::PyObjectInit {} -impl PyObjectInit for I where I: crate::internal::pyclass_init::PyObjectInit {} - /// Implementation of tp_dealloc for pyclasses without gc pub(crate) unsafe extern "C" fn tp_dealloc(obj: *mut ffi::PyObject) { unsafe { crate::impl_::trampoline::dealloc(obj, ::Layout::tp_dealloc) } diff --git a/src/internal/pyclass_init.rs b/src/internal/pyclass_init.rs index f0ddfd43a80..19195d19774 100644 --- a/src/internal/pyclass_init.rs +++ b/src/internal/pyclass_init.rs @@ -71,10 +71,10 @@ pub struct ResolveToArbitraryObject(()); /// /// Call `.resolve()` on the returned tag to get the final tag type. /// -/// This resolution step is necessary in order to encode the preference for `PyClassInitializer` -/// to use proper machinery instead of `IntoPyObject`; if we removed the implementation of -/// `IntoPyObject` for `PyClassInitializer` then this machinery could probably collapse away to -/// type inference. +/// This resolution step is necessary in order to encode the preference to go via `PyClassInitializer` +/// for new instances of `ClassT`. Without this step, the fallback to `IntoPyObject` conflicts for +/// `ClassT` because that implementation ignores the `cls` parameter for `PyClassInit` (and would +/// therefore be incorrect when instantiating subclasses). pub fn tp_new_resolver(_: &ValueT) -> TpNewValueTypeResolver { TpNewValueTypeResolver(ResolveToArbitraryObject(()), PhantomData) }