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

Implement ml_meth as an union. #2166

Merged
merged 3 commits into from
Feb 15, 2022
Merged
Show file tree
Hide file tree
Changes from all 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: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- `PyTZInfo_CheckExact`
- `PyDateTime_FromTimestamp`
- `PyDate_FromTimestamp`
- The `ml_meth` field of `PyMethodDef` is now represented by the `PyMethodDefPointer` union [2166](https://github.com/PyO3/pyo3/pull/2166)

### Removed

- Remove all functionality deprecated in PyO3 0.14. [#2007](https://github.com/PyO3/pyo3/pull/2007)
- Remove `Default` impl for `PyMethodDef` [2166](https://github.com/PyO3/pyo3/pull/2166)

### Fixed

Expand Down
14 changes: 6 additions & 8 deletions pyo3-ffi/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ features = ["extension-module"]

**`src/lib.rs`**
```rust
use std::mem::transmute;
use std::os::raw::c_char;
use std::ptr;

use pyo3_ffi::*;

Expand All @@ -72,13 +72,11 @@ pub unsafe extern "C" fn PyInit_string_sum() -> *mut PyObject {
PyUnicode_FromStringAndSize(version.as_ptr() as *const c_char, version.len() as isize),
);

// It's necessary to transmute `sum_as_string` here because functions marked with `METH_FASTCALL`
// have a different signature. However the `PyMethodDef` struct currently represents all
// functions as a `PyCFunction`. The python interpreter will cast the function pointer back
// to `_PyCFunctionFast`.
let wrapped_sum_as_string = PyMethodDef {
ml_name: "sum_as_string\0".as_ptr() as *const c_char,
ml_meth: Some(transmute::<_PyCFunctionFast, PyCFunction>(sum_as_string)),
ml_meth: PyMethodDefPointer {
_PyCFunctionFast: sum_as_string
},
ml_flags: METH_FASTCALL,
ml_doc: "returns the sum of two integers as a string\0".as_ptr() as *const c_char,
};
Expand Down Expand Up @@ -127,7 +125,7 @@ pub unsafe extern "C" fn sum_as_string(

let arg1 = PyLong_AsLong(arg1);
if !PyErr_Occurred().is_null() {
return ptr::null()
return ptr::null_mut()
}

let arg2 = *args.add(1);
Expand All @@ -137,7 +135,7 @@ pub unsafe extern "C" fn sum_as_string(

let arg2 = PyLong_AsLong(arg2);
if !PyErr_Occurred().is_null() {
return ptr::null()
return ptr::null_mut()
}


Expand Down
14 changes: 6 additions & 8 deletions pyo3-ffi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@
//!
//! **`src/lib.rs`**
//! ```rust
//! use std::mem::transmute;
//! use std::os::raw::c_char;
//! use std::ptr;
//!
//! use pyo3_ffi::*;
//!
Expand All @@ -111,13 +111,11 @@
//! PyUnicode_FromStringAndSize(version.as_ptr() as *const c_char, version.len() as isize),
//! );
//!
//! // It's necessary to transmute `sum_as_string` here because functions marked with `METH_FASTCALL`
//! // have a different signature. However the `PyMethodDef` struct currently represents all
//! // functions as a `PyCFunction`. The python interpreter will cast the function pointer back
//! // to `_PyCFunctionFast`.
//! let wrapped_sum_as_string = PyMethodDef {
//! ml_name: "sum_as_string\0".as_ptr() as *const c_char,
//! ml_meth: Some(transmute::<_PyCFunctionFast, PyCFunction>(sum_as_string)),
//! ml_meth: PyMethodDefPointer {
//! _PyCFunctionFast: sum_as_string
//! },
//! ml_flags: METH_FASTCALL,
//! ml_doc: "returns the sum of two integers as a string\0".as_ptr() as *const c_char,
//! };
Expand Down Expand Up @@ -166,7 +164,7 @@
//!
//! let arg1 = PyLong_AsLong(arg1);
//! if !PyErr_Occurred().is_null() {
//! return ptr::null()
//! return ptr::null_mut()
//! }
//!
//! let arg2 = *args.add(1);
Expand All @@ -176,7 +174,7 @@
//!
//! let arg2 = PyLong_AsLong(arg2);
//! if !PyErr_Occurred().is_null() {
//! return ptr::null()
//! return ptr::null_mut()
//! }
//! let res = (arg1 + arg2).to_string();
//! PyUnicode_FromStringAndSize(res.as_ptr() as *const c_char, res.len() as isize)
Expand Down
53 changes: 45 additions & 8 deletions pyo3-ffi/src/methodobject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ pub unsafe fn PyCFunction_Check(op: *mut PyObject) -> c_int {
pub type PyCFunction =
unsafe extern "C" fn(slf: *mut PyObject, args: *mut PyObject) -> *mut PyObject;

#[cfg(not(Py_LIMITED_API))]
#[cfg(any(Py_3_10, not(Py_LIMITED_API)))]
pub type _PyCFunctionFast = unsafe extern "C" fn(
slf: *mut PyObject,
args: *mut *mut PyObject,
Expand All @@ -52,7 +52,14 @@ pub type _PyCFunctionFastWithKeywords = unsafe extern "C" fn(
kwnames: *mut PyObject,
) -> *mut PyObject;

// skipped PyCMethod (since 3.9)
#[cfg(all(Py_3_9, not(Py_LIMITED_API)))]
pub type PyCMethod = unsafe extern "C" fn(
slf: *mut PyObject,
defining_class: *mut PyTypeObject,
args: *const *mut PyObject,
nargs: crate::pyport::Py_ssize_t,
kwnames: *mut PyObject,
) -> *mut PyObject;

extern "C" {
#[cfg_attr(PyPy, link_name = "PyPyCFunction_GetFunction")]
Expand All @@ -71,17 +78,45 @@ extern "C" {
#[derive(Copy, Clone)]
pub struct PyMethodDef {
pub ml_name: *const c_char,
pub ml_meth: Option<PyCFunction>,
pub ml_meth: PyMethodDefPointer,
pub ml_flags: c_int,
pub ml_doc: *const c_char,
}

impl Default for PyMethodDef {
fn default() -> PyMethodDef {
unsafe { mem::zeroed() }
}
Comment on lines -79 to -82
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this default impl makes sense if PyMethodDefPointer is never null.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed; I also actually generally don't like having any implementations of traits or methods for pyo3-ffi types if possible (except for Copy & Clone). Makes it closer to the C that they come from.

/// Function types used to implement Python callables.
///
/// This function pointer must be accompanied by the correct [ml_flags](PyMethodDef::ml_flags),
/// otherwise the behavior is undefined.
///
/// See the [Python C API documentation][1] for more information.
///
/// [1]: https://docs.python.org/3/c-api/structures.html#implementing-functions-and-methods
#[repr(C)]
#[derive(Copy, Clone)]
pub union PyMethodDefPointer {
/// This variant corresponds with [`METH_VARARGS`] *or* [`METH_NOARGS`] *or* [`METH_O`].
pub PyCFunction: PyCFunction,

/// This variant corresponds with [`METH_VARARGS`] | [`METH_KEYWORDS`].
pub PyCFunctionWithKeywords: PyCFunctionWithKeywords,

/// This variant corresponds with [`METH_FASTCALL`].
#[cfg(any(Py_3_10, not(Py_LIMITED_API)))]
pub _PyCFunctionFast: _PyCFunctionFast,

/// This variant corresponds with [`METH_FASTCALL`] | [`METH_KEYWORDS`].
#[cfg(not(Py_LIMITED_API))]
pub _PyCFunctionFastWithKeywords: _PyCFunctionFastWithKeywords,

/// This variant corresponds with [`METH_METHOD`] | [`METH_FASTCALL`] | [`METH_KEYWORDS`].
#[cfg(all(Py_3_9, not(Py_LIMITED_API)))]
pub PyCMethod: PyCMethod,
}

// TODO: This can be a const assert on Rust 1.57
const _: () =
[()][mem::size_of::<PyMethodDefPointer>() - mem::size_of::<Option<extern "C" fn()>>()];

extern "C" {
#[cfg_attr(PyPy, link_name = "PyPyCFunction_New")]
pub fn PyCFunction_New(ml: *mut PyMethodDef, slf: *mut PyObject) -> *mut PyObject;
Expand Down Expand Up @@ -122,7 +157,9 @@ be specified alone or with METH_KEYWORDS. */
pub const METH_FASTCALL: c_int = 0x0080;

// skipped METH_STACKLESS
// skipped METH_METHOD

#[cfg(all(Py_3_9, not(Py_LIMITED_API)))]
pub const METH_METHOD: c_int = 0x0200;

extern "C" {
#[cfg(not(Py_3_9))]
Expand Down
14 changes: 9 additions & 5 deletions src/impl_/pymethods.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,17 +163,21 @@ impl PyMethodDef {
/// Convert `PyMethodDef` to Python method definition struct `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) },
PyMethodType::PyCFunction(meth) => ffi::PyMethodDefPointer {
PyCFunction: meth.0,
},
PyMethodType::PyCFunctionWithKeywords(meth) => ffi::PyMethodDefPointer {
PyCFunctionWithKeywords: meth.0,
},
#[cfg(not(Py_LIMITED_API))]
PyMethodType::PyCFunctionFastWithKeywords(meth) => unsafe {
std::mem::transmute(meth.0)
PyMethodType::PyCFunctionFastWithKeywords(meth) => ffi::PyMethodDefPointer {
_PyCFunctionFastWithKeywords: meth.0,
},
};

Ok(ffi::PyMethodDef {
ml_name: get_name(self.ml_name)?.as_ptr(),
ml_meth: Some(meth),
ml_meth: meth,
ml_flags: self.ml_flags,
ml_doc: get_doc(self.ml_doc)?.as_ptr(),
})
Expand Down