Unwrap right hand proxy operand in pure Python binary operators

docs/changes.rst

@@ -75,6 +75,38 @@ Version 2.4.2
   Normal attribute lookup was not affected as Python always supplies both
   arguments in that case.
 
+* Applying a binary operator such as ``+`` where both operands were an
+  ``ObjectProxy`` could give a different result with the pure Python
+  implementation than with the C extension. The C extension unwrapped both
+  operands before applying the operator to the wrapped objects, whereas the
+  pure Python implementation unwrapped only the left hand operand and
+  passed the right hand proxy through to the operator method of the
+  wrapped object. This usually went unnoticed, as the wrapped object's
+  operator method would return ``NotImplemented`` when given a proxy, and
+  Python would then try the reflected method of the right hand proxy,
+  which unwrapped the other side.
+
+  The difference was visible where the wrapped type raised ``TypeError``
+  for an operand it did not recognise instead of returning
+  ``NotImplemented``, since Python never tries the reflected method after
+  an exception, so the operation failed with the pure Python
+  implementation but succeeded with the C extension. It was also visible
+  where the right hand operand's type was a subclass of the left hand
+  operand's type and overrode the reflected method. Python gives that
+  reflected method priority, but only when it sees the real types, so
+  with the pure Python implementation the forward method of the left hand
+  wrapped object was called instead.
+
+  The pure Python implementation now also unwraps a right hand operand
+  which is a proxy, for the binary, reflected and in-place operators, so
+  the result is the same as applying the operator to the two wrapped
+  objects in both implementations. Where a proxy is on the right hand side
+  but the left hand operand is not a proxy, Python still selects the
+  method to call from the types it can see, so the reflected method of a
+  proxied subclass on the right hand side is not given priority in either
+  implementation. The modulo argument of the three argument form of
+  ``pow()`` is still not unwrapped, as described in the known issues.
+
 Version 2.4.1
 -------------
 

docs/issues.rst

@@ -977,33 +977,30 @@ Ternary ``pow()`` with ObjectProxy
 ----------------------------------
 
 The three-argument form of the builtin ``pow()`` function does not accept
-an ``ObjectProxy`` in every argument position, and the set of positions
-that is accepted depends on whether the C extension is in use.
-
-With the pure Python implementation, only the first argument (the base)
-may be an ``ObjectProxy``. This is because the ``__pow__`` method on
-``ObjectProxy`` unwraps ``self`` before delegating to ``pow()``, but it
-does not unwrap the second argument or the modulo argument. When
-``pow()`` is called with a proxy in either of those positions, the
-underlying numeric type has no way to coerce the proxy and a
+an ``ObjectProxy`` in every argument position.
+
+The base may be an ``ObjectProxy``, and when it is, the exponent may be
+one as well. The ``__pow__`` method on ``ObjectProxy`` unwraps ``self``
+and, if it is also a proxy, the exponent, before delegating to
+``pow()``. If the base is not a proxy but the exponent is, Python calls
+the ``__pow__`` method of the base's type first, and unlike the two
+argument form, the three argument form has no reflected ``__rpow__``
+fallback, so the proxy never gets a chance to unwrap itself and a
 ``TypeError`` is raised.
 
-With the C extension, the first and second arguments may both be an
-``ObjectProxy`` because the ``nb_power`` slot explicitly unwraps them
-before dispatching to ``PyNumber_Power``. The modulo argument is still
-not unwrapped, for two reasons. Firstly, unwrapping modulo would make
-the C extension behaviour diverge further from the pure Python and PyPy
-implementations, which cannot be made to support a proxy in that slot.
-Secondly, if ``PyNumber_Power`` were invoked with a proxy modulo, the
+The modulo argument is never unwrapped. If the C extension's
+``nb_power`` slot invoked ``PyNumber_Power`` with a proxy modulo, the
 CPython ternary operator fallback would end up calling back into the
-proxy's ``nb_power`` slot indefinitely, overflowing the C stack. A
-proxy passed as the modulo argument therefore always results in a
-``TypeError`` regardless of implementation.
+proxy's ``nb_power`` slot indefinitely, overflowing the C stack, so the
+slot returns ``NotImplemented`` for a proxy modulo instead. The pure
+Python implementation does not unwrap modulo either, so that behaviour
+is the same across the pure Python implementation, the C extension and
+PyPy. A proxy passed as the modulo argument therefore always results in
+a ``TypeError``.
 
