-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Throw TypeError when subclasses forget to call __init__ #2152
Conversation
0146834
to
5e5d3cf
Compare
This change looks great, and I am happy to merge it. Performance impact should be minimal as you say. But can you please fix pep8? (See the failing "STYLE" test). |
In particular, there is a long line with the error message string that should be split. |
5e5d3cf
to
8383222
Compare
FIxed, sorry for the delay! |
Ready for merge @wjakob , thanks! |
8383222
to
674a400
Compare
Still ready. :) |
Ping. |
Ping? :( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As gently pinged by @virtuald on Gitter, this should be ready to finish, as it was already reviewed.
So this functionality is not present when a custom metaclass is used? For completeness, it might be nice to mention that in the docs, still.
Thanks @virtuald, and for the reminder @YannickJadoul. |
@YannickJadoul the docs I added indirectly say what you want?
Implying that if you set your own metaclass then you'll get different behavior... ? |
OK, yes, that's a bit implicit, but good enough. Using custom metaclasses comes with a lot of caveats anyway, currently. |
Cherry-pick of pybind#2152 Co-authored-by: Dustin Spicuzza <dustin@virtualroadside.com>
It turns out this PR #2152 leads to a test breakage related to this OSS deepmind code: The situation top-down:
When |
Are you sure? "No
|
When naively adding a call to
Looking at the wrapper code (pyspiel.cc link in previous comment), there isn't an But I'm not sure where to start fixing. Fake an |
This is just checking to see if this has been initialized; currently it is not. If pyspiel.Game is not default constructible, why would SomeGame be? It needs to call one of the factory functions, or a constructor of some sort, or otherwise pyspiel.Game is not fully constructed. |
Thanks Henry, I'll dig in deeper to see what they are doing currently. I know eventually they are calling |
It's possible that inheriting from a class implemented only by a factory function might not be properly constructing the holder? Seems weird. |
Thanks @bstaletic, @henryiii, @virtuald for taking a look! That made it clear I wasn't overlooking something obvious. After digging in I found a surprising but simple explanation why there were no problems with the existing code (I even tried ASAN & MSAN before digging in): the only function of inheriting from After that realization the fix was very simple (maybe to be refined by the open_spiel people):
Interesting to see how any little hole gets inadvertently (mis)used somehow. @virtuald, thanks so much for plugging this one! |
Should we adjust the error messages to make this sort of thing easier to diagnose in the future? |
What would you propose? |
Here's another chance to bring up your favorite law of software engineering, @bstaletic ;-) |
is a little bit cryptic, to @virtuald's point. How about something roughly like:
(I'm also fine with it as is, the biggest problem above was the usage of it, not the message) |
BTW: I worked out another suggested solution for the open_spiel folks, adding a @henryiii and @virtuald: I think some small tweaks to the docs and error message could indeed help a lot.
The comment seems to need updating, how about: It would also be good to add a sentence like: An extension class without an For the error message, @virtuald, do you know how much trouble it is to check if the base class implements
Do we have a mechanism for short URLs pointing into the docs? |
I've now just spent half an hour looking around into this code and pybind11's dark magic, thinking whether there ought to be a way to surpass this check when a class is used as "interface", but it feels rather dangerous since it allows to access uninitialized data from Python (triggering UB from Python). There's a way to override The docs should be updated indeed; that part must've been overlooked when merging this PR. I'll go over it and see if I can fix it.
I'm not sure this is something we'd want in the error message, though. This is a message that users of a pybind11-library might get to see, and pybind11's docs are meant as a reference for the developers. You don't want to confuse Python users with C++ thing, if you ask me. I was not a fan of the error message either (since it's basically just 2 steps: a. you need to call
Careful with this, though. Inheritance in pybind11 is complex and can be subtle, so I'd prefer users would read the whole section. And if someone already knows how pybind11 interacts with inheritance and subclassing, it should be easy enough to scan that section and find the relevant part, no? |
@rwgk Actually, in the case of class X {
public:
static X create() { return X(42); }
int getZ() const { return z; }
virtual ~X() = default;
private:
X(int y) : z(y) {}
int z;
};
void f(const X &x) {
py::print(x.getZ());
}
class PyX : public X {
public:
PyX() : X(X::create()) {}
};
PYBIND11_MODULE(example, m)
{
py::class_<X, PyX>(m, "X")
.def_property_readonly("z", &X::getZ)
.def(py::init([]() { throw py::type_error("Cannot construct an X object without subclassing."); return static_cast<X*>(nullptr); },
[]() { return PyX(); }));
m.def("create", &X::create);
m.def("f", &f);
} This results in: Python 3.8.0 (default, Oct 28 2019, 16:14:01)
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import example
>>> example.X
<class 'example.X'>
>>> example.X()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: Cannot construct an X object without subclassing.
>>> class Y:
...
KeyboardInterrupt
>>> class Y(example.X):
... pass
...
>>> Y()
<__main__.Y object at 0x7f1bdfc8a360>
>>> class Y(example.X):
... def __init__(self):
... example.X.__init__(self)
...
>>> Y()
<__main__.Y object at 0x7f1bdfc93f40>
>>> |
See #2429. |
Hi @YannickJadoul, my current solution is this:
There is a list of known/allowed names to pick from. It works with one I picked randomly. I asked for advice about creating a "bare" game, to use your word for it. Let's see what they say. To have an
Mostly out of curiosity, I also tried this:
It builds fine but |
Nice, you bumped into a bug! :-D I managed to figure out what goes wrong; see #2430.
Right, I understand. I was mostly trying to point out the trampoline class, which allows to add the |
Ha, I found a hacky way to determine if the constructor was overridden or not. I'll make a PR. |
Hidden bug discovered while preparing for a third_party/pybind11 update (testing with current pybind11 github master branch). Backward compatible. Relevant pybind11 change: pybind/pybind11#2152 PiperOrigin-RevId: 327643330 Change-Id: Ic867f6d1e1932c3898d4a6f8fb63acde4739204b
Backward compatible change preparing for pybind11 update. Relevant pybind11 change: * pybind/pybind11#2152 * Throw TypeError when subclasses forget to call __init__ PiperOrigin-RevId: 328182655 Change-Id: I8dc8cd69ee328f95bdca58b0f9045d65d11307a8
… *>()` introduced with PR pybind#2152 The `reinterpret_cast<instance *>(self)` is unsafe if `__new__` is mocked, which was actually found in the wild: the mock returned `None` for `self`. This was inconsequential because `inst` is currently cast straight back to `PyObject *` to compute `all_type_info()`, which is empty if `self` is not a pybind11 `instance`, and then `inst` is never dereferenced. However, the unsafe detour through `instance *` is easily avoided and the updated implementation is less prone to accidents while debugging or refactoring.
#4762) * Equivalent of google/clif@5718e4d * Resolve clang-tidy errors. * Moving test_PPCCInit() first changes the behavior! * Resolve new Clang dev C++11 errors: ``` The CXX compiler identification is Clang 17.0.0 ``` ``` pytypes.h:1615:23: error: identifier '_s' preceded by whitespace in a literal operator declaration is deprecated [-Werror,-Wdeprecated-literal-operator] ``` ``` cast.h:1380:26: error: identifier '_a' preceded by whitespace in a literal operator declaration is deprecated [-Werror,-Wdeprecated-literal-operator] ``` * Resolve gcc 4.8.5 error: ``` pytypes.h:1615:12: error: missing space between '""' and suffix identifier ``` * Specifically exclude `__clang__` * Snapshot of debugging code (does NOT pass pre-commit checks). * Revert "Snapshot of debugging code (does NOT pass pre-commit checks)." This reverts commit 1d4f9ff. * [ci skip] Order Dependence Demo * Revert "[ci skip] Order Dependence Demo" This reverts commit d37b540. * One way to deal with the order dependency issue. This is not the best way, more like a proof of concept. * Move test_PC() first again. * Add `all_type_info_add_base_most_derived_first()`, use in `all_type_info_populate()` * Revert "One way to deal with the order dependency issue. This is not the best way, more like a proof of concept." This reverts commit eb09c6c. * clang-tidy fixes (automatic) * Add `is_redundant_value_and_holder()` and use to avoid forcing `__init__` overrides when they are not needed. * Streamline implementation and avoid unsafe `reinterpret_cast<instance *>()` introduced with PR #2152 The `reinterpret_cast<instance *>(self)` is unsafe if `__new__` is mocked, which was actually found in the wild: the mock returned `None` for `self`. This was inconsequential because `inst` is currently cast straight back to `PyObject *` to compute `all_type_info()`, which is empty if `self` is not a pybind11 `instance`, and then `inst` is never dereferenced. However, the unsafe detour through `instance *` is easily avoided and the updated implementation is less prone to accidents while debugging or refactoring. * Fix actual undefined behavior exposed by previous changes. It turns out the previous commit message is incorrect, the `inst` pointer is actually dereferenced, in the `value_and_holder` ctor here: https://github.com/pybind/pybind11/blob/f3e0602802c7840992c97f4960515777cad6a5c7/include/pybind11/detail/type_caster_base.h#L262-L263 ``` 259 // Main constructor for a found value/holder: 260 value_and_holder(instance *i, const detail::type_info *type, size_t vpos, size_t index) 261 : inst{i}, index{index}, type{type}, 262 vh{inst->simple_layout ? inst->simple_value_holder 263 : &inst->nonsimple.values_and_holders[vpos]} {} ``` * Add test_mock_new() * Experiment: specify indirect bases * Revert "Experiment: specify indirect bases" This reverts commit 4f90d85. * Add `all_type_info_check_for_divergence()` and some tests. * Call `all_type_info_check_for_divergence()` also from `type_caster_generic::load_impl<>` * Resolve clang-tidy error: ``` include/pybind11/detail/type_caster_base.h:795:21: error: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty,-warnings-as-errors] if (matching_bases.size() != 0) { ^~~~~~~~~~~~~~~~~~~~~~~~~~ !matching_bases.empty() ``` * Revert "Resolve clang-tidy error:" This reverts commit df27188. * Revert "Call `all_type_info_check_for_divergence()` also from `type_caster_generic::load_impl<>`" This reverts commit 5f5fd6a. * Revert "Add `all_type_info_check_for_divergence()` and some tests." This reverts commit 0a9599f.
* Call from new `tp_init_intercepted()` (adopting mechanism first added in PyCLIF: google/clif@7cba87d). * Remove `pybind11_meta_call()` (which was added with pybind/pybind11#2152).
* Call from new `tp_init_intercepted()` (adopting mechanism first added in PyCLIF: google/clif@7cba87d). * Remove `pybind11_meta_call()` (which was added with pybind/pybind11#2152).
…ng __init__` safety feature to work for any metaclass. (#30095) * Also wrap with `py::metaclass((PyObject *) &PyType_Type)` * Transfer additional tests from PyCLIF python_multiple_inheritance_test.py * Expand tests to fully cover wrapping with alternative metaclasses. * * Factor out `ensure_base_init_functions_were_called()`. * Call from new `tp_init_intercepted()` (adopting mechanism first added in PyCLIF: google/clif@7cba87d). * Remove `pybind11_meta_call()` (which was added with pybind/pybind11#2152). * Bug fix (maybe actually two bugs?): simplify condition to `type->tp_init != tp_init_intercepted` * Removing `Py_DECREF(self)` that leads to MSAN failure (Google toolchain). ``` ==6380==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x5611589c9a58 in Py_DECREF third_party/python_runtime/v3_11/Include/object.h:537:9 ... Uninitialized value was created by a heap deallocation #0 0x5611552757b0 in free third_party/llvm/llvm-project/compiler-rt/lib/msan/msan_interceptors.cpp:218:3 #1 0x56115898e06b in _PyMem_RawFree third_party/python_runtime/v3_11/Objects/obmalloc.c:154:5 #2 0x56115898f6ad in PyObject_Free third_party/python_runtime/v3_11/Objects/obmalloc.c:769:5 #3 0x561158271bcc in PyObject_GC_Del third_party/python_runtime/v3_11/Modules/gcmodule.c:2407:5 #4 0x7f21224b070c in pybind11_object_dealloc third_party/pybind11/include/pybind11/detail/class.h:483:5 #5 0x5611589c2ed0 in subtype_dealloc third_party/python_runtime/v3_11/Objects/typeobject.c:1463:5 ... ``` * IncludeCleaner fixes (Google toolchain). * Restore `type->tp_call = pybind11_meta_call;` for PyPy only. * pytest.skip("ensure_base_init_functions_were_called() does not work with PyPy and Python `type` as metaclass") * Do not intercept our own `tp_init` function (`pybind11_object_init`). * Add `derived_tp_init_registry` weakref-based cleanup. * Replace `assert()` with `if` to resolve erroneous `lambda capture 'type' is not used` diagnostics (many CI jobs; seems to be a clang issue). * Add `derived_tp_init_registry()->count(type) == 0` condition. * Changes based on feedback from @rainwoodman * Use PYBIND11_INIT_SAFETY_CHECKS_VIA_* macros, based on suggestion from @rainwoodman
…e (#30056) * Snapshot of pybind#4762 applied to pywrapcc * Universal `bases.size() != vhs.size()` (not as `assert()`) * Revert "Universal `bases.size() != vhs.size()` (not as `assert()`)" This reverts commit 4c2407d17e982e1512f43ad89bf8752c0d2c7fe0. * Streamline implementation and avoid unsafe `reinterpret_cast<instance *>()` introduced with PR pybind#2152 The `reinterpret_cast<instance *>(self)` is unsafe if `__new__` is mocked, which was actually found in the wild: the mock returned `None` for `self`. This was inconsequential because `inst` is currently cast straight back to `PyObject *` to compute `all_type_info()`, which is empty if `self` is not a pybind11 `instance`, and then `inst` is never dereferenced. However, the unsafe detour through `instance *` is easily avoided and the updated implementation is less prone to accidents while debugging or refactoring. * Fix actual undefined behavior exposed by previous changes. It turns out the previous commit message is incorrect, the `inst` pointer is actually dereferenced, in the `value_and_holder` ctor here: https://github.com/pybind/pybind11/blob/f3e0602802c7840992c97f4960515777cad6a5c7/include/pybind11/detail/type_caster_base.h#L262-L263 ``` 259 // Main constructor for a found value/holder: 260 value_and_holder(instance *i, const detail::type_info *type, size_t vpos, size_t index) 261 : inst{i}, index{index}, type{type}, 262 vh{inst->simple_layout ? inst->simple_value_holder 263 : &inst->nonsimple.values_and_holders[vpos]} {} ``` * Add test_mock_new()
Forgetting to call
__init__
in inherited classes is one of the biggest issues my users run into (and it's frustrating because it typically causes a segfault), I'd love to see this merged. It would make pybind11 so much more usable!... this probably causes a minor performance impact when creating new objects. I would expect it to be in the noise, though I haven't looked at the timing yet.