From 8133aaa5d8b4a5078f62da486625a790368e7609 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sun, 10 Dec 2023 16:55:13 +0100 Subject: [PATCH 1/4] Try harder by looking for a __bool__ magic method when extracing bool values from Python objects. I decided to not implement the full protocol for truth value testing [1] as it seems confusing in the context of function arguments if basically any instance of custom class or non-empty collection turns into `true`. [1] https://docs.python.org/3/library/stdtypes.html#truth --- newsfragments/3638.changed.md | 1 + src/types/any.rs | 1 - src/types/boolobject.rs | 59 +++++++++++++++++++++++++++++++++-- 3 files changed, 57 insertions(+), 4 deletions(-) create mode 100644 newsfragments/3638.changed.md diff --git a/newsfragments/3638.changed.md b/newsfragments/3638.changed.md new file mode 100644 index 00000000..83f0bd74 --- /dev/null +++ b/newsfragments/3638.changed.md @@ -0,0 +1 @@ +Values of type `bool` can now be extracted from all Python values defining a `__bool__` magic method. diff --git a/src/types/any.rs b/src/types/any.rs index f6c5ef14..8d42e9f2 100644 --- a/src/types/any.rs +++ b/src/types/any.rs @@ -146,7 +146,6 @@ impl PyAny { /// /// To avoid repeated temporary allocations of Python strings, the [`intern!`] macro can be used /// to intern `attr_name`. - #[allow(dead_code)] // Currently only used with num-complex+abi3, so dead without that. pub(crate) fn lookup_special(&self, attr_name: N) -> PyResult> where N: IntoPy>, diff --git a/src/types/boolobject.rs b/src/types/boolobject.rs index 7e75c424..7ca2d4d3 100644 --- a/src/types/boolobject.rs +++ b/src/types/boolobject.rs @@ -1,7 +1,8 @@ #[cfg(feature = "experimental-inspect")] use crate::inspect::types::TypeInfo; use crate::{ - ffi, instance::Py2, FromPyObject, IntoPy, PyAny, PyObject, PyResult, Python, ToPyObject, + exceptions::PyTypeError, ffi, instance::Py2, FromPyObject, IntoPy, PyAny, PyObject, PyResult, + Python, ToPyObject, }; /// Represents a Python `bool`. @@ -76,7 +77,16 @@ impl IntoPy for bool { /// Fails with `TypeError` if the input is not a Python `bool`. impl<'source> FromPyObject<'source> for bool { fn extract(obj: &'source PyAny) -> PyResult { - Ok(obj.downcast::()?.is_true()) + if let Ok(obj) = obj.downcast::() { + return Ok(obj.is_true()); + } + + let meth = obj + .lookup_special(intern!(obj.py(), "__bool__"))? + .ok_or_else(|| PyTypeError::new_err("object has no __bool__ magic method"))?; + + let obj = meth.call0()?.downcast::()?; + Ok(obj.is_true()) } #[cfg(feature = "experimental-inspect")] @@ -87,7 +97,7 @@ impl<'source> FromPyObject<'source> for bool { #[cfg(test)] mod tests { - use crate::types::{PyAny, PyBool}; + use crate::types::{PyAny, PyBool, PyModule}; use crate::Python; use crate::ToPyObject; @@ -110,4 +120,47 @@ mod tests { assert!(false.to_object(py).is(PyBool::new(py, false))); }); } + + #[test] + fn test_magic_method() { + Python::with_gil(|py| { + let module = PyModule::from_code( + py, + r#" +class A: + def __bool__(self): return True +class B: + def __bool__(self): return "not a bool" +class C: + def __len__(self): return 23 +class D: + pass + "#, + "test.py", + "test", + ) + .unwrap(); + + let a = module.getattr("A").unwrap().call0().unwrap(); + assert!(a.extract::().unwrap()); + + let b = module.getattr("B").unwrap().call0().unwrap(); + assert_eq!( + b.extract::().unwrap_err().to_string(), + "TypeError: 'str' object cannot be converted to 'PyBool'", + ); + + let c = module.getattr("C").unwrap().call0().unwrap(); + assert_eq!( + c.extract::().unwrap_err().to_string(), + "TypeError: object has no __bool__ magic method", + ); + + let d = module.getattr("D").unwrap().call0().unwrap(); + assert_eq!( + d.extract::().unwrap_err().to_string(), + "TypeError: object has no __bool__ magic method", + ); + }); + } } From 3e10d64fa26c12244afd1bc4e3fc32df82230bb3 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sun, 10 Dec 2023 20:21:11 +0100 Subject: [PATCH 2/4] Avoid attribute lookup overhead for __bool__ if the unlimited API is available. --- src/types/any.rs | 1 + src/types/boolobject.rs | 39 ++++++++++++++++++++++++++++++--------- 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/src/types/any.rs b/src/types/any.rs index 8d42e9f2..f6c5ef14 100644 --- a/src/types/any.rs +++ b/src/types/any.rs @@ -146,6 +146,7 @@ impl PyAny { /// /// To avoid repeated temporary allocations of Python strings, the [`intern!`] macro can be used /// to intern `attr_name`. + #[allow(dead_code)] // Currently only used with num-complex+abi3, so dead without that. pub(crate) fn lookup_special(&self, attr_name: N) -> PyResult> where N: IntoPy>, diff --git a/src/types/boolobject.rs b/src/types/boolobject.rs index 7ca2d4d3..fdedbfea 100644 --- a/src/types/boolobject.rs +++ b/src/types/boolobject.rs @@ -81,12 +81,32 @@ impl<'source> FromPyObject<'source> for bool { return Ok(obj.is_true()); } - let meth = obj - .lookup_special(intern!(obj.py(), "__bool__"))? - .ok_or_else(|| PyTypeError::new_err("object has no __bool__ magic method"))?; + #[cfg(not(any(Py_LIMITED_API, PyPy)))] + unsafe { + let ptr = obj.as_ptr(); - let obj = meth.call0()?.downcast::()?; - Ok(obj.is_true()) + if let Some(tp_as_number) = (*(*ptr).ob_type).tp_as_number.as_ref() { + if let Some(nb_bool) = tp_as_number.nb_bool { + match (nb_bool)(ptr) { + 0 => return Ok(false), + 1 => return Ok(true), + _ => return Err(crate::PyErr::fetch(obj.py())), + } + } + } + + Err(PyTypeError::new_err("object has no __bool__ magic method")) + } + + #[cfg(any(Py_LIMITED_API, PyPy))] + { + let meth = obj + .lookup_special(crate::intern!(obj.py(), "__bool__"))? + .ok_or_else(|| PyTypeError::new_err("object has no __bool__ magic method"))?; + + let obj = meth.call0()?.downcast::()?; + Ok(obj.is_true()) + } } #[cfg(feature = "experimental-inspect")] @@ -145,10 +165,11 @@ class D: assert!(a.extract::().unwrap()); let b = module.getattr("B").unwrap().call0().unwrap(); - assert_eq!( - b.extract::().unwrap_err().to_string(), - "TypeError: 'str' object cannot be converted to 'PyBool'", - ); + assert!(matches!( + &*b.extract::().unwrap_err().to_string(), + "TypeError: 'str' object cannot be converted to 'PyBool'" + | "TypeError: __bool__ should return bool, returned str" + )); let c = module.getattr("C").unwrap().call0().unwrap(); assert_eq!( From 57002d2389d8456a556c1e7ba9bd9f2e1c7daa53 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sat, 16 Dec 2023 09:35:18 +0100 Subject: [PATCH 3/4] Align error message when no method __bool__ is defined with CPython's general style. --- src/types/boolobject.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/types/boolobject.rs b/src/types/boolobject.rs index fdedbfea..8609872b 100644 --- a/src/types/boolobject.rs +++ b/src/types/boolobject.rs @@ -81,6 +81,13 @@ impl<'source> FromPyObject<'source> for bool { return Ok(obj.is_true()); } + let missing_conversion = |obj: &PyAny| { + PyTypeError::new_err(format!( + "object of type '{}' does not define a '__bool__' conversion", + obj.get_type() + )) + }; + #[cfg(not(any(Py_LIMITED_API, PyPy)))] unsafe { let ptr = obj.as_ptr(); @@ -95,14 +102,14 @@ impl<'source> FromPyObject<'source> for bool { } } - Err(PyTypeError::new_err("object has no __bool__ magic method")) + Err(missing_conversion(obj)) } #[cfg(any(Py_LIMITED_API, PyPy))] { let meth = obj .lookup_special(crate::intern!(obj.py(), "__bool__"))? - .ok_or_else(|| PyTypeError::new_err("object has no __bool__ magic method"))?; + .ok_or_else(|| missing_conversion(obj))?; let obj = meth.call0()?.downcast::()?; Ok(obj.is_true()) @@ -174,13 +181,13 @@ class D: let c = module.getattr("C").unwrap().call0().unwrap(); assert_eq!( c.extract::().unwrap_err().to_string(), - "TypeError: object has no __bool__ magic method", + "TypeError: object of type '' does not define a '__bool__' conversion", ); let d = module.getattr("D").unwrap().call0().unwrap(); assert_eq!( d.extract::().unwrap_err().to_string(), - "TypeError: object has no __bool__ magic method", + "TypeError: object of type '' does not define a '__bool__' conversion", ); }); } From 4177dfcc81bd5052c3d4c01088e9da00b2e5f32e Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sun, 17 Dec 2023 14:45:05 +0100 Subject: [PATCH 4/4] Apply __bool__ conversion only to numpy.bool_ to avoid false positives. --- newsfragments/3638.changed.md | 2 +- pytests/src/misc.rs | 6 ++ pytests/tests/test_misc.py | 10 +++ src/types/boolobject.rs | 111 ++++++++++++---------------------- 4 files changed, 55 insertions(+), 74 deletions(-) diff --git a/newsfragments/3638.changed.md b/newsfragments/3638.changed.md index 83f0bd74..6bdafde8 100644 --- a/newsfragments/3638.changed.md +++ b/newsfragments/3638.changed.md @@ -1 +1 @@ -Values of type `bool` can now be extracted from all Python values defining a `__bool__` magic method. +Values of type `bool` can now be extracted from NumPy's `bool_`. diff --git a/pytests/src/misc.rs b/pytests/src/misc.rs index 69f3b75e..029e8b16 100644 --- a/pytests/src/misc.rs +++ b/pytests/src/misc.rs @@ -12,9 +12,15 @@ fn get_type_full_name(obj: &PyAny) -> PyResult> { obj.get_type().name() } +#[pyfunction] +fn accepts_bool(val: bool) -> bool { + val +} + #[pymodule] pub fn misc(_py: Python<'_>, m: &PyModule) -> PyResult<()> { m.add_function(wrap_pyfunction!(issue_219, m)?)?; m.add_function(wrap_pyfunction!(get_type_full_name, m)?)?; + m.add_function(wrap_pyfunction!(accepts_bool, m)?)?; Ok(()) } diff --git a/pytests/tests/test_misc.py b/pytests/tests/test_misc.py index 537ee119..06b2ce73 100644 --- a/pytests/tests/test_misc.py +++ b/pytests/tests/test_misc.py @@ -54,3 +54,13 @@ def test_type_full_name_includes_module(): numpy = pytest.importorskip("numpy") assert pyo3_pytests.misc.get_type_full_name(numpy.bool_(True)) == "numpy.bool_" + + +def test_accepts_numpy_bool(): + # binary numpy wheel not available on all platforms + numpy = pytest.importorskip("numpy") + + assert pyo3_pytests.misc.accepts_bool(True) is True + assert pyo3_pytests.misc.accepts_bool(False) is False + assert pyo3_pytests.misc.accepts_bool(numpy.bool_(True)) is True + assert pyo3_pytests.misc.accepts_bool(numpy.bool_(False)) is False diff --git a/src/types/boolobject.rs b/src/types/boolobject.rs index 8609872b..71c91c8e 100644 --- a/src/types/boolobject.rs +++ b/src/types/boolobject.rs @@ -77,43 +77,52 @@ impl IntoPy for bool { /// Fails with `TypeError` if the input is not a Python `bool`. impl<'source> FromPyObject<'source> for bool { fn extract(obj: &'source PyAny) -> PyResult { - if let Ok(obj) = obj.downcast::() { - return Ok(obj.is_true()); - } - - let missing_conversion = |obj: &PyAny| { - PyTypeError::new_err(format!( - "object of type '{}' does not define a '__bool__' conversion", - obj.get_type() - )) + let err = match obj.downcast::() { + Ok(obj) => return Ok(obj.is_true()), + Err(err) => err, }; - #[cfg(not(any(Py_LIMITED_API, PyPy)))] - unsafe { - let ptr = obj.as_ptr(); + if obj + .get_type() + .name() + .map_or(false, |name| name == "numpy.bool_") + { + let missing_conversion = |obj: &PyAny| { + PyTypeError::new_err(format!( + "object of type '{}' does not define a '__bool__' conversion", + obj.get_type() + )) + }; - if let Some(tp_as_number) = (*(*ptr).ob_type).tp_as_number.as_ref() { - if let Some(nb_bool) = tp_as_number.nb_bool { - match (nb_bool)(ptr) { - 0 => return Ok(false), - 1 => return Ok(true), - _ => return Err(crate::PyErr::fetch(obj.py())), + #[cfg(not(any(Py_LIMITED_API, PyPy)))] + unsafe { + let ptr = obj.as_ptr(); + + if let Some(tp_as_number) = (*(*ptr).ob_type).tp_as_number.as_ref() { + if let Some(nb_bool) = tp_as_number.nb_bool { + match (nb_bool)(ptr) { + 0 => return Ok(false), + 1 => return Ok(true), + _ => return Err(crate::PyErr::fetch(obj.py())), + } } } + + return Err(missing_conversion(obj)); } - Err(missing_conversion(obj)) + #[cfg(any(Py_LIMITED_API, PyPy))] + { + let meth = obj + .lookup_special(crate::intern!(obj.py(), "__bool__"))? + .ok_or_else(|| missing_conversion(obj))?; + + let obj = meth.call0()?.downcast::()?; + return Ok(obj.is_true()); + } } - #[cfg(any(Py_LIMITED_API, PyPy))] - { - let meth = obj - .lookup_special(crate::intern!(obj.py(), "__bool__"))? - .ok_or_else(|| missing_conversion(obj))?; - - let obj = meth.call0()?.downcast::()?; - Ok(obj.is_true()) - } + Err(err.into()) } #[cfg(feature = "experimental-inspect")] @@ -124,7 +133,7 @@ impl<'source> FromPyObject<'source> for bool { #[cfg(test)] mod tests { - use crate::types::{PyAny, PyBool, PyModule}; + use crate::types::{PyAny, PyBool}; use crate::Python; use crate::ToPyObject; @@ -147,48 +156,4 @@ mod tests { assert!(false.to_object(py).is(PyBool::new(py, false))); }); } - - #[test] - fn test_magic_method() { - Python::with_gil(|py| { - let module = PyModule::from_code( - py, - r#" -class A: - def __bool__(self): return True -class B: - def __bool__(self): return "not a bool" -class C: - def __len__(self): return 23 -class D: - pass - "#, - "test.py", - "test", - ) - .unwrap(); - - let a = module.getattr("A").unwrap().call0().unwrap(); - assert!(a.extract::().unwrap()); - - let b = module.getattr("B").unwrap().call0().unwrap(); - assert!(matches!( - &*b.extract::().unwrap_err().to_string(), - "TypeError: 'str' object cannot be converted to 'PyBool'" - | "TypeError: __bool__ should return bool, returned str" - )); - - let c = module.getattr("C").unwrap().call0().unwrap(); - assert_eq!( - c.extract::().unwrap_err().to_string(), - "TypeError: object of type '' does not define a '__bool__' conversion", - ); - - let d = module.getattr("D").unwrap().call0().unwrap(); - assert_eq!( - d.extract::().unwrap_err().to_string(), - "TypeError: object of type '' does not define a '__bool__' conversion", - ); - }); - } }