Fable 5.1 review of pybind11

[henryiii]

Finally got a proper report from Fable 5.1. I think my Fable 5 one hit a downgrade.

🤖 AI text below 🤖

Review of pybind11 master (d87cf0b). Seven reviewers covered the headers, Python package, CMake, and CI. I verified every finding below against the source. Items marked "reproduced" were also exercised against the build tree by a reviewer. No files were changed. Reviewer repro scripts are in the session scratchpad.

Bugs

High

1. include/pybind11/pybind11.h:1156 and :1209: when named args follow py::args, the *args stub is pushed into call.args but its convert flag is appended last. Each kw-only arg gets its successor's flag, so noconvert() lands on the wrong argument. Reproduced. Fix: push false with the stub and append in step 4a only when appending the tuple.
2. pybind11.h:1249: the second-pass loop stops at pos_args, so overloads whose only convertible args are keyword-only never get a converting pass. Reproduced. Iterate to second_pass_convert.size().
3. include/pybind11/gil_safe_call_once.h:175-244: nothing resets the cached pointer or the initialized flag when an interpreter is finalized. After finalize_interpreter(); initialize_interpreter() the fast path returns freed memory. py::register_exception uses this class, so re-importing an embedded module that registers an exception hits it. Reproduced with ASan. Clear the owner's cache from ~call_once_storage.
4. include/pybind11/detail/struct_smart_holder.h:347: from_unique_ptr builds shared_ptr<T>(raw, deleter), which always hooks enable_shared_from_this. The void-cast intent from detail/init.h:224 is defeated, so factories returning unique_ptr<Trampoline> lose the bad_weak_ptr guard and the Python override can silently vanish. Regression from Fix smart_holder multiple/virtual inheritance bugs in shared_ptr and unique_ptr to-Python conversions #5836, reproduced. Take an explicit flag and construct the owner as shared_ptr<void>.
5. include/pybind11/eigen/matrix.h:181-194: strides that are not a multiple of sizeof(Scalar) are truncated by integer division. A packed structured-field view maps as contiguous and a mutable Ref writes into the neighbouring field. Reproduced. Treat a non-divisible stride as non-conformable.
6. include/pybind11/iostream.h:152: ~pythonbuf() calls into Python; a failing write throws out of a destructor and terminates the process. Reproduced. Catch and discard_as_unraisable.
7. CMakeLists.txt:387: cmake_path(RELATIVE_PATH ... BASE_DIRECTORY CMAKE_INSTALL_DATAROOTDIR) uses the literal string as the base, so an installed pybind11.pc has an empty prefix= on CMake 3.20+. Confirmed in the current build tree. Wheels are unaffected because pyproject sets it. Use the while() branch unconditionally.

Medium

