Merge 82557504ba into 10c6db2df2

AUTHORS

@@ -159,6 +159,7 @@ Floris Bruynooghe
 Fraser Stark
 Gabriel Landau
 Gabriel Reis
+Ganden Schaffner
 Garvit Shubham
 Gene Wood
 George Kussumoto

changelog/10644.bugfix.rst

@@ -0,0 +1,3 @@
+`monkeypatch.setattr` no longer leaves a leftover item in the target's dictionary after cleanup when patching an inherited attribute of a non-class object. This fixes a bug where a `__get__` descriptor's dynamic lookup was blocked and replaced by a cached static lookup after `monkeypatch` teardown.
+
+Previously `monkeypatch.setattr` avoided leaving leftover items in the target's dictionary when patching class objects but not when patching non-class objects.

changelog/10646.bugfix.rst

@@ -0,0 +1,3 @@
+Fix bug where `monkeypatch.setattr` and `monkeypatch.delattr` bind descriptors (possibly causing unwanted side-effects) to implement their `hasattr` checks even when their checks are disabled by `raising=False`.
+
+Note that they still must bind descriptors to implement their checks if `raising=True`.

src/_pytest/monkeypatch.py

@@ -222,7 +222,6 @@ class MonkeyPatch:
         applies to ``monkeypatch.setattr`` as well.
         """
         __tracebackhide__ = True
-        import inspect
 
         if isinstance(value, Notset):
             if not isinstance(target, str):
@@ -241,13 +240,13 @@ class MonkeyPatch:
                     "import string"
                 )
 
-        oldval = getattr(target, name, notset)
-        if raising and oldval is notset:
+        if raising and not hasattr(target, name):
             raise AttributeError(f"{target!r} has no attribute {name!r}")
 
-        # avoid class descriptors like staticmethod/classmethod
-        if inspect.isclass(target):
-            oldval = target.__dict__.get(name, notset)
+        # Prevent `undo` from polluting `vars(target)` with an object that was not in it
+        # before monkeypatching, such as inherited attributes or the results of
+        # descriptor binding.
+        oldval = vars(target).get(name, notset)
         self._setattr.append((target, name, oldval))
         setattr(target, name, value)
 
@@ -267,7 +266,6 @@ class MonkeyPatch:
         ``raising`` is set to False.
         """
         __tracebackhide__ = True
-        import inspect
 
         if isinstance(name, Notset):
             if not isinstance(target, str):
@@ -278,16 +276,18 @@ class MonkeyPatch:
                 )
             name, target = derive_importpath(target, raising)
 
-        if not hasattr(target, name):
-            if raising:
-                raise AttributeError(name)
-        else:
-            oldval = getattr(target, name, notset)
-            # Avoid class descriptors like staticmethod/classmethod.
-            if inspect.isclass(target):
-                oldval = target.__dict__.get(name, notset)
-            self._setattr.append((target, name, oldval))
+        if raising and not hasattr(target, name):
+            raise AttributeError(f"{target!r} has no attribute {name!r}")
+
+        # Prevent `undo` from overwriting class descriptors (like
+        # staticmethod/classmethod) with the results of descriptor binding.
+        oldval = vars(target).get(name, notset)
+        try:
             delattr(target, name)
+        except AttributeError:
+            pass
+        else:
+            self._setattr.append((target, name, oldval))
 
     def setitem(self, dic: Mapping[K, V], name: K, value: V) -> None:
         """Set dictionary entry ``name`` to value."""

testing/test_monkeypatch.py

@@ -6,12 +6,46 @@ import sys
 import textwrap
 from typing import Dict
 from typing import Generator
+from typing import NoReturn
+from typing import Optional
+from typing import overload
 from typing import Type
+from typing import TypeVar
 
 from _pytest.monkeypatch import MonkeyPatch
 from _pytest.pytester import Pytester
 import pytest
 
+T = TypeVar("T")
+
+
+class SomeDescriptor:
+    @overload
+    def __get__(self, instance: None, owner: type) -> int:
+        ...
+
+    @overload
+    def __get__(self, instance: T, owner: Optional[Type[T]] = ...) -> int:
+        ...
+
+    def __get__(self, instance: Optional[T], owner: Optional[Type[T]] = None) -> int:
+        return 1
+
+
+class RaisingDescriptor:
+    @overload
+    def __get__(self, instance: None, owner: type) -> NoReturn:
+        ...
+
+    @overload
+    def __get__(self, instance: T, owner: Optional[Type[T]] = ...) -> NoReturn:
+        ...
+
+    def __get__(
+        self, instance: Optional[T], owner: Optional[Type[T]] = None
+    ) -> NoReturn:
+        assert False, "descriptor was bound"
+
 
 @pytest.fixture
 def mp() -> Generator[MonkeyPatch, None, None]:
