Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pymodule bound #3897

Merged
merged 8 commits into from
Feb 27, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion guide/src/module.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ fn parent_module(py: Python<'_>, m: &PyModule) -> PyResult<()> {

fn register_child_module(py: Python<'_>, parent_module: &PyModule) -> PyResult<()> {
let child_module = PyModule::new_bound(py, "child_module")?;
child_module.add_function(&wrap_pyfunction!(func, child_module.as_gil_ref())?.as_borrowed())?;
child_module.add_function(wrap_pyfunction_bound!(func, &child_module)?)?;
parent_module.add_submodule(child_module.as_gil_ref())?;
Ok(())
}
Expand Down
2 changes: 1 addition & 1 deletion guide/src/python_from_rust.md
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ fn main() -> PyResult<()> {
Python::with_gil(|py| {
// Create new module
let foo_module = PyModule::new_bound(py, "foo")?;
foo_module.add_function(&wrap_pyfunction!(add_one, foo_module.as_gil_ref())?.as_borrowed())?;
foo_module.add_function(wrap_pyfunction_bound!(add_one, &foo_module)?)?;

// Import and get sys.modules
let sys = PyModule::import_bound(py, "sys")?;
Expand Down
12 changes: 6 additions & 6 deletions pyo3-macros-backend/src/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ pub fn pymodule_module_impl(mut module: syn::ItemMod) -> Result<TokenStream> {
/// module
pub fn pymodule_function_impl(mut function: syn::ItemFn) -> Result<TokenStream> {
let options = PyModuleOptions::from_attrs(&mut function.attrs)?;
process_functions_in_module(&options, &mut function)?;
process_functions_in_module(&mut function)?;
let krate = get_pyo3_crate(&options.krate);
let ident = &function.sig.ident;
let vis = &function.vis;
Expand All @@ -213,15 +213,16 @@ pub fn pymodule_function_impl(mut function: syn::ItemFn) -> Result<TokenStream>
// inside a function body)
const _: () = {
use #krate::impl_::pymodule as impl_;
use #krate::impl_::pymethods::BoundRef;

fn __pyo3_pymodule(module: &#krate::Bound<'_, #krate::types::PyModule>) -> #krate::PyResult<()> {
#ident(module.py(), module.as_gil_ref())
#ident(module.py(), ::std::convert::Into::into(BoundRef(module)))
}

impl #ident::MakeDef {
const fn make_def() -> impl_::ModuleDef {
const INITIALIZER: impl_::ModuleInitializer = impl_::ModuleInitializer(__pyo3_pymodule);
unsafe {
const INITIALIZER: impl_::ModuleInitializer = impl_::ModuleInitializer(__pyo3_pymodule);
impl_::ModuleDef::new(
#ident::__PYO3_NAME,
#doc,
Expand Down Expand Up @@ -260,9 +261,8 @@ fn module_initialization(options: PyModuleOptions, ident: &syn::Ident) -> TokenS
}

/// Finds and takes care of the #[pyfn(...)] in `#[pymodule]`
fn process_functions_in_module(options: &PyModuleOptions, func: &mut syn::ItemFn) -> Result<()> {
fn process_functions_in_module(func: &mut syn::ItemFn) -> Result<()> {
let mut stmts: Vec<syn::Stmt> = Vec::new();
let krate = get_pyo3_crate(&options.krate);

for mut stmt in func.block.stmts.drain(..) {
if let syn::Stmt::Item(Item::Fn(func)) = &mut stmt {
Expand All @@ -272,7 +272,7 @@ fn process_functions_in_module(options: &PyModuleOptions, func: &mut syn::ItemFn
let name = &func.sig.ident;
let statements: Vec<syn::Stmt> = syn::parse_quote! {
#wrapped_function
#module_name.add_function(#krate::impl_::pyfunction::_wrap_pyfunction(&#name::DEF, #module_name)?)?;
#module_name.add_function(#module_name.wrap_pyfunction(&#name::DEF)?)?;
};
stmts.extend(statements);
}
Expand Down
6 changes: 3 additions & 3 deletions pyo3-macros-backend/src/pyfunction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -268,12 +268,12 @@ pub fn impl_wrap_pyfunction(
#[doc(hidden)]
#vis mod #name {
pub(crate) struct MakeDef;
pub const DEF: #krate::impl_::pyfunction::PyMethodDef = MakeDef::DEF;
pub const DEF: #krate::impl_::pymethods::PyMethodDef = MakeDef::DEF;

pub fn add_to_module(module: &#krate::Bound<'_, #krate::types::PyModule>) -> #krate::PyResult<()> {
use #krate::prelude::PyModuleMethods;
use ::std::convert::Into;
module.add_function(&#krate::types::PyCFunction::internal_new(&DEF, module.as_gil_ref().into())?)
module.add_function(#krate::types::PyCFunction::internal_new(&DEF, module.as_gil_ref().into())?)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit sad to still need this as_gil_ref() call for now.

}
}

Expand All @@ -284,7 +284,7 @@ pub fn impl_wrap_pyfunction(
const _: () = {
use #krate as _pyo3;
impl #name::MakeDef {
const DEF: #krate::impl_::pyfunction::PyMethodDef = #methoddef;
const DEF: #krate::impl_::pymethods::PyMethodDef = #methoddef;
}

#[allow(non_snake_case)]
Expand Down
10 changes: 9 additions & 1 deletion src/derive_utils.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
//! Functionality for the code generated by the derive backend

use crate::{types::PyModule, Python};
use crate::impl_::pymethods::PyMethodDef;
use crate::{
types::{PyCFunction, PyModule},
PyResult, Python,
};

/// Enum to abstract over the arguments of Python function wrappers.
pub enum PyFunctionArguments<'a> {
Expand All @@ -18,6 +22,10 @@ impl<'a> PyFunctionArguments<'a> {
}
}
}

pub fn wrap_pyfunction(self, method_def: &'a PyMethodDef) -> PyResult<&PyCFunction> {
Ok(PyCFunction::internal_new(method_def, self)?.into_gil_ref())
}
}

impl<'a> From<Python<'a>> for PyFunctionArguments<'a> {
Expand Down
1 change: 0 additions & 1 deletion src/impl_.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ pub(crate) mod not_send;
pub mod panic;
pub mod pycell;
pub mod pyclass;
pub mod pyfunction;
pub mod pymethods;
pub mod pymodule;
#[doc(hidden)]
Expand Down
10 changes: 0 additions & 10 deletions src/impl_/pyfunction.rs

This file was deleted.

27 changes: 25 additions & 2 deletions src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,13 +126,36 @@ macro_rules! py_run_impl {
macro_rules! wrap_pyfunction {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one should now get a deprecation warning I thing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot! I think maybe this makes sense to do in a separate PR since it'll cause a lot of churn in tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like that is #3899

($function:path) => {
&|py_or_module| {
use $crate::derive_utils::PyFunctionArguments;
use $function as wrapped_pyfunction;
$crate::impl_::pyfunction::_wrap_pyfunction(&wrapped_pyfunction::DEF, py_or_module)
let function_arguments: PyFunctionArguments<'_> =
::std::convert::Into::into(py_or_module);
function_arguments.wrap_pyfunction(&wrapped_pyfunction::DEF)
}
};
($function:path, $py_or_module:expr) => {{
use $crate::derive_utils::PyFunctionArguments;
use $function as wrapped_pyfunction;
$crate::impl_::pyfunction::_wrap_pyfunction(&wrapped_pyfunction::DEF, $py_or_module)
let function_arguments: PyFunctionArguments<'_> = ::std::convert::Into::into($py_or_module);
function_arguments.wrap_pyfunction(&wrapped_pyfunction::DEF)
}};
}

/// Wraps a Rust function annotated with [`#[pyfunction]`](macro@crate::pyfunction).
///
/// This can be used with [`PyModule::add_function`](crate::types::PyModule::add_function) to add free
/// functions to a [`PyModule`](crate::types::PyModule) - see its documentation for more information.
#[macro_export]
macro_rules! wrap_pyfunction_bound {
($function:path) => {
&|module| {
use $function as wrapped_pyfunction;
module.wrap_pyfunction(&wrapped_pyfunction::DEF)
}
};
($function:path, $module:expr) => {{
use $function as wrapped_pyfunction;
$module.wrap_pyfunction(&wrapped_pyfunction::DEF)
LilyFoote marked this conversation as resolved.
Show resolved Hide resolved
}};
}

Expand Down
2 changes: 1 addition & 1 deletion src/prelude.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ pub use crate::PyNativeType;
pub use pyo3_macros::{pyclass, pyfunction, pymethods, pymodule, FromPyObject};

#[cfg(feature = "macros")]
pub use crate::wrap_pyfunction;
pub use crate::{wrap_pyfunction, wrap_pyfunction_bound};

pub use crate::types::any::PyAnyMethods;
pub use crate::types::boolobject::PyBoolMethods;
Expand Down
72 changes: 59 additions & 13 deletions src/types/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@ use crate::ffi_ptr_ext::FfiPtrExt;
use crate::methods::PyMethodDefDestructor;
use crate::py_result_ext::PyResultExt;
use crate::types::capsule::PyCapsuleMethods;
use crate::types::module::PyModuleMethods;
use crate::{
ffi,
impl_::pymethods::{self, PyMethodDef},
types::{PyCapsule, PyDict, PyString, PyTuple},
types::{PyCapsule, PyDict, PyModule, PyString, PyTuple},
};
use crate::{Bound, IntoPy, Py, PyAny, PyResult, Python};
use std::cell::UnsafeCell;
Expand All @@ -33,23 +34,33 @@ impl PyCFunction {
doc: &'static str,
py_or_module: PyFunctionArguments<'a>,
) -> PyResult<&'a Self> {
Self::new_with_keywords_bound(fun, name, doc, py_or_module).map(Bound::into_gil_ref)
Self::internal_new(
&PyMethodDef::cfunction_with_keywords(
name,
pymethods::PyCFunctionWithKeywords(fun),
doc,
),
py_or_module,
)
.map(Bound::into_gil_ref)
}

/// Create a new built-in function with keywords (*args and/or **kwargs).
pub fn new_with_keywords_bound<'a>(
pub fn new_with_keywords_bound<'py>(
py: Python<'py>,
fun: ffi::PyCFunctionWithKeywords,
name: &'static str,
doc: &'static str,
py_or_module: PyFunctionArguments<'a>,
) -> PyResult<Bound<'a, Self>> {
Self::internal_new(
module: Option<&Bound<'py, PyModule>>,
) -> PyResult<Bound<'py, Self>> {
Self::internal_new_bound(
py,
&PyMethodDef::cfunction_with_keywords(
name,
pymethods::PyCFunctionWithKeywords(fun),
doc,
),
py_or_module,
module,
)
}

Expand All @@ -67,19 +78,25 @@ impl PyCFunction {
doc: &'static str,
py_or_module: PyFunctionArguments<'a>,
) -> PyResult<&'a Self> {
Self::new_bound(fun, name, doc, py_or_module).map(Bound::into_gil_ref)
Self::internal_new(
&PyMethodDef::noargs(name, pymethods::PyCFunction(fun), doc),
py_or_module,
)
.map(Bound::into_gil_ref)
}

/// Create a new built-in function which takes no arguments.
pub fn new_bound<'a>(
pub fn new_bound<'py>(
py: Python<'py>,
fun: ffi::PyCFunction,
name: &'static str,
doc: &'static str,
py_or_module: PyFunctionArguments<'a>,
) -> PyResult<Bound<'a, Self>> {
Self::internal_new(
module: Option<&Bound<'py, PyModule>>,
) -> PyResult<Bound<'py, Self>> {
Self::internal_new_bound(
py,
&PyMethodDef::noargs(name, pymethods::PyCFunction(fun), doc),
py_or_module,
module,
)
}

Expand Down Expand Up @@ -189,6 +206,35 @@ impl PyCFunction {
.downcast_into_unchecked()
}
}

#[doc(hidden)]
pub(crate) fn internal_new_bound<'py>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need this. We can change the signature of internal_new to match the new _bound APIs and move let (py, module) = py_or_module.into_py_and_maybe_module(); into the deprecated gil-ref APIs

py: Python<'py>,
method_def: &PyMethodDef,
module: Option<&Bound<'py, PyModule>>,
) -> PyResult<Bound<'py, Self>> {
let (mod_ptr, module_name): (_, Option<Py<PyString>>) = if let Some(m) = module {
let mod_ptr = m.as_ptr();
(mod_ptr, Some(m.name()?.into_py(py)))
} else {
(std::ptr::null_mut(), None)
};
let (def, destructor) = method_def.as_method_def()?;

// FIXME: stop leaking the def and destructor
let def = Box::into_raw(Box::new(def));
std::mem::forget(destructor);

let module_name_ptr = module_name
.as_ref()
.map_or(std::ptr::null_mut(), Py::as_ptr);

unsafe {
ffi::PyCFunction_NewEx(def, mod_ptr, module_name_ptr)
.assume_owned_or_err(py)
.downcast_into_unchecked()
}
}
}

fn closure_capsule_name() -> &'static CStr {
Expand Down
21 changes: 18 additions & 3 deletions src/types/module.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use crate::callback::IntoPyCallbackOutput;
use crate::derive_utils::PyFunctionArguments;
use crate::err::{PyErr, PyResult};
use crate::ffi_ptr_ext::FfiPtrExt;
use crate::impl_::pymethods::PyMethodDef;
use crate::py_result_ext::PyResultExt;
use crate::pyclass::PyClass;
use crate::types::{
Expand Down Expand Up @@ -399,7 +401,13 @@ impl PyModule {
/// [1]: crate::prelude::pyfunction
/// [2]: crate::wrap_pyfunction
pub fn add_function<'a>(&'a self, fun: &'a PyCFunction) -> PyResult<()> {
self.as_borrowed().add_function(&fun.as_borrowed())
let name = fun.getattr(__name__(self.py()))?.extract()?;
self.add(name, fun)
}

#[doc(hidden)]
pub fn wrap_pyfunction<'a>(&'a self, method_def: &'a PyMethodDef) -> PyResult<&PyCFunction> {
PyFunctionArguments::PyModule(self).wrap_pyfunction(method_def)
}
LilyFoote marked this conversation as resolved.
Show resolved Hide resolved
}

Expand Down Expand Up @@ -590,7 +598,10 @@ pub trait PyModuleMethods<'py> {
///
/// [1]: crate::prelude::pyfunction
/// [2]: crate::wrap_pyfunction
fn add_function(&self, fun: &Bound<'_, PyCFunction>) -> PyResult<()>;
fn add_function(&self, fun: Bound<'_, PyCFunction>) -> PyResult<()>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is just to make it nicer, so that we don't have to use&wrap_pyfunction! instead of just wrap_pyfunction!?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think it's probably worth making this change? It's not like we expect this to be called in a hot loop. I guess we could also have impl AsRef<Bound<'_, PyCFunction>>?


#[doc(hidden)]
fn wrap_pyfunction(&self, method_def: &PyMethodDef) -> PyResult<Bound<'_, PyCFunction>>;
}

impl<'py> PyModuleMethods<'py> for Bound<'py, PyModule> {
Expand Down Expand Up @@ -700,10 +711,14 @@ impl<'py> PyModuleMethods<'py> for Bound<'py, PyModule> {
self.add(name, module)
}

fn add_function(&self, fun: &Bound<'_, PyCFunction>) -> PyResult<()> {
fn add_function(&self, fun: Bound<'_, PyCFunction>) -> PyResult<()> {
let name = fun.getattr(__name__(self.py()))?;
self.add(name.downcast_into::<PyString>()?, fun)
}

fn wrap_pyfunction(&self, method_def: &PyMethodDef) -> PyResult<Bound<'_, PyCFunction>> {
PyCFunction::internal_new_bound(self.py(), method_def, Some(self))
}
}

fn __all__(py: Python<'_>) -> &Bound<'_, PyString> {
Expand Down
13 changes: 7 additions & 6 deletions tests/test_module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ fn subfunction() -> String {
}

fn submodule(module: &Bound<'_, PyModule>) -> PyResult<()> {
module.add_function(&wrap_pyfunction!(subfunction, module.as_gil_ref())?.as_borrowed())?;
module.add_function(wrap_pyfunction_bound!(subfunction, module)?)?;
Ok(())
}

Expand Down Expand Up @@ -295,18 +295,19 @@ fn test_module_nesting() {
// Test that argument parsing specification works for pyfunctions

#[pyfunction(signature = (a=5, *args))]
fn ext_vararg_fn(py: Python<'_>, a: i32, args: &PyTuple) -> PyObject {
[a.to_object(py), args.into()].to_object(py)
fn ext_vararg_fn(py: Python<'_>, a: i32, args: &Bound<'_, PyTuple>) -> PyObject {
[a.to_object(py), args.into_py(py)].to_object(py)
}

#[pymodule]
fn vararg_module(_py: Python<'_>, m: &PyModule) -> PyResult<()> {
fn vararg_module(_py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> {
#[pyfn(m, signature = (a=5, *args))]
fn int_vararg_fn(py: Python<'_>, a: i32, args: &PyTuple) -> PyObject {
fn int_vararg_fn(py: Python<'_>, a: i32, args: &Bound<'_, PyTuple>) -> PyObject {
ext_vararg_fn(py, a, args)
}

m.add_function(wrap_pyfunction!(ext_vararg_fn, m)?).unwrap();
m.add_function(wrap_pyfunction_bound!(ext_vararg_fn, m)?)
.unwrap();
Ok(())
}

Expand Down
Loading
Loading