diff --git a/guide/src/migration.md b/guide/src/migration.md index f7cd6bf39a8..51a12b76ea9 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -105,6 +105,55 @@ Because there is no such distinction from Python, implementing these methods wil The PyO3 behavior in 0.16 has been changed to be closer to this Python behavior by default. +### `wrap_pymodule!` now respects privacy correctly + +Prior to PyO3 0.16 the `wrap_pymodule!` macro could use modules declared in Rust modules which were not reachable. + +For example, the following code was legal before 0.16, but in 0.16 is rejected because the `wrap_pymodule!` macro cannot access the `private_submodule` function: + +```rust,compile_fail +mod foo { + use pyo3::prelude::*; + + #[pymodule] + fn private_submodule(_py: Python, m: &PyModule) -> PyResult<()> { + Ok(()) + } +} + +use pyo3::prelude::*; +use foo::*; + +#[pymodule] +fn my_module(_py: Python, m: &PyModule) -> PyResult<()> { + m.add_wrapped(wrap_pymodule!(private_submodule))?; + Ok(()) +} +``` + +To fix it, make the private submodule visible, e.g. with `pub` or `pub(crate)`. + +```rust +mod foo { + use pyo3::prelude::*; + + #[pymodule] + pub(crate) fn private_submodule(_py: Python, m: &PyModule) -> PyResult<()> { + Ok(()) + } +} + +use pyo3::prelude::*; +use pyo3::wrap_pymodule; +use foo::*; + +#[pymodule] +fn my_module(_py: Python, m: &PyModule) -> PyResult<()> { + m.add_wrapped(wrap_pymodule!(private_submodule))?; + Ok(()) +} +``` + ## from 0.14.* to 0.15 ### Changes in sequence indexing diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index 78b01b5e83a..cb9d0f3aa76 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -324,7 +324,7 @@ fn impl_traverse_slot(cls: &syn::Type, spec: FnSpec) -> Result { let visit = _pyo3::class::gc::PyVisit::from_raw(visit, arg, py); let borrow = slf.try_borrow(); if let ::std::result::Result::Ok(borrow) = borrow { - _pyo3::class::gc::unwrap_traverse_result(borrow.#ident(visit)) + _pyo3::impl_::pymethods::unwrap_traverse_result(borrow.#ident(visit)) } else { 0 } diff --git a/src/class/gc.rs b/src/class/gc.rs index d3b2e08e1b4..34d9fbdd221 100644 --- a/src/class/gc.rs +++ b/src/class/gc.rs @@ -3,11 +3,10 @@ //! Python GC support -use crate::{ffi, AsPyPointer, PyCell, PyClass, Python}; +use crate::{ffi, PyCell, PyClass}; use std::os::raw::{c_int, c_void}; -#[repr(transparent)] -pub struct PyTraverseError(c_int); +pub use crate::impl_::pymethods::{PyTraverseError, PyVisit}; /// GC support #[deprecated(since = "0.16.0", note = "prefer `#[pymethods]` to `#[pyproto]`")] @@ -60,45 +59,3 @@ where slf.borrow_mut().__clear__(); 0 } - -/// Object visitor for GC. -#[derive(Clone)] -pub struct PyVisit<'p> { - visit: ffi::visitproc, - arg: *mut c_void, - /// VisitProc contains a Python instance to ensure that - /// 1) it is cannot be moved out of the traverse() call - /// 2) it cannot be sent to other threads - _py: Python<'p>, -} - -impl<'p> PyVisit<'p> { - /// Visit `obj`. - pub fn call(&self, obj: &T) -> Result<(), PyTraverseError> - where - T: AsPyPointer, - { - let r = unsafe { (self.visit)(obj.as_ptr(), self.arg) }; - if r == 0 { - Ok(()) - } else { - Err(PyTraverseError(r)) - } - } - - /// Creates the PyVisit from the arguments to tp_traverse - #[doc(hidden)] - pub unsafe fn from_raw(visit: ffi::visitproc, arg: *mut c_void, _py: Python<'p>) -> Self { - Self { visit, arg, _py } - } -} - -/// Unwraps the result of __traverse__ for tp_traverse -#[doc(hidden)] -#[inline] -pub fn unwrap_traverse_result(result: Result<(), PyTraverseError>) -> c_int { - match result { - Ok(()) => 0, - Err(PyTraverseError(value)) => value, - } -} diff --git a/src/impl_/pymethods.rs b/src/impl_/pymethods.rs index b53c7db46f5..57024ae7cfe 100644 --- a/src/impl_/pymethods.rs +++ b/src/impl_/pymethods.rs @@ -1,8 +1,8 @@ use crate::internal_tricks::{extract_cstr_or_leak_cstring, NulByteInString}; -use crate::{ffi, FromPyObject, PyAny, PyObject, PyResult, Python}; +use crate::{ffi, AsPyPointer, FromPyObject, PyAny, PyObject, PyResult, Python}; use std::ffi::CStr; use std::fmt; -use std::os::raw::c_int; +use std::os::raw::{c_int, c_void}; /// Python 3.8 and up - __ipow__ has modulo argument correctly populated. #[cfg(Py_3_8)] @@ -252,3 +252,48 @@ fn get_name(name: &'static str) -> Result<&'static CStr, NulByteInString> { fn get_doc(doc: &'static str) -> Result<&'static CStr, NulByteInString> { extract_cstr_or_leak_cstring(doc, "Document cannot contain NUL byte.") } + +#[repr(transparent)] +pub struct PyTraverseError(pub(crate) c_int); + +/// Object visitor for GC. +#[derive(Clone)] +pub struct PyVisit<'p> { + pub(crate) visit: ffi::visitproc, + pub(crate) arg: *mut c_void, + /// VisitProc contains a Python instance to ensure that + /// 1) it is cannot be moved out of the traverse() call + /// 2) it cannot be sent to other threads + pub(crate) _py: Python<'p>, +} + +impl<'p> PyVisit<'p> { + /// Visit `obj`. + pub fn call(&self, obj: &T) -> Result<(), PyTraverseError> + where + T: AsPyPointer, + { + let r = unsafe { (self.visit)(obj.as_ptr(), self.arg) }; + if r == 0 { + Ok(()) + } else { + Err(PyTraverseError(r)) + } + } + + /// Creates the PyVisit from the arguments to tp_traverse + #[doc(hidden)] + pub unsafe fn from_raw(visit: ffi::visitproc, arg: *mut c_void, _py: Python<'p>) -> Self { + Self { visit, arg, _py } + } +} + +/// Unwraps the result of __traverse__ for tp_traverse +#[doc(hidden)] +#[inline] +pub fn unwrap_traverse_result(result: Result<(), PyTraverseError>) -> c_int { + match result { + Ok(()) => 0, + Err(PyTraverseError(value)) => value, + } +} diff --git a/src/lib.rs b/src/lib.rs index 62a2a86b5b7..41c4c074c01 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -62,7 +62,10 @@ //! ## Default feature flags //! //! The following features are turned on by default: -//! - `macros`: Enables various macros, including all the attribute macros. +//! - `macros`: Enables various macros, including all the attribute macros excluding the deprecated +//! `#[pyproto]` attribute. +//! - `pyproto`: Adds the deprecated `#[pyproto]` attribute macro. Likely to become optional and +//! then removed in the future. //! //! ## Optional feature flags //! @@ -306,6 +309,8 @@ pub mod class { #[doc(hidden)] pub use crate::impl_::pymethods as methods; + pub use self::gc::{PyTraverseError, PyVisit}; + #[doc(hidden)] pub use self::methods::{ PyClassAttributeDef, PyGetterDef, PyMethodDef, PyMethodDefType, PyMethodType, PySetterDef, @@ -322,6 +327,10 @@ pub mod class { pub mod iter { pub use crate::pyclass::{IterNextOutput, PyIterNextOutput}; } + + pub mod gc { + pub use crate::impl_::pymethods::{PyTraverseError, PyVisit}; + } } #[cfg(feature = "macros")] diff --git a/tests/test_gc.rs b/tests/test_gc.rs index 839517b9618..8b55b3870d5 100644 --- a/tests/test_gc.rs +++ b/tests/test_gc.rs @@ -278,3 +278,47 @@ fn gc_during_borrow() { drop(guard); } } + +#[pyclass] +struct PanickyTraverse { + member: PyObject, +} + +impl PanickyTraverse { + fn new(py: Python) -> Self { + Self { member: py.None() } + } +} + +#[pymethods] +impl PanickyTraverse { + fn __traverse__(&self, visit: PyVisit) -> Result<(), PyTraverseError> { + visit.call(&self.member)?; + // In the test, we expect this to never be hit + unreachable!() + } +} + +#[test] +fn traverse_error() { + Python::with_gil(|py| unsafe { + // declare a visitor function which errors (returns nonzero code) + extern "C" fn visit_error( + _object: *mut pyo3::ffi::PyObject, + _arg: *mut core::ffi::c_void, + ) -> std::os::raw::c_int { + -1 + } + + // get the traverse function + let ty = PanickyTraverse::type_object(py).as_type_ptr(); + let traverse = get_type_traverse(ty).unwrap(); + + // confirm that traversing errors + let obj = Py::new(py, PanickyTraverse::new(py)).unwrap(); + assert_eq!( + traverse(obj.as_ptr(), visit_error, std::ptr::null_mut()), + -1 + ); + }) +}