Skip to content

Check scope's __dict__ instead of using hasattr when registering classes and exceptions #2335

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Oct 8, 2020
Merged

Check scope's __dict__ instead of using hasattr when registering classes and exceptions #2335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
95342d9
Check scope's __dict__ instead of using hasattr when registering clas…
YannickJadoul Jul 27, 2020
1eaacb7
Extend test_base_and_derived_nested_scope test
YannickJadoul Jul 28, 2020
8368c76
Add tests on error being thrown registering duplicate classes
YannickJadoul Oct 5, 2020
0f66572
Circumvent bug with combination of test_class.py::test_register_dupli…
YannickJadoul Oct 8, 2020
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
4 changes: 2 additions & 2 deletions include/pybind11/pybind11.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1012,7 +1012,7 @@ class generic_type : public object {
PYBIND11_OBJECT_DEFAULT(generic_type, object, PyType_Check)
protected:
void initialize(const type_record &rec) {
if (rec.scope && hasattr(rec.scope, rec.name))
if (rec.scope && hasattr(rec.scope, "__dict__") && rec.scope.attr("__dict__").contains(rec.name))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Potentially, we could drop hasattr(rec.scope, "__dict__"), if we're happy throwing an exception when the user tries to use a scope that doesn't have a __dict__ attribute?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The idea of throwing an exception in this situation worries me, a lot.

I'm surprised to see the strong runtime performance concerns from both Yannick and Boris. Is it correct that this code only runs when a pybind11 module is imported? Is that so slow that people complain already? What's the incremental extra runtime overhead? My first guess is it's no measurable, given all the other things happening at import time (files are opened, dlls loaded), but maybe I'm overlooking something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm surprised to see the strong runtime performance concerns from both Yannick and Boris.

It's not really a concern. It is only run once at import time, so it hardly matter.

What's the incremental extra runtime overhead?

I wasn't measuring import whatever, but rather this specific patch in isolation.

>>> timeit.timeit(stmt='B().__dict__.get("X")', setup='class A:\n  def X(self):pass\nclass B(A):pass')
0.17064448199380422
>>> timeit.timeit(stmt='hasattr(B(), "X")', setup='class A:\n  def X(self):pass\nclass B(A):pass')
0.12798299700079951

Again, this is executed exactly once. It really shouldn't be a concern. I just wanted to satisfy my own curiosity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not too worried about it, either! Just want to point it out.
The reasoning is/was, if hasattr(rec.scope, "__dict__") fails, most likely the later scope.attr(name) = *this; will also fail/throw an exception?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I misunderstood this code initially (github is helpfully hiding the next line...). Would this be logically equivalent?

if (rec.scope && hasattr(rec.scope, rec.name) && hasattr(rec.scope, "__dict__") && rec.scope.attr("__dict__").contains(rec.name))

Now suspecting: the existing hasattr returns true, but we want to raise an error only if we find that the attribute is defined in __dict__?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure how to solve this, though. Checking hasattr/getattr is too wide (i.e., checks base classes), while __dict__ is too strict (would miss the crazy case where __getattr__/__setattr__, or __getattribute__/__setattribute__, would be overwritten).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we shouldn't fix it? If it's this hard to do it right, perhaps we shouldn't "fix" it and break things that currently work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting suggestion, yes. I must say the current situation feels wrong, though, where it matters in which order you register a class first in the base class or first in a derived class.

And the above shouldn't break too much. there's a thing where, if getattr and setattr are overwritten, things might already be broken anyway. In any case, the scope should be a class or a module, no?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In any case, the scope should be a class or a module, no?

Do you want the good news or the bad news? The bad news?

>>> def f():print('f')
...
>>> def g():print('g')
...
>>> f.g = g
>>> del g
>>> f.g()
g
>>> f()
f

The good news is that using functions as scopes isn't allowed with pybind. I tried. I really really tried to make pybind accept a function object as a scope.

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'm indeed not sure if this is good or bad news :-/

pybind11_fail("generic_type: cannot initialize type \"" + std::string(rec.name) +
"\": an object with that name is already defined");

Expand Down Expand Up @@ -1930,7 +1930,7 @@ class exception : public object {
std::string full_name = scope.attr("__name__").cast<std::string>() +
std::string(".") + name;
m_ptr = PyErr_NewException(const_cast<char *>(full_name.c_str()), base.ptr(), NULL);
if (hasattr(scope, name))
if (hasattr(scope, "__dict__") && scope.attr("__dict__").contains(name))
pybind11_fail("Error during initialization: multiple incompatible "
"definitions with name \"" + std::string(name) + "\"");
scope.attr(name) = *this;
Expand Down
40 changes: 40 additions & 0 deletions tests/test_class.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,7 @@ TEST_SUBMODULE(class_, m) {
struct IsNonFinalFinal {};
py::class_<IsNonFinalFinal>(m, "IsNonFinalFinal", py::is_final());

// test_exception_rvalue_abort
struct PyPrintDestructor {
PyPrintDestructor() = default;
~PyPrintDestructor() {
Expand All @@ -417,6 +418,7 @@ TEST_SUBMODULE(class_, m) {
.def(py::init<>())
.def("throw_something", &PyPrintDestructor::throw_something);

// test_multiple_instances_with_same_pointer
struct SamePointer {};
static SamePointer samePointer;
py::class_<SamePointer, std::unique_ptr<SamePointer, py::nodelete>>(m, "SamePointer")
Expand All @@ -426,6 +428,44 @@ TEST_SUBMODULE(class_, m) {
struct Empty {};
py::class_<Empty>(m, "Empty")
.def(py::init<>());

// test_base_and_derived_nested_scope
struct BaseWithNested {
struct Nested {};
};

struct DerivedWithNested : BaseWithNested {
struct Nested {};
};

py::class_<BaseWithNested> baseWithNested_class(m, "BaseWithNested");
py::class_<DerivedWithNested, BaseWithNested> derivedWithNested_class(m, "DerivedWithNested");
py::class_<BaseWithNested::Nested>(baseWithNested_class, "Nested")
.def_static("get_name", []() { return "BaseWithNested::Nested"; });
py::class_<DerivedWithNested::Nested>(derivedWithNested_class, "Nested")
.def_static("get_name", []() { return "DerivedWithNested::Nested"; });

// test_register_duplicate_class
struct Duplicate {};
struct OtherDuplicate {};
struct DuplicateNested {};
struct OtherDuplicateNested {};
m.def("register_duplicate_class_name", [](py::module_ m) {
py::class_<Duplicate>(m, "Duplicate");
py::class_<OtherDuplicate>(m, "Duplicate");
});
m.def("register_duplicate_class_type", [](py::module_ m) {
py::class_<OtherDuplicate>(m, "OtherDuplicate");
py::class_<OtherDuplicate>(m, "YetAnotherDuplicate");
});
m.def("register_duplicate_nested_class_name", [](py::object gt) {
py::class_<DuplicateNested>(gt, "DuplicateNested");
py::class_<OtherDuplicateNested>(gt, "DuplicateNested");
});
m.def("register_duplicate_nested_class_type", [](py::object gt) {
py::class_<OtherDuplicateNested>(gt, "OtherDuplicateNested");
py::class_<OtherDuplicateNested>(gt, "YetAnotherDuplicateNested");
});
}

template <int N> class BreaksBase { public:
Expand Down
35 changes: 35 additions & 0 deletions tests/test_class.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,3 +383,38 @@ def test_multiple_instances_with_same_pointer(capture):
# No assert: if this does not trigger the error
# pybind11_fail("pybind11_object_dealloc(): Tried to deallocate unregistered instance!");
# and just completes without crashing, we're good.


# https://github.com/pybind/pybind11/issues/1624
def test_base_and_derived_nested_scope():
assert issubclass(m.DerivedWithNested, m.BaseWithNested)
assert m.BaseWithNested.Nested != m.DerivedWithNested.Nested
assert m.BaseWithNested.Nested.get_name() == "BaseWithNested::Nested"
assert m.DerivedWithNested.Nested.get_name() == "DerivedWithNested::Nested"


@pytest.mark.skip("See https://github.com/pybind/pybind11/pull/2564")
def test_register_duplicate_class():
import types
module_scope = types.ModuleType("module_scope")
with pytest.raises(RuntimeError) as exc_info:
m.register_duplicate_class_name(module_scope)
expected = ('generic_type: cannot initialize type "Duplicate": '
'an object with that name is already defined')
assert str(exc_info.value) == expected
with pytest.raises(RuntimeError) as exc_info:
m.register_duplicate_class_type(module_scope)
expected = 'generic_type: type "YetAnotherDuplicate" is already registered!'
assert str(exc_info.value) == expected

class ClassScope:
pass
with pytest.raises(RuntimeError) as exc_info:
m.register_duplicate_nested_class_name(ClassScope)
expected = ('generic_type: cannot initialize type "DuplicateNested": '
'an object with that name is already defined')
assert str(exc_info.value) == expected
with pytest.raises(RuntimeError) as exc_info:
m.register_duplicate_nested_class_type(ClassScope)
expected = 'generic_type: type "YetAnotherDuplicateNested" is already registered!'
assert str(exc_info.value) == expected