From 46d103401860b504136f3401ad0082db65712725 Mon Sep 17 00:00:00 2001 From: Graham Dumpleton Date: Tue, 22 Sep 2026 09:44:10 +1000 Subject: [PATCH] Unwrap right hand proxy operand in pure Python binary operators. The C extension unwraps both operands of a binary operator before applying it to the wrapped objects, but the pure Python implementation only unwrapped self and passed a right hand proxy through to the wrapped object's operator method. This was hidden where the wrapped type returned NotImplemented, as the right hand proxy's reflected method then unwrapped the other side, but gave a different result to the C extension where the wrapped type raised TypeError instead, or where the right hand operand was a proxied subclass overriding the reflected method. The pure Python binary, reflected and in-place operators now unwrap a right hand operand which is a proxy, matching the C extension. Tests cover all affected operators against the result of the operation on the unwrapped objects. The ternary pow() known issue is updated as a proxy exponent now works alongside a proxy base in both implementations. --- docs/changes.rst | 32 +++ docs/issues.rst | 52 ++-- src/wrapt/wrappers.py | 59 +++++ tests/core/test_proxy_operands.py | 397 ++++++++++++++++++++++++++++++ 4 files changed, 513 insertions(+), 27 deletions(-) create mode 100644 tests/core/test_proxy_operands.py diff --git a/docs/changes.rst b/docs/changes.rst index 25420b6c..67d3fa3a 100644 --- a/docs/changes.rst +++ b/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 ------------- diff --git a/docs/issues.rst b/docs/issues.rst index 83264deb..40d18a40 100644 --- a/docs/issues.rst +++ b/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 diff --git a/src/wrapt/wrappers.py b/src/wrapt/wrappers.py index f34ec90e..0907509d 100644 --- a/src/wrapt/wrappers.py +++ b/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,84 +452,111 @@ 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 @@ -519,6 +564,7 @@ def __iadd__(self, other): 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 @@ -526,6 +572,7 @@ def __isub__(self, other): 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 @@ -533,6 +580,7 @@ def __imul__(self, other): 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 @@ -540,6 +588,7 @@ def __itruediv__(self, other): 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 @@ -547,6 +596,7 @@ def __ifloordiv__(self, other): 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 @@ -554,6 +604,7 @@ def __imod__(self, other): 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 @@ -561,6 +612,7 @@ def __ipow__(self, other): # type: ignore[misc] 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 @@ -568,6 +620,7 @@ def __ilshift__(self, other): 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 @@ -575,6 +628,7 @@ def __irshift__(self, other): 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 @@ -582,6 +636,7 @@ def __iand__(self, other): 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 @@ -589,6 +644,7 @@ def __ixor__(self, other): 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 diff --git a/tests/core/test_proxy_operands.py b/tests/core/test_proxy_operands.py new file mode 100644 index 00000000..cda0a1aa --- /dev/null +++ b/tests/core/test_proxy_operands.py @@ -0,0 +1,397 @@ +"""Tests for binary operators where both operands are an ObjectProxy. + +The pure Python implementation of the binary operator dunder methods only +unwraps ``self`` before delegating to the operator on the wrapped object, +whereas the C extension unwraps both operands. In the common case this +difference is hidden, because the wrapped object's own dunder method +returns ``NotImplemented`` when handed a proxy, and the proxy's reflected +dunder method then gets a turn and unwraps the other side. + +The difference becomes visible in two situations, both of which only +arise when the right hand operand is also a proxy: + +* The wrapped type raises ``TypeError`` for an operand it does not + recognise, rather than returning ``NotImplemented``. Python treats a + raised exception as final and never tries the reflected method, so + the right hand proxy never gets to unwrap itself. + +* The right hand operand's type is a subclass of the left hand operand's + type and overrides the reflected method. Python gives the reflected + method priority in that case, but only when it sees the real types. + The left hand proxy running the operator on the wrapped objects + restores that priority if it unwraps the right hand side first. + +Both cases pass with the C extension and are expected to pass with the +pure Python implementation once it also unwraps a proxy on the right +hand side. +""" + +import operator +import unittest + +import wrapt + +# Names of the binary operators, in the form used for the dunder methods. +# All except divmod also have an in-place form. + +BINARY_OPERATORS = [ + ("add", operator.add), + ("sub", operator.sub), + ("mul", operator.mul), + ("truediv", operator.truediv), + ("floordiv", operator.floordiv), + ("mod", operator.mod), + ("divmod", divmod), + ("pow", operator.pow), + ("lshift", operator.lshift), + ("rshift", operator.rshift), + ("and", operator.and_), + ("xor", operator.xor), + ("or", operator.or_), + ("matmul", operator.matmul), +] + +INPLACE_OPERATORS = [ + ("add", operator.iadd), + ("sub", operator.isub), + ("mul", operator.imul), + ("truediv", operator.itruediv), + ("floordiv", operator.ifloordiv), + ("mod", operator.imod), + ("pow", operator.ipow), + ("lshift", operator.ilshift), + ("rshift", operator.irshift), + ("and", operator.iand), + ("xor", operator.ixor), + ("or", operator.ior), + ("matmul", operator.imatmul), +] + + +class Strict: + """A type which only combines with other instances of itself. + + Every binary operator raises TypeError when given anything other + than a Strict instance, instead of returning NotImplemented. The + result records which operator was applied and to what. + """ + + def __init__(self, value): + self.value = value + + def __eq__(self, other): + return type(other) is Strict and other.value == self.value + + __hash__ = None + + def __repr__(self): + return "Strict(%r)" % (self.value,) + + +class StrictInplace(Strict): + """As Strict, but also implements the in-place operators, which + mutate the instance and return it.""" + + def __eq__(self, other): + return type(other) is StrictInplace and other.value == self.value + + __hash__ = None + + +def _strict_binary(name): + def method(self, other): + if type(other) is not type(self): + raise TypeError( + "unsupported operand type for %s: %r" % (name, type(other).__name__) + ) + return type(self)((name, self.value, other.value)) + + method.__name__ = "__%s__" % name + return method + + +def _strict_inplace(name): + def method(self, other): + if type(other) is not type(self): + raise TypeError( + "unsupported operand type for i%s: %r" % (name, type(other).__name__) + ) + self.value = (name, self.value, other.value) + return self + + method.__name__ = "__i%s__" % name + return method + + +for _name, _ in BINARY_OPERATORS: + setattr(Strict, "__%s__" % _name, _strict_binary(_name)) + +for _name, _ in INPLACE_OPERATORS: + setattr(StrictInplace, "__i%s__" % _name, _strict_inplace(_name)) + + +class Base: + """Accepts any Base instance in its binary operators and returns + NotImplemented otherwise. The reflected operators are also defined + so that a subclass can override them.""" + + def __repr__(self): + return "Base()" + + +class Sub(Base): + """Overrides only the reflected operators. Python gives these + priority over the forward operator of a Base instance on the left + hand side.""" + + def __repr__(self): + return "Sub()" + + +def _base_forward(name): + def method(self, other): + if isinstance(other, Base): + return "Base.__%s__" % name + return NotImplemented + + method.__name__ = "__%s__" % name + return method + + +def _reflected(cls, name): + def method(self, other): + return "%s.__r%s__" % (cls, name) + + method.__name__ = "__r%s__" % name + return method + + +for _name, _ in BINARY_OPERATORS: + setattr(Base, "__%s__" % _name, _base_forward(_name)) + setattr(Base, "__r%s__" % _name, _reflected("Base", _name)) + setattr(Sub, "__r%s__" % _name, _reflected("Sub", _name)) + + +class TestStrictOperandBinary(unittest.TestCase): + """The wrapped type raises TypeError instead of returning + NotImplemented, so the operator only succeeds if the proxy on the + right hand side is unwrapped before the wrapped operator runs.""" + + def check(self, name, op): + expected = op(Strict(1), Strict(2)) + + self.assertEqual(expected, Strict((name, 1, 2))) + + # A proxy on the left only has always worked. + + self.assertEqual(op(wrapt.ObjectProxy(Strict(1)), Strict(2)), expected) + + # A proxy on both sides requires the left hand proxy to unwrap + # the right hand one, since Strict never returns NotImplemented + # and so the right hand proxy's reflected method never runs. + + self.assertEqual( + op(wrapt.ObjectProxy(Strict(1)), wrapt.ObjectProxy(Strict(2))), + expected, + ) + + def test_add(self): + self.check("add", operator.add) + + def test_sub(self): + self.check("sub", operator.sub) + + def test_mul(self): + self.check("mul", operator.mul) + + def test_truediv(self): + self.check("truediv", operator.truediv) + + def test_floordiv(self): + self.check("floordiv", operator.floordiv) + + def test_mod(self): + self.check("mod", operator.mod) + + def test_divmod(self): + self.check("divmod", divmod) + + def test_pow(self): + self.check("pow", operator.pow) + + def test_lshift(self): + self.check("lshift", operator.lshift) + + def test_rshift(self): + self.check("rshift", operator.rshift) + + def test_and(self): + self.check("and", operator.and_) + + def test_xor(self): + self.check("xor", operator.xor) + + def test_or(self): + self.check("or", operator.or_) + + def test_matmul(self): + self.check("matmul", operator.matmul) + + +class TestStrictOperandInplace(unittest.TestCase): + """In-place operators with a proxy on both sides. Covers both the + case where the wrapped type implements the in-place dunder method + and the proxy is updated in place, and the case where it does not + and the proxy falls back to the binary operator and wraps the + result in a new proxy.""" + + def check(self, name, iop): + # Wrapped type implements the in-place operator. The same proxy + # is returned and the wrapped object mutated in place. + + expected = StrictInplace(1) + iop(expected, StrictInplace(2)) + + self.assertEqual(expected, StrictInplace((name, 1, 2))) + + value = StrictInplace(1) + proxy = wrapt.ObjectProxy(value) + + result = iop(proxy, wrapt.ObjectProxy(StrictInplace(2))) + + self.assertIs(result, proxy) + self.assertIs(result.__wrapped__, value) + self.assertEqual(value, expected) + + # Wrapped type only implements the binary operator. A new proxy + # is returned wrapping the result. + + expected = Strict(1) + expected = iop(expected, Strict(2)) + + self.assertEqual(expected, Strict((name, 1, 2))) + + value = Strict(1) + proxy = wrapt.ObjectProxy(value) + + result = iop(proxy, wrapt.ObjectProxy(Strict(2))) + + self.assertIsNot(result, proxy) + self.assertIsInstance(result, wrapt.ObjectProxy) + self.assertEqual(result.__wrapped__, expected) + self.assertEqual(value, Strict(1)) + + def test_iadd(self): + self.check("add", operator.iadd) + + def test_isub(self): + self.check("sub", operator.isub) + + def test_imul(self): + self.check("mul", operator.imul) + + def test_itruediv(self): + self.check("truediv", operator.itruediv) + + def test_ifloordiv(self): + self.check("floordiv", operator.ifloordiv) + + def test_imod(self): + self.check("mod", operator.imod) + + def test_ipow(self): + self.check("pow", operator.ipow) + + def test_ilshift(self): + self.check("lshift", operator.ilshift) + + def test_irshift(self): + self.check("rshift", operator.irshift) + + def test_iand(self): + self.check("and", operator.iand) + + def test_ixor(self): + self.check("xor", operator.ixor) + + def test_ior(self): + self.check("or", operator.ior) + + def test_imatmul(self): + self.check("matmul", operator.imatmul) + + +class TestSubclassReflectedPriority(unittest.TestCase): + """When the right hand operand's type is a subclass of the left hand + operand's type and overrides the reflected method, Python calls the + reflected method first. With a proxy on both sides, Python only + sees the proxy types, so the left hand proxy must unwrap the right + hand operand for the wrapped objects to be dispatched correctly.""" + + def check(self, name, op): + expected = "Sub.__r%s__" % name + + self.assertEqual(op(Base(), Sub()), expected) + + # A proxy on the left only has always worked, as Python sees + # Sub on the right and gives its reflected method priority. + + self.assertEqual(op(wrapt.ObjectProxy(Base()), Sub()), expected) + + # A proxy on both sides. Python sees two proxies and calls the + # left hand proxy's forward method. That must then apply the + # operator to the two wrapped objects for Python to see that + # Sub is a subclass of Base and prefer Sub's reflected method. + # Without unwrapping, Base sees the proxy as a Base instance via + # isinstance() and handles it itself. + + self.assertEqual( + op(wrapt.ObjectProxy(Base()), wrapt.ObjectProxy(Sub())), + expected, + ) + + def test_add(self): + self.check("add", operator.add) + + def test_sub(self): + self.check("sub", operator.sub) + + def test_mul(self): + self.check("mul", operator.mul) + + def test_truediv(self): + self.check("truediv", operator.truediv) + + def test_floordiv(self): + self.check("floordiv", operator.floordiv) + + def test_mod(self): + self.check("mod", operator.mod) + + def test_divmod(self): + self.check("divmod", divmod) + + def test_pow(self): + self.check("pow", operator.pow) + + def test_lshift(self): + self.check("lshift", operator.lshift) + + def test_rshift(self): + self.check("rshift", operator.rshift) + + def test_and(self): + self.check("and", operator.and_) + + def test_xor(self): + self.check("xor", operator.xor) + + def test_or(self): + self.check("or", operator.or_) + + def test_matmul(self): + self.check("matmul", operator.matmul) + + +if __name__ == "__main__": + unittest.main()