8. include/pybind11/cast.h:992, :1246, detail/type_caster_base.h:869: the alias flag is read from src, but after an implicit conversion the loaded instance is the converter's temporary. src may not be a pybind11 instance at all. Record it in load_value from v_h.inst.
9. cast.h:1738: py::cast of an unregistered type returns a null object with a pending TypeError instead of throwing. Downstream use gives SystemError.
10. cast.h:1219: returning unique_ptr<T> for a shared_ptr-held or custom-holder T copy-constructs the holder from unique_ptr bytes. tinfo->holder_enum_v is available to throw cast_error instead.
11. include/pybind11/detail/class.h:396: tp_alloc result is unchecked and allocate_layout() can throw out of the extern "C" tp_new. Calling pybind11_object() from Python aborts. Reproduced.
12. include/pybind11/detail/common.h:90: __cpp_lib_launder is tested before <new> or <version> is included, so std::launder is never used with libstdc++ in C++17 mode. Move the block below the includes.
13. include/pybind11/stl.h:77-100, :199, :316, :451: the tp_name heuristic matches any user class named zip, map, or dict_keys; the following assert(isinstance<iterable>) aborts debug builds and release builds throw instead of returning false. Reproduced. Reject heap types and return false.
14. include/pybind11/stl_bind.h:470: stride-0 (broadcast) buffers give end == p, so the buffer constructor returns an empty vector. Reproduced.
15. iostream.h:251: calling __enter__ twice destroys the first redirect while the second holds its buffer pointer, then segfaults. Reproduced.
16. include/pybind11/chrono.h:46: days plus seconds plus microseconds is computed in int64 microseconds regardless of the target period, so timedelta.max overflows. Line 188 does not compile for floating-point durations. Line 171 leaves mktime unchecked.
17. chrono.h:105: the Annex-K localtime_s call uses MSVC's argument order, so it is a compile error on any libc that actually provides Annex K. Use localtime_r on POSIX and drop the global mutex.
18. include/pybind11/numpy.h:1453-1469: at() divides the byte offset by itemsize(), wrong for strides that are not multiples of sizeof(T). Reproduced. Use *data(index...).
19. numpy.h:1357: no negative-index check, so at(-1) reads before the buffer. Reproduced.
20. numpy.h:785-793: the numpy_scalar caster uses function-local statics that run import numpy under the static-init guard, the GIL deadlock this header warns about elsewhere. Use gil_safe_call_once_and_store.
21. include/pybind11/eigen/tensor.h:40: modulo by EIGEN_DEFAULT_ALIGN_BYTES is modulo zero with EIGEN_DONT_VECTORIZE.
22. eigen/matrix.h:678-695: the sparse loader throws instead of returning false (the null checks are dead) and calls mutable_data() on inputs it only reads. Reproduced.
23. include/pybind11/pytypes.h:2335: set::add returns false with the error still set. stl.h:213 then reports a generic conversion error instead of "unhashable type". Throw like list::append.
24. include/pybind11/detail/argument_vector.h:83, :377: ref_small_vector move leaves the inline source with its size intact, so both destructors DECREF. Latent because the only user relies on copy elision, and the comment asserts the opposite.
25. pybind11.h:1690: PyModule_GetFilenameObject returns a new reference stored in a handle; def_submodule leaks one per call. Reproduced.
26. pybind11.h:3926: PYBIND11_OVERRIDE_IMPL uses a function-static caster for reference returns, a data race under free-threading. thread_local is the cheap fix.
27. tools/pybind11Common.cmake:51 with CMakeLists.txt:107: the CMP0190 cross-compiling default flips to OFF on reconfigure because option() caches OFF and the "defaulted" marker is not cached. Reproduced.
28. tools/pybind11Common.cmake:223: COMPAT-mode PYTHON_* variables are plain set(), invisible to an add_subdirectory parent. Reproduced.
29. tools/pybind11Common.cmake:206: PYBIND11_FINDPYTHON=OLD is truthy, so the documented value selects the NEW mode.
30. tools/pybind11Common.cmake:375-395: the LTO fallbacks run even in the ppc64le/mips64 "do nothing" and Emscripten "no check" branches, and HAS_FLTO_THIN is reused for thin and full probes so -flto=thin is never actually tested.

Low

