assertion rewrite: never rewrite stdlib and pytest modules (fixes lazy imports crash) - #14844
Open
RonnyPfannschmidt wants to merge 1 commit into
Open
Conversation
Under lazy imports (PEP 810, PYTHON_LAZY_IMPORTS=all) an attribute access can resolve an import, and resolving an import runs the meta path finders. AssertionRewritingHook.find_spec was therefore asked about the very modules it needs in order to answer - fnmatch_ex and fnmatch - which recursed until the stack blew resp. raised ImportCycleError, before pytest could start. Bail out for those names up front, without imports, path lookups or filesystem access, so the cycle cannot form: everything find_spec touches is either stdlib or _pytest.*. Explicit register_assert_rewrite() still wins, so a local module shadowing a stdlib name keeps being rewritten. As a side effect stdlib and pytest imports no longer pay for the PurePath construction and fnmatch loop of the early bailout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #14632. Supersedes #14633, whose approach this builds on — same idea, with the skip list widened to close the remaining half of the cycle and
register_assert_rewrite()kept working (details below).With lazy imports enabled (PEP 810,
PYTHON_LAZY_IMPORTS=allon Python 3.15+) pytest dies during startup, before it gets to run anything:(3.15.0b2 as in the issue report recursed until it blew the stack; 3.15.0rc1 detects the cycle and raises.)
Why it happens
Under lazy imports an ordinary attribute access can resolve an import, and resolving an import runs the meta path finders — including
AssertionRewritingHook. Sofind_spec()gets asked about the very modules it needs in order to answer:The hook has no way to answer that question without first answering it.
The fix
A stateless skip list of top level packages that never contain test code:
_early_rewrite_bailout()consults it before anything else, so the answer for those names needs no imports, no path lookups and no filesystem access. That makes the recursion impossible rather than merely unlikely: everything reachable fromfind_spec()is either stdlib or_pytest.*, and both are covered.Two details worth flagging for review, both differences to #14633:
_pytest/pytestare in the list on purpose. Skipping only the stdlib fixes the traceback above, but the outer half of the cycle — resolving_pytest.pathlibin order to callfnmatch_ex— is left in place. Today we get away with it because_pytest.pathlibhappens to be imported before the hook is installed (verified: at the firstfind_spec()call_pytest.pathlibis insys.modules,fnmatchis not). That is an accident of import order, not an invariant.register_assert_rewrite()still wins, so the bailout isreturn not self._is_marked_for_rewrite(name, state), notreturn True.sys.stdlib_module_namesholds ~300 names includingcode,platform,types,string,jsonandlogging, and a local top level module shadows the stdlib one (sys.path[0]beats the stdlib for pure-Python modules). An unconditional skip would silently drop rewriting for such a package. The check is a cached dict lookup and only runs for names already known to be in the skip list.(
testis not insys.stdlib_module_names— CPython excludes its own test package — so the common top leveltest/package layout was never affected.)Besides fixing the crash this is a small speedup: every stdlib and
_pytestimport during a run now skips thePurePathconstruction and thefnmatchloop.Behaviour change
Stdlib and pytest modules are no longer rewritten under a catch-all
python_filespattern such aspython_files = *.py. This was possible before, is essentially never wanted, and is now impossible.Testing
TestEarlyRewriteBailout::test_stdlib_and_pytest_modules— uses the existingPathFinder.find_specspy to assert those names bail out without a path lookup, withpython_files = *.pyso the old code would have rewritten them. Fails onmain.TestEarlyRewriteBailout::test_marked_for_rewrite_beats_stdlib_name— a registered module shadowing a stdlib name is still rewritten.test_lazy_imports_keep_assertion_rewriting_working— runs a failing test in a subprocess withPYTHON_LAZY_IMPORTS=alland asserts the rewritten output (assert 1 == 2), so it covers both the crash and the rewriting still doing its job. No version guard: older interpreters ignore the variable and it degrades to a smoke test. Fails onmainunder 3.15.Manually verified against CPython 3.15.0rc1:
PYTHON_LAZY_IMPORTS=all pytest --helpworks, and a failing assertion still gets full introspection.Unrelated 3.15 findings
Running pytest's own suite under
PYTHON_LAZY_IMPORTS=allon 3.15.0rc1 leaves 32 failures. They are identical with and without this change and none involve assertion rewriting — they are tests that assert on eagerImportErrorbehaviour, which PEP 810 defers by design (e.g.test_monkeypatch.py::test_importerrornow collects cleanly becauseimport doesnotexistno longer raises at import time). Worth a separate issue rather than blocking this one.hypothesisalso fails to import underPYTHON_LAZY_IMPORTS=all(itsdir()-based export check inhypothesis/strategies/__init__.pysees unresolved lazy names), which is whytesting/acceptance_test.pyandtesting/python/metafunc.pywere excluded from that run.🤖 Generated with Claude Code