Merge branch 'release/2.2.1'

Author: GrahamDumpleton

docs/api.rst

@@ -1,6 +1,46 @@
 Release Notes
 =============
 
+Version 2.2.1
+-------------
+
+**Bugs Fixed**
+
+* Reverted the change in 2.2.0 which had aligned the C implementation of
+  ``FunctionWrapper.__get__`` with the pure Python implementation by
+  substituting ``Py_None`` for ``NULL`` before invoking the wrapped
+  descriptor's ``__get__`` slot. The change was based on a misreading of
+  what the pure Python path does once it crosses back into C. The pure
+  Python path calls ``self.__wrapped__.__get__(None, owner)`` from Python,
+  and for any built-in descriptor that call is dispatched through the
+  ``__get__`` slot wrapper inside CPython, which converts ``Py_None`` back
+  to ``NULL`` before the wrapped descriptor's ``tp_descr_get`` is invoked.
+  The pre 2.2.0 C path called ``tp_descr_get`` directly with ``obj`` as
+  received, which is ``NULL`` on class access, so it was already producing
+  the same value the Python path produces after the slot wrapper's
+  ``Py_None`` to ``NULL`` conversion. Substituting ``Py_None`` for ``NULL``
+  before ``tp_descr_get`` was called caused the wrapped descriptor to see
+  a value it would never see during ordinary class attribute lookup.
+  Native CPython descriptors other than ``func_descr_get`` fast path on
+  ``obj == NULL`` and return the descriptor unchanged. With ``Py_None``
+  substituted in they fall through to a type check against the owner type
+  of the descriptor, and ``NoneType`` does not satisfy that check, so a
+  ``TypeError`` is raised. This broke class attribute access for any
+  built-in or C extension descriptor (``method_descriptor``,
+  ``wrapper_descriptor``, ``getset_descriptor``, ``member_descriptor``)
+  wrapped by ``@wrapt.decorator`` or ``@wrapt.function_wrapper``. The
+  failure mode is most likely to show up in instrumentation libraries that
+  monkey patch built-in methods onto classes and where some inspection or
+  binding step then accesses the wrapped attribute through the class. The
+  existing test suite did not catch the regression because all wrappers in
+  the test suite are applied to pure Python functions, whose
+  ``func_descr_get`` slot treats ``NULL`` and ``Py_None`` equivalently. A
+  new regression test has been added which wraps a ``method_descriptor``
+  and exercises class attribute access, so the missing coverage of
+  non-function descriptors is now in place. With thanks to
+  `brettlangdon <https://github.com/brettlangdon>`_ for reporting the
+  regression and identifying the underlying cause.
+
 Version 2.2.0
 -------------
 

docs/changes.rst

@@ -54,6 +54,7 @@ Documentation
    monkey
    typing
    bundled
+   api
    examples
    benchmarks
    changes

docs/index.rst

@@ -201,6 +201,122 @@ This is an inherent limitation of the transparent proxy pattern: the
 proxy can override ``__class__`` at the Python level, but it cannot
 change the object's C-level type.
 
