#1064: Comparisons with __eq__ should not raise TypeError (#1072)

* Add (failing) tests for issue #1064 * Return NotImplemented when richcmp doesn't match the expected type. * Fix tests that expect TypeError when richcmp returns NotImplemented. - The python code 'class Other: pass; c2 {} Other()' was raising a NameError: c2 not found - eq and ne never raise a TypeError, so I split the those cases. * Return NotImplemented for number-like binary operations. * Add dummy impl PyNumberProtocol for the test struct. * Rework tests of NotImplemented. * Make py_ternary_num_func return NotImplemented when type mismatches. * Return NotImplement for type mismatches in binary inplace operators. * Reduce boilerplate with `extract_or_return_not_implemented!` * Extract common definition 'Other' into a function. * Test explicitly for NotImplemented in the __ipow__ test. * Add entry in CHANGELOG for PR #1072. * Add the section 'Emulating numeric types' to the guide. * Ensure we're returning NotImplemented in tests. * Simplify the tests: only test we return NotImplemented. Our previous test were rather indirect: were relying that Python behaves correctly when we return NotImplemented. Now we only test that calling a pyclass dunder method returns NotImplemented when the argument doesn't match the type signature. This is the expected behavior. * Remove reverse operators in tests of NotImplemented The won't be used because of #844. * Apply suggestions from code review Co-authored-by: Yuji Kanagawa <yuji.kngw.80s.revive@gmail.com> * Add a note about #844 below the list of reflected operations. Co-authored-by: Yuji Kanagawa <yuji.kngw.80s.revive@gmail.com>
PyO3 · Aug 5, 2020 · f2ba3e6 · f2ba3e6
1 parent 3ac327c
commit f2ba3e6
Show file tree

Hide file tree