31. pytypes.h tuple, dict, list, set, int_, float_, bytes, and memoryview constructors call pybind11_fail after a NULL return, which asserts that no error is set.
32. pytypes.h:1469: PYBIND11_OBJECT_CVT on a null object calls the converter with nullptr (segfault for bool_, silent empty dict).
33. argument_vector.h:294: the assert should be idx < ha.size; exactly 64 kwargs aborts debug builds. Reproduced.
34. detail/function_record_pyobject.h:145-186: raw tp_new and tp_methods slots let C++ exceptions escape into CPython.
35. pytypes.h:2067, :2137: capsule destructor lambdas throw from a C callback, and only one installs error_scope.
36. type_caster_base.h:701-720: the new reference from find_registered_python_instance is held in a handle and leaked on the cast_error path.
37. type_caster_base.h:545: get_object_handle returns a borrowed pointer from the instance map, a use-after-free window under free-threading.
38. cast.h:767: tuple_caster::load keeps subcaster pointers into seq[Is] temporaries that die before implicit_cast.
39. detail/internals.h:517-527: nested-exception chaining for error_already_set and builtin_exception is overwritten by the following restore() or set_error(). Reproduced.
40. internals.h:702: the interpreter TLS key is updated before the state-dict fetch can throw, leaving the fast path pointing at the previous interpreter's internals.
41. internals.h:708, :884: instance_base check-and-create and internals_singleton_pp_ are unsynchronized under free-threading.
42. include/pybind11/conduit/pybind11_platform_abi_id.h:59: _GLIBCXX_DEBUG changes container layouts in internals but is not part of the ABI id.
43. common.h:1163: format_descriptor<PyObject*>::value has no pre-C++17 out-of-class definition; undefined symbol in C++14 when ODR-used.
44. struct_smart_holder.h:347: if the control-block allocation throws, the armed deleter and the still-owning unique_ptr both delete.
45. class.h:91, :579: pybind11_static_property on 3.12+ replaces property's GC slots, hiding fget and fset from the collector. Reproduced.
46. class.h:141, :176: _PyType_Lookup plus Py_INCREF races under free-threading; _PyType_LookupRef exists on 3.13+.
47. class.h:779: make_new_python_type leaks the heap type on every failure path.
48. pybind11.h:3688: PyErr_NewException result unchecked; pybind11.h:1735: PyModule_AddObject result ignored.
49. numpy.h:1088, :1267: PyArray_NewCopy_ and PyArray_Squeeze_ NULL results unchecked. numpy.h:148, :1750: registered_dtypes accessed without the internals lock. numpy.h:1415: array_t<T>(const buffer_info&) does not check T against the format.
50. eigen/tensor.h:158, :188, :384: non-default IndexType does not compile; dtypes compared by identity rather than equivalence.
51. include/pybind11/embed.h:95: PyConfig_SetBytesArgv runs unconditionally, clobbering a caller-populated argv. subinterpreter.h:237, :344: two unchecked C-API results.
52. CMakeLists.txt:172: .venv autodetect checks ENV{VIRTUALENV}; the real variable is VIRTUAL_ENV.
53. tools/pybind11NewTools.cmake:167 and tools/FindPythonLibsNew.cmake:206: message(AUTHOR_WARNING, ...) has a stray comma, demoting it to a plain message.
54. .pre-commit-config.yaml:94: the remove-tabs exclude regex matches every path, so the hook is a no-op.
55. tools/pybind11GuessPythonExtSuffix.cmake:75: the debug regex misses the t before d in cpython-313td, and its test script is not run anywhere in CI.
56. .github/workflows/tests-cibw.yml: no permissions: block. Third-party actions are tag-pinned, and the PyPI publish job with id-token: write uses a moving branch ref.
57. pybind11/setup_helpers.py:481: ParallelCompile patches the base compile, which MSVCCompiler overrides, so Windows builds stay serial. Line 309 matches package_dir by raw startswith.
58. CMakeLists.txt:33: the version-type regex expects .dev1; the v3 scheme never matches.

Performance

59. detail/function_record_pyobject.h:93 via pybind11.h:970: every dispatch calls get_function_record_PyTypeObject(), which takes the internals lock. A reviewer measured roughly 270 ns/call at 8 threads on 3.14t versus 50 ns with a direct cast. self is always the record object, so cast directly in dispatcher.
60. type_caster_base.h:62: loader_life_support holds an unordered_set by value; MSVC's default constructor allocates, so every call on Windows pays two allocations. Allocate lazily.
61. type_caster_base.h:1198: every failed load of a registered type does PyObject_HasAttrString plus getattr with a long key, materializing an AttributeError on older CPython. Intern the key and use _PyType_Lookup.
62. pytypes.h:2618: contains() goes through attr("__contains__"); PySequence_Contains is the in operator and handles the iteration fallback.
63. pytypes.h:1797: bytes(const str&) copies the UTF-8 buffer into a second bytes object. pytypes.h:1694: str::operator std::string round-trips through a temporary bytes where PyUnicode_AsUTF8AndSize (already used in cast.h:522) would do.
64. include/pybind11/functional.h:21: func_handle has no move constructor, so loading one std::function does three GIL-acquiring copies and three GIL-acquiring destructions.
65. pybind11.h:1413: keyword_index re-interns the name on every lookup and never checks for NULL. Intern once at binding time (a function_record layout change, so bump the record ABI id).
66. pybind11.h:3336: keep_alive_impl copies the type-info vector just to test .empty().
67. numpy.h:2200: vectorize runs the full buffer protocol per argument per call; line 2111 converts the F-style output through a second PyArray_FromAny; the contiguity check does not skip length-1 dims, so x[:, None] misses the trivial path.
68. eigen/matrix.h:488, :546: the Ref loader heap-allocates a Map that Ref never references again.
69. stl.h:327: list_caster iterates via PyIter_Next even for list/tuple. stl.h:107, :132 import collections.abc per miss. stl/filesystem.h:52 imports pathlib per cast.
70. CI: tests/CMakeLists.txt:285 clones full Eigen history in every job (tarball plus SHA256 is a few MB). No ccache anywhere. reusable-standard.yml:42 still forces a Python download for a June 2025 issue.

