From e9bd41efb28df60535262b5d425f3888c4b231e5 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Thu, 21 Apr 2022 08:03:45 +0100 Subject: [PATCH] better mutability inheritance rules --- guide/src/class.md | 22 +- pyo3-macros-backend/src/pyclass.rs | 60 ++-- src/impl_/pyclass.rs | 30 +- src/pycell.rs | 278 +++++++++++++++--- src/pyclass.rs | 14 +- src/pyclass_init.rs | 32 +- src/types/mod.rs | 5 +- tests/test_compile_error.rs | 4 +- tests/test_mutable_pyclass.rs | 201 +++++++++++++ tests/ui/abi3_nativetype_inheritance.stderr | 26 +- tests/ui/invalid_immutable_pyclass_borrow.rs | 12 +- .../invalid_immutable_pyclass_borrow.stderr | 13 + 12 files changed, 555 insertions(+), 142 deletions(-) create mode 100644 tests/test_mutable_pyclass.rs diff --git a/guide/src/class.md b/guide/src/class.md index 2dc9194e..81802df7 100644 --- a/guide/src/class.md +++ b/guide/src/class.md @@ -956,13 +956,7 @@ unsafe impl ::pyo3::type_object::PyTypeInfo for MyClass { } } -impl ::pyo3::PyClass for MyClass { - type Dict = ::pyo3::impl_::pyclass::PyClassDummySlot; - type WeakRef = ::pyo3::impl_::pyclass::PyClassDummySlot; - type BaseNativeType = ::pyo3::PyAny; -} - -unsafe impl ::pyo3::pyclass::MutablePyClass for MyClass {} +impl ::pyo3::PyClass for MyClass { } impl<'a> ::pyo3::derive_utils::ExtractExt<'a> for &'a mut MyClass { type Target = ::pyo3::PyRefMut<'a, MyClass>; @@ -985,7 +979,11 @@ impl pyo3::impl_::pyclass::PyClassImpl for MyClass { type Layout = PyCell; type BaseType = PyAny; type ThreadChecker = pyo3::impl_::pyclass::ThreadCheckerStub; - type Mutabilty = pyo3::pyclass::Mutable; + type Mutability = pyo3::pycell::Mutable; + type PyClassMutability = pyo3::pycell::MutableClass; + type Dict = ::pyo3::impl_::pyclass::PyClassDummySlot; + type WeakRef = ::pyo3::impl_::pyclass::PyClassDummySlot; + type BaseNativeType = ::pyo3::PyAny; fn for_all_items(visitor: &mut dyn FnMut(&pyo3::impl_::pyclass::PyClassItems)) { use pyo3::impl_::pyclass::*; @@ -996,14 +994,6 @@ impl pyo3::impl_::pyclass::PyClassImpl for MyClass { } } -impl ::pyo3::impl_::pyclass::PyClassDescriptors - for ::pyo3::impl_::pyclass::PyClassImplCollector -{ - fn py_class_descriptors(self) -> &'static [::pyo3::class::methods::PyMethodDefType] { - static METHODS: &[::pyo3::class::methods::PyMethodDefType] = &[]; - METHODS - } -} # Python::with_gil(|py| { # let cls = py.get_type::(); # pyo3::py_run!(py, cls, "assert cls.__name__ == 'MyClass'") diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index 99bc545c..d421acf5 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -689,32 +689,9 @@ impl<'a> PyClassImplsBuilder<'a> { fn impl_pyclass(&self) -> TokenStream { let cls = self.cls; - let attr = self.attr; - let dict = if attr.options.dict.is_some() { - quote! { _pyo3::impl_::pyclass::PyClassDictSlot } - } else { - quote! { _pyo3::impl_::pyclass::PyClassDummySlot } - }; - - // insert space for weak ref - let weakref = if attr.options.weakref.is_some() { - quote! { _pyo3::impl_::pyclass::PyClassWeakRefSlot } - } else { - quote! { _pyo3::impl_::pyclass::PyClassDummySlot } - }; - - let base_nativetype = if attr.options.extends.is_some() { - quote! { >::BaseNativeType } - } else { - quote! { _pyo3::PyAny } - }; quote! { - impl _pyo3::PyClass for #cls { - type Dict = #dict; - type WeakRef = #weakref; - type BaseNativeType = #base_nativetype; - } + impl _pyo3::PyClass for #cls { } } } fn impl_extractext(&self) -> TokenStream { @@ -856,6 +833,37 @@ impl<'a> PyClassImplsBuilder<'a> { } }; + let class_mutability = if self.attr.options.immutable.is_some() { + quote! { + ImmutableChild + } + } else { + quote! { + MutableChild + } + }; + + let cls = self.cls; + let attr = self.attr; + let dict = if attr.options.dict.is_some() { + quote! { _pyo3::impl_::pyclass::PyClassDictSlot } + } else { + quote! { _pyo3::impl_::pyclass::PyClassDummySlot } + }; + + // insert space for weak ref + let weakref = if attr.options.weakref.is_some() { + quote! { _pyo3::impl_::pyclass::PyClassWeakRefSlot } + } else { + quote! { _pyo3::impl_::pyclass::PyClassDummySlot } + }; + + let base_nativetype = if attr.options.extends.is_some() { + quote! { ::BaseNativeType } + } else { + quote! { _pyo3::PyAny } + }; + quote! { impl _pyo3::impl_::pyclass::PyClassImpl for #cls { const DOC: &'static str = #doc; @@ -868,6 +876,10 @@ impl<'a> PyClassImplsBuilder<'a> { type ThreadChecker = #thread_checker; #inventory type Mutability = #mutability; + type PyClassMutability = <<#base as _pyo3::impl_::pyclass::PyClassBaseType>::PyClassMutability as _pyo3::pycell::PyClassMutability>::#class_mutability; + type Dict = #dict; + type WeakRef = #weakref; + type BaseNativeType = #base_nativetype; fn for_all_items(visitor: &mut dyn ::std::ops::FnMut(& _pyo3::impl_::pyclass::PyClassItems)) { use _pyo3::impl_::pyclass::*; diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index ed569888..43468de9 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -2,8 +2,7 @@ use crate::{ exceptions::{PyAttributeError, PyNotImplementedError}, ffi, impl_::freelist::FreeList, - pycell::{Mutability, Mutable, PyCellLayout}, - pyclass::MutablePyClass, + pycell::{GetBorrowChecker, Mutability, PyCellLayout, PyClassMutability}, pyclass_init::PyObjectInit, type_object::{PyLayout, PyTypeObject}, Py, PyAny, PyCell, PyClass, PyErr, PyMethodDefType, PyNativeType, PyResult, PyTypeInfo, Python, @@ -163,11 +162,24 @@ pub trait PyClassImpl: Sized { type Layout: PyLayout; /// Base class - type BaseType: PyTypeInfo + PyTypeObject + PyClassBaseType; + type BaseType: PyTypeInfo + PyTypeObject + PyClassBaseType; /// Immutable or mutable type Mutability: Mutability; + /// Immutable or mutable + type PyClassMutability: PyClassMutability + GetBorrowChecker; + + /// Specify this class has `#[pyclass(dict)]` or not. + type Dict: PyClassDict; + + /// Specify this class has `#[pyclass(weakref)]` or not. + type WeakRef: PyClassWeakRef; + + /// The closest native ancestor. This is `PyAny` by default, and when you declare + /// `#[pyclass(extends=PyDict)]`, it's `PyDict`. + type BaseNativeType: PyTypeInfo + PyNativeType; + /// This handles following two situations: /// 1. In case `T` is `Send`, stub `ThreadChecker` is used and does nothing. /// This implementation is used by default. Compile fails if `T: !Send`. @@ -870,12 +882,12 @@ impl PyClassThreadChecker for ThreadCheckerImpl { /// Thread checker for types that have `Send` and `extends=...`. /// Ensures that `T: Send` and the parent is not accessed by another thread. #[doc(hidden)] -pub struct ThreadCheckerInherited>( +pub struct ThreadCheckerInherited( PhantomData, U::ThreadChecker, ); -impl> PyClassThreadChecker +impl PyClassThreadChecker for ThreadCheckerInherited { fn ensure(&self) { @@ -888,21 +900,23 @@ impl> PyClassThreadChecker< } /// Trait denoting that this class is suitable to be used as a base type for PyClass. -pub trait PyClassBaseType: Sized { - type LayoutAsBase: PyCellLayout; +pub trait PyClassBaseType: Sized { + type LayoutAsBase: PyCellLayout; type BaseNativeType; type ThreadChecker: PyClassThreadChecker; type Initializer: PyObjectInit; + type PyClassMutability: PyClassMutability; } /// All mutable PyClasses can be used as a base type. /// /// In the future this will be extended to immutable PyClasses too. -impl PyClassBaseType for T { +impl PyClassBaseType for T { type LayoutAsBase = crate::pycell::PyCell; type BaseNativeType = T::BaseNativeType; type ThreadChecker = T::ThreadChecker; type Initializer = crate::pyclass_init::PyClassInitializer; + type PyClassMutability = T::PyClassMutability; } /// Implementation of tp_dealloc for all pyclasses diff --git a/src/pycell.rs b/src/pycell.rs index da9c1bcf..61ef252e 100644 --- a/src/pycell.rs +++ b/src/pycell.rs @@ -173,7 +173,9 @@ //! [Interior Mutability]: https://doc.rust-lang.org/book/ch15-05-interior-mutability.html "RefCell and the Interior Mutability Pattern - The Rust Programming Language" use crate::exceptions::PyRuntimeError; -use crate::impl_::pyclass::{PyClassBaseType, PyClassDict, PyClassThreadChecker, PyClassWeakRef}; +use crate::impl_::pyclass::{ + PyClassBaseType, PyClassDict, PyClassImpl, PyClassThreadChecker, PyClassWeakRef, +}; use crate::pyclass::{MutablePyClass, PyClass}; use crate::pyclass_init::PyClassInitializer; use crate::type_object::{PyLayout, PySizedLayout}; @@ -191,6 +193,212 @@ use std::marker::PhantomData; use std::mem::ManuallyDrop; use std::ops::{Deref, DerefMut}; +pub struct EmptySlot(()); +pub struct BorrowChecker(Cell); +pub struct FilledInAncestor(()); + +impl BorrowChecker { + fn try_borrow(&self) -> Result<(), PyBorrowError> { + let flag = self.0.get(); + if flag != BorrowFlag::HAS_MUTABLE_BORROW { + self.0.set(flag.increment()); + Ok(()) + } else { + Err(PyBorrowError { _private: () }) + } + } + + fn try_borrow_unguarded(&self) -> Result<(), PyBorrowError> { + let flag = self.0.get(); + if flag != BorrowFlag::HAS_MUTABLE_BORROW { + Ok(()) + } else { + Err(PyBorrowError { _private: () }) + } + } + + fn release_borrow(&self) { + let flag = self.0.get(); + self.0.set(flag.decrement()) + } + + fn try_borrow_mut(&self) -> Result<(), PyBorrowMutError> { + let flag = self.0.get(); + if flag == BorrowFlag::UNUSED { + self.0.set(BorrowFlag::HAS_MUTABLE_BORROW); + Ok(()) + } else { + Err(PyBorrowMutError { _private: () }) + } + } + + fn release_borrow_mut(&self) { + self.0.set(BorrowFlag::UNUSED) + } +} + +pub trait PyClassMutabilityStorage { + fn new() -> Self; +} + +impl PyClassMutabilityStorage for EmptySlot { + fn new() -> Self { + Self(()) + } +} + +impl PyClassMutabilityStorage for BorrowChecker { + fn new() -> Self { + Self(Cell::new(BorrowFlag::UNUSED)) + } +} + +// - Storage type, either empty, present, or in ancestor +// - Mutability is either +// - Immutable - i.e. EmptySlot +// - Mutable - i.e. BorrowChecker +// - ExtendsMutableAncestor - FilledInAncestor +// - Mutability trait needs to encode the inheritance + +pub trait PyClassMutability { + // The storage for this inheritance layer. Only the first mutable class in + // an inheritance hierarchy needs to store the borrow flag. + type Storage: PyClassMutabilityStorage; + // The borrow flag needed to implement this class' mutability. Empty until + // the first mutable class, at which point it is BorrowChecker and will be + // for all subclasses. + type Checker; + type ImmutableChild: PyClassMutability; + type MutableChild: PyClassMutability; + + /// Increments immutable borrow count, if possible + fn try_borrow(checker: &Self::Checker) -> Result<(), PyBorrowError>; + + fn try_borrow_unguarded(checker: &Self::Checker) -> Result<(), PyBorrowError>; + + /// Decrements immutable borrow count + fn release_borrow(checker: &Self::Checker); + /// Increments mutable borrow count, if possible + fn try_borrow_mut(checker: &Self::Checker) -> Result<(), PyBorrowMutError>; + /// Decremements mutable borrow count + fn release_borrow_mut(checker: &Self::Checker); +} + +pub trait GetBorrowChecker { + fn borrow_checker(cell: &PyCell) -> &::Checker; +} + +impl> GetBorrowChecker for MutableClass { + fn borrow_checker(cell: &PyCell) -> &BorrowChecker { + &cell.contents.borrow_checker + } +} + +impl> GetBorrowChecker for ImmutableClass { + fn borrow_checker(cell: &PyCell) -> &EmptySlot { + &cell.contents.borrow_checker + } +} + +impl, M: PyClassMutability> GetBorrowChecker + for ExtendsMutableAncestor +where + T::BaseType: PyClassImpl> + + PyClassBaseType>, + ::PyClassMutability: PyClassMutability, +{ + fn borrow_checker(cell: &PyCell) -> &BorrowChecker { + <::PyClassMutability as GetBorrowChecker>::borrow_checker(&cell.ob_base) + } +} + +pub struct ImmutableClass(()); +pub struct MutableClass(()); +pub struct ExtendsMutableAncestor(PhantomData); + +impl PyClassMutability for ImmutableClass { + type Storage = EmptySlot; + type Checker = EmptySlot; + type ImmutableChild = ImmutableClass; + type MutableChild = MutableClass; + + fn try_borrow(_: &EmptySlot) -> Result<(), PyBorrowError> { + Ok(()) + } + + fn try_borrow_unguarded(_: &EmptySlot) -> Result<(), PyBorrowError> { + Ok(()) + } + + fn release_borrow(_: &EmptySlot) {} + + fn try_borrow_mut(_: &EmptySlot) -> Result<(), PyBorrowMutError> { + unreachable!() + } + + fn release_borrow_mut(_: &EmptySlot) { + unreachable!() + } +} + +impl PyClassMutability for MutableClass { + type Storage = BorrowChecker; + type Checker = BorrowChecker; + type ImmutableChild = ExtendsMutableAncestor; + type MutableChild = ExtendsMutableAncestor; + + // FIXME the below are all wrong + + fn try_borrow(checker: &BorrowChecker) -> Result<(), PyBorrowError> { + checker.try_borrow() + } + + fn try_borrow_unguarded(checker: &BorrowChecker) -> Result<(), PyBorrowError> { + checker.try_borrow_unguarded() + } + + fn release_borrow(checker: &BorrowChecker) { + checker.release_borrow() + } + + fn try_borrow_mut(checker: &BorrowChecker) -> Result<(), PyBorrowMutError> { + checker.try_borrow_mut() + } + + fn release_borrow_mut(checker: &BorrowChecker) { + checker.release_borrow_mut() + } +} + +impl PyClassMutability for ExtendsMutableAncestor { + type Storage = EmptySlot; + type Checker = BorrowChecker; + type ImmutableChild = ExtendsMutableAncestor; + type MutableChild = ExtendsMutableAncestor; + + // FIXME the below are all wrong + + fn try_borrow(checker: &BorrowChecker) -> Result<(), PyBorrowError> { + checker.try_borrow() + } + + fn try_borrow_unguarded(checker: &BorrowChecker) -> Result<(), PyBorrowError> { + checker.try_borrow_unguarded() + } + + fn release_borrow(checker: &BorrowChecker) { + checker.release_borrow() + } + + fn try_borrow_mut(checker: &BorrowChecker) -> Result<(), PyBorrowMutError> { + checker.try_borrow_mut() + } + + fn release_borrow_mut(checker: &BorrowChecker) { + checker.release_borrow_mut() + } +} + pub trait Mutability { /// Creates a new borrow checker fn new() -> Self; @@ -284,20 +492,13 @@ impl Mutability for Immutable { } /// Base layout of PyCell. -/// This is necessary for sharing BorrowFlag between parents and children. #[doc(hidden)] #[repr(C)] -pub struct PyCellBase { +pub struct PyCellBase { ob_base: T, - borrow_impl: M, } -unsafe impl PyLayout for PyCellBase -where - U: PySizedLayout, - M: Mutability, -{ -} +unsafe impl PyLayout for PyCellBase where U: PySizedLayout {} /// A container type for (mutably) accessing [`PyClass`] values /// @@ -335,14 +536,15 @@ where /// For more information on how, when and why (not) to use `PyCell` please see the /// [module-level documentation](self). #[repr(C)] -pub struct PyCell { - ob_base: >::LayoutAsBase, +pub struct PyCell { + ob_base: ::LayoutAsBase, contents: PyCellContents, } #[repr(C)] -pub(crate) struct PyCellContents { +pub(crate) struct PyCellContents { pub(crate) value: ManuallyDrop>, + pub(crate) borrow_checker: ::Storage, pub(crate) thread_checker: T::ThreadChecker, pub(crate) dict: T::Dict, pub(crate) weakref: T::WeakRef, @@ -412,9 +614,8 @@ impl PyCell { /// }); /// ``` pub fn try_borrow(&self) -> Result, PyBorrowError> { - self.borrow_checker() - .try_borrow() - .map(|_| PyRef { inner: self }) + self.ensure_threadsafe(); + T::PyClassMutability::try_borrow(self.borrow_checker()).map(|_| PyRef { inner: self }) } /// Mutably borrows the value `T`, returning an error if the value is currently borrowed. @@ -442,8 +643,8 @@ impl PyCell { where T: MutablePyClass, { - self.borrow_checker() - .try_borrow_mut() + self.ensure_threadsafe(); + T::PyClassMutability::try_borrow_mut(self.borrow_checker()) .map(|_| PyRefMut { inner: self }) } @@ -477,8 +678,8 @@ impl PyCell { /// }); /// ``` pub unsafe fn try_borrow_unguarded(&self) -> Result<&T, PyBorrowError> { - self.borrow_checker() - .try_borrow_unguarded() + self.ensure_threadsafe(); + T::PyClassMutability::try_borrow_unguarded(self.borrow_checker()) .map(|_: ()| &*self.contents.value.get()) } @@ -593,7 +794,13 @@ impl PyCell { } } -unsafe impl PyLayout for PyCell {} +impl PyCell { + fn borrow_checker(&self) -> &::Checker { + T::PyClassMutability::borrow_checker(self) + } +} + +unsafe impl PyLayout for PyCell {} impl PySizedLayout for PyCell {} impl AsPyPointer for PyCell { @@ -776,7 +983,7 @@ impl<'p, T: PyClass> Deref for PyRef<'p, T> { impl<'p, T: PyClass> Drop for PyRef<'p, T> { fn drop(&mut self) { - self.inner.borrow_checker().release_borrow() + T::PyClassMutability::release_borrow(self.inner.borrow_checker()) } } @@ -874,7 +1081,7 @@ impl<'p, T: MutablePyClass> DerefMut for PyRefMut<'p, T> { impl<'p, T: MutablePyClass> Drop for PyRefMut<'p, T> { fn drop(&mut self) { - self.inner.borrow_checker().release_borrow_mut() + T::PyClassMutability::release_borrow_mut(self.inner.borrow_checker()) } } @@ -969,11 +1176,8 @@ impl From for PyErr { } #[doc(hidden)] -pub trait PyCellLayout: PyLayout -where - M: Mutability, -{ - fn borrow_checker(&self) -> &M; +pub trait PyCellLayout: PyLayout { + fn ensure_threadsafe(&self); /// Implementation of tp_dealloc. /// # Safety /// - slf must be a valid pointer to an instance of a T or a subclass. @@ -981,15 +1185,12 @@ where unsafe fn tp_dealloc(slf: *mut ffi::PyObject, py: Python<'_>); } -impl PyCellLayout for PyCellBase +impl PyCellLayout for PyCellBase where U: PySizedLayout, T: PyTypeInfo, - M: Mutability, { - fn borrow_checker(&self) -> &M { - &self.borrow_impl - } + fn ensure_threadsafe(&self) {} 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 { @@ -1011,14 +1212,13 @@ where } } -impl PyCellLayout for PyCell +impl PyCellLayout for PyCell where - >::LayoutAsBase: - PyCellLayout, + ::LayoutAsBase: PyCellLayout, { - fn borrow_checker(&self) -> &T::Mutability { + fn ensure_threadsafe(&self) { self.contents.thread_checker.ensure(); - self.ob_base.borrow_checker() + self.ob_base.ensure_threadsafe(); } unsafe fn tp_dealloc(slf: *mut ffi::PyObject, py: Python<'_>) { // Safety: Python only calls tp_dealloc when no references to the object remain. @@ -1026,6 +1226,6 @@ where ManuallyDrop::drop(&mut cell.contents.value); cell.contents.dict.clear_dict(py); cell.contents.weakref.clear_weakrefs(slf, py); - >::LayoutAsBase::tp_dealloc(slf, py) + ::LayoutAsBase::tp_dealloc(slf, py) } } diff --git a/src/pyclass.rs b/src/pyclass.rs index 77966d82..7cdbd8e9 100644 --- a/src/pyclass.rs +++ b/src/pyclass.rs @@ -5,11 +5,10 @@ use crate::{ exceptions::PyTypeError, ffi, impl_::pyclass::{ - assign_sequence_item_from_mapping, get_sequence_item_from_mapping, tp_dealloc, PyClassDict, - PyClassImpl, PyClassItems, PyClassWeakRef, + assign_sequence_item_from_mapping, get_sequence_item_from_mapping, tp_dealloc, PyClassImpl, + PyClassItems, }, - IntoPy, IntoPyPointer, PyCell, PyErr, PyMethodDefType, PyNativeType, PyObject, PyResult, - PyTypeInfo, Python, + IntoPy, IntoPyPointer, PyCell, PyErr, PyMethodDefType, PyObject, PyResult, PyTypeInfo, Python, }; use std::{ convert::TryInto, @@ -26,13 +25,6 @@ use std::{ pub trait PyClass: PyTypeInfo> + PyClassImpl> { - /// Specify this class has `#[pyclass(dict)]` or not. - type Dict: PyClassDict; - /// Specify this class has `#[pyclass(weakref)]` or not. - type WeakRef: PyClassWeakRef; - /// The closest native ancestor. This is `PyAny` by default, and when you declare - /// `#[pyclass(extends=PyDict)]`, it's `PyDict`. - type BaseNativeType: PyTypeInfo + PyNativeType; } pub trait MutablePyClass: PyClass {} diff --git a/src/pyclass_init.rs b/src/pyclass_init.rs index da55011e..be44c4a3 100644 --- a/src/pyclass_init.rs +++ b/src/pyclass_init.rs @@ -1,12 +1,11 @@ //! Contains initialization utilities for `#[pyclass]`. use crate::callback::IntoPyCallbackOutput; use crate::impl_::pyclass::{PyClassBaseType, PyClassDict, PyClassThreadChecker, PyClassWeakRef}; -use crate::pycell::Mutability; use crate::pyclass::MutablePyClass; use crate::{ffi, PyCell, PyClass, PyErr, PyResult, Python}; use crate::{ ffi::PyTypeObject, - pycell::PyCellContents, + pycell::{PyCellContents, PyClassMutability, PyClassMutabilityStorage}, type_object::{get_tp_alloc, PyTypeInfo}, }; use std::{ @@ -132,17 +131,14 @@ impl PyObjectInit for PyNativeTypeInitializer { /// ``` pub struct PyClassInitializer { init: T, - super_init: >::Initializer, + super_init: ::Initializer, } impl PyClassInitializer { /// Constructs a new initializer from value `T` and base class' initializer. /// /// It is recommended to use `add_subclass` instead of this method for most usage. - pub fn new( - init: T, - super_init: >::Initializer, - ) -> Self { + pub fn new(init: T, super_init: ::Initializer) -> Self { Self { init, super_init } } @@ -195,7 +191,7 @@ impl PyClassInitializer { pub fn add_subclass(self, subclass_value: S) -> PyClassInitializer where S: PyClass, - S::BaseType: PyClassBaseType, + S::BaseType: PyClassBaseType, { PyClassInitializer::new(subclass_value, self) } @@ -233,35 +229,23 @@ impl PyObjectInit for PyClassInitializer { py: Python<'_>, subtype: *mut PyTypeObject, ) -> PyResult<*mut ffi::PyObject> { - /// Layout of a PyCellBase after base new has been called, but the borrow flag has not - /// yet been initialized. - #[repr(C)] - struct PartiallyInitializedPyCellBase { - _ob_base: T, - borrow_flag: MaybeUninit, - } - /// Layout of a PyCell after base new has been called, but the contents have not yet been /// written. #[repr(C)] struct PartiallyInitializedPyCell { - _ob_base: >::LayoutAsBase, + _ob_base: ::LayoutAsBase, contents: MaybeUninit>, } let Self { init, super_init } = self; let obj = super_init.into_new_object(py, subtype)?; - // FIXME: Only need to initialize borrow flag once per whole hierarchy - let base: *mut PartiallyInitializedPyCellBase = obj as _; - std::ptr::write((*base).borrow_flag.as_mut_ptr(), T::Mutability::new()); - - // FIXME: Initialize borrow flag if necessary?? let cell: *mut PartiallyInitializedPyCell = obj as _; std::ptr::write( (*cell).contents.as_mut_ptr(), PyCellContents { value: ManuallyDrop::new(UnsafeCell::new(init)), + borrow_checker: ::Storage::new(), thread_checker: T::ThreadChecker::new(), dict: T::Dict::new(), weakref: T::WeakRef::new(), @@ -276,7 +260,7 @@ impl PyObjectInit for PyClassInitializer { impl From for PyClassInitializer where T: PyClass, - T::BaseType: PyClassBaseType>, + T::BaseType: PyClassBaseType>, { #[inline] fn from(value: T) -> PyClassInitializer { @@ -288,7 +272,7 @@ impl From<(S, B)> for PyClassInitializer where S: MutablePyClass, B: MutablePyClass, - B::BaseType: PyClassBaseType>, + B::BaseType: PyClassBaseType>, { fn from(sub_and_base: (S, B)) -> PyClassInitializer { let (sub, base) = sub_and_base; diff --git a/src/types/mod.rs b/src/types/mod.rs index a596f0af..f0419cbe 100644 --- a/src/types/mod.rs +++ b/src/types/mod.rs @@ -192,11 +192,12 @@ macro_rules! pyobject_native_type_sized { ($name:ty, $layout:path $(;$generics:ident)*) => { unsafe impl $crate::type_object::PyLayout<$name> for $layout {} impl $crate::type_object::PySizedLayout<$name> for $layout {} - impl $crate::impl_::pyclass::PyClassBaseType for $name { - type LayoutAsBase = $crate::pycell::PyCellBase<$layout, M>; + impl<$($generics,)*> $crate::impl_::pyclass::PyClassBaseType for $name { + type LayoutAsBase = $crate::pycell::PyCellBase<$layout>; type BaseNativeType = $name; type ThreadChecker = $crate::impl_::pyclass::ThreadCheckerStub<$crate::PyObject>; type Initializer = $crate::pyclass_init::PyNativeTypeInitializer; + type PyClassMutability = $crate::pycell::ImmutableClass; } } } diff --git a/tests/test_compile_error.rs b/tests/test_compile_error.rs index 9e69e2a8..713c5092 100644 --- a/tests/test_compile_error.rs +++ b/tests/test_compile_error.rs @@ -28,7 +28,6 @@ fn test_compile_errors() { #[cfg(not(feature = "nightly"))] fn _test_compile_errors() { let t = trybuild::TestCases::new(); - t.compile_fail("tests/ui/invalid_immutable_pyclass_borrow.rs"); t.compile_fail("tests/ui/invalid_macro_args.rs"); t.compile_fail("tests/ui/invalid_need_module_arg_position.rs"); @@ -41,8 +40,6 @@ fn _test_compile_errors() { t.compile_fail("tests/ui/invalid_pymethod_names.rs"); t.compile_fail("tests/ui/invalid_pymodule_args.rs"); t.compile_fail("tests/ui/reject_generics.rs"); - t.compile_fail("tests/ui/invalid_immutable_pyclass_borrow.rs"); - t.compile_fail("tests/ui/wrong_aspyref_lifetimes.rs"); t.compile_fail("tests/ui/invalid_pymethod_proto_args.rs"); t.compile_fail("tests/ui/invalid_pymethod_proto_args_py.rs"); @@ -97,6 +94,7 @@ fn _test_compile_errors() { #[rustversion::since(1.60)] fn tests_rust_1_60(t: &trybuild::TestCases) { + t.compile_fail("tests/ui/invalid_immutable_pyclass_borrow.rs"); t.compile_fail("tests/ui/invalid_pymethod_receiver.rs"); } diff --git a/tests/test_mutable_pyclass.rs b/tests/test_mutable_pyclass.rs new file mode 100644 index 00000000..8d5072e5 --- /dev/null +++ b/tests/test_mutable_pyclass.rs @@ -0,0 +1,201 @@ +#![cfg(feature = "macros")] + +use pyo3::impl_::pyclass::{PyClassBaseType, PyClassImpl}; +use pyo3::prelude::*; +use pyo3::pycell::{ + BorrowChecker, ExtendsMutableAncestor, ImmutableClass, MutableClass, PyClassMutability, +}; +use pyo3::PyClass; + +#[pyclass(subclass)] +struct MutableBase; + +#[pyclass(extends = MutableBase, subclass)] +struct MutableChildOfMutableBase; + +#[pyclass(extends = MutableBase, immutable, subclass)] +struct ImmutableChildOfMutableBase; + +#[pyclass(extends = MutableChildOfMutableBase)] +struct MutableChildOfMutableChildOfMutableBase; + +#[pyclass(extends = ImmutableChildOfMutableBase)] +struct MutableChildOfImmutableChildOfMutableBase; + +#[pyclass(extends = MutableChildOfMutableBase, immutable)] +struct ImmutableChildOfMutableChildOfMutableBase; + +#[pyclass(extends = ImmutableChildOfMutableBase, immutable)] +struct ImmutableChildOfImmutableChildOfMutableBase; + +#[pyclass(immutable, subclass)] +struct ImmutableBase; + +#[pyclass(extends = ImmutableBase, subclass)] +struct MutableChildOfImmutableBase; + +#[pyclass(extends = ImmutableBase, immutable, subclass)] +struct ImmutableChildOfImmutableBase; + +#[pyclass(extends = MutableChildOfImmutableBase)] +struct MutableChildOfMutableChildOfImmutableBase; + +#[pyclass(extends = ImmutableChildOfImmutableBase)] +struct MutableChildOfImmutableChildOfImmutableBase; + +#[pyclass(extends = MutableChildOfImmutableBase, immutable)] +struct ImmutableChildOfMutableChildOfImmutableBase; + +#[pyclass(extends = ImmutableChildOfImmutableBase, immutable)] +struct ImmutableChildOfImmutableChildOfImmutableBase; + +fn assert_mutable>() {} +fn assert_immutable>() {} +fn assert_mutable_with_mutable_ancestor< + T: PyClass>, +>() +// These horrible bounds are necessary for Rust 1.48 but not newer versions +where + ::BaseType: PyClassImpl>, + <::BaseType as PyClassImpl>::PyClassMutability: + PyClassMutability, + ::BaseType: PyClassBaseType>, +{ +} +fn assert_immutable_with_mutable_ancestor< + T: PyClass>, +>() +// These horrible bounds are necessary for Rust 1.48 but not newer versions +where + ::BaseType: PyClassImpl>, + <::BaseType as PyClassImpl>::PyClassMutability: + PyClassMutability, + ::BaseType: PyClassBaseType>, +{ +} + +#[test] +fn test_inherited_mutability() { + // mutable base + assert_mutable::(); + + // children of mutable base have a mutable ancestor + assert_mutable_with_mutable_ancestor::(); + assert_immutable_with_mutable_ancestor::(); + + // grandchildren of mutable base have a mutable ancestor + assert_mutable_with_mutable_ancestor::(); + assert_mutable_with_mutable_ancestor::(); + assert_immutable_with_mutable_ancestor::(); + assert_immutable_with_mutable_ancestor::(); + + // immutable base and children + assert_immutable::(); + assert_immutable::(); + assert_immutable::(); + + // mutable children of immutable at any level are simply mutable + assert_mutable::(); + assert_mutable::(); + + // children of the mutable child display this property + assert_mutable_with_mutable_ancestor::(); + assert_immutable_with_mutable_ancestor::(); +} + +#[test] +fn test_mutable_borrow_prevents_further_borrows() { + Python::with_gil(|py| { + let mmm = Py::new( + py, + PyClassInitializer::from(MutableBase) + .add_subclass(MutableChildOfMutableBase) + .add_subclass(MutableChildOfMutableChildOfMutableBase), + ) + .unwrap(); + + let mmm_cell: &PyCell = mmm.as_ref(py); + + let mmm_refmut = mmm_cell.borrow_mut(); + + // Cannot take any other mutable or immutable borrows whilst the object is borrowed mutably + assert!(mmm_cell + .extract::>() + .is_err()); + assert!(mmm_cell + .extract::>() + .is_err()); + assert!(mmm_cell.extract::>().is_err()); + assert!(mmm_cell + .extract::>() + .is_err()); + assert!(mmm_cell + .extract::>() + .is_err()); + assert!(mmm_cell.extract::>().is_err()); + + // With the borrow dropped, all other borrow attempts will succeed + drop(mmm_refmut); + + assert!(mmm_cell + .extract::>() + .is_ok()); + assert!(mmm_cell + .extract::>() + .is_ok()); + assert!(mmm_cell.extract::>().is_ok()); + assert!(mmm_cell + .extract::>() + .is_ok()); + assert!(mmm_cell + .extract::>() + .is_ok()); + assert!(mmm_cell.extract::>().is_ok()); + }) +} + +#[test] +fn test_immutable_borrows_prevent_mutable_borrows() { + Python::with_gil(|py| { + let mmm = Py::new( + py, + PyClassInitializer::from(MutableBase) + .add_subclass(MutableChildOfMutableBase) + .add_subclass(MutableChildOfMutableChildOfMutableBase), + ) + .unwrap(); + + let mmm_cell: &PyCell = mmm.as_ref(py); + + let mmm_refmut = mmm_cell.borrow(); + + // Further immutable borrows are ok + assert!(mmm_cell + .extract::>() + .is_ok()); + assert!(mmm_cell + .extract::>() + .is_ok()); + assert!(mmm_cell.extract::>().is_ok()); + + // Further mutable borrows are not ok + assert!(mmm_cell + .extract::>() + .is_err()); + assert!(mmm_cell + .extract::>() + .is_err()); + assert!(mmm_cell.extract::>().is_err()); + + // With the borrow dropped, all mutable borrow attempts will succeed + drop(mmm_refmut); + + assert!(mmm_cell + .extract::>() + .is_ok()); + assert!(mmm_cell + .extract::>() + .is_ok()); + assert!(mmm_cell.extract::>().is_ok()); + }) +} diff --git a/tests/ui/abi3_nativetype_inheritance.stderr b/tests/ui/abi3_nativetype_inheritance.stderr index bd425a23..cf8c2218 100644 --- a/tests/ui/abi3_nativetype_inheritance.stderr +++ b/tests/ui/abi3_nativetype_inheritance.stderr @@ -1,24 +1,22 @@ -error[E0277]: the trait bound `PyDict: PyClass` is not satisfied - --> tests/ui/abi3_nativetype_inheritance.rs:5:1 - | -5 | #[pyclass(extends=PyDict)] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `PyClass` is not implemented for `PyDict` - | - = note: required because of the requirements on the impl of `MutablePyClass` for `PyDict` - = note: required because of the requirements on the impl of `PyClassBaseType` for `PyDict` - = note: this error originates in the attribute macro `pyclass` (in Nightly builds, run with -Z macro-backtrace for more info) - error[E0277]: the trait bound `PyDict: PyClass` is not satisfied --> tests/ui/abi3_nativetype_inheritance.rs:5:1 | 5 | #[pyclass(extends=PyDict)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `PyClass` is not implemented for `PyDict` | - = note: required because of the requirements on the impl of `MutablePyClass` for `PyDict` - = note: required because of the requirements on the impl of `PyClassBaseType` for `PyDict` + = note: required because of the requirements on the impl of `PyClassBaseType` for `PyDict` note: required by a bound in `ThreadCheckerInherited` --> src/impl_/pyclass.rs | - | pub struct ThreadCheckerInherited>( - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `ThreadCheckerInherited` + | pub struct ThreadCheckerInherited( + | ^^^^^^^^^^^^^^^ required by this bound in `ThreadCheckerInherited` = note: this error originates in the attribute macro `pyclass` (in Nightly builds, run with -Z macro-backtrace for more info) + +error[E0277]: the trait bound `PyDict: PyClass` is not satisfied + --> tests/ui/abi3_nativetype_inheritance.rs:5:1 + | +5 | #[pyclass(extends=PyDict)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `PyClass` is not implemented for `PyDict` + | + = note: required because of the requirements on the impl of `PyClassBaseType` for `PyDict` + = note: this error originates in the attribute macro `pyclass` (in Nightly builds, run with -Z macro-backtrace for more info) diff --git a/tests/ui/invalid_immutable_pyclass_borrow.rs b/tests/ui/invalid_immutable_pyclass_borrow.rs index d020fbec..a50e7ffd 100644 --- a/tests/ui/invalid_immutable_pyclass_borrow.rs +++ b/tests/ui/invalid_immutable_pyclass_borrow.rs @@ -10,4 +10,14 @@ fn borrow_mut_fails(foo: Py, py: Python){ let borrow = foo.as_ref(py).borrow_mut(); } -fn main(){} \ No newline at end of file +#[pyclass(subclass)] +struct MutableBase; + +#[pyclass(immutable, extends = MutableBase)] +struct ImmutableChild; + +fn borrow_mut_of_child_fails(child: Py, py: Python){ + let borrow = child.as_ref(py).borrow_mut(); +} + +fn main(){} diff --git a/tests/ui/invalid_immutable_pyclass_borrow.stderr b/tests/ui/invalid_immutable_pyclass_borrow.stderr index 49290887..2b0653a9 100644 --- a/tests/ui/invalid_immutable_pyclass_borrow.stderr +++ b/tests/ui/invalid_immutable_pyclass_borrow.stderr @@ -10,3 +10,16 @@ note: required by a bound in `PyCell::::borrow_mut` | | T: MutablePyClass, | ^^^^^^^^^^^^^^ required by this bound in `PyCell::::borrow_mut` + +error[E0271]: type mismatch resolving `::Mutability == Mutable` + --> tests/ui/invalid_immutable_pyclass_borrow.rs:20:35 + | +20 | let borrow = child.as_ref(py).borrow_mut(); + | ^^^^^^^^^^ expected struct `Mutable`, found struct `Immutable` + | + = note: required because of the requirements on the impl of `MutablePyClass` for `ImmutableChild` +note: required by a bound in `PyCell::::borrow_mut` + --> src/pycell.rs + | + | T: MutablePyClass, + | ^^^^^^^^^^^^^^ required by this bound in `PyCell::::borrow_mut`