diff --git a/CHANGELOG.md b/CHANGELOG.md index f561d137355..a95063b8140 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,7 +53,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `PyCapsule::name` now returns `PyResult>` instead of `&CStr`. - `FromPyObject::extract` now raises an error if source type is `PyString` and target type is `Vec`. [#2500](https://github.com/PyO3/pyo3/pull/2500) - Only allow each `#[pymodule]` to be initialized once. [#2523](https://github.com/PyO3/pyo3/pull/2523) -- `pyo3_build_config::add_extension_module_link_args()` now also emits linker arguments for `wasm32-unknown-emscripten`. [#2538](https://github.com/PyO3/pyo3/pull/2538) +- `pyo3_build_config::add_extension_module_link_args()` now also emits linker arguments for `wasm32-unknown-emscripten`. [#2500](https://github.com/PyO3/pyo3/pull/2500) +- Downcasting (`PyTryFrom`) behavior has changed for `PySequence` and `PyMapping`: classes are now required to inherit from (or register with) the corresponding Python standard library abstract base class. See the [migration guide](https://pyo3.rs/latest/migration.html) for information on fixing broken downcasts. [#2477](https://github.com/PyO3/pyo3/pull/2477) ### Removed diff --git a/guide/src/migration.md b/guide/src/migration.md index c65304718cc..a353642c56a 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -5,6 +5,58 @@ For a detailed list of all changes, see the [CHANGELOG](changelog.md). ## from 0.16.* to 0.17 +### Type checks have been changed for `PyMapping` and `PySequence` types + +Previously the type checks for `PyMapping` and `PySequence` (implemented in `PyTryFrom`) +used the Python C-API functions `PyMapping_Check` and `PySequence_Check`. +Unfortunately these functions are not sufficient for distinguishing such types, +leading to inconsistent behavior (see +[pyo3/pyo3#2072](https://github.com/PyO3/pyo3/issues/2072)). + +PyO3 0.17 changes these downcast checks to explicityly test if the type is a +subclass of the corresponding abstract base class `collections.abc.Mapping` or +`collections.abc.Sequence`. Note this requires calling into Python, which may +incur a performance penalty over the previous method. If this performance +penatly is a problem, you may be able to perform your own checks and use +`try_from_unchecked` (unsafe). + +Another side-effect is that a pyclass defined in Rust with PyO3 will need to +be _registered_ with the corresponding Python abstract base class for +downcasting to succeed. `PySequence::register` and `PyMapping:register` have +been added to make it easy to do this from Rust code. These are equivalent to +calling `collections.abc.Mapping.register(MappingPyClass)` or +`collections.abc.Sequence.register(SequencePyClass)` from Python. + +For example, for a mapping class defined in Rust: +```rust,compile_fail +use pyo3::prelude::*; +use std::collections::HashMap; + +#[pyclass(mapping)] +struct Mapping { + index: HashMap, +} + +#[pymethods] +impl Mapping { + #[new] + fn new(elements: Option<&PyList>) -> PyResult { + // ... + // truncated implementation of this mapping pyclass - basically a wrapper around a HashMap +} + +``` + +You must register the class with `collections.abc.Mapping` before the downcast will work: +```rust,compile_fail +let m = Py::new(py, Mapping { index }).unwrap(); +assert!(m.as_ref(py).downcast::().is_err()); +PyMapping::register::(py).unwrap(); +assert!(m.as_ref(py).downcast::().is_ok()); +``` + +Note that this requirement may go away in the future when a pyclass is able to inherit from the abstract base class directly (see [pyo3/pyo3#991](https://github.com/PyO3/pyo3/issues/991)). + ### The `multiple-pymethods` feature now requires Rust 1.62 Due to limitations in the `inventory` crate which the `multiple-pymethods` feature depends on, this feature now diff --git a/src/types/mapping.rs b/src/types/mapping.rs index 897f3ed8220..ad4a50290e7 100644 --- a/src/types/mapping.rs +++ b/src/types/mapping.rs @@ -1,8 +1,14 @@ // Copyright (c) 2017-present PyO3 Project and Contributors use crate::err::{PyDowncastError, PyErr, PyResult}; -use crate::types::{PyAny, PySequence}; -use crate::{ffi, AsPyPointer, IntoPyPointer, Py, PyNativeType, PyTryFrom, Python, ToPyObject}; +use crate::once_cell::GILOnceCell; +use crate::type_object::PyTypeInfo; +use crate::types::{PyAny, PySequence, PyType}; +use crate::{ + ffi, AsPyPointer, IntoPy, IntoPyPointer, Py, PyNativeType, PyTryFrom, Python, ToPyObject, +}; + +static MAPPING_ABC: GILOnceCell>> = GILOnceCell::new(); /// Represents a reference to a Python object supporting the mapping protocol. #[repr(transparent)] @@ -102,18 +108,45 @@ impl PyMapping { .from_owned_ptr_or_err(ffi::PyMapping_Items(self.as_ptr())) } } + + /// Register a pyclass as a subclass of `collections.abc.Mapping` (from the Python standard + /// library). This is equvalent to `collections.abc.Mapping.register(T)` in Python. + /// This registration is required for a pyclass to be downcastable from `PyAny` to `PyMapping`. + pub fn register(py: Python<'_>) -> PyResult<()> { + let ty = T::type_object(py); + get_mapping_abc(py)?.call_method1("register", (ty,))?; + Ok(()) + } +} + +fn get_mapping_abc(py: Python<'_>) -> Result<&PyType, PyErr> { + MAPPING_ABC + .get_or_init(py, || { + Ok(py + .import("collections.abc")? + .getattr("Mapping")? + .downcast::()? + .into_py(py)) + }) + .as_ref() + .map_or_else(|e| Err(e.clone_ref(py)), |t| Ok(t.as_ref(py))) } impl<'v> PyTryFrom<'v> for PyMapping { + /// Downcasting to `PyMapping` requires the concrete class to be a subclass (or registered + /// subclass) of `collections.abc.Mapping` (from the Python standard library) - i.e. + /// `isinstance(, collections.abc.Mapping) == True`. fn try_from>(value: V) -> Result<&'v PyMapping, PyDowncastError<'v>> { let value = value.into(); - unsafe { - if ffi::PyMapping_Check(value.as_ptr()) != 0 { - Ok(::try_from_unchecked(value)) - } else { - Err(PyDowncastError::new(value, "Mapping")) + + // TODO: surface specific errors in this chain to the user + if let Ok(abc) = get_mapping_abc(value.py()) { + if value.is_instance(abc).unwrap_or(false) { + unsafe { return Ok(::try_from_unchecked(value)) } } } + + Err(PyDowncastError::new(value, "Mapping")) } #[inline] diff --git a/src/types/sequence.rs b/src/types/sequence.rs index dcc4a2e729c..b01beadf52f 100644 --- a/src/types/sequence.rs +++ b/src/types/sequence.rs @@ -2,11 +2,15 @@ use crate::err::{self, PyDowncastError, PyErr, PyResult}; use crate::exceptions::PyValueError; use crate::internal_tricks::get_ssize_index; -use crate::types::{PyAny, PyList, PyString, PyTuple}; +use crate::once_cell::GILOnceCell; +use crate::type_object::PyTypeInfo; +use crate::types::{PyAny, PyList, PyString, PyTuple, PyType}; use crate::{ffi, PyNativeType, ToPyObject}; -use crate::{AsPyPointer, IntoPyPointer, Py, Python}; +use crate::{AsPyPointer, IntoPy, IntoPyPointer, Py, Python}; use crate::{FromPyObject, PyTryFrom}; +static SEQUENCE_ABC: GILOnceCell>> = GILOnceCell::new(); + /// Represents a reference to a Python object supporting the sequence protocol. #[repr(transparent)] pub struct PySequence(PyAny); @@ -250,6 +254,15 @@ impl PySequence { .from_owned_ptr_or_err(ffi::PySequence_Tuple(self.as_ptr())) } } + + /// Register a pyclass as a subclass of `collections.abc.Sequence` (from the Python standard + /// library). This is equvalent to `collections.abc.Sequence.register(T)` in Python. + /// This registration is required for a pyclass to be downcastable from `PyAny` to `PySequence`. + pub fn register(py: Python<'_>) -> PyResult<()> { + let ty = T::type_object(py); + get_sequence_abc(py)?.call_method1("register", (ty,))?; + Ok(()) + } } #[inline] @@ -289,16 +302,34 @@ where Ok(v) } +fn get_sequence_abc(py: Python<'_>) -> Result<&PyType, PyErr> { + SEQUENCE_ABC + .get_or_init(py, || { + Ok(py + .import("collections.abc")? + .getattr("Sequence")? + .downcast::()? + .into_py(py)) + }) + .as_ref() + .map_or_else(|e| Err(e.clone_ref(py)), |t| Ok(t.as_ref(py))) +} + impl<'v> PyTryFrom<'v> for PySequence { + /// Downcasting to `PySequence` requires the concrete class to be a subclass (or registered + /// subclass) of `collections.abc.Sequence` (from the Python standard library) - i.e. + /// `isinstance(, collections.abc.Sequence) == True`. fn try_from>(value: V) -> Result<&'v PySequence, PyDowncastError<'v>> { let value = value.into(); - unsafe { - if ffi::PySequence_Check(value.as_ptr()) != 0 { - Ok(::try_from_unchecked(value)) - } else { - Err(PyDowncastError::new(value, "Sequence")) + + // TODO: surface specific errors in this chain to the user + if let Ok(abc) = get_sequence_abc(value.py()) { + if value.is_instance(abc).unwrap_or(false) { + unsafe { return Ok(::try_from_unchecked(value)) } } } + + Err(PyDowncastError::new(value, "Sequence")) } fn try_from_exact>(value: V) -> Result<&'v PySequence, PyDowncastError<'v>> { diff --git a/tests/test_mapping.rs b/tests/test_mapping.rs index 4d7f6b69401..fd81005d9c6 100644 --- a/tests/test_mapping.rs +++ b/tests/test_mapping.rs @@ -119,6 +119,9 @@ fn mapping_is_not_sequence() { index.insert("Foo".into(), 1); index.insert("Bar".into(), 2); let m = Py::new(py, Mapping { index }).unwrap(); + + PyMapping::register::(py).unwrap(); + assert!(m.as_ref(py).downcast::().is_ok()); assert!(m.as_ref(py).downcast::().is_err()); }); diff --git a/tests/test_proto_methods.rs b/tests/test_proto_methods.rs index 24bcbce3f8f..b68a7e746c1 100644 --- a/tests/test_proto_methods.rs +++ b/tests/test_proto_methods.rs @@ -210,6 +210,8 @@ impl Mapping { #[test] fn mapping() { Python::with_gil(|py| { + PyMapping::register::(py).unwrap(); + let inst = Py::new( py, Mapping { @@ -218,7 +220,6 @@ fn mapping() { ) .unwrap(); - // let mapping: &PyMapping = inst.as_ref(py).downcast().unwrap(); py_assert!(py, inst, "len(inst) == 0"); @@ -317,6 +318,8 @@ impl Sequence { #[test] fn sequence() { Python::with_gil(|py| { + PySequence::register::(py).unwrap(); + let inst = Py::new(py, Sequence { values: vec![] }).unwrap(); let sequence: &PySequence = inst.as_ref(py).downcast().unwrap(); diff --git a/tests/test_sequence.rs b/tests/test_sequence.rs index 36aa9df6132..f2aaa078d12 100644 --- a/tests/test_sequence.rs +++ b/tests/test_sequence.rs @@ -2,7 +2,7 @@ use pyo3::exceptions::{PyIndexError, PyValueError}; use pyo3::prelude::*; -use pyo3::types::{IntoPyDict, PyList}; +use pyo3::types::{IntoPyDict, PyList, PyMapping, PySequence}; use pyo3::py_run; @@ -312,3 +312,22 @@ fn test_option_list_get() { py_expect_exception!(py, list, "list[2]", PyIndexError); }); } + +#[test] +fn sequence_is_not_mapping() { + let gil = Python::acquire_gil(); + let py = gil.python(); + + let list = PyCell::new( + py, + OptionList { + items: vec![Some(1), None], + }, + ) + .unwrap(); + + PySequence::register::(py).unwrap(); + + assert!(list.as_ref().downcast::().is_err()); + assert!(list.as_ref().downcast::().is_ok()); +}