Showing 6 changed files with 307 additions and 15 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - `PyType::as_type_ptr` is no longer `unsafe`. [#1047](https://github.com/PyO3/pyo3/pull/1047)
 - Change `PyIterator::from_object` to return `PyResult<PyIterator>` instead of `Result<PyIterator, PyDowncastError>`. [#1051](https://github.com/PyO3/pyo3/pull/1051)
 - Implement `Send + Sync` for `PyErr`. `PyErr::new`, `PyErr::from_type`, `PyException::py_err` and `PyException::into` have had these bounds added to their arguments. [#1067](https://github.com/PyO3/pyo3/pull/1067)
+- Change `#[pyproto]` to return NotImplemented for operators for which Python can try a reversed operation. [1072](https://github.com/PyO3/pyo3/pull/1072)
 
 ### Removed
 - Remove `PyString::as_bytes`. [#1023](https://github.com/PyO3/pyo3/pull/1023)

diff --git a/guide/src/class.md b/guide/src/class.md
@@ -756,6 +756,94 @@ Each method corresponds to Python's `self.attr`, `self.attr = value` and `del se
 
     Determines the "truthyness" of the object.
 
+### Emulating numeric types
+
+The [`PyNumberProtocol`] trait allows [emulate numeric types](https://docs.python.org/3/reference/datamodel.html#emulating-numeric-types).
+
+  * `fn __add__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __sub__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __mul__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __matmul__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __truediv__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __floordiv__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __mod__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __divmod__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __pow__(lhs: impl FromPyObject, rhs: impl FromPyObject, modulo: Option<impl FromPyObject>) -> PyResult<impl ToPyObject>`
+  * `fn __lshift__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __rshift__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __and__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __or__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __xor__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+
+These methods are called to implement the binary arithmetic operations
+(`+`, `-`, `*`, `@`, `/`, `//`, `%`, `divmod()`, `pow()` and `**`, `<<`, `>>`, `&`, `^`, and `|`).
+
+If `rhs` is not of the type specified in the signature, the generated code
+will automatically `return NotImplemented`.  This is not the case for `lhs`
+which must match signature or else raise a TypeError.
+
+
+The reflected operations are also available:
+
+  * `fn __radd__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __rsub__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __rmul__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __rmatmul__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __rtruediv__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __rfloordiv__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __rmod__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __rdivmod__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __rpow__(lhs: impl FromPyObject, rhs: impl FromPyObject, modulo: Option<impl FromPyObject>) -> PyResult<impl ToPyObject>`
+  * `fn __rlshift__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __rrshift__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __rand__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __ror__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+  * `fn __rxor__(lhs: impl FromPyObject, rhs: impl FromPyObject) -> PyResult<impl ToPyObject>`
+
+The code generated for these methods expect that all arguments match the
+signature, or raise a TypeError.
+
+*Note*: Currently implementing the method for a binary arithmetic operations
+(e.g, `__add__`) shadows the reflected operation (e.g, `__radd__`).  This is
+being addressed in [#844](https://github.com/PyO3/pyo3/issues/844).  to make
+these methods
+
+
+This trait also has support the augmented arithmetic assignments (`+=`, `-=`,
+`*=`, `@=`, `/=`, `//=`, `%=`, `**=`, `<<=`, `>>=`, `&=`, `^=`, `|=`):
+
+  * `fn __iadd__(&'p mut self, other: impl FromPyObject) -> PyResult<()>`
+  * `fn __isub__(&'p mut self, other: impl FromPyObject) -> PyResult<()>`
+  * `fn __imul__(&'p mut self, other: impl FromPyObject) -> PyResult<()>`
+  * `fn __imatmul__(&'p mut self, other: impl FromPyObject) -> PyResult<()>`
+  * `fn __itruediv__(&'p mut self, other: impl FromPyObject) -> PyResult<()>`
+  * `fn __ifloordiv__(&'p mut self, other: impl FromPyObject) -> PyResult<()>`
+  * `fn __imod__(&'p mut self, other: impl FromPyObject) -> PyResult<()>`
+  * `fn __ipow__(&'p mut self, other: impl FromPyObject) -> PyResult<()>`
+  * `fn __ilshift__(&'p mut self, other: impl FromPyObject) -> PyResult<()>`
+  * `fn __irshift__(&'p mut self, other: impl FromPyObject) -> PyResult<()>`
+  * `fn __iand__(&'p mut self, other: impl FromPyObject) -> PyResult<()>`
+  * `fn __ior__(&'p mut self, other: impl FromPyObject) -> PyResult<()>`
+  * `fn __ixor__(&'p mut self, other: impl FromPyObject) -> PyResult<()>`
+
+The following methods implement the unary arithmetic operations (`-`, `+`, `abs()` and `~`):
+
+  * `fn __neg__(&'p self) -> PyResult<impl ToPyObject>`
+  * `fn __pos__(&'p self) -> PyResult<impl ToPyObject>`
+  * `fn __abs__(&'p self) -> PyResult<impl ToPyObject>`
+  * `fn __invert__(&'p self) -> PyResult<impl ToPyObject>`
+
+Support for coercions:
+
+  * `fn __complex__(&'p self) -> PyResult<impl ToPyObject>`
+  * `fn __int__(&'p self) -> PyResult<impl ToPyObject>`
+  * `fn __float__(&'p self) -> PyResult<impl ToPyObject>`
+
+Other:
+
+  * `fn __index__(&'p self) -> PyResult<impl ToPyObject>`
+  * `fn __round__(&'p self, ndigits: Option<impl FromPyObject>) -> PyResult<impl ToPyObject>`
+
 ### Garbage Collector Integration
 
 If your type owns references to other Python objects, you will need to

diff --git a/src/class/basic.rs b/src/class/basic.rs
@@ -275,10 +275,8 @@ where
     {
         crate::callback_body!(py, {
             let slf = py.from_borrowed_ptr::<crate::PyCell<T>>(slf);
-            let arg = py.from_borrowed_ptr::<PyAny>(arg);
-
+            let arg = extract_or_return_not_implemented!(py, arg);
             let op = extract_op(op)?;
-            let arg = arg.extract()?;
 
             slf.try_borrow()?.__richcmp__(arg, op).convert(py)
         })

diff --git a/src/class/macros.rs b/src/class/macros.rs
@@ -91,9 +91,8 @@ macro_rules! py_binary_num_func {
         {
             $crate::callback_body!(py, {
                 let lhs = py.from_borrowed_ptr::<$crate::PyAny>(lhs);
-                let rhs = py.from_borrowed_ptr::<$crate::PyAny>(rhs);
-
-                $class::$f(lhs.extract()?, rhs.extract()?).convert(py)
+                let rhs = extract_or_return_not_implemented!(py, rhs);
+                $class::$f(lhs.extract()?, rhs).convert(py)
             })
         }
         Some(wrap::<$class>)
@@ -138,7 +137,7 @@ macro_rules! py_binary_self_func {
             $crate::callback_body!(py, {
                 let slf_ = py.from_borrowed_ptr::<$crate::PyCell<T>>(slf);
                 let arg = py.from_borrowed_ptr::<$crate::PyAny>(arg);
-                call_mut!(slf_, $f, arg).convert(py)?;
+                call_operator_mut!(py, slf_, $f, arg).convert(py)?;
                 ffi::Py_INCREF(slf);
                 Ok(slf)
             })
@@ -222,13 +221,8 @@ macro_rules! py_ternary_num_func {
                 let arg1 = py
                     .from_borrowed_ptr::<$crate::types::PyAny>(arg1)
                     .extract()?;
-                let arg2 = py
-                    .from_borrowed_ptr::<$crate::types::PyAny>(arg2)
-                    .extract()?;
-                let arg3 = py
-                    .from_borrowed_ptr::<$crate::types::PyAny>(arg3)
-                    .extract()?;
-
+                let arg2 = extract_or_return_not_implemented!(py, arg2);
+                let arg3 = extract_or_return_not_implemented!(py, arg3);
                 $class::$f(arg1, arg2, arg3).convert(py)
             })
         }
@@ -279,7 +273,7 @@ macro_rules! py_dummy_ternary_self_func {
             $crate::callback_body!(py, {
                 let slf_cell = py.from_borrowed_ptr::<$crate::PyCell<T>>(slf);
                 let arg1 = py.from_borrowed_ptr::<$crate::PyAny>(arg1);
-                call_mut!(slf_cell, $f, arg1).convert(py)?;
+                call_operator_mut!(py, slf_cell, $f, arg1).convert(py)?;
                 ffi::Py_INCREF(slf);
                 Ok(slf)
             })
@@ -375,13 +369,35 @@ macro_rules! py_func_set_del {
     }};
 }
 
+macro_rules! extract_or_return_not_implemented {
+    ($py: ident, $arg: ident) => {
+        match $py
+            .from_borrowed_ptr::<$crate::types::PyAny>($arg)
+            .extract()
+        {
+            Ok(value) => value,
+            Err(_) => return $py.NotImplemented().convert($py),
+        }
+    };
+}
+
 macro_rules! _call_impl {
     ($slf: expr, $fn: ident $(; $args: expr)*) => {
         $slf.$fn($($args,)*)
     };
     ($slf: expr, $fn: ident, $raw_arg: expr $(,$raw_args: expr)* $(; $args: expr)*) => {
         _call_impl!($slf, $fn $(,$raw_args)* $(;$args)* ;$raw_arg.extract()?)
     };
+    (op $py:ident; $slf: expr, $fn: ident, $raw_arg: expr $(,$raw_args: expr)* $(; $args: expr)*) => {
+        _call_impl!(
+            $slf, $fn ;
+            (match $raw_arg.extract() {
+                Ok(res) => res,
+                _=> return Ok($py.NotImplemented().convert($py)?)
+            })
+            $(;$args)*
+        )
+    }
 }
 
 /// Call `slf.try_borrow()?.$fn(...)`
@@ -397,3 +413,9 @@ macro_rules! call_mut {
         _call_impl!($slf.try_borrow_mut()?, $fn $(,$raw_args)* $(;$args)*)
     };
 }
+
+macro_rules! call_operator_mut {
+    ($py:ident, $slf: expr, $fn: ident $(,$raw_args: expr)* $(; $args: expr)*) => {
+        _call_impl!(op $py; $slf.try_borrow_mut()?, $fn $(,$raw_args)* $(;$args)*)
+    };
+}
diff --git a/src/lib.rs b/src/lib.rs
@@ -169,6 +169,7 @@ pub mod buffer;
 pub mod callback;
 pub mod class;
 pub mod conversion;
+#[macro_use]
 #[doc(hidden)]
 pub mod derive_utils;
 mod err;