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

Moving tp_class access, and consistent fully-qualified naming for PyPy, to detail::get_tp_name #2520

Merged
merged 2 commits into from
Oct 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions include/pybind11/cast.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@
PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
PYBIND11_NAMESPACE_BEGIN(detail)

// Forward-declaration; see detail/class.h
std::string get_fully_qualified_tp_name(PyTypeObject*);

/// A life support system for temporary objects created by `type_caster::load()`.
/// Adding a patient will keep it alive up until the enclosing function returns.
class loader_life_support {
Expand Down Expand Up @@ -342,8 +345,8 @@ PYBIND11_NOINLINE inline value_and_holder instance::get_value_and_holder(const t
"(compile in debug mode for type details)");
#else
pybind11_fail("pybind11::detail::instance::get_value_and_holder: `" +
std::string(find_type->type->tp_name) + "' is not a pybind11 base of the given `" +
std::string(Py_TYPE(this)->tp_name) + "' instance");
get_fully_qualified_tp_name(find_type->type) + "' is not a pybind11 base of the given `" +
get_fully_qualified_tp_name(Py_TYPE(this)) + "' instance");
#endif
}

Expand Down
24 changes: 13 additions & 11 deletions include/pybind11/detail/class.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,14 @@ PYBIND11_NAMESPACE_BEGIN(detail)
# define PYBIND11_SET_OLDPY_QUALNAME(obj, nameobj) setattr((PyObject *) obj, "__qualname__", nameobj)
#endif

