diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index e40e4fe1c..1c1531bb5 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -25,23 +25,18 @@ repos: # Clang format the codebase automatically - repo: https://github.com/pre-commit/mirrors-clang-format - rev: "v17.0.3" + rev: "v17.0.4" hooks: - id: clang-format types_or: [c++, c, cuda] -# Black, the code formatter, natively supports pre-commit -- repo: https://github.com/psf/black-pre-commit-mirror - rev: "23.10.1" # Keep in sync with blacken-docs - hooks: - - id: black - -# Ruff, the Python auto-correcting linter written in Rust +# Ruff, the Python auto-correcting linter/formatter written in Rust - repo: https://github.com/astral-sh/ruff-pre-commit - rev: v0.1.2 + rev: v0.1.4 hooks: - id: ruff args: ["--fix", "--show-fixes"] + - id: ruff-format # Check static types with mypy - repo: https://github.com/pre-commit/mirrors-mypy @@ -88,7 +83,7 @@ repos: hooks: - id: blacken-docs additional_dependencies: - - black==23.3.0 # keep in sync with black hook + - black==23.* # Changes tabs to spaces - repo: https://github.com/Lucas-C/pre-commit-hooks diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index 3ece0643b..b31727167 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -189,12 +189,10 @@ extern "C" inline PyObject *pybind11_meta_call(PyObject *type, PyObject *args, P return nullptr; } - // This must be a pybind11 instance - auto *instance = reinterpret_cast(self); - // Ensure that the base __init__ function(s) were called - for (const auto &vh : values_and_holders(instance)) { - if (!vh.holder_constructed()) { + values_and_holders vhs(self); + for (const auto &vh : vhs) { + if (!vh.holder_constructed() && !vhs.is_redundant_value_and_holder(vh)) { PyErr_Format(PyExc_TypeError, "%.200s.__init__() must be called when overriding __init__", get_fully_qualified_tp_name(vh.type->type).c_str()); diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 68512b5bd..476646ee8 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -102,8 +102,22 @@ public: inline std::pair all_type_info_get_cache(PyTypeObject *type); +// Band-aid workaround to fix a subtle but serious bug in a minimalistic fashion. See PR #4762. +inline void all_type_info_add_base_most_derived_first(std::vector &bases, + type_info *addl_base) { + for (auto it = bases.begin(); it != bases.end(); it++) { + type_info *existing_base = *it; + if (PyType_IsSubtype(addl_base->type, existing_base->type) != 0) { + bases.insert(it, addl_base); + return; + } + } + bases.push_back(addl_base); +} + // Populates a just-created cache entry. PYBIND11_NOINLINE void all_type_info_populate(PyTypeObject *t, std::vector &bases) { + assert(bases.empty()); std::vector check; for (handle parent : reinterpret_borrow(t->tp_bases)) { check.push_back((PyTypeObject *) parent.ptr()); @@ -136,7 +150,7 @@ PYBIND11_NOINLINE void all_type_info_populate(PyTypeObject *t, std::vectortp_bases) { @@ -322,18 +336,29 @@ public: explicit values_and_holders(instance *inst) : inst{inst}, tinfo(all_type_info(Py_TYPE(inst))) {} + explicit values_and_holders(PyObject *obj) + : inst{nullptr}, tinfo(all_type_info(Py_TYPE(obj))) { + if (!tinfo.empty()) { + inst = reinterpret_cast(obj); + } + } + struct iterator { private: instance *inst = nullptr; const type_vec *types = nullptr; value_and_holder curr; friend struct values_and_holders; - iterator(instance *inst, const type_vec *tinfo) - : inst{inst}, types{tinfo}, - curr(inst /* instance */, - types->empty() ? nullptr : (*types)[0] /* type info */, - 0, /* vpos: (non-simple types only): the first vptr comes first */ - 0 /* index */) {} + iterator(instance *inst, const type_vec *tinfo) : inst{inst}, types{tinfo} { + if (inst != nullptr) { + assert(!types->empty()); + curr = value_and_holder( + inst /* instance */, + (*types)[0] /* type info */, + 0, /* vpos: (non-simple types only): the first vptr comes first */ + 0 /* index */); + } + } // Past-the-end iterator: explicit iterator(size_t end) : curr(end) {} @@ -364,6 +389,16 @@ public: } size_t size() { return tinfo.size(); } + + // Band-aid workaround to fix a subtle but serious bug in a minimalistic fashion. See PR #4762. + bool is_redundant_value_and_holder(const value_and_holder &vh) { + for (size_t i = 0; i < vh.index; i++) { + if (PyType_IsSubtype(tinfo[i]->type, tinfo[vh.index]->type) != 0) { + return true; + } + } + return false; + } }; /** diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 3e1a057db..b87fe66b2 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2759,7 +2759,6 @@ get_type_override(const void *this_ptr, const type_info *this_type, const char * PyObject *self_arg = PyTuple_GET_ITEM(co_varnames, 0); Py_DECREF(co_varnames); PyObject *self_caller = dict_getitem(locals, self_arg); - Py_DECREF(locals); if (self_caller == self.ptr()) { Py_DECREF(f_code); Py_DECREF(frame); diff --git a/pybind11/setup_helpers.py b/pybind11/setup_helpers.py index aeeee9dcf..3b16dca88 100644 --- a/pybind11/setup_helpers.py +++ b/pybind11/setup_helpers.py @@ -66,7 +66,9 @@ try: from setuptools import Extension as _Extension from setuptools.command.build_ext import build_ext as _build_ext except ImportError: - from distutils.command.build_ext import build_ext as _build_ext # type: ignore[assignment] + from distutils.command.build_ext import ( # type: ignore[assignment] + build_ext as _build_ext, + ) from distutils.extension import Extension as _Extension # type: ignore[assignment] import distutils.ccompiler diff --git a/pyproject.toml b/pyproject.toml index 7769860c4..5af6015a6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -60,7 +60,6 @@ messages_control.disable = [ [tool.ruff] target-version = "py37" src = ["src"] -line-length = 120 [tool.ruff.lint] extend-select = [ @@ -71,7 +70,6 @@ extend-select = [ "C4", # flake8-comprehensions "EM", # flake8-errmsg "ICN", # flake8-import-conventions - "ISC", # flake8-implicit-str-concat "PGH", # pygrep-hooks "PIE", # flake8-pie "PL", # pylint @@ -90,7 +88,6 @@ ignore = [ "SIM118", # iter(x) is not always the same as iter(x.keys()) ] unfixable = ["T20"] -exclude = [] isort.known-first-party = ["env", "pybind11_cross_module_tests", "pybind11_tests"] [tool.ruff.lint.per-file-ignores] diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 4ee0c7697..18d5d5bff 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -145,6 +145,7 @@ set(PYBIND11_TEST_FILES test_opaque_types test_operator_overloading test_pickling + test_python_multiple_inheritance test_pytypes test_sequences_and_iterators test_smart_ptr diff --git a/tests/test_class.py b/tests/test_class.py index ee7467cf8..73a48309e 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -1,3 +1,5 @@ +from unittest import mock + import pytest import env @@ -203,6 +205,18 @@ def test_inheritance_init(msg): assert msg(exc_info.value) == expected +@pytest.mark.parametrize( + "mock_return_value", [None, (1, 2, 3), m.Pet("Polly", "parrot"), m.Dog("Molly")] +) +def test_mock_new(mock_return_value): + with mock.patch.object( + m.Pet, "__new__", return_value=mock_return_value + ) as mock_new: + obj = m.Pet("Noname", "Nospecies") + assert obj is mock_return_value + mock_new.assert_called_once_with(m.Pet, "Noname", "Nospecies") + + def test_automatic_upcasting(): assert type(m.return_class_1()).__name__ == "DerivedClass1" assert type(m.return_class_2()).__name__ == "DerivedClass2" diff --git a/tests/test_enum.py b/tests/test_enum.py index b97b0fa56..6b75b7ae5 100644 --- a/tests/test_enum.py +++ b/tests/test_enum.py @@ -60,9 +60,7 @@ Members: ETwo : Docstring for ETwo - EThree : Docstring for EThree""".split( - "\n" - ): + EThree : Docstring for EThree""".split("\n"): assert docstring_line in m.UnscopedEnum.__doc__ # Unscoped enums will accept ==/!= int comparisons diff --git a/tests/test_methods_and_attributes.py b/tests/test_methods_and_attributes.py index 955a85f67..7fdf4e3af 100644 --- a/tests/test_methods_and_attributes.py +++ b/tests/test_methods_and_attributes.py @@ -232,25 +232,29 @@ def test_no_mixed_overloads(): with pytest.raises(RuntimeError) as excinfo: m.ExampleMandA.add_mixed_overloads1() - assert str( - excinfo.value - ) == "overloading a method with both static and instance methods is not supported; " + ( - "#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for more details" - if not detailed_error_messages_enabled - else "error while attempting to bind static method ExampleMandA.overload_mixed1" - "(arg0: float) -> str" + assert ( + str(excinfo.value) + == "overloading a method with both static and instance methods is not supported; " + + ( + "#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for more details" + if not detailed_error_messages_enabled + else "error while attempting to bind static method ExampleMandA.overload_mixed1" + "(arg0: float) -> str" + ) ) with pytest.raises(RuntimeError) as excinfo: m.ExampleMandA.add_mixed_overloads2() - assert str( - excinfo.value - ) == "overloading a method with both static and instance methods is not supported; " + ( - "#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for more details" - if not detailed_error_messages_enabled - else "error while attempting to bind instance method ExampleMandA.overload_mixed2" - "(self: pybind11_tests.methods_and_attributes.ExampleMandA, arg0: int, arg1: int)" - " -> str" + assert ( + str(excinfo.value) + == "overloading a method with both static and instance methods is not supported; " + + ( + "#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for more details" + if not detailed_error_messages_enabled + else "error while attempting to bind instance method ExampleMandA.overload_mixed2" + "(self: pybind11_tests.methods_and_attributes.ExampleMandA, arg0: int, arg1: int)" + " -> str" + ) ) diff --git a/tests/test_python_multiple_inheritance.cpp b/tests/test_python_multiple_inheritance.cpp new file mode 100644 index 000000000..689917158 --- /dev/null +++ b/tests/test_python_multiple_inheritance.cpp @@ -0,0 +1,45 @@ +#include "pybind11_tests.h" + +namespace test_python_multiple_inheritance { + +// Copied from: +// https://github.com/google/clif/blob/5718e4d0807fd3b6a8187dde140069120b81ecef/clif/testing/python_multiple_inheritance.h + +struct CppBase { + explicit CppBase(int value) : base_value(value) {} + int get_base_value() const { return base_value; } + void reset_base_value(int new_value) { base_value = new_value; } + +private: + int base_value; +}; + +struct CppDrvd : CppBase { + explicit CppDrvd(int value) : CppBase(value), drvd_value(value * 3) {} + int get_drvd_value() const { return drvd_value; } + void reset_drvd_value(int new_value) { drvd_value = new_value; } + + int get_base_value_from_drvd() const { return get_base_value(); } + void reset_base_value_from_drvd(int new_value) { reset_base_value(new_value); } + +private: + int drvd_value; +}; + +} // namespace test_python_multiple_inheritance + +TEST_SUBMODULE(python_multiple_inheritance, m) { + using namespace test_python_multiple_inheritance; + + py::class_(m, "CppBase") + .def(py::init()) + .def("get_base_value", &CppBase::get_base_value) + .def("reset_base_value", &CppBase::reset_base_value); + + py::class_(m, "CppDrvd") + .def(py::init()) + .def("get_drvd_value", &CppDrvd::get_drvd_value) + .def("reset_drvd_value", &CppDrvd::reset_drvd_value) + .def("get_base_value_from_drvd", &CppDrvd::get_base_value_from_drvd) + .def("reset_base_value_from_drvd", &CppDrvd::reset_base_value_from_drvd); +} diff --git a/tests/test_python_multiple_inheritance.py b/tests/test_python_multiple_inheritance.py new file mode 100644 index 000000000..3bddd67df --- /dev/null +++ b/tests/test_python_multiple_inheritance.py @@ -0,0 +1,35 @@ +# Adapted from: +# https://github.com/google/clif/blob/5718e4d0807fd3b6a8187dde140069120b81ecef/clif/testing/python/python_multiple_inheritance_test.py + +from pybind11_tests import python_multiple_inheritance as m + + +class PC(m.CppBase): + pass + + +class PPCC(PC, m.CppDrvd): + pass + + +def test_PC(): + d = PC(11) + assert d.get_base_value() == 11 + d.reset_base_value(13) + assert d.get_base_value() == 13 + + +def test_PPCC(): + d = PPCC(11) + assert d.get_drvd_value() == 33 + d.reset_drvd_value(55) + assert d.get_drvd_value() == 55 + + assert d.get_base_value() == 11 + assert d.get_base_value_from_drvd() == 11 + d.reset_base_value(20) + assert d.get_base_value() == 20 + assert d.get_base_value_from_drvd() == 20 + d.reset_base_value_from_drvd(30) + assert d.get_base_value() == 30 + assert d.get_base_value_from_drvd() == 30