Fable review of pybind11_mkdoc

[henryiii]

See https://iscinumpy.dev/post/claude-code-reviews/.

🤖 AI text below 🤖

Review complete. I read the whole package (mkdoc_lib.py, __init__.py, tests, CMake files, CI) and verified the most serious finding with a live reproduction. Findings below, most severe first.

Bugs

1. extract_all can only be called once per process — second call deadlocks (mkdoc_lib.py:697-707). Each ExtractionThread acquires job_semaphore in __init__ and releases it in run, but extract_all then drains all job_count permits as its "join" and never releases them. I confirmed this with a repro: the second extract_all call blocks forever in ExtractionThread.__init__ (faulthandler shows it stuck in Semaphore.acquire at line 564). This is why the test suite needs pytest --forked. Related: errors_detected is a global that is never reset, so one failure poisons all later in-process calls. Replacing the hand-rolled Thread/Semaphore machinery with concurrent.futures.ThreadPoolExecutor fixes both, shrinks the code, and would let you drop the pytest-forked dependency.

2. Failures exit with status 0 and no message (mkdoc_lib.py:767-769, __init__.py:172). When a worker thread raises, errors_detected is set, mkdoc() returns silently, and main() still returns 0. A build system (including the bundled CMake function) sees "success" with no output file written, and the real error is only a thread traceback on stderr. mkdoc() should raise or return a failure status, and main() should propagate it.

3. Linux: LIBCLANG_PATH alone doesn't work, contradicting the error message (mkdoc_lib.py:632-651). If no /usr/lib*/llvm-* directory exists, the code raises FileNotFoundError before ever looking at LIBCLANG_PATH — but the message it raises explicitly tells the user that setting LIBCLANG_PATH is a valid override. The elif llvm_dir is None check needs to also consider LIBCLANG_PATH (with care: llvm_dir is later used unguarded for include paths at lines 663/669, which would TypeError on None).

4. macOS: only the Xcode.app location is searched (mkdoc_lib.py:588-595). LIBCLANG_PATH is honored on Windows and Linux but ignored on Darwin, and Command Line Tools-only installs (/Library/Developer/CommandLineTools/usr/lib/libclang.dylib) are not checked. My repro on this machine hit exactly this: dlopen(libclang.dylib) fails because nothing configured the library path.

5. macOS SDK selection is nondeterministic (mkdoc_lib.py:598). next(os.walk(sdk_dir))[1][0] takes the first directory in OS-arbitrary order; with several SDKs installed the sysroot choice is random. Prefer MacOSX.sdk or sort and take the newest.

6. Duplicate-name suffixing can collide (mkdoc_lib.py:739-747). Overloads foo, foo become foo, foo_2; if a real symbol named foo_2 also exists, the generated header contains two static const char *mkd_doc_..._2 definitions and fails to compile. Rare, but a collision check would be cheap. (Inherited from upstream mkdoc.)

7. -DFOO= changes meaning (__init__.py:63). An explicit empty value (value.strip() if value else "1") is rewritten to FOO=1; clang defines it as empty. Edge case, but a silent semantic change.

8. CMake EXTRA_ARGS only supports a single token (pybind11_mkdoc_functions.cmake:27,58). It's declared as a one-value argument and expanded unquoted, so EXTRA_ARGS "-DFOO -DBAR" reaches the command as one argv entry containing a space. Make it a multi-value argument (or separate_arguments). The test only exercises a single flag, which is why this hasn't surfaced.

9. Clang diagnostics are never reported (ExtractionThread.run). A header with a missing include parses "successfully" with incomplete ASTs, silently producing missing/empty docstrings. Printing tu.diagnostics at severity ≥ error (and tying it into finding 2) would make failures visible.

Performance

Nothing significant — libclang parsing dominates, the threading is effective (ctypes releases the GIL), and the samefile cache from 2ff4d5d handles the hot path. Only micro-nits: the inline re.sub patterns in process_comment (lines 415-422) recompile per call, and a fresh TextWrapper is built per comment; both are noise compared to parse time.

Simplifications

• _append_include_dir (__init__.py:34-37): elif verbose: pass does nothing, though the docstring promises a warning for missing directories. Either print the warning or drop the parameter.
• _append_definition (__init__.py:60-74): both except blocks are unreachable — str.partition never raises ValueError. The function reduces to three lines. (The re.search(r"^...") is also missing a $/fullmatch, but that code is dead anyway.)
• param_arg_re and raises_arg_re (mkdoc_lib.py:124-125) are identical patterns.
• extract mixes return 0 and return None (mkdoc_lib.py:531-555); no caller uses the value.
• mkdoc_args.extend(header for header in parsed_args.header) → mkdoc_args.extend(parsed_args.header); [parsed_args, unparsed_args] = ... → plain tuple unpacking; int(width) is redundant since argparse already has type=int.
• Version is duplicated: __version__ = "2.6.2.dev1" vs hardcoded VERSION 2.6.2 in cmake/CMakeLists.txt:9 — these will drift.

Modernizations

• ThreadPoolExecutor instead of Thread + Semaphore + module globals (fixes findings 1 and 2, removes the PLW0603 ruff ignore and the pytest-forked test dependency).
• OrderedDict wrapper at mkdoc_lib.py:95 is unnecessary since 3.7 — plain dicts preserve order; keep the length sort.
• multiprocessing.cpu_count() → os.cpu_count() (avoids importing multiprocessing just for this).
• The d() bytes-decode helper is a Python 2 relic; python3 cindex returns str.
• '...'.format(...) in write_header:749 → f-string; re.sub("_$", ...) → removesuffix.
• The default -std=c++11 (line 585) is dated; c++17 would parse modern headers better — behavior change, so a deliberate decision rather than a drive-by.
• dependencies = ["clang"] is unpinned while CI pins clang<19 for compatibility — users installing fresh get clang 19+ bindings that may mismatch their libclang; consider mirroring the constraint or documenting it.

The highest-value fix is a single refactor: move extract_all to ThreadPoolExecutor with error propagation (findings 1, 2, and the groundwork for 9) — it deletes code, fixes the deadlock and exit-code bugs, and simplifies the test setup.