From b70ee9a5ad6a8fc788c49a701725f9df1ed287ae Mon Sep 17 00:00:00 2001 From: kngwyu Date: Sun, 21 Jun 2020 23:38:26 +0900 Subject: [PATCH 1/2] Use subclass correctly in tp_new --- CHANGELOG.md | 2 + examples/rustapi_module/src/subclassing.rs | 7 ++++ .../rustapi_module/tests/test_subclassing.py | 5 ++- pyo3-derive-backend/src/pymethod.rs | 7 ++-- src/freelist.rs | 15 ++++--- src/pycell.rs | 9 ++-- src/pyclass.rs | 40 ++++++++++-------- src/pyclass_init.rs | 21 ++++++++-- tests/test_class_new.rs | 41 +++++++++++++++++++ 9 files changed, 112 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ca6070c94e..e6756cb4609 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Update `num-complex` optional dependendency from `0.2` to `0.3`. [#977](https://github.com/PyO3/pyo3/pull/977) - Update `num-bigint` optional dependendency from `0.2` to `0.3`. [#978](https://github.com/PyO3/pyo3/pull/978) - `#[pyproto]` is re-implemented without specialization. [#961](https://github.com/PyO3/pyo3/pull/961) +- `PyClassAlloc::alloc` is renamed to `PyClassAlloc::new`. [#990](https://github.com/PyO3/pyo3/pull/990) ### Removed - Remove `ManagedPyRef` (unused, and needs specialization) [#930](https://github.com/PyO3/pyo3/pull/930) @@ -32,6 +33,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed - Fix passing explicit `None` to `Option` argument `#[pyfunction]` with a default value. [#936](https://github.com/PyO3/pyo3/pull/936) +- Fix `PyClass.__new__`'s behavior when inherited by a Python class. [#990](https://github.com/PyO3/pyo3/pull/990) ## [0.10.1] - 2020-05-14 ### Fixed diff --git a/examples/rustapi_module/src/subclassing.rs b/examples/rustapi_module/src/subclassing.rs index 61ead72fe1e..20e7d431cdb 100644 --- a/examples/rustapi_module/src/subclassing.rs +++ b/examples/rustapi_module/src/subclassing.rs @@ -13,6 +13,13 @@ impl Subclassable { } } +#[pyproto] +impl pyo3::PyObjectProtocol for Subclassable { + fn __str__(&self) -> PyResult<&'static str> { + Ok("Subclassable") + } +} + #[pymodule] fn subclassing(_py: Python<'_>, m: &PyModule) -> PyResult<()> { m.add_class::()?; diff --git a/examples/rustapi_module/tests/test_subclassing.py b/examples/rustapi_module/tests/test_subclassing.py index ccd61aa7784..185162018c2 100644 --- a/examples/rustapi_module/tests/test_subclassing.py +++ b/examples/rustapi_module/tests/test_subclassing.py @@ -6,10 +6,11 @@ class SomeSubClass(Subclassable): - pass + def __str__(self): + return "Subclass" def test_subclassing(): if not PYPY: a = SomeSubClass() - _b = str(a) + repr(a) + assert str(a) == "Subclass" diff --git a/pyo3-derive-backend/src/pymethod.rs b/pyo3-derive-backend/src/pymethod.rs index a4ef8c497ac..fce1b702acf 100644 --- a/pyo3-derive-backend/src/pymethod.rs +++ b/pyo3-derive-backend/src/pymethod.rs @@ -193,7 +193,7 @@ pub fn impl_wrap_new(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream { quote! { #[allow(unused_mut)] unsafe extern "C" fn __wrap( - _cls: *mut pyo3::ffi::PyTypeObject, + subcls: *mut pyo3::ffi::PyTypeObject, _args: *mut pyo3::ffi::PyObject, _kwargs: *mut pyo3::ffi::PyObject) -> *mut pyo3::ffi::PyObject { @@ -204,8 +204,9 @@ pub fn impl_wrap_new(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream { let _args = _py.from_borrowed_ptr::(_args); let _kwargs: Option<&pyo3::types::PyDict> = _py.from_borrowed_ptr_or_opt(_kwargs); - let _result = pyo3::derive_utils::IntoPyNewResult::into_pynew_result(#body); - let cell = pyo3::PyClassInitializer::from(_result?).create_cell(_py)?; + let _result = pyo3::derive_utils::IntoPyNewResult::into_pynew_result(#body)?; + let initializer = pyo3::PyClassInitializer::from(_result); + let cell = initializer.create_cell_from_subtype(_py, subcls)?; Ok(cell as *mut pyo3::ffi::PyObject) }) } diff --git a/src/freelist.rs b/src/freelist.rs index f7a6e4c6ed1..6bd9e2a4f24 100644 --- a/src/freelist.rs +++ b/src/freelist.rs @@ -72,13 +72,16 @@ impl PyClassAlloc for T where T: PyTypeInfo + PyClassWithFreeList, { - unsafe fn alloc(py: Python) -> *mut Self::Layout { - if let Some(obj) = ::get_free_list().pop() { - ffi::PyObject_Init(obj, Self::type_object_raw(py) as *const _ as _); - obj as _ - } else { - crate::pyclass::default_alloc::(py) as _ + unsafe fn new(py: Python, subtype: *mut ffi::PyTypeObject) -> *mut Self::Layout { + let type_obj = Self::type_object_raw(py) as *const _ as _; + // if subtype is not equal to this type, we cannot use the freelist + if subtype == type_obj { + if let Some(obj) = ::get_free_list().pop() { + ffi::PyObject_Init(obj, subtype); + return obj as _; + } } + crate::pyclass::default_new::(py, subtype) as _ } unsafe fn dealloc(py: Python, self_: *mut Self::Layout) { diff --git a/src/pycell.rs b/src/pycell.rs index 15ef1035a72..2fa672882b0 100644 --- a/src/pycell.rs +++ b/src/pycell.rs @@ -334,13 +334,16 @@ impl PyCell { std::mem::swap(&mut *self.borrow_mut(), &mut *other.borrow_mut()) } - /// Allocates new PyCell without initilizing value. + /// Allocates a new PyCell given a type object `subtype`. Used by our `tp_new` implementation. /// Requires `T::BaseLayout: PyBorrowFlagLayout` to ensure `self` has a borrow flag. - pub(crate) unsafe fn internal_new(py: Python) -> PyResult<*mut Self> + pub(crate) unsafe fn internal_new( + py: Python, + subtype: *mut ffi::PyTypeObject, + ) -> PyResult<*mut Self> where T::BaseLayout: PyBorrowFlagLayout, { - let base = T::alloc(py); + let base = T::new(py, subtype); if base.is_null() { return Err(PyErr::fetch(py)); } diff --git a/src/pyclass.rs b/src/pyclass.rs index a92f7a3d743..9b4cc184882 100644 --- a/src/pyclass.rs +++ b/src/pyclass.rs @@ -11,27 +11,29 @@ use std::os::raw::c_void; use std::ptr; #[inline] -pub(crate) unsafe fn default_alloc(py: Python) -> *mut ffi::PyObject { - let type_obj = T::type_object_raw(py); +pub(crate) unsafe fn default_new( + py: Python, + subtype: *mut ffi::PyTypeObject, +) -> *mut ffi::PyObject { // if the class derives native types(e.g., PyDict), call special new if T::FLAGS & type_flags::EXTENDED != 0 && T::BaseLayout::IS_NATIVE_TYPE { let base_tp = T::BaseType::type_object_raw(py); if let Some(base_new) = base_tp.tp_new { - return base_new(type_obj as *const _ as _, ptr::null_mut(), ptr::null_mut()); + return base_new(subtype, ptr::null_mut(), ptr::null_mut()); } } - let alloc = type_obj.tp_alloc.unwrap_or(ffi::PyType_GenericAlloc); - alloc(type_obj as *const _ as _, 0) + let alloc = (*subtype).tp_alloc.unwrap_or(ffi::PyType_GenericAlloc); + alloc(subtype, 0) as _ } -/// This trait enables custom alloc/dealloc implementations for `T: PyClass`. +/// This trait enables custom `tp_new`/`tp_dealloc` implementations for `T: PyClass`. pub trait PyClassAlloc: PyTypeInfo + Sized { /// Allocate the actual field for `#[pyclass]`. /// /// # Safety /// This function must return a valid pointer to the Python heap. - unsafe fn alloc(py: Python) -> *mut Self::Layout { - default_alloc::(py) as _ + unsafe fn new(py: Python, subtype: *mut ffi::PyTypeObject) -> *mut Self::Layout { + default_new::(py, subtype) as _ } /// Deallocate `#[pyclass]` on the Python heap. @@ -52,6 +54,18 @@ pub trait PyClassAlloc: PyTypeInfo + Sized { } } +fn tp_dealloc() -> Option { + unsafe extern "C" fn dealloc(obj: *mut ffi::PyObject) + where + T: PyClassAlloc, + { + let pool = crate::GILPool::new(); + let py = pool.python(); + ::dealloc(py, (obj as *mut T::Layout) as _) + } + Some(dealloc::) +} + #[doc(hidden)] pub unsafe fn tp_free_fallback(obj: *mut ffi::PyObject) { let ty = ffi::Py_TYPE(obj); @@ -115,15 +129,7 @@ where }; // dealloc - unsafe extern "C" fn tp_dealloc_callback(obj: *mut ffi::PyObject) - where - T: PyClassAlloc, - { - let pool = crate::GILPool::new(); - let py = pool.python(); - ::dealloc(py, (obj as *mut T::Layout) as _) - } - type_object.tp_dealloc = Some(tp_dealloc_callback::); + type_object.tp_dealloc = tp_dealloc::(); // type size type_object.tp_basicsize = std::mem::size_of::() as ffi::Py_ssize_t; diff --git a/src/pyclass_init.rs b/src/pyclass_init.rs index 3be018f241c..162690d9654 100644 --- a/src/pyclass_init.rs +++ b/src/pyclass_init.rs @@ -114,14 +114,27 @@ impl PyClassInitializer { PyClassInitializer::new(subclass_value, self) } - // Create a new PyCell + initialize it - #[doc(hidden)] - pub unsafe fn create_cell(self, py: Python) -> PyResult<*mut PyCell> + // Create a new PyCell and initialize it. + pub(crate) unsafe fn create_cell(self, py: Python) -> PyResult<*mut PyCell> where T: PyClass, T::BaseLayout: PyBorrowFlagLayout, { - let cell = PyCell::internal_new(py)?; + self.create_cell_from_subtype(py, T::type_object_raw(py) as *const _ as _) + } + + /// Create a new PyCell and initialize it given a typeobject `subtype`. + /// Called by our `tp_new` generated by the `#[new]` attribute. + pub unsafe fn create_cell_from_subtype( + self, + py: Python, + subtype: *mut crate::ffi::PyTypeObject, + ) -> PyResult<*mut PyCell> + where + T: PyClass, + T::BaseLayout: PyBorrowFlagLayout, + { + let cell = PyCell::internal_new(py, subtype)?; self.init_class(&mut *cell); Ok(cell) } diff --git a/tests/test_class_new.rs b/tests/test_class_new.rs index c225608f1aa..8d4d6068f32 100644 --- a/tests/test_class_new.rs +++ b/tests/test_class_new.rs @@ -79,3 +79,44 @@ fn new_with_two_args() { assert_eq!(obj_ref._data1, 10); assert_eq!(obj_ref._data2, 20); } + +#[pyclass(subclass)] +struct SuperClass { + #[pyo3(get)] + from_rust: bool, +} + +#[pymethods] +impl SuperClass { + #[new] + fn new() -> Self { + SuperClass { from_rust: true } + } +} + +/// Checks that `subclass.__new__` works correctly. +/// See https://github.com/PyO3/pyo3/issues/947 for the corresponding bug. +#[test] +fn subclass_new() { + let gil = Python::acquire_gil(); + let py = gil.python(); + let super_cls = py.get_type::(); + let source = pyo3::indoc::indoc!( + r#" +class Class(SuperClass): + def __new__(cls): + return super().__new__(cls) # This should return an instance of Class + + @property + def from_rust(self): + return False +c = Class() +assert c.from_rust is False +"# + ); + let globals = PyModule::import(py, "__main__").unwrap().dict(); + globals.set_item("SuperClass", super_cls).unwrap(); + py.run(source, Some(globals), None) + .map_err(|e| e.print(py)) + .unwrap(); +} From f053bc388180cad17d148e94e8d8c214d271477a Mon Sep 17 00:00:00 2001 From: kngwyu Date: Mon, 22 Jun 2020 02:38:08 +0900 Subject: [PATCH 2/2] Fix dealloc implementation to collectly use subtype's tp_free --- CHANGELOG.md | 2 +- .../rustapi_module/tests/test_subclassing.py | 5 +++-- pyo3-derive-backend/src/pymethod.rs | 4 ++-- src/freelist.rs | 14 +++++++------- src/instance.rs | 2 +- src/pyclass.rs | 19 ++++++++++--------- src/pyclass_init.rs | 7 +++++-- 7 files changed, 29 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e6756cb4609..2247dbeac7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,7 +33,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed - Fix passing explicit `None` to `Option` argument `#[pyfunction]` with a default value. [#936](https://github.com/PyO3/pyo3/pull/936) -- Fix `PyClass.__new__`'s behavior when inherited by a Python class. [#990](https://github.com/PyO3/pyo3/pull/990) +- Fix `PyClass.__new__`'s not respecting subclasses when inherited by a Python class. [#990](https://github.com/PyO3/pyo3/pull/990) ## [0.10.1] - 2020-05-14 ### Fixed diff --git a/examples/rustapi_module/tests/test_subclassing.py b/examples/rustapi_module/tests/test_subclassing.py index 185162018c2..1f8ce06404e 100644 --- a/examples/rustapi_module/tests/test_subclassing.py +++ b/examples/rustapi_module/tests/test_subclassing.py @@ -7,10 +7,11 @@ class SomeSubClass(Subclassable): def __str__(self): - return "Subclass" + return "SomeSubclass" def test_subclassing(): if not PYPY: a = SomeSubClass() - assert str(a) == "Subclass" + assert str(a) == "SomeSubclass" + assert type(a) is SomeSubClass diff --git a/pyo3-derive-backend/src/pymethod.rs b/pyo3-derive-backend/src/pymethod.rs index fce1b702acf..56471a63974 100644 --- a/pyo3-derive-backend/src/pymethod.rs +++ b/pyo3-derive-backend/src/pymethod.rs @@ -193,7 +193,7 @@ pub fn impl_wrap_new(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream { quote! { #[allow(unused_mut)] unsafe extern "C" fn __wrap( - subcls: *mut pyo3::ffi::PyTypeObject, + subtype: *mut pyo3::ffi::PyTypeObject, _args: *mut pyo3::ffi::PyObject, _kwargs: *mut pyo3::ffi::PyObject) -> *mut pyo3::ffi::PyObject { @@ -206,7 +206,7 @@ pub fn impl_wrap_new(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream { let _result = pyo3::derive_utils::IntoPyNewResult::into_pynew_result(#body)?; let initializer = pyo3::PyClassInitializer::from(_result); - let cell = initializer.create_cell_from_subtype(_py, subcls)?; + let cell = initializer.create_cell_from_subtype(_py, subtype)?; Ok(cell as *mut pyo3::ffi::PyObject) }) } diff --git a/src/freelist.rs b/src/freelist.rs index 6bd9e2a4f24..2f47c7daefc 100644 --- a/src/freelist.rs +++ b/src/freelist.rs @@ -2,10 +2,9 @@ //! Free allocation list -use crate::ffi; use crate::pyclass::{tp_free_fallback, PyClassAlloc}; use crate::type_object::{PyLayout, PyTypeInfo}; -use crate::Python; +use crate::{ffi, AsPyPointer, FromPyPointer, PyAny, Python}; use std::mem; use std::os::raw::c_void; @@ -86,14 +85,15 @@ where unsafe fn dealloc(py: Python, self_: *mut Self::Layout) { (*self_).py_drop(py); - - let obj = self_ as _; - if ffi::PyObject_CallFinalizerFromDealloc(obj) < 0 { + let obj = PyAny::from_borrowed_ptr_or_panic(py, self_ as _); + if Self::is_exact_instance(obj) && ffi::PyObject_CallFinalizerFromDealloc(obj.as_ptr()) < 0 + { + // tp_finalize resurrected. return; } - if let Some(obj) = ::get_free_list().insert(obj) { - match Self::type_object_raw(py).tp_free { + if let Some(obj) = ::get_free_list().insert(obj.as_ptr()) { + match (*ffi::Py_TYPE(obj)).tp_free { Some(free) => free(obj as *mut c_void), None => tp_free_fallback(obj), } diff --git a/src/instance.rs b/src/instance.rs index 2dce9efd9f0..97b6335b7e2 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -59,7 +59,7 @@ where T::BaseLayout: PyBorrowFlagLayout, { let initializer = value.into(); - let obj = unsafe { initializer.create_cell(py)? }; + let obj = initializer.create_cell(py)?; let ob = unsafe { Py::from_owned_ptr(py, obj as _) }; Ok(ob) } diff --git a/src/pyclass.rs b/src/pyclass.rs index 9b4cc184882..7fa2ace42c5 100644 --- a/src/pyclass.rs +++ b/src/pyclass.rs @@ -1,10 +1,10 @@ //! `PyClass` trait use crate::class::methods::{PyClassAttributeDef, PyMethodDefType, PyMethods}; use crate::class::proto_methods::PyProtoMethods; -use crate::conversion::{IntoPyPointer, ToPyObject}; +use crate::conversion::{AsPyPointer, FromPyPointer, IntoPyPointer, ToPyObject}; use crate::pyclass_slots::{PyClassDict, PyClassWeakRef}; use crate::type_object::{type_flags, PyLayout}; -use crate::types::PyDict; +use crate::types::{PyAny, PyDict}; use crate::{class, ffi, PyCell, PyErr, PyNativeType, PyResult, PyTypeInfo, Python}; use std::ffi::CString; use std::os::raw::c_void; @@ -42,14 +42,16 @@ pub trait PyClassAlloc: PyTypeInfo + Sized { /// `self_` must be a valid pointer to the Python heap. unsafe fn dealloc(py: Python, self_: *mut Self::Layout) { (*self_).py_drop(py); - let obj = self_ as _; - if ffi::PyObject_CallFinalizerFromDealloc(obj) < 0 { + let obj = PyAny::from_borrowed_ptr_or_panic(py, self_ as _); + if Self::is_exact_instance(obj) && ffi::PyObject_CallFinalizerFromDealloc(obj.as_ptr()) < 0 + { + // tp_finalize resurrected. return; } - match Self::type_object_raw(py).tp_free { - Some(free) => free(obj as *mut c_void), - None => tp_free_fallback(obj), + match (*ffi::Py_TYPE(obj.as_ptr())).tp_free { + Some(free) => free(obj.as_ptr() as *mut c_void), + None => tp_free_fallback(obj.as_ptr()), } } } @@ -66,8 +68,7 @@ fn tp_dealloc() -> Option { Some(dealloc::) } -#[doc(hidden)] -pub unsafe fn tp_free_fallback(obj: *mut ffi::PyObject) { +pub(crate) unsafe fn tp_free_fallback(obj: *mut ffi::PyObject) { let ty = ffi::Py_TYPE(obj); if ffi::PyType_IS_GC(ty) != 0 { ffi::PyObject_GC_Del(obj as *mut c_void); diff --git a/src/pyclass_init.rs b/src/pyclass_init.rs index 162690d9654..51ed4f64059 100644 --- a/src/pyclass_init.rs +++ b/src/pyclass_init.rs @@ -115,16 +115,19 @@ impl PyClassInitializer { } // Create a new PyCell and initialize it. - pub(crate) unsafe fn create_cell(self, py: Python) -> PyResult<*mut PyCell> + pub(crate) fn create_cell(self, py: Python) -> PyResult<*mut PyCell> where T: PyClass, T::BaseLayout: PyBorrowFlagLayout, { - self.create_cell_from_subtype(py, T::type_object_raw(py) as *const _ as _) + unsafe { self.create_cell_from_subtype(py, T::type_object_raw(py) as *const _ as _) } } /// Create a new PyCell and initialize it given a typeobject `subtype`. /// Called by our `tp_new` generated by the `#[new]` attribute. + /// + /// # Safety + /// `cls` must be a subclass of T. pub unsafe fn create_cell_from_subtype( self, py: Python,