+Deriving from ObjectProxy alongside an ABCMeta-based class
+----------------------------------------------------------
+
+A custom proxy that derives from both ``ObjectProxy`` and a second base
+class whose metaclass is ``abc.ABCMeta`` will fail when used with
+``isinstance()`` or ``issubclass()``::
+
+    from abc import ABC
+    from wrapt import ObjectProxy
+
+    class Base(ABC):
+        pass
+
+    class Proxy(ObjectProxy, Base):
+        pass
+
+    isinstance(1, Proxy)
+    # TypeError: descriptor '__subclasscheck__' for '_wrappers.ObjectProxy'
+    # objects doesn't apply to a 'type' object
+
+The same failure occurs when the second base class is one of the
+abstract base classes exported from ``collections.abc`` (for example
+``Hashable``, ``Iterable``, ``Container``), since they too use
+``ABCMeta`` as their metaclass.
+
+The cause is the way ``ObjectProxy`` implements ``__instancecheck__``
+and ``__subclasscheck__``. These are defined as instance methods on the
+proxy class so that an ``ObjectProxy`` instance can appear on the right
+hand side of an ``isinstance()`` or ``issubclass()`` check and have the
+check delegate to the wrapped type. They rely on ``self`` being a real
+proxy instance, and the C extension enforces this at the descriptor
+level.
+
+When ``ObjectProxy`` is mixed in as a base class alongside an
+``ABCMeta``-based class, those methods are inherited as ordinary
+instance methods on the resulting class. Unlike the default
+``type.__instancecheck__``, ``ABCMeta.__instancecheck__`` performs its
+work by calling ``cls.__subclasscheck__(...)`` via normal attribute
+access on the class. That attribute access finds the inherited
+``__subclasscheck__`` from ``ObjectProxy`` and invokes it with a class
+as the first argument. The C descriptor sees that the first argument is
+not an ``ObjectProxy`` instance and raises the ``TypeError`` shown
+above.
+
+Mixing ``ObjectProxy`` with one of these abstract base classes at
+runtime is almost always the wrong approach to begin with.
+``ObjectProxy`` is designed to be used as a single base class, with
+derived classes overriding only the specific methods that need to
+change. Adding a second, unrelated base class brings in extra
+protocol-level behaviour which interacts poorly with what
+``ObjectProxy`` already does internally.
+
+The usual motivation for adding an abstract base class such as
+``Hashable`` to the base list is to satisfy a static type checker which
+has been told to expect the proxy to be declared as a subtype of that
+abstract base class. At runtime the inheritance is typically redundant.
+The abstract base classes in ``collections.abc`` use a structural
+``__subclasshook__`` (``Hashable`` is satisfied by anything that
+defines ``__hash__``, ``Iterable`` by anything that defines
+``__iter__``, and so on), and ``ObjectProxy`` already defines those
+methods where appropriate, forwarding to the wrapped object. So
+``isinstance(proxy, Hashable)`` is already ``True`` for an
+``ObjectProxy`` instance without any explicit inheritance::
+
+    from collections.abc import Hashable
+    import wrapt
+
+    isinstance(wrapt.ObjectProxy("s"), Hashable)    # True
+
+The runtime inheritance from the abstract base class adds nothing
+useful in this case, and brings in the ``ABCMeta`` metaclass which then
+collides with ``ObjectProxy`` as described above.
+
+The recommended approach is to keep the runtime class hierarchy clean
+and present the type-checker-required relationship using typing
+constructs rather than runtime inheritance. When the annotation site is
+under your own control, the cleanest option is to define a
+``typing.Protocol`` that captures the required structural shape and use
+that as the annotation, instead of inheriting from an abstract base
+class. ``ObjectProxy`` will structurally satisfy such a ``Protocol``
+through the dunder methods it already forwards, with no inheritance and
+no runtime change at all.
+
+When the annotation site is not under your own control and demands a
+nominal subtype of a specific abstract base class, the class can be
+declared twice in the same file, guarded by ``typing.TYPE_CHECKING``::
+
+    from typing import TYPE_CHECKING
+
+    from wrapt import ObjectProxy
+
+    if TYPE_CHECKING:
+        from collections.abc import Hashable
+
+        class Proxy(ObjectProxy, Hashable):
+            ...
+    else:
+        class Proxy(ObjectProxy):
+            pass
+
+``TYPE_CHECKING`` is ``False`` at runtime and ``True`` during static
+analysis, and both mypy and pyright honour this. The type checker sees
+the multi-base version, which satisfies whatever annotation required
+``Hashable`` to appear in the inheritance chain. The Python interpreter
+only ever executes the ``else`` branch, so at runtime ``Proxy`` is a
+plain ``ObjectProxy`` subclass with no ``ABCMeta`` in the picture and
+the original ``TypeError`` does not occur.
+
+The same trick generalises to any case where the view of a class
+presented to a static type checker needs to differ from the runtime
+class hierarchy. If several such declarations need to be maintained
+together it can be cleaner to lift them into a sibling ``.pyi`` stub
+file, which the type checker will honour in preference to the ``.py``
+source. For a single case the inline ``TYPE_CHECKING`` form is usually
+enough.
+
 Using the json module with ObjectProxy
 --------------------------------------
 

docs/issues.rst

@@ -2,14 +2,18 @@
 Wrapt is a library for decorators, wrappers and monkey patching.
 """
 
+
 def _format_version(parts):
     base = ".".join(parts[:3])
     if len(parts) == 3:
         return base
     suffix = parts[3]
-    return f"{base}.{suffix}" if suffix.startswith(("dev", "post")) else f"{base}{suffix}"
+    return (
+        f"{base}.{suffix}" if suffix.startswith(("dev", "post")) else f"{base}{suffix}"
+    )
+
 
-__version_info__ = ("2", "2", "0")
+__version_info__ = ("2", "2", "1")
 __version__ = _format_version(__version_info__)
 
 from .__wrapt__ import (

src/wrapt/__init__.py

@@ -3821,9 +3821,6 @@ WraptFunctionWrapperBase_descr_get(WraptFunctionWrapperObject *self,
       return (PyObject *)self;
     }
 
-    if (obj == NULL)
-      obj = Py_None;
-
     descriptor = (Py_TYPE(self->object_proxy.wrapped)->tp_descr_get)(
         self->object_proxy.wrapped, obj, type);
 
@@ -3845,6 +3842,9 @@ WraptFunctionWrapperBase_descr_get(WraptFunctionWrapperObject *self,
       }
     }
 
+    if (obj == NULL)
+      obj = Py_None;
+
     result = PyObject_CallFunctionObjArgs(
         bound_type ? bound_type : (PyObject *)state->BoundFunctionWrapper_Type,
         descriptor, obj, self->wrapper, self->enabled, self->binding, self,
@@ -3899,9 +3899,6 @@ WraptFunctionWrapperBase_descr_get(WraptFunctionWrapperObject *self,
         return NULL;
       }
 
-      if (obj == NULL)
-        obj = Py_None;
-
       descriptor = (Py_TYPE(wrapped)->tp_descr_get)(wrapped, obj, type);
 
       Py_DECREF(wrapped);
@@ -3925,6 +3922,9 @@ WraptFunctionWrapperBase_descr_get(WraptFunctionWrapperObject *self,
         }
       }
 
+      if (obj == NULL)
+        obj = Py_None;
+
       result = PyObject_CallFunctionObjArgs(
           bound_type ? bound_type : (PyObject *)state->BoundFunctionWrapper_Type,
           descriptor, obj, self->wrapper, self->enabled, self->binding,