Repository navigation
Fix monkeypatch resolution of cached submodules - #15157
SharifWaqas wants to merge 2 commits into
Conversation
Use the module returned by importlib when a parent package no longer exposes a cached submodule. Cover setattr, delattr, and undo behavior. Developed with OpenAI Codex assistance.
This comment was marked as low quality.
This comment was marked as low quality.
RonnyPfannschmidt
left a comment
There was a problem hiding this comment.
🤖 Written by Claude Opus 5.5 via Claude Code for the pytest maintainers; I prompted it, it did the work, I read it.
Thanks for the PR. The change does fix the case it targets: a submodule that is cached in sys.modules but no longer an attribute of its parent package. The new tests fail on main and pass here, and testing/test_monkeypatch.py passes. I'm requesting changes anyway, because the patch makes resolve() less consistent, not more. A design and backwards-compatibility decision is needed before we change how dotted paths resolve.
Findings
All results below come from resolve(<module part>) on CPython 3.13, run against main (4fa645f) and this branch (86e7a2f).
1. The PR mixes two resolution rules. resolve() looks up attributes first and imports only when an attribute is missing. With this PR it now answers from sys.modules in that fallback case, but still answers from the attribute whenever one exists. So two near-identical package layouts give opposite results:
# shadowobj/__init__.py: from . import child as _c; child = object()
# shadowobj/child/sub.py: v = 1
resolve("shadowobj.child.sub") # main: AttributeError this PR: <module shadowobj.child.sub>
# shadowfn/__init__.py: from .foo import foo
# shadowfn/foo.py: helper = 1; def foo(): ...
resolve("shadowfn.foo") # main and this PR: <function foo>This behaviour change isn't tested or mentioned in the changelog.
2. On main too, raising=False can silently patch the wrong object. With the shadowfn layout, monkeypatch.setattr("shadowfn.foo.helper", 2, raising=False) sets helper on the function foo. The module attribute shadowfn.foo.helper stays 1, and no error is raised.
3. On main, the "missing module" check is dead code. expected = str(ex).split()[-1] evaluates to 'pkg.nope', with the quotes included, so it never equals used. Every missing module therefore gets wrapped, and ModuleNotFoundError is downgraded to a plain ImportError:
resolve("cached.nope") -> ImportError: import error in cached.nope: No module named 'cached.nope'
4. On main, the error points at the wrong object. The fallback's annotated_getattr(found, part, used) receives the path after it has already been extended. The resulting message is 'module' object at cached.child has no attribute 'child', but it should say at cached. This PR deletes that line rather than fixing it.
5. On main, malformed paths aren't rejected. setattr("os.path.", 1, raising=False) sets an attribute named '' on posixpath. "os..path" and ".os" give confusing import or ValueError messages rather than a clear "invalid import path".
6. On main and this PR, stale sys.modules entries are invisible. When pkg.child and sys.modules["pkg.child"] are different objects, the patch goes to the attribute. Code that later does import pkg.child or importlib.import_module sees the unpatched object.
Decision needed
For reference, stdlib pkgutil.resolve_name imports the longest importable module prefix first and only then walks attributes. That rule would settle finding 1, the shadowing cases and finding 3 consistently. However, it changes which object gets patched for existing users when an attribute shadows a submodule. So the question for maintainers is one of these:
- Keep "attribute first" and fix only the error handling (findings 3, 4 and 5).
- Switch to "module first", as
pkgutil.resolve_namedoes, with a deprecation path for the cases where the result changes.
Until that's decided, I'd rather not merge a partial change to the lookup order. The error-handling fixes (findings 3 to 5) could go in independently.
Generated by Claude Code
Fix string-based monkeypatch.setattr and delattr for cached submodules whose parent-package attribute is missing. Use the module returned by importlib. Regression tests cover both operations and undo.
Validation: 4,688 tests passed with no failures. Both regressions fail before the fix. Pre-commit checks passed.
Developed with OpenAI Codex assistance. The temporary changelog/0.bugfix.rst will be renamed to this PR number.