From 8416433623b4efc9276a36742b78a203891d76f1 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Wed, 23 Jun 2021 09:18:33 +0100 Subject: [PATCH] docs: implement final missing safety docs --- Contributing.md | 4 ++-- src/class/impl_.rs | 20 ++++++++++++-------- src/lib.rs | 1 - src/pycell.rs | 29 ++++++++++++++++------------- src/pyclass_init.rs | 4 +++- src/pyclass_slots.rs | 3 +++ 6 files changed, 36 insertions(+), 25 deletions(-) diff --git a/Contributing.md b/Contributing.md index 9a66612c..d2f8155b 100644 --- a/Contributing.md +++ b/Contributing.md @@ -30,9 +30,9 @@ Don't be afraid if the solution is not clear to you! The core PyO3 contributors PyO3 has a user guide (using mdbook) as well as the usual Rust API docs. The aim is for both of these to be detailed, easy to understand, and up-to-date. Pull requests are always welcome to fix typos, change wording, add examples, etc. There are some specific areas of focus where help is currently needed for the documentation: + - Issues requesting documentation improvements are tracked with the [documentation](https://github.com/PyO3/pyo3/issues?q=is%3Aissue+is%3Aopen+label%3Adocumentation) label. - Not all APIs had docs or examples when they were made. The goal is to have documentation on all PyO3 APIs ([#306](https://github.com/PyO3/pyo3/issues/306)). If you see an API lacking a doc, please write one and open a PR! -- Not all `unsafe` APIs had safety notes when they made. We'd like to ensure all `unsafe` APIs are carefully explained ([#698](https://github.com/PyO3/pyo3/issues/698)). If you see an `unsafe` function missing safety notes, please write some and open a PR! ### Help design the next PyO3 @@ -92,4 +92,4 @@ At the moment there is no official organisation that accepts sponsorship on PyO3 In the meanwhile, some of our maintainers have personal Github sponsorship pages and would be grateful for your support: -* [davidhewitt](https://github.com/sponsors/davidhewitt) +- [davidhewitt](https://github.com/sponsors/davidhewitt) diff --git a/src/class/impl_.rs b/src/class/impl_.rs index 9b747c96..86bf8496 100644 --- a/src/class/impl_.rs +++ b/src/class/impl_.rs @@ -6,7 +6,7 @@ use crate::{ pycell::PyCellLayout, pyclass_init::PyObjectInit, type_object::{PyLayout, PyTypeObject}, - PyCell, PyClass, PyMethodDefType, PyNativeType, PyTypeInfo, Python, + PyClass, PyMethodDefType, PyNativeType, PyTypeInfo, Python, }; use std::{marker::PhantomData, os::raw::c_void, thread}; @@ -136,6 +136,10 @@ pub trait PyClassWithFreeList: PyClass { } /// Implementation of tp_alloc for `freelist` classes. +/// +/// # Safety +/// - `subtype` must be a valid pointer to the type object of T or a subclass. +/// - The GIL must be held. pub unsafe extern "C" fn alloc_with_freelist( subtype: *mut ffi::PyTypeObject, nitems: ffi::Py_ssize_t, @@ -159,6 +163,10 @@ pub unsafe extern "C" fn alloc_with_freelist( } /// Implementation of tp_free for `freelist` classes. +/// +/// # Safety +/// - `obj` must be a valid pointer to an instance of T (not a subclass). +/// - The GIL must be held. #[allow(clippy::collapsible_if)] // for if cfg! pub unsafe extern "C" fn free_with_freelist(obj: *mut c_void) { let obj = obj as *mut ffi::PyObject; @@ -392,8 +400,7 @@ impl PyClassBaseType for T { type Initializer = crate::pyclass_init::PyClassInitializer; } -// Default new implementation - +/// Default new implementation pub(crate) unsafe extern "C" fn fallback_new( _subtype: *mut ffi::PyTypeObject, _args: *mut ffi::PyObject, @@ -406,10 +413,7 @@ pub(crate) unsafe extern "C" fn fallback_new( }) } +/// Implementation of tp_dealloc for all pyclasses pub(crate) unsafe extern "C" fn tp_dealloc(obj: *mut ffi::PyObject) { - crate::callback_body!(py, { - // Safety: Python will only call tp_dealloc when no references to the object remain. - let cell: &mut PyCell = &mut *(obj as *mut _); - cell.tp_dealloc(py); - }) + crate::callback_body!(py, T::Layout::tp_dealloc(obj, py)) } diff --git a/src/lib.rs b/src/lib.rs index 78a6721b..3dc0fb2f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,6 +1,5 @@ #![cfg_attr(feature = "nightly", feature(specialization))] #![cfg_attr(docsrs, feature(doc_cfg))] -#![allow(clippy::missing_safety_doc)] // FIXME (#698) //! Rust bindings to the Python interpreter. //! diff --git a/src/pycell.rs b/src/pycell.rs index 398cc9e5..6eea718a 100644 --- a/src/pycell.rs +++ b/src/pycell.rs @@ -675,8 +675,11 @@ impl From for PyErr { pub trait PyCellLayout: PyLayout { fn get_borrow_flag(&self) -> BorrowFlag; fn set_borrow_flag(&self, flag: BorrowFlag); - /// Implementation of tp_dealloc. Do not attempt to use &self after calling this! - unsafe fn tp_dealloc(&mut self, py: Python); + /// Implementation of tp_dealloc. + /// # Safety + /// - slf must be a valid pointer to an instance of a T or a subclass. + /// - slf must not be used after this call (as it will be freed). + unsafe fn tp_dealloc(slf: *mut ffi::PyObject, py: Python); } impl PyCellLayout for PyCellBase @@ -690,21 +693,19 @@ where fn set_borrow_flag(&self, flag: BorrowFlag) { self.borrow_flag.set(flag) } - unsafe fn tp_dealloc(&mut self, py: Python) { - let obj: *mut ffi::PyObject = self as *mut _ as *mut _; - + unsafe fn tp_dealloc(slf: *mut ffi::PyObject, py: Python) { // For `#[pyclass]` types which inherit from PyAny, we can just call tp_free if T::type_object_raw(py) == &mut PyBaseObject_Type { - return get_tp_free(ffi::Py_TYPE(obj))(obj as _); + return get_tp_free(ffi::Py_TYPE(slf))(slf as _); } // More complex native types (e.g. `extends=PyDict`) require calling the base's dealloc. #[cfg(not(Py_LIMITED_API))] { if let Some(dealloc) = (*T::type_object_raw(py)).tp_dealloc { - dealloc(obj as _); + dealloc(slf as _); } else { - get_tp_free(ffi::Py_TYPE(obj))(obj as _); + get_tp_free(ffi::Py_TYPE(slf))(slf as _); } } @@ -724,10 +725,12 @@ where fn set_borrow_flag(&self, flag: BorrowFlag) { self.ob_base.set_borrow_flag(flag) } - unsafe fn tp_dealloc(&mut self, py: Python) { - ManuallyDrop::drop(&mut self.contents.value); - self.contents.dict.clear_dict(py); - self.contents.weakref.clear_weakrefs(self.as_ptr(), py); - self.ob_base.tp_dealloc(py); + unsafe fn tp_dealloc(slf: *mut ffi::PyObject, py: Python) { + // Safety: Python only calls tp_dealloc when no references to the object remain. + let cell = &mut *(slf as *mut PyCell); + ManuallyDrop::drop(&mut cell.contents.value); + cell.contents.dict.clear_dict(py); + cell.contents.weakref.clear_weakrefs(slf, py); + ::LayoutAsBase::tp_dealloc(slf, py) } } diff --git a/src/pyclass_init.rs b/src/pyclass_init.rs index eeab7323..0e3fdb1b 100644 --- a/src/pyclass_init.rs +++ b/src/pyclass_init.rs @@ -19,6 +19,8 @@ use std::{ /// This trait is intended to use internally for distinguishing `#[pyclass]` and /// Python native types. pub trait PyObjectInit: Sized { + /// # Safety + /// - `subtype` must be a valid pointer to a type object of T or a subclass. unsafe fn into_new_object( self, py: Python, @@ -185,7 +187,7 @@ impl PyClassInitializer { /// Called by our `tp_new` generated by the `#[new]` attribute. /// /// # Safety - /// `subtype` must be a subclass of T. + /// `subtype` must be a valid pointer to the type object of T or a subclass. #[doc(hidden)] pub unsafe fn create_cell_from_subtype( self, diff --git a/src/pyclass_slots.rs b/src/pyclass_slots.rs index 74b6ef81..fd006a39 100644 --- a/src/pyclass_slots.rs +++ b/src/pyclass_slots.rs @@ -15,6 +15,9 @@ pub trait PyClassDict { pub trait PyClassWeakRef { const IS_DUMMY: bool = true; fn new() -> Self; + /// # Safety + /// - `_obj` must be a pointer to the pyclass instance which contains `self`. + /// - The GIL must be held. #[inline] unsafe fn clear_weakrefs(&mut self, _obj: *mut ffi::PyObject, _py: Python) {} private_decl! {}