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()