Simplifications and modernizations

71. internals.h:45 requires internals version 12+, so every >= 12 and <= 11 branch is dead (internals.h:311, 344, 424; class.h:229; type_caster_base.h:241-259; pybind11.h:1852, 2421).
72. common.h:1387: PYBIND11_WORKAROUND_INCORRECT_MSVC_C4100 has been empty since feat!: drop support for Python 3.8, MSVC 2017 #6110 but has 14 call sites.
73. pybind11.h:3054-3100: the PYBIND11_ENUM_OP_* macros are defined and undefined with no uses since Improve performance of enum_ operators by going back to specific implementation #5887.
74. stl_bind.h:539-577: an #if 0 block. stl_bind.h:136 and :332 duplicate the wrap_i lambda. stl.h:182 clears the set twice.
75. include/pybind11/gil.h:33-43: the forward declaration is redundant with internals.h:148.
76. pytypes.h:1438 and :2713: get_scope_module and get_module_name_if_available are byte-identical.
77. pytypes.h:549, :999, :1039: dead error branches. pytypes.h:2224 is the last PyObject_CallFunctionObjArgs; pybind11.h:3637 builds a 1-tuple for PyObject_Call. Both can use PyObject_CallOneArg. pytypes.h:1885: bool_::operator bool via PyLong_AsLong instead of comparing to Py_True.
78. pytypes.h:532-825: PyErr_Fetch and PyErr_Restore could use PyErr_GetRaisedException on 3.12+.
79. type_caster_base.h:728: a no-op branch with a misleading comment. numpy.h:317: the "Unused" comment now sits on a used slot.
80. common.h:110-138 asserts clang 3.3 and gcc 4.8 floors while CI tests GCC 9+. The experimental filesystem and optional paths and the Eigen 3.2 fallbacks are never built by CI. cast.h:262, :580: PyPy 3.8-era workarounds worth retesting.
81. tools/cmake_uninstall.cmake.in:12 uses deprecated exec_program. pybind11Common.cmake:277 keeps a pkg_resources fallback. pyproject.toml lists two classifiers twice and names ruff rules in per-file ignores that are never selected. tools/pybind11Config.cmake.in documents variables that are never set.

Suggested order of attack: the two dispatcher bugs and the ref_small_vector move fix are small and self-contained; the interpreter-restart use-after-free, the smart_holder shared_from_this regression, and the Eigen stride truncation each need a regression test plus a focused fix; the pybind11.pc prefix and the CMP0190 reconfigure flip are one-line CMake fixes.

[henryiii]

🤖 AI text below 🤖

PRs for the first set: #6160 (items 1, 2), #6161 (24), #6162 (3), #6163 (4), #6164 (5), #6165 (7, 27).

New item found while writing the tests for #6160:

82. pybind11.h:150 (generate_function_signature): when py::args is the first parameter and named keyword-only arguments follow it, the generated docstring shows the tuple type instead of *args. For m.def("f", [](const py::args &, bool x, bool y) {...}, py::arg("x"), py::arg("y")) the signature reads f(tuple, x: bool, y: bool) instead of f(*args, x: bool, y: bool). The existing tests only place py::args after positional arguments, where the output is correct. No test asserts this docstring.