Merge pull request #2192 from davidhewitt/misc-0.16-fixes

misc: tidy ups pre 0.16

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

pyo3-macros-backend/src/pymethod.rs

@@ -324,7 +324,7 @@ fn impl_traverse_slot(cls: &syn::Type, spec: FnSpec) -> Result<TokenStream> {
                 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
                 }

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<T>(&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,
-    }
-}

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<T>(&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,
+    }
+}

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")]

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
+        );
+    })
+}