@@ -23,15 +57,22 @@ def mp() -> Generator[MonkeyPatch, None, None]:
 
 
 def test_setattr() -> None:
+    import inspect
+
     class A:
         x = 1
+        y = SomeDescriptor()
+        z = RaisingDescriptor()
 
     monkeypatch = MonkeyPatch()
     pytest.raises(AttributeError, monkeypatch.setattr, A, "notexists", 2)
-    monkeypatch.setattr(A, "y", 2, raising=False)
-    assert A.y == 2  # type: ignore
+    monkeypatch.setattr(A, "w", 2, raising=False)
+    assert A.w == 2  # type: ignore
     monkeypatch.undo()
-    assert not hasattr(A, "y")
+    assert not hasattr(A, "w")
+
+    with pytest.raises(TypeError):
+        monkeypatch.setattr(A, "w")  # type: ignore[call-overload]
 
     monkeypatch = MonkeyPatch()
     monkeypatch.setattr(A, "x", 2)
@@ -45,8 +86,32 @@ def test_setattr() -> None:
     monkeypatch.undo()  # double-undo makes no modification
     assert A.x == 5
 
-    with pytest.raises(TypeError):
-        monkeypatch.setattr(A, "y")  # type: ignore[call-overload]
+    # Test that inherited attributes don't get written into the target's instance
+    # dictionary.
+    a = A()
+    monkeypatch = MonkeyPatch()
+    assert "x" not in vars(a)
+    monkeypatch.setattr(a, "x", 2)
+    monkeypatch.undo()
+    assert "x" not in vars(a)
+
+    for obj in (A, A()):
+        # Test that class/instance descriptors don't get bound and written into the
+        # target's dictionary.
+        monkeypatch = MonkeyPatch()
+        assert isinstance(inspect.getattr_static(obj, "y"), SomeDescriptor)
+        monkeypatch.setattr(obj, "y", 2)
+        monkeypatch.undo()
+        assert isinstance(inspect.getattr_static(obj, "y"), SomeDescriptor)
+
+        # Test that the `raising=True` check binds descriptors (to check if they raise
+        # AttributeError).
+        monkeypatch = MonkeyPatch()
+        with pytest.raises(AssertionError, match="descriptor was bound"):
+            monkeypatch.setattr(obj, "z", 2)
+        # Test that descriptors don't get bound if `raising=False`.
+        monkeypatch.setattr(obj, "z", 2, raising=False)
+        monkeypatch.undo()
 
 
 class TestSetattrWithImportPath:
@@ -97,8 +162,12 @@ class TestSetattrWithImportPath:
 
 
 def test_delattr() -> None:
+    import inspect
+
     class A:
         x = 1
+        y = SomeDescriptor()
+        z = RaisingDescriptor()
 
     monkeypatch = MonkeyPatch()
     monkeypatch.delattr(A, "x")
@@ -107,14 +176,31 @@ def test_delattr() -> None:
     assert A.x == 1
 
     monkeypatch = MonkeyPatch()
+    pytest.raises(AttributeError, monkeypatch.delattr, A, "w")
+    monkeypatch.delattr(A, "w", raising=False)
     monkeypatch.delattr(A, "x")
-    pytest.raises(AttributeError, monkeypatch.delattr, A, "y")
-    monkeypatch.delattr(A, "y", raising=False)
     monkeypatch.setattr(A, "x", 5, raising=False)
     assert A.x == 5
     monkeypatch.undo()
     assert A.x == 1
 
+    # Test that (non-inherited) class descriptors don't get bound and written into the
+    # target's dictionary.
+    monkeypatch = MonkeyPatch()
+    assert isinstance(inspect.getattr_static(A, "y"), SomeDescriptor)
+    monkeypatch.delattr(A, "y")
+    monkeypatch.undo()
+    assert isinstance(inspect.getattr_static(A, "y"), SomeDescriptor)
+
+    # Test that the `raising=True` check binds descriptors (to check if they raise
+    # AttributeError).
+    monkeypatch = MonkeyPatch()
+    with pytest.raises(AssertionError, match="descriptor was bound"):
+        monkeypatch.delattr(A, "z")
+    # Test that descriptor's don't get bound if `raising=False`.
+    monkeypatch.delattr(A, "z", raising=False)
+    monkeypatch.undo()
+
 
 def test_setitem() -> None:
     d = {"x": 1}