pycfunction: take &'static str arguments to new

CHANGELOG.md

@@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Deprecate FFI definition `PyCFunction_Call` for Python 3.9 and later. [#1425](https://github.com/PyO3/pyo3/pull/1425)
 - Deprecate FFI definitions `PyModule_GetFilename`. [#1425](https://github.com/PyO3/pyo3/pull/1425)
 - The `auto-initialize` feature is no longer enabled by default. [#1443](https://github.com/PyO3/pyo3/pull/1443)
+- Change `PyCFunction::new()` and `PyCFunction::new_with_keywords()` to take `&'static str` arguments rather than implicitly copying (and leaking) them. [#1450](https://github.com/PyO3/pyo3/pull/1450)
 
 ### Removed
 - Remove deprecated exception names `BaseException` etc. [#1426](https://github.com/PyO3/pyo3/pull/1426)

pyo3-macros-backend/src/module.rs

@@ -192,8 +192,7 @@ pub fn add_fn_to_module(
  doc,
  };
 
- let doc = syn::LitByteStr::new(spec.doc.value().as_bytes(), spec.doc.span());
-
+ let doc = &spec.doc;
  let python_name = &spec.python_name;
 
  let name = &func.sig.ident;
@@ -205,13 +204,13 @@ pub fn add_fn_to_module(
  args: impl Into<pyo3::derive_utils::PyFunctionArguments<'a>>
  ) -> pyo3::PyResult<&'a pyo3::types::PyCFunction> {
  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,
- unsafe { std::mem::transmute(#wrapper_ident as *const std::os::raw::c_void) },
- pyo3::ffi::METH_VARARGS | pyo3::ffi::METH_KEYWORDS,
+ pyo3::class::methods::PyMethodDef::cfunction_with_keywords(
+ name,
+ pyo3::class::methods::PyCFunctionWithKeywords(#wrapper_ident),
+ 0,
+ #doc,
+ ),
  args.into(),
  )
  }

src/class/methods.rs

@@ -1,7 +1,7 @@
 // Copyright (c) 2017-present PyO3 Project and Contributors
 
 use crate::{ffi, PyObject, Python};
-use std::ffi::CStr;
+use std::ffi::{CStr, CString};
 use std::fmt;
 use std::os::raw::c_int;
 
@@ -73,15 +73,6 @@ unsafe impl Sync for PyGetterDef {}
 
 unsafe impl Sync for PySetterDef {}
 
-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: &str) -> &CStr {
- CStr::from_bytes_with_nul(doc.as_bytes()).expect("Document must be terminated with NULL byte")
-}
-
 impl PyMethodDef {
  /// Define a function with no `*args` and `**kwargs`.
  pub const fn cfunction(name: &'static str, cfunction: PyCFunction, doc: &'static str) -> Self {
@@ -109,26 +100,26 @@ impl PyMethodDef {
  }
 
  /// Convert `PyMethodDef` to Python method definition struct `ffi::PyMethodDef`
- pub fn as_method_def(&self) -> ffi::PyMethodDef {
+ pub(crate) fn as_method_def(&self) -> Result<ffi::PyMethodDef, NulByteInString> {
  let meth = match self.ml_meth {
  PyMethodType::PyCFunction(meth) => meth.0,
  PyMethodType::PyCFunctionWithKeywords(meth) => unsafe { std::mem::transmute(meth.0) },
  };
 
- ffi::PyMethodDef {
- ml_name: get_name(self.ml_name).as_ptr(),
+ Ok(ffi::PyMethodDef {
+ ml_name: get_name(self.ml_name)?.as_ptr(),
  ml_meth: Some(meth),
  ml_flags: self.ml_flags,
- ml_doc: get_doc(self.ml_doc).as_ptr(),
- }
+ ml_doc: get_doc(self.ml_doc)?.as_ptr(),
+ })
  }
 }
 
 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),
+ name: get_name(name).unwrap(),
  meth,
  }
  }
@@ -148,9 +139,9 @@ impl PyGetterDef {
  /// Define a getter.
  pub fn new(name: &'static str, getter: ffi::getter, doc: &'static str) -> Self {
  Self {
- name: get_name(name),
+ name: get_name(name).unwrap(),
  meth: getter,
- doc: get_doc(doc),
+ doc: get_doc(doc).unwrap(),
  }
  }
 
@@ -170,9 +161,9 @@ impl PySetterDef {
  /// Define a setter.
  pub fn new(name: &'static str, setter: ffi::setter, doc: &'static str) -> Self {
  Self {
- name: get_name(name),
+ name: get_name(name).unwrap(),
  meth: setter,
- doc: get_doc(doc),
+ doc: get_doc(doc).unwrap(),
  }
  }
 
@@ -209,3 +200,25 @@ pub trait PyMethodsInventory: inventory::Collect {
 pub trait HasMethodsInventory {
  type Methods: PyMethodsInventory;
 }
+
+#[derive(Debug)]
+pub(crate) struct NulByteInString(pub(crate) &'static str);
+
+fn get_name(name: &'static str) -> Result<&'static CStr, NulByteInString> {
+ extract_cstr_or_leak_cstring(name, "Function name cannot contain NUL byte.")
+}
+
+fn get_doc(doc: &'static str) -> Result<&'static CStr, NulByteInString> {
+ extract_cstr_or_leak_cstring(doc, "Document cannot contain NUL byte.")
+}
+
+fn extract_cstr_or_leak_cstring(
+ src: &'static str,
+ err_msg: &'static str,
+) -> Result<&'static CStr, NulByteInString> {
+ CStr::from_bytes_with_nul(src.as_bytes())
+ .or_else(|_| {
+ CString::new(src.as_bytes()).map(|c_string| &*Box::leak(c_string.into_boxed_c_str()))
+ })
+ .map_err(|_| NulByteInString(err_msg))
+}

src/pyclass.rs

@@ -318,7 +318,7 @@ fn py_class_method_defs<T: PyClassImpl>() -> Vec<ffi::PyMethodDef> {
  PyMethodDefType::Method(def)
  | PyMethodDefType::Class(def)
  | PyMethodDefType::Static(def) => {
- defs.push(def.as_method_def());
+ defs.push(def.as_method_def().unwrap());
  }
  _ => (),
  });

src/types/function.rs

@@ -1,78 +1,60 @@
-use std::ffi::{CStr, CString};
-
-use crate::derive_utils::PyFunctionArguments;
 use crate::exceptions::PyValueError;
 use crate::prelude::*;
-use crate::{ffi, AsPyPointer};
+use crate::{
+ class::methods::{self, PyMethodDef},
+ ffi, AsPyPointer,
+};
+use crate::{derive_utils::PyFunctionArguments, methods::NulByteInString};
 
 /// Represents a builtin Python function object.
 #[repr(transparent)]
 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: &str,
- doc: &str,
+ name: &'static str,
+ doc: &'static str,
  py_or_module: PyFunctionArguments<'a>,
  ) -> PyResult<&'a Self> {
  Self::internal_new(
- get_name(name)?,
- get_doc(doc)?,
- unsafe { std::mem::transmute(fun) },
- ffi::METH_VARARGS | ffi::METH_KEYWORDS,
+ PyMethodDef::cfunction_with_keywords(
+ name,
+ methods::PyCFunctionWithKeywords(fun),
+ 0,
+ doc,
+ ),
  py_or_module,
  )
  }
 
  /// Create a new built-in function without keywords.
  pub fn new<'a>(
  fun: ffi::PyCFunction,
- name: &str,
- doc: &str,
+ name: &'static str,
+ doc: &'static str,
  py_or_module: PyFunctionArguments<'a>,
  ) -> PyResult<&'a Self> {
  Self::internal_new(
- get_name(name)?,
- get_doc(doc)?,
- fun,
- ffi::METH_NOARGS,
+ PyMethodDef::cfunction(name, methods::PyCFunction(fun), doc),
  py_or_module,
  )
  }
 
  #[doc(hidden)]
- pub fn internal_new<'a>(
- name: &'static CStr,
- doc: &'static CStr,
- method: ffi::PyCFunction,
- flags: std::os::raw::c_int,
- py_or_module: PyFunctionArguments<'a>,
- ) -> PyResult<&'a Self> {
+ pub fn internal_new(
+ method_def: PyMethodDef,
+ py_or_module: PyFunctionArguments,
+ ) -> PyResult<&Self> {
  let (py, module) = py_or_module.into_py_and_maybe_module();
- let def = ffi::PyMethodDef {
- ml_name: name.as_ptr(),
- ml_meth: Some(method),
- ml_flags: flags,
- ml_doc: doc.as_ptr(),
- };
+ let def = method_def
+ .as_method_def()
+ .map_err(|NulByteInString(err)| PyValueError::new_err(err))?;
  let (mod_ptr, module_name) = if let Some(m) = module {
  let mod_ptr = m.as_ptr();
  let name = m.name()?.into_py(py);