From ddfbee4b3c652aa95b0d5eb587477229c446eb02 Mon Sep 17 00:00:00 2001 From: kngwyu Date: Wed, 9 Sep 2020 17:26:54 +0900 Subject: [PATCH 1/8] Add null-check for function's documents --- pyo3-derive-backend/src/pyproto.rs | 2 +- src/class/methods.rs | 32 ++++++++++++++++++------------ 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/pyo3-derive-backend/src/pyproto.rs b/pyo3-derive-backend/src/pyproto.rs index c9bebbe08d4..67976f5901d 100644 --- a/pyo3-derive-backend/src/pyproto.rs +++ b/pyo3-derive-backend/src/pyproto.rs @@ -99,7 +99,7 @@ fn impl_proto_impl( ml_name: stringify!(#name), ml_meth: pyo3::class::PyMethodType::PyCFunctionWithKeywords(__wrap), ml_flags: pyo3::ffi::METH_VARARGS | pyo3::ffi::METH_KEYWORDS | #coexist, - ml_doc: "" + ml_doc: "\0" } }) }); diff --git a/src/class/methods.rs b/src/class/methods.rs index a34260d6c5c..173dfe02c18 100644 --- a/src/class/methods.rs +++ b/src/class/methods.rs @@ -2,7 +2,7 @@ use crate::{ffi, PyObject, Python}; use libc::c_int; -use std::ffi::CString; +use std::ffi::{CStr, CString}; use std::fmt; /// `PyMethodDefType` represents different types of Python callable objects. @@ -73,6 +73,18 @@ unsafe impl Sync for PySetterDef {} unsafe impl Sync for ffi::PyGetSetDef {} +fn get_name(name: &str) -> *const std::os::raw::c_char { + CString::new(name) + .expect("Method name must not contain NULL byte") + .into_raw() as _ +} + +fn get_doc(doc: &'static str) -> *const std::os::raw::c_char { + CStr::from_bytes_with_nul(doc.as_bytes()) + .expect("Document must be terminated with NULL byte") + .as_ptr() +} + impl PyMethodDef { /// Convert `PyMethodDef` to Python method definition struct `ffi::PyMethodDef` pub fn as_method_def(&self) -> ffi::PyMethodDef { @@ -84,12 +96,10 @@ impl PyMethodDef { }; ffi::PyMethodDef { - ml_name: CString::new(self.ml_name) - .expect("Method name must not contain NULL byte") - .into_raw(), + ml_name: get_name(self.ml_name), ml_meth: Some(meth), ml_flags: self.ml_flags, - ml_doc: self.ml_doc.as_ptr() as *const _, + ml_doc: get_doc(self.ml_doc), } } } @@ -108,12 +118,10 @@ impl PyGetterDef { /// Copy descriptor information to `ffi::PyGetSetDef` pub fn copy_to(&self, dst: &mut ffi::PyGetSetDef) { if dst.name.is_null() { - dst.name = CString::new(self.name) - .expect("Method name must not contain NULL byte") - .into_raw(); + dst.name = get_name(self.name) as _; } if dst.doc.is_null() { - dst.doc = self.doc.as_ptr() as *mut libc::c_char; + dst.doc = get_doc(self.doc) as _; } dst.get = Some(self.meth); } @@ -123,12 +131,10 @@ impl PySetterDef { /// Copy descriptor information to `ffi::PyGetSetDef` pub fn copy_to(&self, dst: &mut ffi::PyGetSetDef) { if dst.name.is_null() { - dst.name = CString::new(self.name) - .expect("Method name must not contain NULL byte") - .into_raw(); + dst.name = get_name(self.name) as _; } if dst.doc.is_null() { - dst.doc = self.doc.as_ptr() as *mut libc::c_char; + dst.doc = get_doc(self.doc) as _; } dst.set = Some(self.meth); } From 41c2f5a748b9ba5a1797a5ef26b992094e38a94c Mon Sep 17 00:00:00 2001 From: kngwyu Date: Thu, 8 Oct 2020 13:42:36 +0900 Subject: [PATCH 2/8] Use &'static CStr for representing method names and docs --- guide/src/class.md | 8 +- pyo3-derive-backend/src/pyclass.rs | 10 +- pyo3-derive-backend/src/pyimpl.rs | 2 +- pyo3-derive-backend/src/pymethod.rs | 87 ++++++---------- pyo3-derive-backend/src/pyproto.rs | 14 +-- src/class/methods.rs | 153 ++++++++++++++++++++++------ src/pyclass.rs | 30 +++--- src/type_object.rs | 13 +-- 8 files changed, 188 insertions(+), 129 deletions(-) diff --git a/guide/src/class.md b/guide/src/class.md index 3e8d8c820ce..e284efc4de3 100644 --- a/guide/src/class.md +++ b/guide/src/class.md @@ -754,14 +754,14 @@ impl pyo3::IntoPy for MyClass { } pub struct Pyo3MethodsInventoryForMyClass { - methods: &'static [pyo3::class::PyMethodDefType], + methods: Vec, } impl pyo3::class::methods::PyMethodsInventory for Pyo3MethodsInventoryForMyClass { - fn new(methods: &'static [pyo3::class::PyMethodDefType]) -> Self { + fn new(methods: Vec) -> Self { Self { methods } } - fn get(&self) -> &'static [pyo3::class::PyMethodDefType] { - self.methods + fn get(&'static self) -> &'static [pyo3::class::PyMethodDefType] { + &self.methods } } impl pyo3::class::methods::HasMethodsInventory for MyClass { diff --git a/pyo3-derive-backend/src/pyclass.rs b/pyo3-derive-backend/src/pyclass.rs index 693d5004e09..bc2fd66e05f 100644 --- a/pyo3-derive-backend/src/pyclass.rs +++ b/pyo3-derive-backend/src/pyclass.rs @@ -215,14 +215,14 @@ fn impl_methods_inventory(cls: &syn::Ident) -> TokenStream { quote! { #[doc(hidden)] pub struct #inventory_cls { - methods: &'static [pyo3::class::PyMethodDefType], + methods: Vec, } impl pyo3::class::methods::PyMethodsInventory for #inventory_cls { - fn new(methods: &'static [pyo3::class::PyMethodDefType]) -> Self { + fn new(methods: Vec) -> Self { Self { methods } } - fn get(&self) -> &'static [pyo3::class::PyMethodDefType] { - self.methods + fn get(&'static self) -> &'static [pyo3::class::PyMethodDefType] { + &self.methods } } @@ -483,7 +483,7 @@ fn impl_descriptors( pyo3::inventory::submit! { #![crate = pyo3] { type Inventory = <#cls as pyo3::class::methods::HasMethodsInventory>::Methods; - ::new(&[#(#py_methods),*]) + ::new(vec![#(#py_methods),*]) } } }) diff --git a/pyo3-derive-backend/src/pyimpl.rs b/pyo3-derive-backend/src/pyimpl.rs index 00999af526f..e59cb43fb87 100644 --- a/pyo3-derive-backend/src/pyimpl.rs +++ b/pyo3-derive-backend/src/pyimpl.rs @@ -43,7 +43,7 @@ pub fn impl_methods(ty: &syn::Type, impls: &mut Vec) -> syn::Resu pyo3::inventory::submit! { #![crate = pyo3] { type Inventory = <#ty as pyo3::class::methods::HasMethodsInventory>::Methods; - ::new(&[#( + ::new(vec![#( #(#cfg_attributes)* #methods ),*]) diff --git a/pyo3-derive-backend/src/pymethod.rs b/pyo3-derive-backend/src/pymethod.rs index 62ced447077..6afb66ef309 100644 --- a/pyo3-derive-backend/src/pymethod.rs +++ b/pyo3-derive-backend/src/pymethod.rs @@ -570,12 +570,11 @@ pub fn impl_py_method_def(spec: &FnSpec, wrapper: &TokenStream) -> TokenStream { pyo3::class::PyMethodDefType::Method({ #wrapper - pyo3::class::PyMethodDef { - ml_name: stringify!(#python_name), - ml_meth: pyo3::class::PyMethodType::PyCFunction(__wrap), - ml_flags: pyo3::ffi::METH_NOARGS, - ml_doc: #doc, - } + pyo3::class::PyMethodDef::cfunction( + concat!(stringify!(#python_name), "\0"), + __wrap, + #doc + ) }) } } else { @@ -583,12 +582,12 @@ pub fn impl_py_method_def(spec: &FnSpec, wrapper: &TokenStream) -> TokenStream { pyo3::class::PyMethodDefType::Method({ #wrapper - pyo3::class::PyMethodDef { - ml_name: stringify!(#python_name), - ml_meth: pyo3::class::PyMethodType::PyCFunctionWithKeywords(__wrap), - ml_flags: pyo3::ffi::METH_VARARGS | pyo3::ffi::METH_KEYWORDS, - ml_doc: #doc, - } + pyo3::class::PyMethodDef::cfunction_with_keywords( + concat!(stringify!(#python_name), "\0"), + __wrap, + 0, + #doc + ) }) } } @@ -601,12 +600,7 @@ pub fn impl_py_method_def_new(spec: &FnSpec, wrapper: &TokenStream) -> TokenStre pyo3::class::PyMethodDefType::New({ #wrapper - pyo3::class::PyMethodDef { - ml_name: stringify!(#python_name), - ml_meth: pyo3::class::PyMethodType::PyNewFunc(__wrap), - ml_flags: pyo3::ffi::METH_VARARGS | pyo3::ffi::METH_KEYWORDS, - ml_doc: #doc, - } + pyo3::class::PyMethodDef::new_func(concat!(stringify!(#python_name), "\0"), __wrap, #doc) }) } } @@ -618,13 +612,12 @@ pub fn impl_py_method_def_class(spec: &FnSpec, wrapper: &TokenStream) -> TokenSt pyo3::class::PyMethodDefType::Class({ #wrapper - pyo3::class::PyMethodDef { - ml_name: stringify!(#python_name), - ml_meth: pyo3::class::PyMethodType::PyCFunctionWithKeywords(__wrap), - ml_flags: pyo3::ffi::METH_VARARGS | pyo3::ffi::METH_KEYWORDS | + pyo3::class::PyMethodDef::cfunction_with_keywords( + concat!(stringify!(#python_name), "\0"), + __wrap, pyo3::ffi::METH_CLASS, - ml_doc: #doc, - } + #doc + ) }) } } @@ -636,12 +629,12 @@ pub fn impl_py_method_def_static(spec: &FnSpec, wrapper: &TokenStream) -> TokenS pyo3::class::PyMethodDefType::Static({ #wrapper - pyo3::class::PyMethodDef { - ml_name: stringify!(#python_name), - ml_meth: pyo3::class::PyMethodType::PyCFunctionWithKeywords(__wrap), - ml_flags: pyo3::ffi::METH_VARARGS | pyo3::ffi::METH_KEYWORDS | pyo3::ffi::METH_STATIC, - ml_doc: #doc, - } + pyo3::class::PyMethodDef::cfunction_with_keywords( + concat!(stringify!(#python_name), "\0"), + __wrap, + pyo3::ffi::METH_STATIC, + #doc + ) }) } } @@ -652,10 +645,7 @@ pub fn impl_py_method_class_attribute(spec: &FnSpec<'_>, wrapper: &TokenStream) pyo3::class::PyMethodDefType::ClassAttribute({ #wrapper - pyo3::class::PyClassAttributeDef { - name: stringify!(#python_name), - meth: __wrap, - } + pyo3::class::PyClassAttributeDef::new(concat!(stringify!(#python_name), "\0"), __wrap) }) } } @@ -666,10 +656,7 @@ pub fn impl_py_const_class_attribute(spec: &ConstSpec, wrapper: &TokenStream) -> pyo3::class::PyMethodDefType::ClassAttribute({ #wrapper - pyo3::class::PyClassAttributeDef { - name: stringify!(#python_name), - meth: __wrap, - } + pyo3::class::PyClassAttributeDef::new(concat!(stringify!(#python_name), "\0"), __wrap) }) } } @@ -681,12 +668,12 @@ pub fn impl_py_method_def_call(spec: &FnSpec, wrapper: &TokenStream) -> TokenStr pyo3::class::PyMethodDefType::Call({ #wrapper - pyo3::class::PyMethodDef { - ml_name: stringify!(#python_name), - ml_meth: pyo3::class::PyMethodType::PyCFunctionWithKeywords(__wrap), - ml_flags: pyo3::ffi::METH_VARARGS | pyo3::ffi::METH_KEYWORDS, - ml_doc: #doc, - } + pyo3::class::PyMethodDef::cfunction_with_keywords( + concat!(stringify!(#python_name), "\0"), + __wrap, + pyo3::ffi::METH_STATIC, + #doc + ) }) } } @@ -700,11 +687,7 @@ pub(crate) fn impl_py_setter_def( pyo3::class::PyMethodDefType::Setter({ #wrapper - pyo3::class::PySetterDef { - name: stringify!(#python_name), - meth: __wrap, - doc: #doc, - } + pyo3::class::PySetterDef::new(concat!(stringify!(#python_name), "\0"), __wrap, #doc) }) } } @@ -718,11 +701,7 @@ pub(crate) fn impl_py_getter_def( pyo3::class::PyMethodDefType::Getter({ #wrapper - pyo3::class::PyGetterDef { - name: stringify!(#python_name), - meth: __wrap, - doc: #doc, - } + pyo3::class::PyGetterDef::new(concat!(stringify!(#python_name), "\0"), __wrap, #doc) }) } } diff --git a/pyo3-derive-backend/src/pyproto.rs b/pyo3-derive-backend/src/pyproto.rs index 67976f5901d..7c6cbf1a7ae 100644 --- a/pyo3-derive-backend/src/pyproto.rs +++ b/pyo3-derive-backend/src/pyproto.rs @@ -95,12 +95,12 @@ fn impl_proto_impl( py_methods.push(quote! { pyo3::class::PyMethodDefType::Method({ #method - pyo3::class::PyMethodDef { - ml_name: stringify!(#name), - ml_meth: pyo3::class::PyMethodType::PyCFunctionWithKeywords(__wrap), - ml_flags: pyo3::ffi::METH_VARARGS | pyo3::ffi::METH_KEYWORDS | #coexist, - ml_doc: "\0" - } + pyo3::class::PyMethodDef::cfunction_with_keywords( + concat!(stringify!(#name), "\0"), + __wrap, + #coexist, + "\0" + ) }) }); } @@ -123,7 +123,7 @@ fn inventory_submission(py_methods: Vec, ty: &syn::Type) -> TokenSt pyo3::inventory::submit! { #![crate = pyo3] { type Inventory = <#ty as pyo3::class::methods::HasMethodsInventory>::Methods; - ::new(&[#(#py_methods),*]) + ::new(vec![#(#py_methods),*]) } } } diff --git a/src/class/methods.rs b/src/class/methods.rs index 173dfe02c18..4713d2acfaf 100644 --- a/src/class/methods.rs +++ b/src/class/methods.rs @@ -2,7 +2,7 @@ use crate::{ffi, PyObject, Python}; use libc::c_int; -use std::ffi::{CStr, CString}; +use std::ffi::CStr; use std::fmt; /// `PyMethodDefType` represents different types of Python callable objects. @@ -35,32 +35,33 @@ pub enum PyMethodType { PyInitFunc(ffi::initproc), } -#[derive(Copy, Clone, Debug)] +// TODO(kngwyu): We should also use &'static CStr for this? I'm not sure. +#[derive(Clone, Debug)] pub struct PyMethodDef { - pub ml_name: &'static str, - pub ml_meth: PyMethodType, - pub ml_flags: c_int, - pub ml_doc: &'static str, + ml_name: &'static CStr, + ml_meth: PyMethodType, + ml_flags: c_int, + ml_doc: &'static CStr, } #[derive(Copy, Clone)] pub struct PyClassAttributeDef { - pub name: &'static str, - pub meth: for<'p> fn(Python<'p>) -> PyObject, + pub(crate) name: &'static CStr, + pub(crate) meth: for<'p> fn(Python<'p>) -> PyObject, } -#[derive(Copy, Clone, Debug)] +#[derive(Clone, Debug)] pub struct PyGetterDef { - pub name: &'static str, - pub meth: ffi::getter, - pub doc: &'static str, + pub(crate) name: &'static CStr, + pub(crate) meth: ffi::getter, + doc: &'static CStr, } -#[derive(Copy, Clone, Debug)] +#[derive(Clone, Debug)] pub struct PySetterDef { - pub name: &'static str, - pub meth: ffi::setter, - pub doc: &'static str, + pub(crate) name: &'static CStr, + pub(crate) meth: ffi::setter, + doc: &'static CStr, } unsafe impl Sync for PyMethodDef {} @@ -73,19 +74,80 @@ unsafe impl Sync for PySetterDef {} unsafe impl Sync for ffi::PyGetSetDef {} -fn get_name(name: &str) -> *const std::os::raw::c_char { - CString::new(name) - .expect("Method name must not contain NULL byte") - .into_raw() as _ +fn get_name(name: &'static str) -> &'static CStr { + CStr::from_bytes_with_nul(name.as_bytes()) + .expect("Method name must be terminated with NULL byte") } -fn get_doc(doc: &'static str) -> *const std::os::raw::c_char { - CStr::from_bytes_with_nul(doc.as_bytes()) - .expect("Document must be terminated with NULL byte") - .as_ptr() +fn get_doc(doc: &'static str) -> &'static CStr { + CStr::from_bytes_with_nul(doc.as_bytes()).expect("Document must be terminated with NULL byte") } impl PyMethodDef { + pub(crate) fn get_new_func(&self) -> Option { + if let PyMethodType::PyNewFunc(new_func) = self.ml_meth { + Some(new_func) + } else { + None + } + } + + pub(crate) fn get_cfunction_with_keywords(&self) -> Option { + if let PyMethodType::PyCFunctionWithKeywords(func) = self.ml_meth { + Some(func) + } else { + None + } + } + + pub fn cfunction(name: &'static str, cfunction: ffi::PyCFunction, doc: &'static str) -> Self { + Self::new( + name, + PyMethodType::PyCFunction(cfunction), + ffi::METH_NOARGS, + doc, + ) + } + + pub fn new_func(name: &'static str, newfunc: ffi::newfunc, doc: &'static str) -> Self { + Self::new( + name, + PyMethodType::PyNewFunc(newfunc), + ffi::METH_VARARGS | ffi::METH_KEYWORDS, + doc, + ) + } + + /// Define a function that can take `**kwargs`. + pub fn cfunction_with_keywords( + name: &'static str, + cfunction: ffi::PyCFunctionWithKeywords, + flags: c_int, + doc: &'static str, + ) -> Self { + let flags = flags | ffi::METH_VARARGS | ffi::METH_KEYWORDS; + Self::new( + name, + PyMethodType::PyCFunctionWithKeywords(cfunction), + flags, + doc, + ) + } + + pub(crate) fn new( + name: &'static str, + methodtype: PyMethodType, + flags: c_int, + doc: &'static str, + ) -> Self { + Self { + ml_name: get_name(name), + ml_meth: methodtype, + ml_flags: flags, + ml_doc: get_doc(doc), + } + } + /// Convert `PyMethodDef` to Python method definition struct `ffi::PyMethodDef` pub fn as_method_def(&self) -> ffi::PyMethodDef { let meth = match self.ml_meth { @@ -96,10 +158,19 @@ impl PyMethodDef { }; ffi::PyMethodDef { - ml_name: get_name(self.ml_name), + ml_name: self.ml_name.as_ptr(), ml_meth: Some(meth), ml_flags: self.ml_flags, - ml_doc: get_doc(self.ml_doc), + ml_doc: self.ml_doc.as_ptr(), + } + } +} + +impl PyClassAttributeDef { + pub fn new(name: &'static str, meth: for<'p> fn(Python<'p>) -> PyObject) -> Self { + Self { + name: get_name(name), + meth, } } } @@ -115,26 +186,44 @@ impl fmt::Debug for PyClassAttributeDef { } impl PyGetterDef { + /// Define a getter. + pub fn new(name: &'static str, getter: ffi::getter, doc: &'static str) -> Self { + Self { + name: get_name(name), + meth: getter, + doc: get_doc(doc), + } + } + /// Copy descriptor information to `ffi::PyGetSetDef` pub fn copy_to(&self, dst: &mut ffi::PyGetSetDef) { if dst.name.is_null() { - dst.name = get_name(self.name) as _; + dst.name = self.name.as_ptr() as _; } if dst.doc.is_null() { - dst.doc = get_doc(self.doc) as _; + dst.doc = self.doc.as_ptr() as _; } dst.get = Some(self.meth); } } impl PySetterDef { + /// Define a setter. + pub fn new(name: &'static str, setter: ffi::setter, doc: &'static str) -> Self { + Self { + name: get_name(name), + meth: setter, + doc: get_doc(doc), + } + } + /// Copy descriptor information to `ffi::PyGetSetDef` pub fn copy_to(&self, dst: &mut ffi::PyGetSetDef) { if dst.name.is_null() { - dst.name = get_name(self.name) as _; + dst.name = self.name.as_ptr() as _; } if dst.doc.is_null() { - dst.doc = get_doc(self.doc) as _; + dst.doc = self.doc.as_ptr() as _; } dst.set = Some(self.meth); } @@ -156,10 +245,10 @@ pub trait PyMethods { #[cfg(feature = "macros")] pub trait PyMethodsInventory: inventory::Collect { /// Create a new instance - fn new(methods: &'static [PyMethodDefType]) -> Self; + fn new(methods: Vec) -> Self; /// Returns the methods for a single `#[pymethods] impl` block - fn get(&self) -> &'static [PyMethodDefType]; + fn get(&'static self) -> &'static [PyMethodDefType]; } /// Implemented for `#[pyclass]` in our proc macro code. diff --git a/src/pyclass.rs b/src/pyclass.rs index 52cc3910ca1..11e4060d64a 100644 --- a/src/pyclass.rs +++ b/src/pyclass.rs @@ -6,7 +6,7 @@ use crate::derive_utils::PyBaseTypeUtils; use crate::pyclass_slots::{PyClassDict, PyClassWeakRef}; use crate::type_object::{type_flags, PyLayout}; use crate::types::PyAny; -use crate::{class, ffi, PyCell, PyErr, PyNativeType, PyResult, PyTypeInfo, Python}; +use crate::{ffi, PyCell, PyErr, PyNativeType, PyResult, PyTypeInfo, Python}; use std::ffi::CString; use std::marker::PhantomData; use std::os::raw::c_void; @@ -239,7 +239,7 @@ fn py_class_flags(type_object: &mut ffi::PyTypeObject) { pub(crate) fn py_class_attributes() -> impl Iterator { T::py_methods().into_iter().filter_map(|def| match def { - PyMethodDefType::ClassAttribute(attr) => Some(*attr), + PyMethodDefType::ClassAttribute(attr) => Some(attr.to_owned()), _ => None, }) } @@ -256,16 +256,12 @@ fn py_class_method_defs() -> ( for def in T::py_methods() { match *def { PyMethodDefType::New(ref def) => { - if let class::methods::PyMethodType::PyNewFunc(meth) = def.ml_meth { - new = Some(meth) - } + new = def.get_new_func(); + debug_assert!(new.is_some()); } PyMethodDefType::Call(ref def) => { - if let class::methods::PyMethodType::PyCFunctionWithKeywords(meth) = def.ml_meth { - call = Some(meth) - } else { - panic!("Method type is not supoorted by tp_call slot") - } + call = def.get_cfunction_with_keywords(); + debug_assert!(call.is_some()); } PyMethodDefType::Method(ref def) | PyMethodDefType::Class(ref def) @@ -285,19 +281,17 @@ fn py_class_properties() -> Vec { for def in T::py_methods() { match *def { PyMethodDefType::Getter(ref getter) => { - let name = getter.name.to_string(); - if !defs.contains_key(&name) { - let _ = defs.insert(name.clone(), ffi::PyGetSetDef_INIT); + if !defs.contains_key(getter.name) { + let _ = defs.insert(getter.name.to_owned(), ffi::PyGetSetDef_INIT); } - let def = defs.get_mut(&name).expect("Failed to call get_mut"); + let def = defs.get_mut(getter.name).expect("Failed to call get_mut"); getter.copy_to(def); } PyMethodDefType::Setter(ref setter) => { - let name = setter.name.to_string(); - if !defs.contains_key(&name) { - let _ = defs.insert(name.clone(), ffi::PyGetSetDef_INIT); + if !defs.contains_key(setter.name) { + let _ = defs.insert(setter.name.to_owned(), ffi::PyGetSetDef_INIT); } - let def = defs.get_mut(&name).expect("Failed to call get_mut"); + let def = defs.get_mut(setter.name).expect("Failed to call get_mut"); setter.copy_to(def); } _ => (), diff --git a/src/type_object.rs b/src/type_object.rs index 456835d8408..0a4a787bf5d 100644 --- a/src/type_object.rs +++ b/src/type_object.rs @@ -227,18 +227,15 @@ impl LazyStaticType { fn initialize_tp_dict( py: Python, tp_dict: *mut ffi::PyObject, - items: Vec<(&'static str, PyObject)>, + items: Vec<(&'static std::ffi::CStr, PyObject)>, ) -> PyResult<()> { // We hold the GIL: the dictionary update can be considered atomic from // the POV of other threads. for (key, val) in items { - crate::types::with_tmp_string(py, key, |key| { - let ret = unsafe { ffi::PyDict_SetItem(tp_dict, key, val.into_ptr()) }; - if ret < 0 { - return Err(PyErr::fetch(py)); - } - Ok(()) - })?; + let ret = unsafe { ffi::PyDict_SetItemString(tp_dict, key.as_ptr(), val.into_ptr()) }; + if ret < 0 { + return Err(PyErr::fetch(py)); + } } Ok(()) } From b42886a38a736256b90e0d4b5f99ad77142b6b1e Mon Sep 17 00:00:00 2001 From: kngwyu Date: Thu, 8 Oct 2020 13:54:17 +0900 Subject: [PATCH 3/8] Change PyCFunction to take &'static str as a function name --- pyo3-derive-backend/src/module.rs | 7 ++++- src/class/methods.rs | 1 - src/types/function.rs | 47 +++++++------------------------ tests/test_pyfunction.rs | 2 +- 4 files changed, 17 insertions(+), 40 deletions(-) diff --git a/pyo3-derive-backend/src/module.rs b/pyo3-derive-backend/src/module.rs index 657d1134a8c..1357079bd41 100644 --- a/pyo3-derive-backend/src/module.rs +++ b/pyo3-derive-backend/src/module.rs @@ -212,7 +212,12 @@ pub fn add_fn_to_module( fn #function_wrapper_ident<'a>( args: impl Into> ) -> pyo3::PyResult<&'a pyo3::types::PyCFunction> { - pyo3::types::PyCFunction::new_with_keywords(#wrapper_ident, stringify!(#python_name), #doc, args.into()) + pyo3::types::PyCFunction::new_with_keywords( + #wrapper_ident, + concat!(stringify!(#python_name), "\0"), + #doc, + args.into(), + ) } }) } diff --git a/src/class/methods.rs b/src/class/methods.rs index 4713d2acfaf..67269705706 100644 --- a/src/class/methods.rs +++ b/src/class/methods.rs @@ -35,7 +35,6 @@ pub enum PyMethodType { PyInitFunc(ffi::initproc), } -// TODO(kngwyu): We should also use &'static CStr for this? I'm not sure. #[derive(Clone, Debug)] pub struct PyMethodDef { ml_name: &'static CStr, diff --git a/src/types/function.rs b/src/types/function.rs index e185c83365c..aaf06a3acfc 100644 --- a/src/types/function.rs +++ b/src/types/function.rs @@ -1,9 +1,6 @@ -use std::ffi::{CStr, CString}; - use crate::derive_utils::PyFunctionArguments; -use crate::exceptions::PyValueError; use crate::prelude::*; -use crate::{class, ffi, AsPyPointer, PyMethodType}; +use crate::{ffi, AsPyPointer, PyMethodDef}; /// Represents a builtin Python function object. #[repr(transparent)] @@ -17,56 +14,32 @@ impl PyCFunction { /// See [raw_pycfunction] for documentation on how to get the `fun` argument. pub fn new_with_keywords<'a>( fun: ffi::PyCFunctionWithKeywords, - name: &str, + name: &'static str, doc: &'static str, py_or_module: PyFunctionArguments<'a>, ) -> PyResult<&'a PyCFunction> { - let fun = PyMethodType::PyCFunctionWithKeywords(fun); - Self::new_(fun, name, doc, py_or_module) + Self::new_( + PyMethodDef::cfunction_with_keywords(name, fun, 0, doc), + py_or_module, + ) } /// Create a new built-in function without keywords. pub fn new<'a>( fun: ffi::PyCFunction, - name: &str, + name: &'static str, doc: &'static str, py_or_module: PyFunctionArguments<'a>, ) -> PyResult<&'a PyCFunction> { - let fun = PyMethodType::PyCFunction(fun); - Self::new_(fun, name, doc, py_or_module) + Self::new_(PyMethodDef::cfunction(name, fun, doc), py_or_module) } fn new_<'a>( - fun: class::PyMethodType, - name: &str, - doc: &'static str, + def: PyMethodDef, py_or_module: PyFunctionArguments<'a>, ) -> PyResult<&'a PyCFunction> { let (py, module) = py_or_module.into_py_and_maybe_module(); - let doc: &'static CStr = CStr::from_bytes_with_nul(doc.as_bytes()) - .map_err(|_| PyValueError::new_err("docstring must end with NULL byte."))?; - let name = CString::new(name.as_bytes()).map_err(|_| { - PyValueError::new_err("Function name cannot contain contain NULL byte.") - })?; - let def = match fun { - PyMethodType::PyCFunction(fun) => ffi::PyMethodDef { - ml_name: name.into_raw() as _, - ml_meth: Some(fun), - ml_flags: ffi::METH_VARARGS, - ml_doc: doc.as_ptr() as _, - }, - PyMethodType::PyCFunctionWithKeywords(fun) => ffi::PyMethodDef { - ml_name: name.into_raw() as _, - ml_meth: Some(unsafe { std::mem::transmute(fun) }), - ml_flags: ffi::METH_VARARGS | ffi::METH_KEYWORDS, - ml_doc: doc.as_ptr() as _, - }, - _ => { - return Err(PyValueError::new_err( - "Only PyCFunction and PyCFunctionWithKeywords are valid.", - )) - } - }; + let def = def.as_method_def(); let (mod_ptr, module_name) = if let Some(m) = module { let mod_ptr = m.as_ptr(); let name = m.name()?.into_py(py); diff --git a/tests/test_pyfunction.rs b/tests/test_pyfunction.rs index 89b4dbd01e3..a1563e31404 100644 --- a/tests/test_pyfunction.rs +++ b/tests/test_pyfunction.rs @@ -100,7 +100,7 @@ fn test_raw_function() { let gil = Python::acquire_gil(); let py = gil.python(); let raw_func = raw_pycfunction!(optional_bool); - let fun = PyCFunction::new_with_keywords(raw_func, "fun", "\0", py.into()).unwrap(); + let fun = PyCFunction::new_with_keywords(raw_func, "fun\0", "\0", py.into()).unwrap(); let res = fun.call((), None).unwrap().extract::<&str>().unwrap(); assert_eq!(res, "Some(true)"); let res = fun.call((false,), None).unwrap().extract::<&str>().unwrap(); From 9ee6da8f82be198fcbe284591ef261ef8e7bbe40 Mon Sep 17 00:00:00 2001 From: kngwyu Date: Thu, 8 Oct 2020 19:56:53 +0900 Subject: [PATCH 4/8] Add more comments to class/methods.rs and Reduce LOC --- src/class/methods.rs | 52 +++++++++++++++++--------------------------- 1 file changed, 20 insertions(+), 32 deletions(-) diff --git a/src/class/methods.rs b/src/class/methods.rs index 67269705706..de827de4585 100644 --- a/src/class/methods.rs +++ b/src/class/methods.rs @@ -73,12 +73,12 @@ unsafe impl Sync for PySetterDef {} unsafe impl Sync for ffi::PyGetSetDef {} -fn get_name(name: &'static str) -> &'static CStr { +fn get_name(name: &str) -> &CStr { CStr::from_bytes_with_nul(name.as_bytes()) .expect("Method name must be terminated with NULL byte") } -fn get_doc(doc: &'static str) -> &'static CStr { +fn get_doc(doc: &str) -> &CStr { CStr::from_bytes_with_nul(doc.as_bytes()).expect("Document must be terminated with NULL byte") } @@ -99,50 +99,37 @@ impl PyMethodDef { } } + /// Define a function with no `*args` and `**kwargs`. pub fn cfunction(name: &'static str, cfunction: ffi::PyCFunction, doc: &'static str) -> Self { - Self::new( - name, - PyMethodType::PyCFunction(cfunction), - ffi::METH_NOARGS, - doc, - ) + Self { + ml_name: get_name(name), + ml_meth: PyMethodType::PyCFunction(cfunction), + ml_flags: ffi::METH_NOARGS, + ml_doc: get_doc(doc), + } } + /// Define a `__new__` function. pub fn new_func(name: &'static str, newfunc: ffi::newfunc, doc: &'static str) -> Self { - Self::new( - name, - PyMethodType::PyNewFunc(newfunc), - ffi::METH_VARARGS | ffi::METH_KEYWORDS, - doc, - ) + Self { + ml_name: get_name(name), + ml_meth: PyMethodType::PyNewFunc(newfunc), + ml_flags: ffi::METH_VARARGS | ffi::METH_KEYWORDS, + ml_doc: get_doc(doc), + } } - /// Define a function that can take `**kwargs`. + /// Define a function that can take `*args` and `**kwargs`. pub fn cfunction_with_keywords( name: &'static str, cfunction: ffi::PyCFunctionWithKeywords, flags: c_int, doc: &'static str, - ) -> Self { - let flags = flags | ffi::METH_VARARGS | ffi::METH_KEYWORDS; - Self::new( - name, - PyMethodType::PyCFunctionWithKeywords(cfunction), - flags, - doc, - ) - } - - pub(crate) fn new( - name: &'static str, - methodtype: PyMethodType, - flags: c_int, - doc: &'static str, ) -> Self { Self { ml_name: get_name(name), - ml_meth: methodtype, - ml_flags: flags, + ml_meth: PyMethodType::PyCFunctionWithKeywords(cfunction), + ml_flags: flags | ffi::METH_VARARGS | ffi::METH_KEYWORDS, ml_doc: get_doc(doc), } } @@ -166,6 +153,7 @@ impl PyMethodDef { } impl PyClassAttributeDef { + /// Define a class attribute. pub fn new(name: &'static str, meth: for<'p> fn(Python<'p>) -> PyObject) -> Self { Self { name: get_name(name), From ff644316d78eefd2e312fbb926def3465dba5741 Mon Sep 17 00:00:00 2001 From: kngwyu Date: Thu, 8 Oct 2020 23:31:17 +0900 Subject: [PATCH 5/8] Add a CHANGELOG entry --- CHANGELOG.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bf23660bc8f..207354cd3bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,9 +6,15 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). ## [Unreleased] -## Added +### Added - Add support for keyword-only arguments without default values in `#[pyfunction]`. [#1209](https://github.com/PyO3/pyo3/pull/1209) +### Changed +- Fields of `PyMethodDef`, `PyGetterDef`, `PySetterDef`, and `PyClassAttributeDef` are now private. [#1169](https://github.com/PyO3/pyo3/pull/1169) + +### Fixed +- Fix invalid document for protocol methods. [#1169](https://github.com/PyO3/pyo3/pull/1169) + ## [0.12.1] - 2020-09-16 ### Fixed - Fix building for a 32-bit Python on 64-bit Windows with a 64-bit Rust toolchain. [#1179](https://github.com/PyO3/pyo3/pull/1179) From 359d878fb6a2a2888f260926db760455130aa807 Mon Sep 17 00:00:00 2001 From: kngwyu Date: Fri, 9 Oct 2020 00:21:42 +0900 Subject: [PATCH 6/8] Fix clippy warnings --- pyo3-derive-backend/src/from_pyobject.rs | 17 +++++------------ src/buffer.rs | 10 ++-------- 2 files changed, 7 insertions(+), 20 deletions(-) diff --git a/pyo3-derive-backend/src/from_pyobject.rs b/pyo3-derive-backend/src/from_pyobject.rs index d35af6da9dd..8d8dc95660a 100644 --- a/pyo3-derive-backend/src/from_pyobject.rs +++ b/pyo3-derive-backend/src/from_pyobject.rs @@ -133,7 +133,9 @@ impl<'a> Container<'a> { "Cannot derive FromPyObject for empty structs and variants.", )); } - let transparent = attrs.iter().any(ContainerAttribute::transparent); + let transparent = attrs + .iter() + .any(|attr| *attr == ContainerAttribute::Transparent); if transparent { Self::check_transparent_len(fields)?; } @@ -182,7 +184,6 @@ impl<'a> Container<'a> { let err_name = attrs .iter() .find_map(|a| a.annotation()) - .cloned() .unwrap_or_else(|| path.segments.last().unwrap().ident.to_string()); let v = Container { @@ -306,18 +307,10 @@ enum ContainerAttribute { } impl ContainerAttribute { - /// Return whether this attribute is `Transparent` - fn transparent(&self) -> bool { - match self { - ContainerAttribute::Transparent => true, - _ => false, - } - } - /// Convenience method to access `ErrorAnnotation`. - fn annotation(&self) -> Option<&String> { + fn annotation(&self) -> Option { match self { - ContainerAttribute::ErrorAnnotation(s) => Some(s), + ContainerAttribute::ErrorAnnotation(s) => Some(s.to_string()), _ => None, } } diff --git a/src/buffer.rs b/src/buffer.rs index 2e7c401377f..a80f18392cf 100644 --- a/src/buffer.rs +++ b/src/buffer.rs @@ -132,18 +132,12 @@ fn standard_element_type_from_type_char(type_char: u8) -> ElementType { #[cfg(target_endian = "little")] fn is_matching_endian(c: u8) -> bool { - match c { - b'@' | b'=' | b'<' => true, - _ => false, - } + c == b'@' || c == b'=' || c == b'>' } #[cfg(target_endian = "big")] fn is_matching_endian(c: u8) -> bool { - match c { - b'@' | b'=' | b'>' | b'!' => true, - _ => false, - } + c == b'@' || c == b'=' || c == b'>' || c == b'!' } /// Trait implemented for possible element types of `PyBuffer`. From e7092fe6304169106953eca91d34ec75f38883d0 Mon Sep 17 00:00:00 2001 From: kngwyu Date: Fri, 9 Oct 2020 01:21:31 +0900 Subject: [PATCH 7/8] Make PyCFunction more backward-compatible --- pyo3-derive-backend/src/module.rs | 14 ++++--- src/class/methods.rs | 8 ++-- src/types/function.rs | 62 ++++++++++++++++++++++++------- tests/test_pyfunction.rs | 2 +- 4 files changed, 62 insertions(+), 24 deletions(-) diff --git a/pyo3-derive-backend/src/module.rs b/pyo3-derive-backend/src/module.rs index 1357079bd41..3959fb81046 100644 --- a/pyo3-derive-backend/src/module.rs +++ b/pyo3-derive-backend/src/module.rs @@ -200,7 +200,7 @@ pub fn add_fn_to_module( doc, }; - let doc = &spec.doc; + let doc = syn::LitByteStr::new(spec.doc.value().as_bytes(), spec.doc.span()); let python_name = &spec.python_name; @@ -212,10 +212,14 @@ pub fn add_fn_to_module( fn #function_wrapper_ident<'a>( args: impl Into> ) -> pyo3::PyResult<&'a pyo3::types::PyCFunction> { - pyo3::types::PyCFunction::new_with_keywords( - #wrapper_ident, - concat!(stringify!(#python_name), "\0"), - #doc, + let name = concat!(stringify!(#python_name), "\0"); + let name = std::ffi::CStr::from_bytes_with_nul(name.as_bytes()).unwrap(); + let doc = std::ffi::CStr::from_bytes_with_nul(#doc).unwrap(); + pyo3::types::PyCFunction::internal_new( + name, + doc, + pyo3::class::PyMethodType::PyCFunctionWithKeywords(#wrapper_ident), + pyo3::ffi::METH_VARARGS | pyo3::ffi::METH_KEYWORDS, args.into(), ) } diff --git a/src/class/methods.rs b/src/class/methods.rs index de827de4585..e8d10a658ad 100644 --- a/src/class/methods.rs +++ b/src/class/methods.rs @@ -37,10 +37,10 @@ pub enum PyMethodType { #[derive(Clone, Debug)] pub struct PyMethodDef { - ml_name: &'static CStr, - ml_meth: PyMethodType, - ml_flags: c_int, - ml_doc: &'static CStr, + pub(crate) ml_name: &'static CStr, + pub(crate) ml_meth: PyMethodType, + pub(crate) ml_flags: c_int, + pub(crate) ml_doc: &'static CStr, } #[derive(Copy, Clone)] diff --git a/src/types/function.rs b/src/types/function.rs index aaf06a3acfc..05f001fe521 100644 --- a/src/types/function.rs +++ b/src/types/function.rs @@ -1,6 +1,9 @@ +use std::ffi::{CStr, CString}; + use crate::derive_utils::PyFunctionArguments; +use crate::exceptions::PyValueError; use crate::prelude::*; -use crate::{ffi, AsPyPointer, PyMethodDef}; +use crate::{ffi, AsPyPointer, PyMethodDef, PyMethodType}; /// Represents a builtin Python function object. #[repr(transparent)] @@ -8,18 +11,33 @@ pub struct PyCFunction(PyAny); pyobject_native_var_type!(PyCFunction, ffi::PyCFunction_Type, ffi::PyCFunction_Check); +fn get_name(name: &str) -> PyResult<&'static CStr> { + let cstr = CString::new(name) + .map_err(|_| PyValueError::new_err("Function name cannot contain contain NULL byte."))?; + Ok(Box::leak(cstr.into_boxed_c_str())) +} + +fn get_doc(doc: &str) -> PyResult<&'static CStr> { + let cstr = CString::new(doc) + .map_err(|_| PyValueError::new_err("Document cannot contain contain NULL byte."))?; + Ok(Box::leak(cstr.into_boxed_c_str())) +} + impl PyCFunction { /// Create a new built-in function with keywords. /// /// See [raw_pycfunction] for documentation on how to get the `fun` argument. pub fn new_with_keywords<'a>( fun: ffi::PyCFunctionWithKeywords, - name: &'static str, - doc: &'static str, + name: &str, + doc: &str, py_or_module: PyFunctionArguments<'a>, - ) -> PyResult<&'a PyCFunction> { - Self::new_( - PyMethodDef::cfunction_with_keywords(name, fun, 0, doc), + ) -> PyResult<&'a Self> { + Self::internal_new( + get_name(name)?, + get_doc(doc)?, + PyMethodType::PyCFunctionWithKeywords(fun), + ffi::METH_VARARGS | ffi::METH_KEYWORDS, py_or_module, ) } @@ -27,19 +45,35 @@ impl PyCFunction { /// Create a new built-in function without keywords. pub fn new<'a>( fun: ffi::PyCFunction, - name: &'static str, - doc: &'static str, + name: &str, + doc: &str, py_or_module: PyFunctionArguments<'a>, - ) -> PyResult<&'a PyCFunction> { - Self::new_(PyMethodDef::cfunction(name, fun, doc), py_or_module) + ) -> PyResult<&'a Self> { + Self::internal_new( + get_name(name)?, + get_doc(doc)?, + PyMethodType::PyCFunction(fun), + ffi::METH_NOARGS, + py_or_module, + ) } - fn new_<'a>( - def: PyMethodDef, + #[doc(hidden)] + pub fn internal_new<'a>( + name: &'static CStr, + doc: &'static CStr, + method_type: PyMethodType, + flags: std::os::raw::c_int, py_or_module: PyFunctionArguments<'a>, - ) -> PyResult<&'a PyCFunction> { + ) -> PyResult<&'a Self> { let (py, module) = py_or_module.into_py_and_maybe_module(); - let def = def.as_method_def(); + let method_def = PyMethodDef { + ml_name: name, + ml_meth: method_type, + ml_flags: flags, + ml_doc: doc, + }; + let def = method_def.as_method_def(); let (mod_ptr, module_name) = if let Some(m) = module { let mod_ptr = m.as_ptr(); let name = m.name()?.into_py(py); diff --git a/tests/test_pyfunction.rs b/tests/test_pyfunction.rs index a1563e31404..6f6bf44c077 100644 --- a/tests/test_pyfunction.rs +++ b/tests/test_pyfunction.rs @@ -100,7 +100,7 @@ fn test_raw_function() { let gil = Python::acquire_gil(); let py = gil.python(); let raw_func = raw_pycfunction!(optional_bool); - let fun = PyCFunction::new_with_keywords(raw_func, "fun\0", "\0", py.into()).unwrap(); + let fun = PyCFunction::new_with_keywords(raw_func, "fun", "", py.into()).unwrap(); let res = fun.call((), None).unwrap().extract::<&str>().unwrap(); assert_eq!(res, "Some(true)"); let res = fun.call((false,), None).unwrap().extract::<&str>().unwrap(); From 2684547c98be22a4da85e75088cb9013cac4cc24 Mon Sep 17 00:00:00 2001 From: kngwyu Date: Sat, 10 Oct 2020 00:37:55 +0900 Subject: [PATCH 8/8] Make types in class::methods #[doc(hidden)] --- src/class/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/class/mod.rs b/src/class/mod.rs index 9630a6aafdd..5fc986cb5aa 100644 --- a/src/class/mod.rs +++ b/src/class/mod.rs @@ -13,6 +13,7 @@ pub mod descr; pub mod gc; pub mod iter; pub mod mapping; +#[doc(hidden)] pub mod methods; pub mod number; pub mod proto_methods; @@ -27,6 +28,7 @@ pub use self::descr::PyDescrProtocol; pub use self::gc::{PyGCProtocol, PyTraverseError, PyVisit}; pub use self::iter::PyIterProtocol; pub use self::mapping::PyMappingProtocol; +#[doc(hidden)] pub use self::methods::{ PyClassAttributeDef, PyGetterDef, PyMethodDef, PyMethodDefType, PyMethodType, PySetterDef, };