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

Adapt code example in advanced/classes.rst to new handling of forgetting to call the superclass __init__ #2429

Merged

Conversation

YannickJadoul
Copy link
Collaborator

Cfr. discussion in #2152

TODO: Check how easy it is to display different error when there is no __init__.

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Yannick! IMO this is great to merge as is, to get it out of the way, and figure out the other things separately.

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @rwgk

@YannickJadoul
Copy link
Collaborator Author

TODO: Check how easy it is to display different error when there is no __init__.

I've been trying out some stuff, but this seems quite hard, as the default __init__ (which complains that no constructor has been defined) is set in tp_init, while dynamically added methods override it.

This is already quite ugly, but doesn't work in my tests:

diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h
index 8d36744f..ecd0348b 100644
--- a/include/pybind11/detail/class.h
+++ b/include/pybind11/detail/class.h
@@ -171,8 +171,16 @@ extern "C" inline PyObject *pybind11_meta_call(PyObject *type, PyObject *args, P
     // Ensure that the base __init__ function(s) were called
     for (const auto &vh : values_and_holders(instance)) {
         if (!vh.holder_constructed()) {
-            PyErr_Format(PyExc_TypeError, "%.200s.__init__() must be called when overriding __init__",
-                         vh.type->type->tp_name);
+            PyObject *self_init = PyObject_GetAttrString((PyObject*)vh.type->type, "__init__");
+            if (!self_init)
+                PyErr_Clear();
+            auto pybind11_object_init = PyObject_GetAttrString(get_internals().instance_base, "__init__");
+            auto message = (self_init == pybind11_object_init) ? 
+                           "%.200s has no __init__ and can therefore not be used as a base class in Python" :
+                           "%.200s.__init__() must be called when overriding __init__";
+            PyErr_Format(PyExc_TypeError, message, vh.type->type->tp_name);
+            Py_XDECREF(self_init);
+            Py_DECREF(pybind11_object_init);
             Py_DECREF(self);
             return nullptr;
         }

So yes, agreed, @bstaletic and @rwgk; let's just get this in and worry about the rest later :-)

@YannickJadoul YannickJadoul merged commit b3d8fec into pybind:master Aug 23, 2020
@YannickJadoul YannickJadoul deleted the docs-forgotten-subclass-init branch August 23, 2020 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants