Skip to content

Fix monkeypatch resolution of cached submodules - #15157

Open
SharifWaqas wants to merge 2 commits into
pytest-dev:mainfrom
SharifWaqas:fix-monkeypatch-cached-submodules
Open

SharifWaqas wants to merge 2 commits into
pytest-dev:mainfrom
SharifWaqas:fix-monkeypatch-cached-submodules

Conversation

@SharifWaqas

Copy link
Copy Markdown

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.

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.
@psf-chronographer psf-chronographer Bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Oct 10, 2026
@0xamlab

This comment was marked as low quality.

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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_name does, 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

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided (automation) changelog entry is part of PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants