From 31c7af2ac741c2406b8bd00b1889685deb0282ff Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Mon, 1 Mar 2021 23:07:54 +0000 Subject: [PATCH 1/2] pyclass: move flags to PyClassImpl --- CHANGELOG.md | 1 + guide/src/class.md | 15 ++++--- pyo3-macros-backend/src/pyclass.rs | 70 ++++++++++++++---------------- src/class/impl_.rs | 16 ++++++- src/freelist.rs | 5 ++- src/lib.rs | 2 +- src/pyclass.rs | 14 +++--- src/type_object.rs | 28 ------------ src/types/mod.rs | 1 - 9 files changed, 67 insertions(+), 85 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 290d0378219..3c6756ddd69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - `PyMethodDef_INIT` [#1426](https://github.com/PyO3/pyo3/pull/1426) - `PyTypeObject_INIT` [#1429](https://github.com/PyO3/pyo3/pull/1429) - `PyObject_Check`, `PySuper_Check`, and `FreeFunc` [#1438](https://github.com/PyO3/pyo3/pull/1438) +- Remove pyclass implementation details `Type`, `DESCRIPTION`, and `FLAGS` from `PyTypeInfo`. [#1456](https://github.com/PyO3/pyo3/pull/1456) ### Fixed - Remove FFI definition `PyCFunction_ClearFreeList` for Python 3.9 and later. [#1425](https://github.com/PyO3/pyo3/pull/1425) diff --git a/guide/src/class.md b/guide/src/class.md index 0ada756458d..4e3cae661cb 100644 --- a/guide/src/class.md +++ b/guide/src/class.md @@ -726,7 +726,6 @@ struct MyClass { impl pyo3::pyclass::PyClassAlloc for MyClass {} unsafe impl pyo3::PyTypeInfo for MyClass { - type Type = MyClass; type BaseType = PyAny; type BaseLayout = pyo3::pycell::PyCellBase; type Layout = PyCell; @@ -735,8 +734,6 @@ unsafe impl pyo3::PyTypeInfo for MyClass { const NAME: &'static str = "MyClass"; const MODULE: Option<&'static str> = None; - const DESCRIPTION: &'static str = "Class for demonstration"; - const FLAGS: usize = 0; #[inline] fn type_object_raw(py: pyo3::Python) -> *mut pyo3::ffi::PyTypeObject { @@ -759,6 +756,10 @@ impl pyo3::IntoPy for MyClass { } impl pyo3::class::impl_::PyClassImpl for MyClass { + const DESCRIPTION: &'static str = "Class for demonstration"; + const IS_GC: bool = false; + const IS_BASETYPE: bool = false; + const IS_SUBCLASS: bool = false; type ThreadChecker = pyo3::class::impl_::ThreadCheckerStub; fn for_each_method_def(visitor: impl FnMut(&pyo3::class::PyMethodDefType)) { @@ -776,18 +777,18 @@ impl pyo3::class::impl_::PyClassImpl for MyClass { } fn get_new() -> Option { use pyo3::class::impl_::*; - let collector = PyClassImplCollector::::new(); + let collector = PyClassImplCollector::::new(); collector.new_impl() } fn get_call() -> Option { use pyo3::class::impl_::*; - let collector = PyClassImplCollector::::new(); + let collector = PyClassImplCollector::::new(); collector.call_impl() } fn for_each_proto_slot(visitor: impl FnMut(&pyo3::ffi::PyType_Slot)) { // Implementation which uses dtolnay specialization to load all slots. use pyo3::class::impl_::*; - let collector = PyClassImplCollector::::new(); + let collector = PyClassImplCollector::::new(); collector.object_protocol_slots() .iter() .chain(collector.number_protocol_slots()) @@ -803,7 +804,7 @@ impl pyo3::class::impl_::PyClassImpl for MyClass { fn get_buffer() -> Option<&'static pyo3::class::impl_::PyBufferProcs> { use pyo3::class::impl_::*; - let collector = PyClassImplCollector::::new(); + let collector = PyClassImplCollector::::new(); collector.buffer_procs() } } diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index ca150948f21..28119f42c04 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -17,8 +17,11 @@ use syn::{parse_quote, spanned::Spanned, Expr, Token}; pub struct PyClassArgs { pub freelist: Option, pub name: Option, - pub flags: Vec, pub base: syn::TypePath, + pub has_dict: bool, + pub has_weaklist: bool, + pub is_gc: bool, + pub is_basetype: bool, pub has_extends: bool, pub has_unsendable: bool, pub module: Option, @@ -42,10 +45,11 @@ impl Default for PyClassArgs { freelist: None, name: None, module: None, - // We need the 0 as value for the constant we're later building using quote for when there - // are no other flags - flags: vec![parse_quote! { 0 }], base: parse_quote! { pyo3::PyAny }, + has_dict: false, + has_weaklist: false, + is_gc: false, + is_basetype: false, has_extends: false, has_unsendable: false, } @@ -136,14 +140,19 @@ impl PyClassArgs { /// Match a single flag fn add_path(&mut self, exp: &syn::ExprPath) -> syn::Result<()> { let flag = exp.path.segments.first().unwrap().ident.to_string(); - let mut push_flag = |flag| { - self.flags.push(syn::Expr::Path(flag)); - }; match flag.as_str() { - "gc" => push_flag(parse_quote! {pyo3::type_flags::GC}), - "weakref" => push_flag(parse_quote! {pyo3::type_flags::WEAKREF}), - "subclass" => push_flag(parse_quote! {pyo3::type_flags::BASETYPE}), - "dict" => push_flag(parse_quote! {pyo3::type_flags::DICT}), + "gc" => { + self.is_gc = true; + } + "weakref" => { + self.has_weaklist = true; + } + "subclass" => { + self.is_basetype = true; + } + "dict" => { + self.has_dict = true; + } "unsendable" => { self.has_unsendable = true; } @@ -293,29 +302,14 @@ fn impl_class( }; // insert space for weak ref - let mut has_weakref = false; - let mut has_dict = false; - let mut has_gc = false; - for f in attr.flags.iter() { - if let syn::Expr::Path(epath) = f { - if epath.path == parse_quote! { pyo3::type_flags::WEAKREF } { - has_weakref = true; - } else if epath.path == parse_quote! { pyo3::type_flags::DICT } { - has_dict = true; - } else if epath.path == parse_quote! { pyo3::type_flags::GC } { - has_gc = true; - } - } - } - - let weakref = if has_weakref { + let weakref = if attr.has_weaklist { quote! { pyo3::pyclass_slots::PyClassWeakRefSlot } } else if attr.has_extends { quote! { ::WeakRef } } else { quote! { pyo3::pyclass_slots::PyClassDummySlot } }; - let dict = if has_dict { + let dict = if attr.has_dict { quote! { pyo3::pyclass_slots::PyClassDictSlot } } else if attr.has_extends { quote! { ::Dict } @@ -329,7 +323,7 @@ fn impl_class( }; // Enforce at compile time that PyGCProtocol is implemented - let gc_impl = if has_gc { + let gc_impl = if attr.is_gc { let closure_name = format!("__assertion_closure_{}", cls); let closure_token = syn::Ident::new(&closure_name, Span::call_site()); quote! { @@ -357,12 +351,6 @@ fn impl_class( }; let base = &attr.base; - let flags = &attr.flags; - let extended = if attr.has_extends { - quote! { pyo3::type_flags::EXTENDED } - } else { - quote! { 0 } - }; let base_layout = if attr.has_extends { quote! { ::LayoutAsBase } } else { @@ -397,9 +385,12 @@ fn impl_class( quote! { pyo3::class::impl_::ThreadCheckerStub<#cls> } }; + let is_gc = attr.is_gc; + let is_basetype = attr.is_basetype; + let is_subclass = attr.has_extends; + Ok(quote! { unsafe impl pyo3::type_object::PyTypeInfo for #cls { - type Type = #cls; type BaseType = #base; type Layout = pyo3::PyCell; type BaseLayout = #base_layout; @@ -408,8 +399,6 @@ fn impl_class( const NAME: &'static str = #cls_name; const MODULE: Option<&'static str> = #module; - const DESCRIPTION: &'static str = #doc; - const FLAGS: usize = #(#flags)|* | #extended; #[inline] fn type_object_raw(py: pyo3::Python) -> *mut pyo3::ffi::PyTypeObject { @@ -440,6 +429,11 @@ fn impl_class( #impl_inventory impl pyo3::class::impl_::PyClassImpl for #cls { + const DESCRIPTION: &'static str = #doc; + const IS_GC: bool = #is_gc; + const IS_BASETYPE: bool = #is_basetype; + const IS_SUBCLASS: bool = #is_subclass; + type ThreadChecker = #thread_checker; fn for_each_method_def(visitor: impl FnMut(&pyo3::class::PyMethodDefType)) { diff --git a/src/class/impl_.rs b/src/class/impl_.rs index 3d4b7aca177..b417decadb8 100644 --- a/src/class/impl_.rs +++ b/src/class/impl_.rs @@ -1,6 +1,6 @@ // Copyright (c) 2017-present PyO3 Project and Contributors -use crate::{derive_utils::PyBaseTypeUtils, ffi, PyMethodDefType, PyNativeType}; +use crate::{derive_utils::PyBaseTypeUtils, ffi, PyMethodDefType, PyNativeType, PyTypeInfo}; use std::{marker::PhantomData, thread}; /// This type is used as a "dummy" type on which dtolnay specializations are @@ -31,7 +31,19 @@ impl Copy for PyClassImplCollector {} /// /// Users are discouraged from implementing this trait manually; it is a PyO3 implementation detail /// and may be changed at any time. -pub trait PyClassImpl: Sized { +pub trait PyClassImpl: Sized + PyTypeInfo { + /// Class doc string + const DESCRIPTION: &'static str = "\0"; + + /// #[pyclass(gc)] + const IS_GC: bool = false; + + /// #[pyclass(subclass)] + const IS_BASETYPE: bool = false; + + /// #[pyclass(extends=...)] + const IS_SUBCLASS: bool = false; + /// 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`. diff --git a/src/freelist.rs b/src/freelist.rs index 2c0870c0c18..0d508469862 100644 --- a/src/freelist.rs +++ b/src/freelist.rs @@ -2,8 +2,9 @@ //! Free allocation list +use crate::class::impl_::PyClassImpl; use crate::pyclass::{get_type_free, tp_free_fallback, PyClassAlloc}; -use crate::type_object::{PyLayout, PyTypeInfo}; +use crate::type_object::PyLayout; use crate::{ffi, AsPyPointer, FromPyPointer, PyAny, Python}; use std::mem; use std::os::raw::c_void; @@ -69,7 +70,7 @@ impl FreeList { impl PyClassAlloc for T where - T: PyTypeInfo + PyClassWithFreeList, + T: PyClassImpl + PyClassWithFreeList, { unsafe fn new(py: Python, subtype: *mut ffi::PyTypeObject) -> *mut Self::Layout { // if subtype is not equal to this type, we cannot use the freelist diff --git a/src/lib.rs b/src/lib.rs index 9b7bce50c0d..d103dad4d3c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -159,7 +159,7 @@ pub use crate::pycell::{PyCell, PyRef, PyRefMut}; pub use crate::pyclass::PyClass; pub use crate::pyclass_init::PyClassInitializer; pub use crate::python::{Python, PythonVersionInfo}; -pub use crate::type_object::{type_flags, PyTypeInfo}; +pub use crate::type_object::PyTypeInfo; // Since PyAny is as important as PyObject, we expose it to the top level. pub use crate::types::PyAny; diff --git a/src/pyclass.rs b/src/pyclass.rs index f613b1a1c84..b9ff1807e19 100644 --- a/src/pyclass.rs +++ b/src/pyclass.rs @@ -2,7 +2,7 @@ use crate::class::impl_::PyClassImpl; use crate::class::methods::PyMethodDefType; use crate::pyclass_slots::{PyClassDict, PyClassWeakRef}; -use crate::type_object::{type_flags, PyLayout}; +use crate::type_object::PyLayout; use crate::{ffi, PyCell, PyErr, PyNativeType, PyResult, PyTypeInfo, Python}; use std::convert::TryInto; use std::ffi::CString; @@ -40,12 +40,12 @@ pub(crate) unsafe fn bpo_35810_workaround(_py: Python, ty: *mut ffi::PyTypeObjec } #[inline] -pub(crate) unsafe fn default_new( +pub(crate) unsafe fn default_new( py: Python, subtype: *mut ffi::PyTypeObject, ) -> *mut ffi::PyObject { // if the class derives native types(e.g., PyDict), call special new - if T::FLAGS & type_flags::EXTENDED != 0 && T::BaseLayout::IS_NATIVE_TYPE { + if T::IS_SUBCLASS && T::BaseLayout::IS_NATIVE_TYPE { #[cfg(not(Py_LIMITED_API))] { let base_tp = T::BaseType::type_object_raw(py); @@ -70,7 +70,7 @@ pub(crate) unsafe fn default_new( } /// This trait enables custom `tp_new`/`tp_dealloc` implementations for `T: PyClass`. -pub trait PyClassAlloc: PyTypeInfo + Sized { +pub trait PyClassAlloc: PyClassImpl { /// Allocate the actual field for `#[pyclass]`. /// /// # Safety @@ -153,6 +153,7 @@ pub trait PyClass: struct TypeSlots(Vec); impl TypeSlots { + #[inline] fn push(&mut self, slot: c_int, pfunc: *mut c_void) { self.0.push(ffi::PyType_Slot { slot, pfunc }); } @@ -174,6 +175,7 @@ fn get_type_name(module_name: Option<&str>) -> PyResult<*mut c_ch }) } +#[inline] fn into_raw(vec: Vec) -> *mut c_void { Box::into_raw(vec.into_boxed_slice()) as _ } @@ -300,12 +302,12 @@ fn tp_init_additional(type_object: *mut ffi::PyTypeObject) { fn tp_init_additional(_type_object: *mut ffi::PyTypeObject) {} fn py_class_flags(has_gc_methods: bool) -> c_uint { - let mut flags = if has_gc_methods || T::FLAGS & type_flags::GC != 0 { + let mut flags = if has_gc_methods || T::IS_GC { ffi::Py_TPFLAGS_DEFAULT | ffi::Py_TPFLAGS_HAVE_GC } else { ffi::Py_TPFLAGS_DEFAULT }; - if T::FLAGS & type_flags::BASETYPE != 0 { + if T::IS_BASETYPE { flags |= ffi::Py_TPFLAGS_BASETYPE; } flags.try_into().unwrap() diff --git a/src/type_object.rs b/src/type_object.rs index 4db1aba8ef3..bf8c43e3775 100644 --- a/src/type_object.rs +++ b/src/type_object.rs @@ -46,25 +46,6 @@ pub trait PySizedLayout: PyLayout + Sized {} /// Otherwise, implementing this trait is undefined behavior. pub unsafe trait PyBorrowFlagLayout: PyLayout + Sized {} -/// Our custom type flags -#[doc(hidden)] -pub mod type_flags { - /// Type object supports Python GC - pub const GC: usize = 1; - - /// Type object supports Python weak references - pub const WEAKREF: usize = 1 << 1; - - /// Type object can be used as the base type of another type - pub const BASETYPE: usize = 1 << 2; - - /// The instances of this type have a dictionary containing instance variables - pub const DICT: usize = 1 << 3; - - /// The class declared by #[pyclass(extends=~)] - pub const EXTENDED: usize = 1 << 4; -} - /// Python type information. /// All Python native types(e.g., `PyDict`) and `#[pyclass]` structs implement this trait. /// @@ -72,21 +53,12 @@ pub mod type_flags { /// - specifying the incorrect layout can lead to memory errors /// - the return value of type_object must always point to the same PyTypeObject instance pub unsafe trait PyTypeInfo: Sized { - /// Type of objects to store in PyObject struct - type Type; - /// Class name const NAME: &'static str; /// Module name, if any const MODULE: Option<&'static str>; - /// Class doc string - const DESCRIPTION: &'static str = "\0"; - - /// Type flags (ie PY_TYPE_FLAG_GC, PY_TYPE_FLAG_WEAKREF) - const FLAGS: usize = 0; - /// Base class type BaseType: PyTypeInfo + PyTypeObject; diff --git a/src/types/mod.rs b/src/types/mod.rs index c62557882ca..aad615ced8e 100644 --- a/src/types/mod.rs +++ b/src/types/mod.rs @@ -197,7 +197,6 @@ macro_rules! pyobject_native_type_info( ($name: ty, $layout: path, $typeobject: expr, $module: expr $(, $checkfunction:path)? $(;$generics: ident)*) => { unsafe impl<$($generics,)*> $crate::type_object::PyTypeInfo for $name { - type Type = (); type BaseType = $crate::PyAny; type Layout = $layout; type BaseLayout = $crate::ffi::PyObject; From d9fe404d69e10bb7a141c7e9c874c70c26db1ad3 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Sun, 14 Mar 2021 00:50:59 +0000 Subject: [PATCH 2/2] [review] kngwyu --- guide/src/class.md | 2 +- pyo3-macros-backend/src/pyclass.rs | 2 +- src/class/impl_.rs | 6 +++--- src/freelist.rs | 4 ++-- src/pyclass.rs | 14 ++++++-------- 5 files changed, 13 insertions(+), 15 deletions(-) diff --git a/guide/src/class.md b/guide/src/class.md index 4e3cae661cb..a4a18c7c93b 100644 --- a/guide/src/class.md +++ b/guide/src/class.md @@ -756,7 +756,7 @@ impl pyo3::IntoPy for MyClass { } impl pyo3::class::impl_::PyClassImpl for MyClass { - const DESCRIPTION: &'static str = "Class for demonstration"; + const DOC: &'static str = "Class for demonstration"; const IS_GC: bool = false; const IS_BASETYPE: bool = false; const IS_SUBCLASS: bool = false; diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index 28119f42c04..cd9b21a99e3 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -429,7 +429,7 @@ fn impl_class( #impl_inventory impl pyo3::class::impl_::PyClassImpl for #cls { - const DESCRIPTION: &'static str = #doc; + const DOC: &'static str = #doc; const IS_GC: bool = #is_gc; const IS_BASETYPE: bool = #is_basetype; const IS_SUBCLASS: bool = #is_subclass; diff --git a/src/class/impl_.rs b/src/class/impl_.rs index b417decadb8..a5763453d56 100644 --- a/src/class/impl_.rs +++ b/src/class/impl_.rs @@ -1,6 +1,6 @@ // Copyright (c) 2017-present PyO3 Project and Contributors -use crate::{derive_utils::PyBaseTypeUtils, ffi, PyMethodDefType, PyNativeType, PyTypeInfo}; +use crate::{derive_utils::PyBaseTypeUtils, ffi, PyMethodDefType, PyNativeType}; use std::{marker::PhantomData, thread}; /// This type is used as a "dummy" type on which dtolnay specializations are @@ -31,9 +31,9 @@ impl Copy for PyClassImplCollector {} /// /// Users are discouraged from implementing this trait manually; it is a PyO3 implementation detail /// and may be changed at any time. -pub trait PyClassImpl: Sized + PyTypeInfo { +pub trait PyClassImpl: Sized { /// Class doc string - const DESCRIPTION: &'static str = "\0"; + const DOC: &'static str = "\0"; /// #[pyclass(gc)] const IS_GC: bool = false; diff --git a/src/freelist.rs b/src/freelist.rs index 0d508469862..6da714541c0 100644 --- a/src/freelist.rs +++ b/src/freelist.rs @@ -4,7 +4,7 @@ use crate::class::impl_::PyClassImpl; use crate::pyclass::{get_type_free, tp_free_fallback, PyClassAlloc}; -use crate::type_object::PyLayout; +use crate::type_object::{PyLayout, PyTypeInfo}; use crate::{ffi, AsPyPointer, FromPyPointer, PyAny, Python}; use std::mem; use std::os::raw::c_void; @@ -70,7 +70,7 @@ impl FreeList { impl PyClassAlloc for T where - T: PyClassImpl + PyClassWithFreeList, + T: PyTypeInfo + PyClassImpl + PyClassWithFreeList, { unsafe fn new(py: Python, subtype: *mut ffi::PyTypeObject) -> *mut Self::Layout { // if subtype is not equal to this type, we cannot use the freelist diff --git a/src/pyclass.rs b/src/pyclass.rs index b9ff1807e19..b1eab8144d4 100644 --- a/src/pyclass.rs +++ b/src/pyclass.rs @@ -40,7 +40,7 @@ pub(crate) unsafe fn bpo_35810_workaround(_py: Python, ty: *mut ffi::PyTypeObjec } #[inline] -pub(crate) unsafe fn default_new( +pub(crate) unsafe fn default_new( py: Python, subtype: *mut ffi::PyTypeObject, ) -> *mut ffi::PyObject { @@ -70,7 +70,7 @@ pub(crate) unsafe fn default_new( } /// This trait enables custom `tp_new`/`tp_dealloc` implementations for `T: PyClass`. -pub trait PyClassAlloc: PyClassImpl { +pub trait PyClassAlloc: PyTypeInfo + PyClassImpl { /// Allocate the actual field for `#[pyclass]`. /// /// # Safety @@ -153,14 +153,13 @@ pub trait PyClass: struct TypeSlots(Vec); impl TypeSlots { - #[inline] fn push(&mut self, slot: c_int, pfunc: *mut c_void) { self.0.push(ffi::PyType_Slot { slot, pfunc }); } } fn tp_doc() -> PyResult> { - Ok(match T::DESCRIPTION { + Ok(match T::DOC { "\0" => None, s if s.as_bytes().ends_with(b"\0") => Some(s.as_ptr() as _), // If the description is not null-terminated, create CString and leak it @@ -175,7 +174,6 @@ fn get_type_name(module_name: Option<&str>) -> PyResult<*mut c_ch }) } -#[inline] fn into_raw(vec: Vec) -> *mut c_void { Box::into_raw(vec.into_boxed_slice()) as _ } @@ -255,15 +253,15 @@ fn tp_init_additional(type_object: *mut ffi::PyTypeObject) { // Running this causes PyPy to segfault. #[cfg(all(not(PyPy), not(Py_3_10)))] { - if T::DESCRIPTION != "\0" { + if T::DOC != "\0" { unsafe { // Until CPython 3.10, tp_doc was treated specially for // heap-types, and it removed the text_signature value from it. // We go in after the fact and replace tp_doc with something // that _does_ include the text_signature value! ffi::PyObject_Free((*type_object).tp_doc as _); - let data = ffi::PyObject_Malloc(T::DESCRIPTION.len()); - data.copy_from(T::DESCRIPTION.as_ptr() as _, T::DESCRIPTION.len()); + let data = ffi::PyObject_Malloc(T::DOC.len()); + data.copy_from(T::DOC.as_ptr() as _, T::DOC.len()); (*type_object).tp_doc = data as _; } }