inline std::string get_fully_qualified_tp_name(PyTypeObject *type) {
#if !defined(PYPY_VERSION)
Copy link
Collaborator

@henryiii henryiii Sep 25, 2020

Choose a reason for hiding this comment

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

What happens in 7.3.2? Does the line below start working?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so, no. I believe the @pytest.mark.xfail("env.PYPY and sys.pypy_version_info < (7, 3, 2)") is unrelated (but I just came across it because I tested with 7.3.2.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ugh .. I'd written this yesterday, but it was still a "pending review" ...

return type->tp_name;
#else
return handle((PyObject *) type).attr("__module__").cast<std::string>() + "." + type->tp_name;
#endif
}

inline PyTypeObject *type_incref(PyTypeObject *type) {
Py_INCREF(type);
return type;
Expand Down Expand Up @@ -172,7 +180,7 @@ extern "C" inline PyObject *pybind11_meta_call(PyObject *type, PyObject *args, P
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);
get_fully_qualified_tp_name(vh.type->type).c_str());
Py_DECREF(self);
return nullptr;
}
Expand Down Expand Up @@ -304,12 +312,7 @@ extern "C" inline PyObject *pybind11_object_new(PyTypeObject *type, PyObject *,
/// following default function will be used which simply throws an exception.
extern "C" inline int pybind11_object_init(PyObject *self, PyObject *, PyObject *) {
PyTypeObject *type = Py_TYPE(self);
std::string msg;
#if defined(PYPY_VERSION)
msg += handle((PyObject *) type).attr("__module__").cast<std::string>() + ".";
#endif
msg += type->tp_name;
msg += ": No constructor defined!";
std::string msg = get_fully_qualified_tp_name(type) + ": No constructor defined!";
PyErr_SetString(PyExc_TypeError, msg.c_str());
return -1;
}
Expand Down Expand Up @@ -448,7 +451,7 @@ extern "C" inline PyObject *pybind11_get_dict(PyObject *self, void *) {
extern "C" inline int pybind11_set_dict(PyObject *self, PyObject *new_dict, void *) {
if (!PyDict_Check(new_dict)) {
PyErr_Format(PyExc_TypeError, "__dict__ must be set to a dictionary, not a '%.200s'",
Py_TYPE(new_dict)->tp_name);
get_fully_qualified_tp_name(Py_TYPE(new_dict)).c_str());
return -1;
}
PyObject *&dict = *_PyObject_GetDictPtr(self);
Expand Down Expand Up @@ -476,9 +479,8 @@ extern "C" inline int pybind11_clear(PyObject *self) {
inline void enable_dynamic_attributes(PyHeapTypeObject *heap_type) {
auto type = &heap_type->ht_type;
#if defined(PYPY_VERSION) && (PYPY_VERSION_NUM < 0x06000000)
pybind11_fail(std::string(type->tp_name) + ": dynamic attributes are "
"currently not supported in "
"conjunction with PyPy!");
pybind11_fail(get_fully_qualified_tp_name(type) + ": dynamic attributes are currently not "
"supported in conjunction with PyPy!");
#endif
type->tp_flags |= Py_TPFLAGS_HAVE_GC;
type->tp_dictoffset = type->tp_basicsize; // place dict at the end
Expand Down
4 changes: 2 additions & 2 deletions include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ class cpp_function : public function {

#if !defined(NDEBUG) && !defined(PYBIND11_DISABLE_NEW_STYLE_INIT_WARNING)
if (rec->is_constructor && !rec->is_new_style_constructor) {
const auto class_name = std::string(((PyTypeObject *) rec->scope.ptr())->tp_name);
const auto class_name = detail::get_fully_qualified_tp_name((PyTypeObject *) rec->scope.ptr());
const auto func_name = std::string(rec->name);
PyErr_WarnEx(
PyExc_FutureWarning,
Expand Down Expand Up @@ -1033,7 +1033,7 @@ class generic_type : public object {
if (!type->ht_type.tp_as_buffer)
pybind11_fail(
"To be able to register buffer protocol support for the type '" +
std::string(tinfo->type->tp_name) +
get_fully_qualified_tp_name(tinfo->type) +
"' the associated class<>(..) invocation must "
"include the pybind11::buffer_protocol() annotation!");

Expand Down
11 changes: 4 additions & 7 deletions tests/test_class.py
Original file line number Diff line number Diff line change
Expand Up @@ -152,10 +152,8 @@ def __init__(self):
pass
with pytest.raises(TypeError) as exc_info:
Python()
expected = ["m.class_.Pet.__init__() must be called when overriding __init__",
"Pet.__init__() must be called when overriding __init__"] # PyPy?
# TODO: fix PyPy error message wrt. tp_name/__qualname__?
assert msg(exc_info.value) in expected
expected = "m.class_.Pet.__init__() must be called when overriding __init__"
assert msg(exc_info.value) == expected

# Multiple bases
class RabbitHamster(m.Rabbit, m.Hamster):
Expand All @@ -164,9 +162,8 @@ def __init__(self):

with pytest.raises(TypeError) as exc_info:
RabbitHamster()
expected = ["m.class_.Hamster.__init__() must be called when overriding __init__",
"Hamster.__init__() must be called when overriding __init__"] # PyPy
assert msg(exc_info.value) in expected
expected = "m.class_.Hamster.__init__() must be called when overriding __init__"
assert msg(exc_info.value) == expected


def test_automatic_upcasting():
Expand Down
2 changes: 1 addition & 1 deletion tests/test_local_bindings.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ def test_internal_locals_differ():
assert m.local_cpp_types_addr() != cm.local_cpp_types_addr()


@pytest.mark.xfail("env.PYPY")
@pytest.mark.xfail("env.PYPY and sys.pypy_version_info < (7, 3, 2)")
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Expect this to fail on PyPy < 7.3.2"

Didn't we decide to drop older versions already?

Copy link
Collaborator Author

@YannickJadoul YannickJadoul Sep 22, 2020

Choose a reason for hiding this comment

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

7.3.2 is not out, yet. And it still fails on 7.3.1 :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If 7.3.2 is out before this gets merged, yes, let's remove it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(unrelated change, btw, but something I bumped into, when testing locally)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I probably wouldn't drop it right away; approximately supporting 7.3.0+ would probably be ideal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that's probably best.

def test_stl_caster_vs_stl_bind(msg):
"""One module uses a generic vector caster from `<pybind11/stl.h>` while the other
exports `std::vector<int>` via `py:bind_vector` and `py::module_local`"""
Expand Down