Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG]: Overriding __new__ (or type-level __call__) for a pybind11 class should be supported #3253

Closed
3 tasks done
jbms opened this issue Sep 9, 2021 · 5 comments
Closed
3 tasks done

Comments

@jbms
Copy link
Contributor

jbms commented Sep 9, 2021

Required prerequisites

Problem description

It would be nice if, for a given pybind11-defined class Foo, there were a mechanism for overriding the object returned by a call to Foo(...).

In Python, this can be done by overriding __new__ or by using a custom metaclass with an overridden __call__ method:

https://stackoverflow.com/questions/6760685/creating-a-singleton-in-python

One case where this would be useful is to be able to return a reference to an existing object. Currently, even if we use std::shared_ptr as the holder type and return a reference to an existing object from a pybind11::init function, we will end up with a new Python object, which is unfortunate.

As far as I am aware, that problem is impossible to remedy from __init__, since by the time __init__ is called, the Python object has already been created. Instead, we must either use __new__ or the metaclass __call__.

Currently, pybind11 does not support defining __new__. Attempting to define a method named "__new__" results in an error due to this line:

chain = (detail::function_record *) rec_capsule;

The sibling (the existing __new__ function) is not a PyCapsule, which causes this to fail. Incidentally, pybind11::capsule::{get,set}_pointer have a bug, in that they call pybind11_fail without calling PyErr_Clear(), which leads to an assertion failure elsewhere due to an unexpected error indicator state.

I suspect that it may take a decent amount of work beyond addressing this immediate error to make __new__ work, though.

An alternative workaround is to define a metaclass. That works, but is non-trivial since pybind11 itself provides no support for defining metaclasses, and you must take care to inherit from pybind11's own metaclass.

Reproducible example code

No response

@jbms jbms added the triage New bug, unverified label Sep 9, 2021
@Skylion007
Copy link
Collaborator

PR welcome on this front.
To fix the immediate issue, this can be solved by using the isinstance function to check if it's a capsule before casting. We actually do have metaclass support call in PyBind 's metaclass and currently do use it ensure the base init function is called here: #2152 so feel free to send a PR to modify that with the additional needed functionality.

I think there are also some open issues that show hacks to define metaclasses with PyBind, so we could support if there is demand.

Also please feel free to send a PR to fix the bug pointer methods for the PyCapsule.

@Skylion007
Copy link
Collaborator

Skylion007 commented Sep 10, 2021

--- a/include/pybind11/pybind11.h
+++ b/include/pybind11/pybind11.h
@@ -416,7 +416,13 @@ protected:
         detail::function_record *chain = nullptr, *chain_start = rec;
         if (rec->sibling) {
             if (PyCFunction_Check(rec->sibling.ptr())) {
-                auto rec_capsule = reinterpret_borrow<capsule>(PyCFunction_GET_SELF(rec->sibling.ptr()));
+				auto self = PyCFunction_GET_SELF(rec->sibling.ptr());
+                capsule rec_capsule;
+				if (isinstance<capsule>(self)){
+				    rec_capsule = reinterpret_borrow<capsule>(self);
+				} else {
+				    rec_capsule = capsule(self);
+				}
                 chain = (detail::function_record *) rec_capsule;
                 /* Never append a method to an overload chain of a parent class;
                    instead, hide the parent's overloads in this case */
diff --git a/tests/test_class.cpp b/tests/test_class.cpp
index 246b6c1..ea70a36 100644
--- a/tests/test_class.cpp
+++ b/tests/test_class.cpp
@@ -47,10 +47,26 @@ TEST_SUBMODULE(class_, m) {
         }
         ~NoConstructor() { print_destroyed(this); }
     };
+    struct NoConstructorNew {
+        NoConstructorNew() = default;
+        NoConstructorNew(const NoConstructorNew &) = default;
+        NoConstructorNew(NoConstructorNew &&) = default;
+        static NoConstructorNew *new_instance() {
+            auto *ptr = new NoConstructorNew();
+            print_created(ptr, "via new_instance");
+            return ptr;
+        }
+        ~NoConstructorNew() { print_destroyed(this); }
+    };

     py::class_<NoConstructor>(m, "NoConstructor")
         .def_static("new_instance", &NoConstructor::new_instance, "Return an instance");

+    py::class_<NoConstructorNew>(m, "NoConstructorNew")
+	.def_property_readonly_static("__new__", [](py::object){
+              return py::cpp_function([](py::object){return NoConstructorNew::new_instance();
+        });});
+
     // test_inheritance
     class Pet {
     public:
diff --git a/tests/test_class.py b/tests/test_class.py
index 85d4531..6171a90 100644
--- a/tests/test_class.py
+++ b/tests/test_class.py
@@ -25,6 +25,16 @@ def test_instance(msg):
     assert cstats.alive() == 0


[email protected]("env.PY2")
+def test_instance_new(msg):
+    instance = m.NoConstructorNew() #.__new__(m.NoConstructor.__class__)
+    cstats = ConstructorStats.get(m.NoConstructorNew)
+    assert cstats.alive() == 1
+    del instance
+    assert cstats.alive() == 0
+
+
+
 def test_type():
     assert m.check_type(1) == m.DerivedClass1
     with pytest.raises(RuntimeError) as execinfo:

@Skylion007
Copy link
Collaborator

So solving the sibling bug was pretty trivial, the bigger issue though is that new is not respected on our PyBind11 init in detail/init.h. We supply our own new function which calls our init and we need to make sure that we can override that. Also we can call class methods on Pybind11 objects, but it's a little hacky and we probably should get better first class support on this front so the syntax isn't quite as hacky: #1693

@Skylion007
Copy link
Collaborator

Closed by #3265

@wuxian08
Copy link

wuxian08 commented Jul 12, 2024

With #3265 we can safely define a Singleton:

struct Singleton {
    static Singleton *get_instance() {
        static auto ptr = new Singleton();
        return ptr;
    }
private:
    Singleton()  {
        print_created(this, "via get_instance");
    }
    Singleton(const Singleton &) = delete;
    Singleton(Singleton &&) = delete;
    ~Singleton() { print_destroyed(this); }
};

py::class_<Singleton, std::unique_ptr<Singleton, py::nodelete>>(m, "Singleton")
    .def(py::init([]() { return nullptr; }))  
    .def_static("__new__",
                [](const py::object &) { return Singleton::get_instance(); });

In python, this runs smoothly

a = Singleton()
b = Singleton()
assert a is b

Meanwhile, there is a slight issue: When I add return_value_policy::reference_internal to __new__, python would throw an exception when trying to "construct" Singleton. This is a bit confusing and hopefully it can be solved without too much work.

py::class_<Singleton, std::unique_ptr<Singleton, py::nodelete>>(m, "Singleton")
    .def(py::init([]() { return nullptr; })) 
     // this would not help either:
    // .def(py::init([]() { return nullptr; }), py::return_value_policy::reference_internal)
    .def_static("__new__",
                [](const py::object &) { return Singleton::get_instance(); }, 
                py::return_value_policy::reference_internal);
In [2]: a = m.Singleton()
### test_submodule_class_(module_&)::Singleton @ 0x564a28ebc150 created via get_instance
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[2], line 1
----> 1 a = m.Singleton()

TypeError: pybind11_tests.class_.Singleton.__init__() must be called when overriding __init__

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants