Skip to content

Implement Number Protocol for PyBool #4639

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Implement Number Protocol for PyBool #4639

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 52 additions & 2 deletions vm/src/builtins/bool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,19 @@
use super::{PyInt, PyStrRef, PyType, PyTypeRef};
use crate::{
class::PyClassImpl, convert::ToPyObject, function::OptionalArg, identifier, types::Constructor,
atomic_func,
class::PyClassImpl,
convert::{ToPyObject, ToPyResult},
function::OptionalArg,
identifier,
protocol::PyNumberMethods,
types::{AsNumber, Constructor},
AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyResult, TryFromBorrowedObject,
VirtualMachine,
};
use crossbeam_utils::atomic::AtomicCell;
use num_bigint::Sign;
use num_traits::Zero;
use once_cell::sync::Lazy;
use std::fmt::{Debug, Formatter};

impl ToPyObject for bool {
Expand Down Expand Up @@ -102,7 +110,7 @@ impl Constructor for PyBool {
}
}

#[pyclass(with(Constructor))]
#[pyclass(with(Constructor, AsNumber))]
impl PyBool {
#[pymethod(magic)]
fn repr(zelf: bool, vm: &VirtualMachine) -> PyStrRef {
Expand Down Expand Up @@ -166,6 +174,48 @@ impl PyBool {
}
}

macro_rules! int_method {
($method:ident) => {
AtomicCell::new(PyInt::as_number().$method.load().to_owned())
};
}

impl AsNumber for PyBool {
fn as_number() -> &'static PyNumberMethods {
static AS_NUMBER: Lazy<PyNumberMethods> = Lazy::new(|| PyNumberMethods {
add: int_method!(add),
subtract: int_method!(subtract),
multiply: int_method!(multiply),
remainder: int_method!(remainder),
divmod: int_method!(divmod),
power: int_method!(power),
negative: int_method!(negative),
positive: int_method!(positive),
absolute: int_method!(absolute),
boolean: int_method!(boolean),
invert: int_method!(invert),
lshift: int_method!(lshift),
rshift: int_method!(rshift),
Comment on lines +186 to +198
Copy link
Member

Choose a reason for hiding this comment

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

Not sure yet, but I think those fields in bool have to be None.
In my opinion, If that's not working with None, any inherited object of numbers can make similar problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems int operations are needed for bool in CPython.

>>> (True + True) ** True
2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since PyBool is a subclass of PyInt and inherits its methods, so (True + True) ** True should somehow work without the above overriding in as_number I think.

Copy link
Member

Choose a reason for hiding this comment

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

I am sorry to make confusion. I agree bool need int operations.
But I'd like to say manually filling PyNumberMethods is not the way to do.
We can do that job only for builtin types written in Rust.
Making them None and finding a way to make it work will be helpful to solve the problem in general.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did not test but I think PyBool is working with int number methods via mro search in PyNumber::find_methods().

But Maybe we can override it in PyBool to have some optimize

Copy link
Member

Choose a reason for hiding this comment

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

No, it wasn't. Because that is a static variable. We have no way to edit it later.

Copy link
Member

Choose a reason for hiding this comment

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

@qingshi163 do you have any suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can inherit slots when initializing the subclass

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/python/cpython/blob/71db5dbcd714b2e1297c43538188dd69715feb9a/Objects/typeobject.c#L8849-L8902

slots will be inherit if subclass does not override it.

when there is a change of slots on the type, cpython recusively update slots for all the subclasses.

Copy link
Member

Choose a reason for hiding this comment

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

slots will be inherit if subclass does not override it.

It is true in cpython but not here yet. That's the reason we always call mro_find_map() to find slots.

and: atomic_func!(|number, other, vm| {
PyBool::and(number.obj.to_owned(), other.to_owned(), vm).to_pyresult(vm)
}),
xor: atomic_func!(|number, other, vm| {
PyBool::xor(number.obj.to_owned(), other.to_owned(), vm).to_pyresult(vm)
}),
or: atomic_func!(|number, other, vm| {
PyBool::or(number.obj.to_owned(), other.to_owned(), vm).to_pyresult(vm)
}),
Comment on lines +199 to +207
Copy link
Member

Choose a reason for hiding this comment

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

Here is more room to optimize because and, xor, or actually doesn't require to increase reference count.
But it looks working fine.

To fully optimize it, PyBool::and can be changed like this and AsNumber can call inner_and

    #[pymethod(name = "__rand__")]
    #[pymethod(magic)]
    fn and(lhs: PyObjectRef, rhs: PyObjectRef, vm: &VirtualMachine) -> PyObjectRef {
        Self::inner_and(&lhs, &rhs, vm)
    }
    fn inner_and(lhs: &PyObject, rhs: &PyObject, vm: &VirtualMachine) -> PyObjectRef {
        if lhs.fast_isinstance(vm.ctx.types.bool_type)
            && rhs.fast_isinstance(vm.ctx.types.bool_type)
        {
            let lhs = get_value(&lhs);
            let rhs = get_value(&rhs);
            (lhs && rhs).to_pyobject(vm)
        } else {
            get_py_int(&lhs).and(rhs, vm).to_pyobject(vm)
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This approach seems not working directly, since get_py_int(&lhs).and(rhs, vm) requires rhs to be an PyObjectRef as defined in int.rs.

Copy link
Member

Choose a reason for hiding this comment

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

maybe rhs.to_owned() fornow. we can optimize int.and also in same way later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure but would rhs.to_owned() be an extra burden? If it's ok (for now), I will adopt the above practice.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, rhs.to_owned() is extra cost, and still same as previous one. Adding TODO: or FIXME: can be helpful.
It is on the way to go to the right design.

int: int_method!(int),
float: int_method!(float),
floor_divide: int_method!(floor_divide),
true_divide: int_method!(true_divide),
index: int_method!(index),
..PyNumberMethods::NOT_IMPLEMENTED
});
&AS_NUMBER
}
}

pub(crate) fn init(context: &Context) {
PyBool::extend_class(context, context.types.bool_type);
}
Expand Down