Add object.{lt, le, gt, gt} #424

janczer · 2019-02-09T21:29:21Z

No description provided.

windelbouwman · 2019-02-10T11:18:11Z

vm/src/obj/objobject.rs

+
+    Err(vm.new_type_error(format!(
+        "'>' not supported between instances of {} and {}",
+        i.borrow(),


Instead of using the Display trait, go for vm.py_str to properly call the __str__ or __repr__ of the object.

windelbouwman · 2019-02-10T11:19:09Z

Is this code required? If the __lt__ attribute is not present, < will not work anyway right?

janczer · 2019-02-10T11:38:47Z

Yes, < will not work but, if I run RustPython I get different error from the python:

RustPython:

>>>>> a = object()
>>>>> a < 1
Traceback (most recent call last):
  File <stdin>, line 0, in <module>
TypeError: Unsupported method: __lt__

Python3.7.2:

>>> a = object()
>>> a < 1
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: '<' not supported between instances of 'object' and 'int'

Looks like error from RustPython not so clear. @windelbouwman what do you think?

windelbouwman · 2019-02-10T11:49:19Z

I am unsure what is the proper location to implement this. We might also choose to update the existing error message in Rustpython. We already raise the correct exception type, so the message should be expanded, right? I would look in the vm.rs file for the existing error and add proper type name information like in cpython.

janczer · 2019-02-10T14:58:40Z

We already raise the correct exception type, so the message should be expanded, right?

Yes.

I would look in the vm.rs file for the existing error and add proper type name information like in cpython.

The unsupported method error thorws on 251 line in vm.rs. Can you please tell me how I can expand the error?

windelbouwman · 2019-02-10T15:12:15Z

The proper error message appears already to be implemented here: https://github.com/RustPython/RustPython/blob/master/vm/src/vm.rs#L554

The correct function to take a look at is call_or_unsupported. This function will first try the normal method (__add__), then the reverse method (__radd__ for example).

Take a look at the _lt method (https://github.com/RustPython/RustPython/blob/master/vm/src/vm.rs#L604). Maybe we could call here call_or_unsupported as well?

janczer · 2019-02-11T12:55:49Z

Looks like some work done in #433. Maybe I can add erros the same way?

OddCoincidence · 2019-02-11T17:34:20Z

Looks like some work done in #433. Maybe I can add erros the same way?

Yep! The objobject comparison methods should return NotImplemented:

class MyClass:
    pass

assert MyClass().__le__(MyClass()) == NotImplemented
assert MyClass().__lt__(MyClass()) == NotImplemented
assert MyClass().__ge__(MyClass()) == NotImplemented
assert MyClass().__gt__(MyClass()) == NotImplemented

And VirtualMachine::{_le, _lt, _ge, _gt} should use call_or_unsupported. This page documents the appropriate fallbacks to use.

janczer · 2019-02-12T14:36:31Z

@OddCoincidence I added the methods to object but when I run tests/snippets/builtin_max.py got error:

thread 'main' panicked at 'Inner error getting inner boolean', vm/src/obj/objbool.rs:60:9

What I'm doing wrong? Can you help me please

cthulahoops · 2019-02-12T15:19:31Z

Sounds like the same type of bug I fixed in: #441

builtin_max should use vm._gt rather than calling __gt__ itself.

janczer · 2019-02-12T16:57:16Z

@cthulahoops You are right! Thank you! Should we change all calling __gt__ to vm._gt and another simular methods?

windelbouwman · 2019-02-12T16:59:17Z

Thats probably better. The magic methods should be used sparingly, preferably in wrapper methods.

OddCoincidence · 2019-02-12T17:08:11Z

vm/src/vm.rs

-    pub fn _lt(&mut self, a: &PyObjectRef, b: PyObjectRef) -> PyResult {
-        self.call_method(a, "__lt__", vec![b])
+    pub fn _lt(&mut self, a: PyObjectRef, b: PyObjectRef) -> PyResult {
+        self.call_or_unsupported(a, b, "__lt__", "__lt__", |vm, a, b| {


The reverse methods for these shouldn't be the same, i.e. they're not commutative like __eq__ and __ne__; a > b != b > a. Instead, as described here:

lt() and gt() are each other’s reflection, le() and ge() are each other’s reflection

OddCoincidence · 2019-02-12T17:27:28Z

vm/src/obj/objfloat.rs

            .new_bool(v1 < objint::get_value(i2).to_f64().unwrap()))
    } else {
-        Err(vm.new_type_error(format!("Cannot compare {} and {}", i.borrow(), i2.borrow())))
+        Ok(vm.ctx.not_implemented())


I think float_{gt,ge,le} need this change as well

Done. Thanks

windelbouwman · 2019-02-12T17:49:00Z

vm/src/vm.rs

-    pub fn _lt(&mut self, a: &PyObjectRef, b: PyObjectRef) -> PyResult {
-        self.call_method(a, "__lt__", vec![b])
+    pub fn _lt(&mut self, a: PyObjectRef, b: PyObjectRef) -> PyResult {
+        self.call_or_unsupported(a, b, "__lt__", "__gt__", |vm, a, b| {


The opposite of < is >=, right? I would expect __lt__ to fail over to __ge__.

No, because call_or_unsupported flips the operands. If you think about it, a > b and b < a are equivalent. a > b and b <= a are not.

Also from the python docs:

lt() and gt() are each other’s reflection, le() and ge() are each other’s reflection

Oh, yes, you are right! Sorry for that!

windelbouwman reviewed Feb 10, 2019

View reviewed changes

Add object.{__lt__, __le__, __gt__, __gt__}

0887b55

janczer force-pushed the add_object_lt_le_gt_ge branch from bfb122e to 0887b55 Compare February 12, 2019 14:32

Fix the call __gt__

1ff1bcd

OddCoincidence reviewed Feb 12, 2019

View reviewed changes

Fix the reflections of methods lt, le, gt, ge

9601673

OddCoincidence reviewed Feb 12, 2019

View reviewed changes

Add not_implemented to float.{__le__, __gt__, __ge__}

9277e67

windelbouwman reviewed Feb 12, 2019

View reviewed changes

windelbouwman merged commit e5af4ca into RustPython:master Feb 12, 2019

janczer deleted the add_object_lt_le_gt_ge branch February 12, 2019 18:24

Add object.{__lt__, __le__, __gt__, __gt__} #424

Add object.{__lt__, __le__, __gt__, __gt__} #424

Uh oh!

Conversation

janczer commented Feb 9, 2019

Uh oh!

windelbouwman Feb 10, 2019

Choose a reason for hiding this comment

Uh oh!

windelbouwman commented Feb 10, 2019

Uh oh!

janczer commented Feb 10, 2019

Uh oh!

windelbouwman commented Feb 10, 2019

Uh oh!

janczer commented Feb 10, 2019

Uh oh!

windelbouwman commented Feb 10, 2019

Uh oh!

janczer commented Feb 11, 2019

Uh oh!

OddCoincidence commented Feb 11, 2019

Uh oh!

janczer commented Feb 12, 2019

Uh oh!

cthulahoops commented Feb 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janczer commented Feb 12, 2019

Uh oh!

windelbouwman commented Feb 12, 2019

Uh oh!

OddCoincidence Feb 12, 2019

Choose a reason for hiding this comment

Uh oh!

janczer Feb 12, 2019

Choose a reason for hiding this comment

Uh oh!

OddCoincidence Feb 12, 2019

Choose a reason for hiding this comment

Uh oh!

janczer Feb 12, 2019

Choose a reason for hiding this comment

Uh oh!

windelbouwman Feb 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OddCoincidence Feb 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

windelbouwman Feb 12, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add object.{lt, le, gt, gt} #424

Add object.{lt, le, gt, gt} #424

cthulahoops commented Feb 12, 2019 •

edited

Loading

windelbouwman Feb 12, 2019 •

edited

Loading

OddCoincidence Feb 12, 2019 •

edited

Loading