using subclass, args on new, and overriding __init__ fails

[alonblade]

This issue is about sub classing with overridden init ; This bug prevents it. Details below.

🐛 Bug Reports

When reporting a bug, please provide the following information. If this is not a bug report you can just discard this template.

🌍 Environment

• Your operating system and version: linux, ubuntu focal (20.04)
• Your python version: 3.8
• How did you install python (e.g. apt or pyenv)? Did you use a virtualenv? ubuntu package (apt-get). using a virtualenv for testing.
• Your Rust version (rustc --version): rustc 1.54.0-nightly (b663c0f4f 2021-05-29)
• Your PyO3 version: master 8bf3ade (v0.8.0-1694-g8bf3adee3) (Merge pull request Simplify code generated for for_each_method_def and for_each_proto_slot #1641 from 1tgr/for-each)
• Have you tried using latest PyO3 main (replace version = "0.x.y" with git = "https://github.com/PyO3/pyo3")?yes.

💥 Reproducing

I have a branch with the augmented test (examples/pyo3-pytests/src/subclassing.rs & examples/pyo3-pytests/tests/test_subclassing.py), at https://github.com/alonblade/pyo3/tree/add-test-for-inheritance-with-init ; Here is the change:

#[pyclass(subclass)]
pub struct SubclassableWithParameter {}

#[pymethods]
impl SubclassableWithParameter {
    #[new]
    fn new(foo: bool) -> Self {
        SubclassableWithParameter {}
    }
}

class SubclassWithExtraInitArguments(SubclassableWithParameter):
    def __init__(self, bar):
        print("before super init")
        super().__init__(foo=bar * 2)


def test_subclass_with_init():
    s = SubclassWithExtraInitArguments(10)

And the output

tests/test_subclassing.py .F                                                                                                                                                                                  [100%]

===================================================================================================== FAILURES ======================================================================================================
______________________________________________________________________________________________ test_subclass_with_init ______________________________________________________________________________________________

    def test_subclass_with_init():
>       s = SubclassWithExtraInitArguments(10)
E       TypeError: argument 'foo': 'int' object cannot be converted to 'PyBool'

tests/test_subclassing.py:27: TypeError

[alonblade]

@davidhewitt if you could point me in the right direction I could try to fix this.

[davidhewitt]

I think if you try overriding __new__ instead of __init__, things should just work?

I agree that this is both suprising and undocumented. We should add some information to the guide about this. I'd also be happy to discuss whether this should be changed.

[alonblade]

The problem with overriding __new__ instead of __init__ is that I'm trying to replace an existing python implementation, where there is existing code that does just this - inherits and uses a different signature of __init__.

…

On Tue, Jun 1, 2021 at 9:08 PM David Hewitt ***@***.***> wrote: I think if you try overriding __new__ instead of __init__, things should just work? I agree that this is both suprising and undocumented. We should add some information to the guide about this. I'd also be happy to discuss whether this should be changed. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1644 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ASHRLMOBGGJFDN47G7DKXR3TQUOZDANCNFSM452MSZZQ> .

[davidhewitt]

Note that you can override both (with the same arguments):

class SubclassWithExtraInitArguments(SubclassableWithParameter):
    def __new__(cls, bar):
        print("before super new")
        return super().__new__(cls, foo=bool(bar))

    def __init__(self, bar):
        print("before super init")
        super().__init__()

The above passes the "test" for me. I appreciate that it's perhaps not ideal for your use case.

To explain the current situation, I think quoting from the Python docs is helpful: https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_new

The tp_new function should call subtype->tp_alloc(subtype, nitems) to allocate space for the object, and then do only as much further initialization as is absolutely necessary. Initialization that can safely be ignored or repeated should be placed in the tp_init handler. A good rule of thumb is that for immutable types, all initialization should take place in tp_new, while for mutable types, most initialization should be deferred to tp_init.

In our case, at the moment initializing the Rust pyclass object is part of the "initialization which is absolutely necessary". When we create the PyCell we place the Rust struct into it so that Rust doesn't have to stare at uninitialized memory (which it doesn't like very much).

I'm not saying that this is how it must stay; I'm aware it has drawbacks (and it existed before I was maintaining the project). I'd be happy to discuss alternative designs if we can come up with something better / more flexible. Ideally we'd be able to allow users to choose whether to use __new__ or __init__, so that use cases like yours can be supported.

[armoha]

I have similar problem with self-referenceable class:
(minimized example, original Cython code is here: https://github.com/phu54321/eudplib/blob/master/eudplib/core/allocator/constexpr.pyx#L31-L42)

# Cython code to be rewritten
cdef class ConstExpr:
    def __init__(self, baseobj, offset=0):
        self.baseobj = baseobj
        self.offset = offset

    def __add__(self, other):
        return ConstExpr(self.baseobj, self.offset + offset)

// Rewrite Cython code in Rust
use pyo3::prelude::*;
use pyo3::types::PyNone;
use std::sync::Arc;

pub(crate) struct ConstExpr {
    baseobj: Option<Arc<Self>>,
    offset: i32,
}

#[derive(Clone)]
#[pyclass(frozen, subclass, name = "ConstExpr")]
pub struct PyConstExpr(Arc<ConstExpr>);

#[pymethods]
impl PyConstExpr {
    #[new]
    #[pyo3(signature = (baseobj, offset=0))]
    fn new(baseobj: &PyAny, offset: i32) -> PyResult<Self> {
        let expr = if let Ok(expr) = baseobj.extract::<PyConstExpr>() {
            ConstExpr::new(Some(expr.0.clone()), offset)
        } else if baseobj.is(PyNone::get(baseobj.py())) {
            ConstExpr::new(None, offset)
        } else {
            return Err(PyTypeError::new_err(format!("{baseobj} is not a ConstExpr")));
        };
        Ok(Self(Arc::new(expr)))
    }

    fn __add__(&self, rhs: i32) -> Self {
        Self(Arc::new(ConstExpr {
            baseobj: self.0.baseobj.clone(),
            offset: self.0.offset + rhs,
        }))
    }
}

# test code
import _rust as rs


class Subclass(rs.ConstExpr):
    def __init__(self):
        super().__init__(self)  # TypeError: object.__init__() takes exactly one argument (the instance to initialize)


s = Subclass()  # TypeError: ConstExpr.__new__() missing 1 required positional argument: 'baseobj'
print(s)  # should be Subclass
print(s + 1)  # should be ConstExpr { baseobj: s, offset: 1 } 
print(s + 2)  # should be ConstExpr { baseobj: s, offset: 2 } 

I can't rewrite code as it is because in PyO3 only Python's __new__ method can be specified, and __init__ is not available, but __new__ is a static method in Python.

It's low-level advanced API so making breaking change by requiring (1) overriding both __new__ and __init__, and (2) initializing class with default value (None) and (3) calling mutating method in __init__, is not that big deal for me. But I feel somewhat sad to give up pyclass(frozen) attribute for immutable class.