Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 18 commits into from Aug 5, 2020

Conversation

mvaled
Copy link
Contributor

@mvaled mvaled commented Jul 28, 2020

RichComparisonToSelf expects to compare itself with objects of the same type, but then we shouldn't raise TypeError but return NotImplemented.

Fixes #1064

@mvaled mvaled changed the title Add a (failing test) for issue #1064 #1064: Comparisons with __eq__ should not raise TypeError Jul 28, 2020
tests/test_arithmetics.rs Outdated Show resolved Hide resolved
tests/test_arithmetics.rs Outdated Show resolved Hide resolved
tests/test_arithmetics.rs Outdated Show resolved Hide resolved
@kngwyu
Copy link
Member

kngwyu commented Jul 30, 2020

I think the implementation would be straightforward as @davidhewitt commented, but please give us any question you have 😺

@mvaled
Copy link
Contributor Author

mvaled commented Jul 30, 2020

Is it ok to rewrite the commit for the changes proposed?

@davidhewitt
Copy link
Member

Yes, force-pushing to this branch is fine if you want to rewrite it as a single commit! Really up to you how you prefer to make PR :)

@mvaled
Copy link
Contributor Author

mvaled commented Jul 30, 2020

@kngwyu

I think the implementation would be straightforward as @davidhewitt commented, but please give us any question you have

I'm reading the comment very carefully. I want to understand every bit of it. As soon as I have questions you'll know.

@mvaled mvaled force-pushed the 1064-teach-pyproto-to-be-nicer branch 10 times, most recently from 541ad38 to 52210e0 Compare August 1, 2020 13:25
@mvaled
Copy link
Contributor Author

mvaled commented Aug 1, 2020

@kngwyu @davidhewitt

I've now made the first bit of progress.

  1. The tests

The tests are indirect because they don't actually test the pyclass returns NotImplemented. Instead they rely heavily on Python reversing the operations (BTW, I didn't know Python would try first the reversed operator of other when it is a subclass of type(self). Cool!)

So, the setup is always the same: a Python implementation of Other with all the reversed dunder methods. The idea is that when the Rust pyclass returns NotImplemented, CPython will find the reversed method on Other and succeed. This would work even for in-place assignment operators, which fallback to assign the result of the reversed operation.

There's a second part of each test when Other doesn't implement any of the reversed operations, and they should fail with a TypeError.

For the time being, I'm focusing on richcmp. The rest of the protocols are laid out in the tests but not being run yet.

  1. Further tests?

The previous tests are simply minded. They only test the cases when the pyclass don't have an implementation of the protocol or when extracting the arguments fails.

I'm not really sure if we need to test what happens when the pyclass does provide an implementation. Whether this code returns NotImplemented or fails, is up to the author. Right?

  1. Implementing rich comparison first.

Following the 3rd point in David's comment I came up with commit 52210e0.

I had to comment the case test expecting a TypeError when Other doesn't implement the reversed method. I think this is one of the things I need to find in the macros.

I plan to make richcmp pass all the tests this weekend. Do you any more pointers?

@mvaled
Copy link
Contributor Author

mvaled commented Aug 1, 2020

I had to comment the case test expecting a TypeError when Other doesn't implement the reversed method. I think this is one of the things I need to find in the macros.

It wasn't the macro. It was an error in the Python code of the error. Fixed.

@mvaled mvaled requested a review from kngwyu August 1, 2020 17:04
@mvaled
Copy link
Contributor Author

mvaled commented Aug 1, 2020

I managed to also change the implementation of py_binary_num_func.

The next step would be py_binary_self_func, but there the extract of args is down the rabbit hole ;) I'm following the source code path...

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! If py_binary_self_func is too complicated to change in it's current form, we can discuss thinking about refactors that would make this easier?

  1. Further tests?

I think the tests you've written for this should be sufficient, nice work 👍

Cargo.toml Outdated Show resolved Hide resolved
@mvaled
Copy link
Contributor Author

mvaled commented Aug 2, 2020

If py_binary_self_func is too complicated to change in it's current form, we can discuss thinking about refactors that would make this easier?

My first attempt at it was disastrous 🤪. A change in _call_impl so that we could return NotImplemented:

diff --git a/src/class/macros.rs b/src/class/macros.rs
index e3a0a3401..094466a1e 100644
--- a/src/class/macros.rs
+++ b/src/class/macros.rs
@@ -141,7 +141,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_proto_mut!(py, slf_, $f, arg).convert(py)?;
                 ffi::Py_INCREF(slf);
                 Ok(slf)
             })
