Merge pull request #990 from kngwyu/tpnew-fix

Use subclass correctly in tp_new

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<T>` argument `#[pyfunction]` with a default value. [#936](https://github.com/PyO3/pyo3/pull/936)
+- 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

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::<Subclassable>()?;

examples/rustapi_module/tests/test_subclassing.py

@@ -6,10 +6,12 @@ PYPY = platform.python_implementation() == "PyPy"
 
 
 class SomeSubClass(Subclassable):
-    pass
+    def __str__(self):
+        return "SomeSubclass"
 
 
 def test_subclassing():
     if not PYPY:
         a = SomeSubClass()
-        _b = str(a) + repr(a)
+        assert str(a) == "SomeSubclass"
+        assert type(a) is SomeSubClass

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,
+            subtype: *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::<pyo3::types::PyTuple>(_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, subtype)?;
                 Ok(cell as *mut pyo3::ffi::PyObject)
             })
         }

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;
 
@@ -72,25 +71,29 @@ impl<T> PyClassAlloc for T
 where
     T: PyTypeInfo + PyClassWithFreeList,
 {
-    unsafe fn alloc(py: Python) -> *mut Self::Layout {
-        if let Some(obj) = <Self as PyClassWithFreeList>::get_free_list().pop() {
-            ffi::PyObject_Init(obj, Self::type_object_raw(py) as *const _ as _);
-            obj as _
-        } else {
-            crate::pyclass::default_alloc::<Self>(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) = <Self as PyClassWithFreeList>::get_free_list().pop() {
+                ffi::PyObject_Init(obj, subtype);
+                return obj as _;
+            }
         }
+        crate::pyclass::default_new::<Self>(py, subtype) as _
     }
 
     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) = <Self as PyClassWithFreeList>::get_free_list().insert(obj) {
-            match Self::type_object_raw(py).tp_free {
+        if let Some(obj) = <Self as PyClassWithFreeList>::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),
             }

src/instance.rs

@@ -59,7 +59,7 @@ where
         T::BaseLayout: PyBorrowFlagLayout<T::BaseType>,
     {
         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)
     }

src/pycell.rs

@@ -334,13 +334,16 @@ impl<T: PyClass> PyCell<T> {
         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<T::BaseType>` 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<T::BaseType>,
     {
-        let base = T::alloc(py);
+        let base = T::new(py, subtype);
         if base.is_null() {
             return Err(PyErr::fetch(py));
         }

src/pyclass.rs

@@ -1,37 +1,39 @@
 //! `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;
 use std::ptr;
 
 #[inline]
-pub(crate) unsafe fn default_alloc<T: PyTypeInfo>(py: Python) -> *mut ffi::PyObject {
-    let type_obj = T::type_object_raw(py);
+pub(crate) unsafe fn default_new<T: PyTypeInfo>(
+    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::<Self>(py) as _
+    unsafe fn new(py: Python, subtype: *mut ffi::PyTypeObject) -> *mut Self::Layout {
+        default_new::<Self>(py, subtype) as _
     }
 
     /// Deallocate `#[pyclass]` on the Python heap.
@@ -40,20 +42,33 @@ 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()),
         }
     }
 }
 
-#[doc(hidden)]
-pub unsafe fn tp_free_fallback(obj: *mut ffi::PyObject) {
+fn tp_dealloc<T: PyClassAlloc>() -> Option<ffi::destructor> {
+    unsafe extern "C" fn dealloc<T>(obj: *mut ffi::PyObject)
+    where
+        T: PyClassAlloc,
+    {
+        let pool = crate::GILPool::new();
+        let py = pool.python();
+        <T as PyClassAlloc>::dealloc(py, (obj as *mut T::Layout) as _)
+    }
+    Some(dealloc::<T>)
+}
+
+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);
@@ -115,15 +130,7 @@ where
     };
 
     // dealloc
-    unsafe extern "C" fn tp_dealloc_callback<T>(obj: *mut ffi::PyObject)
-    where
-        T: PyClassAlloc,
-    {
-        let pool = crate::GILPool::new();
-        let py = pool.python();
-        <T as PyClassAlloc>::dealloc(py, (obj as *mut T::Layout) as _)
-    }
-    type_object.tp_dealloc = Some(tp_dealloc_callback::<T>);
+    type_object.tp_dealloc = tp_dealloc::<T>();
 
     // type size
     type_object.tp_basicsize = std::mem::size_of::<T::Layout>() as ffi::Py_ssize_t;

src/pyclass_init.rs

@@ -114,14 +114,30 @@ impl<T: PyClass> PyClassInitializer<T> {
         PyClassInitializer::new(subclass_value, self)
     }
 
-    // Create a new PyCell + initialize it
-    #[doc(hidden)]
-    pub unsafe fn create_cell(self, py: Python) -> PyResult<*mut PyCell<T>>
+    // Create a new PyCell and initialize it.
+    pub(crate) fn create_cell(self, py: Python) -> PyResult<*mut PyCell<T>>
     where
         T: PyClass,
         T::BaseLayout: PyBorrowFlagLayout<T::BaseType>,
     {
-        let cell = PyCell::internal_new(py)?;
+        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,
+        subtype: *mut crate::ffi::PyTypeObject,
+    ) -> PyResult<*mut PyCell<T>>
+    where
+        T: PyClass,
+        T::BaseLayout: PyBorrowFlagLayout<T::BaseType>,
+    {
+        let cell = PyCell::internal_new(py, subtype)?;
         self.init_class(&mut *cell);
         Ok(cell)
     }

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::<SuperClass>();
+    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();
+}