-The practical consequence is that for portability across the pure
-Python implementation, the C extension and PyPy, callers should pass
-only the base as an ``ObjectProxy`` and should unwrap the exponent and
-modulo arguments themselves where necessary.
+The practical consequence is that callers should ensure the base is an
+``ObjectProxy`` whenever the exponent is, and should unwrap the modulo
+argument themselves where necessary.
 
 ::
 
@@ -1013,13 +1010,14 @@ modulo arguments themselves where necessary.
     exponent = wrapt.ObjectProxy(3)
     modulo = wrapt.ObjectProxy(5)
 
-    # Portable: only the base is a proxy.
+    # Supported: base only, or base and exponent, as proxies.
     pow(base, 3, 5)
-
-    # C extension only: exponent may also be a proxy.
     pow(base, exponent, 5)
 
-    # Not supported anywhere: unwrap the modulo yourself.
+    # Not supported: exponent is a proxy but base is not.
+    pow(2, exponent.__wrapped__, 5)
+
+    # Not supported: unwrap the modulo yourself.
     pow(base, exponent, modulo.__wrapped__)
 
 pytest setup_class/teardown_class hooks

src/wrapt/wrappers.py

@@ -162,6 +162,24 @@ def __new__(cls, name, bases, dictionary):
         return klass
 
 
+def _unwrap_operand(other):
+    # When the other operand of a binary operator is itself a proxy, apply
+    # the operator to the two wrapped objects rather than passing the proxy
+    # through to the wrapped object's own operator method. This mirrors
+    # wrapt_unwrap_operand() in the C extension. Without it, a wrapped
+    # type which raises TypeError for an unrecognised operand, rather
+    # than returning NotImplemented, would never give the proxy on the
+    # right hand side the chance to unwrap itself via the reflected
+    # method, and Python would not see the real types of the operands
+    # when deciding whether the reflected method of a subclass on the
+    # right hand side takes priority. The real type is checked, and not
+    # __class__, which the proxy reports as that of the wrapped object.
+
+    if issubclass(type(other), ObjectProxy):
+        return other.__wrapped__
+    return other
+
+
 class ObjectProxy(_ObjectProxyDictBase, metaclass=_ObjectProxyMetaType):
     """A transparent object proxy that delegates attribute access to a
     wrapped object."""
@@ -434,161 +452,199 @@ def __delattr__(self, name):
             delattr(self.__wrapped__, name)
 
     def __add__(self, other):
+        other = _unwrap_operand(other)
         return self.__wrapped__ + other
 
     def __sub__(self, other):
+        other = _unwrap_operand(other)
         return self.__wrapped__ - other
 
     def __mul__(self, other):
+        other = _unwrap_operand(other)
         return self.__wrapped__ * other
 
     def __truediv__(self, other):
+        other = _unwrap_operand(other)
         return operator.truediv(self.__wrapped__, other)
 
     def __floordiv__(self, other):
+        other = _unwrap_operand(other)
         return self.__wrapped__ // other
 
     def __mod__(self, other):
+        other = _unwrap_operand(other)
         return self.__wrapped__ % other
 
     def __divmod__(self, other):
+        other = _unwrap_operand(other)
         return divmod(self.__wrapped__, other)
 
     def __pow__(self, other, *args):
+        other = _unwrap_operand(other)
         return pow(self.__wrapped__, other, *args)
 
     def __lshift__(self, other):
+        other = _unwrap_operand(other)
         return self.__wrapped__ << other
 
     def __rshift__(self, other):
+        other = _unwrap_operand(other)
         return self.__wrapped__ >> other
 
     def __and__(self, other):
+        other = _unwrap_operand(other)
         return self.__wrapped__ & other
 
     def __xor__(self, other):
+        other = _unwrap_operand(other)
         return self.__wrapped__ ^ other
 
     def __or__(self, other):
+        other = _unwrap_operand(other)
         return self.__wrapped__ | other
 
     def __radd__(self, other):
+        other = _unwrap_operand(other)
         return other + self.__wrapped__
 
     def __rsub__(self, other):
+        other = _unwrap_operand(other)
         return other - self.__wrapped__
 
     def __rmul__(self, other):
+        other = _unwrap_operand(other)
         return other * self.__wrapped__
 
     def __rtruediv__(self, other):
+        other = _unwrap_operand(other)
         return operator.truediv(other, self.__wrapped__)
 
     def __rfloordiv__(self, other):
+        other = _unwrap_operand(other)
         return other // self.__wrapped__
 
     def __rmod__(self, other):