@@ -385,6 +385,16 @@ macro_rules! _call_impl {
     ($slf: expr, $fn: ident, $raw_arg: expr $(,$raw_args: expr)* $(; $args: expr)*) => {
         _call_impl!($slf, $fn $(,$raw_args)* $(;$args)* ;$raw_arg.extract()?)
     };
+    (proto $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(...)`
@@ -400,3 +410,9 @@ macro_rules! call_mut {
         _call_impl!($slf.try_borrow_mut()?, $fn $(,$raw_args)* $(;$args)*)
     };
 }
+
+macro_rules! call_proto_mut {
+    ($py:ident, $slf: expr, $fn: ident $(,$raw_args: expr)* $(; $args: expr)*) => {
+        _call_impl!(proto $py, $slf.try_borrow_mut()?, $fn $(,$raw_args)* $(;$args)*)
+    };
+}

You can imagine how that went. Pages of errors...

tests/test_arithmetics.rs Show resolved Hide resolved
tests/test_arithmetics.rs Outdated Show resolved Hide resolved
@kngwyu
Copy link
Member

kngwyu commented Aug 2, 2020

The next step would be py_binary_self_func, but there the extract of args is down the rabbit hole ;) I'm following the source code path...

We don't have to make the situation perfect in a PR. Let's do that in the next one.

no-return-notimplemented = []

So... when do we want not to return NotImplemented error?
I don't think we have to implement it.

@mvaled
Copy link
Contributor Author

mvaled commented Aug 2, 2020

We don't have to make the situation perfect in a PR. Let's do that in the next one.

Ok. Cool. It buys a little bit of time.

Nevertheless, let me solve one remaining puzzle in this PR: I think the tests are not catching one error in this PR. I didn't changed py_ternary_num_func which is backing the __pow__; and still the tests are passing. I fail to see why.

@mvaled mvaled requested a review from kngwyu August 2, 2020 12:19
@mvaled
Copy link
Contributor Author

mvaled commented Aug 2, 2020

Definitively. There's something wrong in the tests as they stand now. I added a trivial one focusing on ** and it fails -- rightfully so.

    #[test]
    fn mistery_power() {
        let gil = Python::acquire_gil();
        let py = gil.python();
        let c2 = PyCell::new(py, RichComparisonToSelf {}).unwrap();
        py_run!(
            py,
            c2,
            "\
class Other:
    def __rpow__(self, other):
        return other

assert c2 ** Other() == c2"
        );
    }

@mvaled
Copy link
Contributor Author

mvaled commented Aug 2, 2020

Ah! The mistery_power was failing for another reason, I'm always returning None in __richcmp__; chaging to assert c2 ** Other() is c2 makes it pass.

And I noticed I didn't implemented __pow__ for RichComparisonToSelf.

@mvaled mvaled force-pushed the 1064-teach-pyproto-to-be-nicer branch from 782e452 to 337aba8 Compare August 3, 2020 12:05
@mvaled
Copy link
Contributor Author

mvaled commented Aug 3, 2020

I've just noticed that the guide promised to return NotImplemented for __richcmp__:

If `other` is not of the type specified in the signature, the generated code will automatically `return NotImplemented`.

Hum... was that implemented previously and got lost?

@kngwyu
Copy link
Member

kngwyu commented Aug 3, 2020

I'm sorry but please wait for one or two days until I find the time to review the ipow problem in detail.

@mvaled
Copy link
Contributor Author

mvaled commented Aug 3, 2020

I'm sorry but please wait for one or two days until I find the time to review the ipow problem in detail.

Ok. I'm completing the guide, which didn't have a section on PyNumberProtocol.

@mvaled
Copy link
Contributor Author

mvaled commented Aug 3, 2020

@kngwyu

I noticed that PyNumber_Add and other operators in CPython expect that reflected operators also return NotImplemented and we didn't do that in this PR.

Should we include such change in this PR?

@mvaled
Copy link
Contributor Author

mvaled commented Aug 3, 2020

I'm a little bit puzzled about how Python tells apart __add__ and __radd__, I see that both are assigned to the nb_add slot. Yet they are both there:

>>> class X: 
...     def __add__(self, other): 
...         return self 
...     def __radd__(self, other): 
...         return other 
...     

>>> X() + 1                                                                                                                                                                                                        
<__main__.X at 0x7fd308820df0>

>>> 1 + X()                                                                                                                                                                                                        
1

🤔

@mvaled
Copy link
Contributor Author

mvaled commented Aug 3, 2020

@kngwyu

I confirmed that __ipow__ doesn't fallback to the reversed operator in Python itself:

>>> class Foo: 
...     def __ipow__(self, other): 
...         return NotImplemented 
...                                                                                                                                                                                                                

>>> class X: 
...     def __rpow__(self, other): 
...         return other 
...                                                                                                                                                                                                                

>>> f = Foo()                                                                                                                                                                                                      
>>> f **= X()                                                                                                                                                                                                      
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-4-16ca508b06d5> in <module>
----> 1 f **= X()

TypeError: unsupported operand type(s) for ** or pow(): 'Foo' and 'X'

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.
@mvaled mvaled force-pushed the 1064-teach-pyproto-to-be-nicer branch from 45460fe to f830b7a Compare August 3, 2020 20:03
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks @mvaled, this PR is now looking excellent to me! Is there anything else you're planning to do with it?

I'm a little bit puzzled about how Python tells apart __add__ and __radd__, I see that both are assigned to the nb_add slot. Yet they are both there:

Yeah, at the moment we actually ensure that only one of them is assigned to the slot: (__add__ if possible, else __radd__):

SlotSetter {
proto_names: &["__add__"],
set_function: "set_add",
skipped_setters: &["set_radd"],
},
SlotSetter::new(&["__radd__"], "set_radd"),

This is actually a bug: #844. There's a little bit of discussion there already. If it interests you, I'd be very happy to mentor you through a solution to that ticket also? (I was planning to look at it myself soon, but so many pieces I'm busy with I'm always happy to share the work!)

@mvaled
Copy link
Contributor Author

mvaled commented Aug 4, 2020

Is there anything else you're planning to do with it?

I was thinking we should return NotImplemented in reversed operators as well. While studying the code of py_binary_reversed_num_func I noticed the nb_ thing, and took a look a Python's code of PyNumber_Add, which basically relies on binary_op1(v, w, NB_SLOT(nb_add)) (https://github.com/python/cpython/blob/582aaf19e8b94a70c1f96792197770d604ba0fdf/Objects/abstract.c#L945)

But, then, I don't see how CPython itself chooses the __radd__ of my programs 🤯

This is actually a bug: #844. There's a little bit of discussion there already. If it interests you, I'd be very happy to mentor you through a solution to that ticket also? (I was planning to look at it myself soon, but so many pieces I'm busy with I'm always happy to share the work!)

I will look at it. Maybe there's already an explanation there. If think I'm up to the challenge I'll let you know.

I rather let this PR as it is now and work the __r*__ in another one.

@davidhewitt
Copy link
Member

I rather let this PR as it is now and work the r* in another one.

Great! I'm satisfied with this PR, let's just wait for @kngwyu to confirm.

But, then, I don't see how CPython itself chooses the radd of my programs 🤯

That's pretty much what I'm trying to get to in #844 - CPython doesn't just set __add__ or __radd__ directly to the nb_add slot, but instead uses a macro to generate a special wrapper function which does different things depending on which of these are present:

https://github.com/python/cpython/blob/05e4a296ecc127641160a04f39cc02c0f66a8c27/Objects/typeobject.c#L6358

guide/src/class.md Outdated Show resolved Hide resolved
guide/src/class.md Outdated Show resolved Hide resolved
guide/src/class.md Outdated Show resolved Hide resolved
@kngwyu
Copy link
Member

kngwyu commented Aug 5, 2020

Thank you!
I totally forgot about #844 and, yeah, I agree that we should address it first (and maybe I can do so this weekend).

mvaled and others added 2 commits August 5, 2020 07:49
@mvaled
Copy link
Contributor Author

mvaled commented Aug 5, 2020

@kngwyu @davidhewitt

Thank you for all your guidance and support.

I've committed the suggestions and also added a note to #844. I was looking at it to see if I could tackle it, but it would take some time before I could go back to it and study the CPython code (some deadlines here to meet). I'll let you know.

@kngwyu kngwyu merged commit f2ba3e6 into PyO3:master Aug 5, 2020
@kngwyu
Copy link
Member

kngwyu commented Aug 5, 2020

Thank you for all your work!

I've committed the suggestions and also added a note to #844. I was looking at it to see if I could tackle it, but it would take some time before I could go back to it and study the CPython code (some deadlines here to meet). I'll let you know.

Since our proc-macro implementation for __r*__ methods is a bit tricky, I'm not sure CPython code helps you.
If you want to tackle it, please let me know that so I can explain the detail. Or I'll implement it next Saturday.

@mvaled
Copy link
Contributor Author

mvaled commented Aug 5, 2020

Since our proc-macro implementation for r* methods is a bit tricky, I'm not sure CPython code helps you. If you want to tackle it, please let me know that so I can explain the detail. Or I'll implement it next Saturday.

Ok. I think I'll pass on this, but I'll be looking at your work to learn a bit more.

I'm gonna review the list of issues remaining for 0.12 to see if there's anything I could do.

@mvaled mvaled deleted the 1064-teach-pyproto-to-be-nicer branch August 5, 2020 19:55
@davidhewitt
Copy link
Member

❤️ as well as 0.12 milestone you're also free to investigate anything else which strikes your interest. You might be interested in the new contributing notes I've written which I've meant to do for a while: #1086

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comparisons with __eq__ should not raise TypeError
3 participants