Merge pull request #1690 from davidhewitt/safety-docs

docs: implement final missing safety docs

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)

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<T: PyClassWithFreeList>(
     subtype: *mut ffi::PyTypeObject,
     nitems: ffi::Py_ssize_t,
@@ -159,6 +163,10 @@ pub unsafe extern "C" fn alloc_with_freelist<T: PyClassWithFreeList>(
 }
 
 /// 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<T: PyClassWithFreeList>(obj: *mut c_void) {
     let obj = obj as *mut ffi::PyObject;
@@ -392,8 +400,7 @@ impl<T: PyClass> PyClassBaseType for T {
     type Initializer = crate::pyclass_init::PyClassInitializer<Self>;
 }
 
-// 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<T: PyClass>(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<T> = &mut *(obj as *mut _);
-        cell.tp_dealloc(py);
-    })
+    crate::callback_body!(py, T::Layout::tp_dealloc(obj, py))
 }

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.
 //!

src/pycell.rs

@@ -675,8 +675,11 @@ impl From<PyBorrowMutError> for PyErr {
 pub trait PyCellLayout<T>: PyLayout<T> {
     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<T, U> PyCellLayout<T> for PyCellBase<U>
@@ -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<T>);
+        ManuallyDrop::drop(&mut cell.contents.value);
+        cell.contents.dict.clear_dict(py);
+        cell.contents.weakref.clear_weakrefs(slf, py);
+        <T::BaseType as PyClassBaseType>::LayoutAsBase::tp_dealloc(slf, py)
     }
 }

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<T>: 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<T: PyClass> PyClassInitializer<T> {
     /// 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,

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! {}