+        other = _unwrap_operand(other)
         return other % self.__wrapped__
 
     def __rdivmod__(self, other):
+        other = _unwrap_operand(other)
         return divmod(other, self.__wrapped__)
 
     def __rpow__(self, other, *args):
+        other = _unwrap_operand(other)
         return pow(other, self.__wrapped__, *args)
 
     def __rlshift__(self, other):
+        other = _unwrap_operand(other)
         return other << self.__wrapped__
 
     def __rrshift__(self, other):
+        other = _unwrap_operand(other)
         return other >> self.__wrapped__
 
     def __rand__(self, other):
+        other = _unwrap_operand(other)
         return other & self.__wrapped__
 
     def __rxor__(self, other):
+        other = _unwrap_operand(other)
         return other ^ self.__wrapped__
 
     def __ror__(self, other):
+        other = _unwrap_operand(other)
         return other | self.__wrapped__
 
     def __iadd__(self, other):
+        other = _unwrap_operand(other)
         if hasattr(self.__wrapped__, "__iadd__"):
             self.__wrapped__ += other
             return self
         else:
             return self.__object_proxy__(self.__wrapped__ + other)
 
     def __isub__(self, other):
+        other = _unwrap_operand(other)
         if hasattr(self.__wrapped__, "__isub__"):
             self.__wrapped__ -= other
             return self
         else:
             return self.__object_proxy__(self.__wrapped__ - other)
 
     def __imul__(self, other):
+        other = _unwrap_operand(other)
         if hasattr(self.__wrapped__, "__imul__"):
             self.__wrapped__ *= other
             return self
         else:
             return self.__object_proxy__(self.__wrapped__ * other)
 
     def __itruediv__(self, other):
+        other = _unwrap_operand(other)
         if hasattr(self.__wrapped__, "__itruediv__"):
             self.__wrapped__ /= other
             return self
         else:
             return self.__object_proxy__(self.__wrapped__ / other)
 
     def __ifloordiv__(self, other):
+        other = _unwrap_operand(other)
         if hasattr(self.__wrapped__, "__ifloordiv__"):
             self.__wrapped__ //= other
             return self
         else:
             return self.__object_proxy__(self.__wrapped__ // other)
 
     def __imod__(self, other):
+        other = _unwrap_operand(other)
         if hasattr(self.__wrapped__, "__imod__"):
             self.__wrapped__ %= other
             return self
         else:
             return self.__object_proxy__(self.__wrapped__ % other)
 
     def __ipow__(self, other):  # type: ignore[misc]
+        other = _unwrap_operand(other)
         if hasattr(self.__wrapped__, "__ipow__"):
             self.__wrapped__ **= other
             return self
         else:
             return self.__object_proxy__(self.__wrapped__**other)
 
     def __ilshift__(self, other):
+        other = _unwrap_operand(other)
         if hasattr(self.__wrapped__, "__ilshift__"):
             self.__wrapped__ <<= other
             return self
         else:
             return self.__object_proxy__(self.__wrapped__ << other)
 
     def __irshift__(self, other):
+        other = _unwrap_operand(other)
         if hasattr(self.__wrapped__, "__irshift__"):
             self.__wrapped__ >>= other
             return self
         else:
             return self.__object_proxy__(self.__wrapped__ >> other)
 
     def __iand__(self, other):
+        other = _unwrap_operand(other)
         if hasattr(self.__wrapped__, "__iand__"):
             self.__wrapped__ &= other
             return self
         else:
             return self.__object_proxy__(self.__wrapped__ & other)
 
     def __ixor__(self, other):
+        other = _unwrap_operand(other)
         if hasattr(self.__wrapped__, "__ixor__"):
             self.__wrapped__ ^= other
             return self
         else:
             return self.__object_proxy__(self.__wrapped__ ^ other)
 
     def __ior__(self, other):
+        other = _unwrap_operand(other)
         if hasattr(self.__wrapped__, "__ior__"):
             self.__wrapped__ |= other
             return self
@@ -620,12 +676,15 @@ def __index__(self):
         return operator.index(self.__wrapped__)
 
     def __matmul__(self, other):
+        other = _unwrap_operand(other)
         return self.__wrapped__ @ other
 
     def __rmatmul__(self, other):
+        other = _unwrap_operand(other)
         return other @ self.__wrapped__
 
     def __imatmul__(self, other):
+        other = _unwrap_operand(other)
         if hasattr(self.__wrapped__, "__imatmul__"):
             self.__wrapped__ @= other
             return self