fix(monkeypatch): don't leave inherited attributes in the instance dict - #14969
fix(monkeypatch): don't leave inherited attributes in the instance dict#14969Irahan2 wants to merge 1 commit into
Conversation
`MonkeyPatch.setattr()` recorded the old value with `getattr()`, which follows the MRO. When the attribute was inherited rather than owned by the instance, `undo()` assigned that inherited value back onto the instance, adding a `__dict__` entry that had not been there before. For a plain class attribute this only leaves the target in a different state than it was found in. For an inherited non-data descriptor it is worse: the value computed during teardown is stored on the instance and shadows the descriptor, so every later lookup returns that frozen value. Look the old value up in the instance `__dict__` instead, which is what `setattr()` and `undo()` actually operate on -- but only when no data descriptor is in the way. Data descriptors intercept the assignment, so for those the `getattr()` value remains the right thing to restore. Closes pytest-dev#10644. Co-authored-by: Claude <noreply@anthropic.com>
612cb41 to
0c601d5
Compare
Shriprasad-P
left a comment
There was a problem hiding this comment.
The inherited-attribute fix itself looks correct for normal instance storage and descriptor behavior, but I found a regression for objects with custom attribute assignment.
The new branch assumes that, when no data descriptor exists, setattr(target, name, value) writes into target.__dict__. That is not always true because a class can override __setattr__ and store attributes elsewhere.
For example, with an object that keeps values in a backing _store through __getattr__ / __setattr__, the current implementation works on base: monkeypatch records the resolved old value and undo restores _store["x"] to that value.
On this PR, x is absent from target.__dict__, so oldval becomes NOTSET. Undo then calls delattr(target, "x"), which can raise AttributeError or remove the proxy-backed value rather than restoring the original state.
Please preserve resolved-value restoration for targets with custom assignment machinery, or otherwise make the snapshot/undo strategy account for custom __setattr__ behavior, and add a regression test for that case.
The normal inherited attribute, non-data descriptor, method, property, slot, and direct-instance cases otherwise look correct.
Closes #10644.
Problem
MonkeyPatch.setattr()records the old value withgetattr(), which follows the MRO. When the patched attribute is inherited rather than owned by the instance,undo()assigns that inherited value back onto the instance, creating a__dict__entry that was never there:For a plain class attribute this only leaves the target in a different state than it was found in. For an inherited non-data descriptor it is worse: the value computed during teardown is stored on the instance and shadows the descriptor, so the attribute is frozen for every later lookup.
Patching a method on an instance hits the same path, leaving a bound method behind in
vars(obj)after teardown.Fix
Look the old value up in the instance
__dict__— which is whatsetattr()andundo()actually operate on — mirroring the handlingsetattr()already has for classes.The lookup is guarded by a data-descriptor check, and that guard is load-bearing. A data descriptor intercepts the assignment, so the attribute never reaches the instance
__dict__and there is nothing there to delete on undo. Without the guard,monkeypatch.setattr()on a property fails during undo withAttributeError: property 'x' of 'Sample' object has no deleter, and__slots__attributes break the same way.test_undo_data_descriptor_on_instanceandtest_undo_slot_attribute_on_instancecover both.Notes
Only
setattr()is changed.delattr()has the same shape but cannot reach this state: deleting an inherited attribute from an instance raisesAttributeError, so nothing is recorded. Its separate undo-ordering issue is #14909.Verified locally on Python 3.13 (Windows): the three new tests reproducing the bug fail without the change and pass with it, and the full test suite is unaffected.
Checklist
closes #XYZWto the PR description and/or commits.changelogdirectory.AUTHORSin alphabetical order.AI/LLM assistance
Per the AI/LLM-Assisted Contributions Policy: this change was developed with AI assistance (Claude Code), credited in the
Co-authored-bytrailer on the commit. Happy to explain any part of it or adjust the approach.