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

NonNull pointer for Py, PyObject #260

Merged
merged 1 commit into from
Nov 11, 2018
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
33 changes: 17 additions & 16 deletions src/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

use std;
use std::marker::PhantomData;
use std::ptr::NonNull;
use std::rc::Rc;

use crate::conversion::{FromPyObject, IntoPyObject, ToPyObject};
Expand All @@ -10,7 +11,7 @@ use crate::ffi;
use crate::instance;
use crate::object::PyObject;
use crate::objectprotocol::ObjectProtocol;
use crate::python::{IntoPyPointer, Python, ToPyPointer};
use crate::python::{IntoPyPointer, Python, ToPyPointer, NonNullPyObject};
use crate::pythonrun;
use crate::typeob::PyTypeCreate;
use crate::typeob::{PyTypeInfo, PyTypeObject};
Expand Down Expand Up @@ -96,7 +97,7 @@ pub trait AsPyRef<T>: Sized {

/// Safe wrapper around unsafe `*mut ffi::PyObject` pointer with specified type information.
#[derive(Debug)]
pub struct Py<T>(pub *mut ffi::PyObject, std::marker::PhantomData<T>);
pub struct Py<T>(NonNullPyObject, std::marker::PhantomData<T>);

// `Py<T>` is thread-safe, because any python related operations require a Python<'p> token.
unsafe impl<T> Send for Py<T> {}
Expand All @@ -112,18 +113,17 @@ impl<T> Py<T> {
!ptr.is_null() && ffi::Py_REFCNT(ptr) > 0,
format!("REFCNT: {:?} - {:?}", ptr, ffi::Py_REFCNT(ptr))
);
Py(ptr, std::marker::PhantomData)
Py(NonNull::new_unchecked(ptr), std::marker::PhantomData)
}

/// Creates a `Py<T>` instance for the given FFI pointer.
/// Panics if the pointer is `null`.
/// Undefined behavior if the pointer is invalid.
#[inline]
pub unsafe fn from_owned_ptr_or_panic(ptr: *mut ffi::PyObject) -> Py<T> {
if ptr.is_null() {
crate::err::panic_after_error();
} else {
Py::from_owned_ptr(ptr)
kngwyu marked this conversation as resolved.
Show resolved Hide resolved
match NonNull::new(ptr) {
Some(nonnull_ptr) => { Py(nonnull_ptr, std::marker::PhantomData) },
None => { crate::err::panic_after_error(); }
}
}

Expand All @@ -132,10 +132,9 @@ impl<T> Py<T> {
/// Returns `Err(PyErr)` if the pointer is `null`.
/// Unsafe because the pointer might be invalid.
pub unsafe fn from_owned_ptr_or_err(py: Python, ptr: *mut ffi::PyObject) -> PyResult<Py<T>> {
if ptr.is_null() {
Err(PyErr::fetch(py))
} else {
Ok(Py::from_owned_ptr(ptr))
match NonNull::new(ptr) {
Some(nonnull_ptr) => { Ok(Py(nonnull_ptr, std::marker::PhantomData)) },
None => { Err(PyErr::fetch(py)) }
}
}

Expand All @@ -149,19 +148,19 @@ impl<T> Py<T> {
format!("REFCNT: {:?} - {:?}", ptr, ffi::Py_REFCNT(ptr))
);
ffi::Py_INCREF(ptr);
Py::from_owned_ptr(ptr)
Py(NonNull::new_unchecked(ptr), std::marker::PhantomData)
}

/// Gets the reference count of the ffi::PyObject pointer.
#[inline]
pub fn get_refcnt(&self) -> isize {
unsafe { ffi::Py_REFCNT(self.0) }
unsafe { ffi::Py_REFCNT(self.0.as_ptr()) }
}

/// Clone self, Calls Py_INCREF() on the ptr.
#[inline]
pub fn clone_ref(&self, _py: Python) -> Py<T> {
unsafe { Py::from_borrowed_ptr(self.0) }
unsafe { Py::from_borrowed_ptr(self.0.as_ptr()) }
}
}

Expand Down Expand Up @@ -241,9 +240,11 @@ impl<T> AsPyRef<T> for Py<T>
where
T: PyTypeInfo,
{
#[inline]
fn as_ref(&self, py: Python) -> &T {
self.as_ref_dispatch(py)
}
#[inline]
fn as_mut(&mut self, py: Python) -> &mut T {
self.as_mut_dispatch(py)
}
Expand All @@ -269,7 +270,7 @@ impl<T> ToPyPointer for Py<T> {
/// Gets the underlying FFI pointer, returns a borrowed pointer.
#[inline]
fn as_ptr(&self) -> *mut ffi::PyObject {
self.0
self.0.as_ptr()
}
}

Expand All @@ -278,7 +279,7 @@ impl<T> IntoPyPointer for Py<T> {
#[inline]
#[must_use]
fn into_ptr(self) -> *mut ffi::PyObject {
let ptr = self.0;
let ptr = self.0.as_ptr();
std::mem::forget(self);
ptr
}
Expand Down
42 changes: 20 additions & 22 deletions src/object.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
// Copyright (c) 2017-present PyO3 Project and Contributors

use std;
use std::ptr::NonNull;

use crate::conversion::{
FromPyObject, IntoPyObject, IntoPyTuple, PyTryFrom, ToBorrowedObject, ToPyObject,
};
use crate::err::{PyDowncastError, PyErr, PyResult};
use crate::ffi;
use crate::instance::{AsPyRef, PyObjectWithToken};
use crate::python::{IntoPyPointer, Python, ToPyPointer};
use crate::python::{IntoPyPointer, Python, ToPyPointer, NonNullPyObject};
use crate::pythonrun;
use crate::types::{PyDict, PyObjectRef, PyTuple};

Expand All @@ -20,7 +21,7 @@ use crate::types::{PyDict, PyObjectRef, PyTuple};
/// Technically, it is a safe wrapper around the unsafe `*mut ffi::PyObject` pointer.
#[derive(Debug)]
#[repr(transparent)]
pub struct PyObject(*mut ffi::PyObject);
pub struct PyObject(NonNullPyObject);

// `PyObject` is thread-safe, any python related operations require a Python<'p> token.
unsafe impl Send for PyObject {}
Expand All @@ -36,40 +37,37 @@ impl PyObject {
!ptr.is_null() && ffi::Py_REFCNT(ptr) > 0,
format!("REFCNT: {:?} - {:?}", ptr, ffi::Py_REFCNT(ptr))
);
PyObject(ptr)
PyObject(NonNull::new_unchecked(ptr))
}

/// Creates a `PyObject` instance for the given FFI pointer.
/// Panics if the pointer is `null`.
/// Undefined behavior if the pointer is invalid.
#[inline]
pub unsafe fn from_owned_ptr_or_panic(py: Python, ptr: *mut ffi::PyObject) -> PyObject {
if ptr.is_null() {
crate::err::panic_after_error();
} else {
PyObject::from_owned_ptr(py, ptr)
pub unsafe fn from_owned_ptr_or_panic(_py: Python, ptr: *mut ffi::PyObject) -> PyObject {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the unused _py arg leads to pyfunction macro breaking in a way I'm not familiar enough to address now.

Copy link
Member

Choose a reason for hiding this comment

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

It's because you need to have the GIL acquired or the python c api calls done by panic_after_error and the debug version of from_owned_ptr are undefined behavior.

match NonNull::new(ptr) {
Some(nonnull_ptr) => { PyObject(nonnull_ptr) },
None => { crate::err::panic_after_error(); }
}
}

/// Construct `PyObject` from the result of a Python FFI call that
/// returns a new reference (owned pointer).
/// Returns `Err(PyErr)` if the pointer is `null`.
pub unsafe fn from_owned_ptr_or_err(py: Python, ptr: *mut ffi::PyObject) -> PyResult<PyObject> {
if ptr.is_null() {
Err(PyErr::fetch(py))
} else {
Ok(PyObject::from_owned_ptr(py, ptr))
match NonNull::new(ptr) {
Some(nonnull_ptr) => { Ok(PyObject(nonnull_ptr)) },
None => { Err(PyErr::fetch(py)) }
}
}

/// Construct `PyObject` from the result of a Python FFI call that
/// returns a new reference (owned pointer).
/// Returns `None` if the pointer is `null`.
pub unsafe fn from_owned_ptr_or_opt(py: Python, ptr: *mut ffi::PyObject) -> Option<PyObject> {
if ptr.is_null() {
None
} else {
Some(PyObject::from_owned_ptr(py, ptr))
pub unsafe fn from_owned_ptr_or_opt(_py: Python, ptr: *mut ffi::PyObject) -> Option<PyObject> {
match NonNull::new(ptr) {
Some(nonnull_ptr) => { Some(PyObject(nonnull_ptr)) },
None => { None }
}
}

Expand All @@ -83,7 +81,7 @@ impl PyObject {
format!("REFCNT: {:?} - {:?}", ptr, ffi::Py_REFCNT(ptr))
);
ffi::Py_INCREF(ptr);
PyObject(ptr)
PyObject(NonNull::new_unchecked(ptr))
}

/// Creates a `PyObject` instance for the given Python FFI pointer.
Expand Down Expand Up @@ -116,7 +114,7 @@ impl PyObject {

/// Gets the reference count of the ffi::PyObject pointer.
pub fn get_refcnt(&self) -> isize {
unsafe { ffi::Py_REFCNT(self.0) }
unsafe { ffi::Py_REFCNT(self.0.as_ptr()) }
}

/// Clone self, Calls Py_INCREF() on the ptr.
Expand Down Expand Up @@ -266,15 +264,15 @@ impl ToPyPointer for PyObject {
/// Gets the underlying FFI pointer, returns a borrowed pointer.
#[inline]
fn as_ptr(&self) -> *mut ffi::PyObject {
self.0
self.0.as_ptr()
}
}

impl<'a> ToPyPointer for &'a PyObject {
/// Gets the underlying FFI pointer, returns a borrowed pointer.
#[inline]
fn as_ptr(&self) -> *mut ffi::PyObject {
self.0
self.0.as_ptr()
}
}

Expand All @@ -283,7 +281,7 @@ impl IntoPyPointer for PyObject {
#[inline]
#[must_use]
fn into_ptr(self) -> *mut ffi::PyObject {
let ptr = self.0;
let ptr = self.0.as_ptr();
std::mem::forget(self); // Avoid Drop
ptr
}
Expand Down
5 changes: 4 additions & 1 deletion src/python.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,13 @@ use crate::pythonrun::{self, GILGuard};
use crate::typeob::PyTypeCreate;
use crate::typeob::{PyTypeInfo, PyTypeObject};
use crate::types::{PyDict, PyModule, PyObjectRef, PyType};
use std;
use std::ffi::CString;
use std::marker::PhantomData;
use std::os::raw::c_int;
use std::ptr::NonNull;
use std;

pub type NonNullPyObject = NonNull<ffi::PyObject>;

/// Marker type that indicates that the GIL is currently held.
///
Expand Down
6 changes: 3 additions & 3 deletions src/pythonrun.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Copyright (c) 2017-present PyO3 Project and Contributors
use crate::ffi;
use crate::python::Python;
use crate::python::{Python, NonNullPyObject};
use crate::types::PyObjectRef;
use spin;
use std::{any, marker, rc, sync};
Expand Down Expand Up @@ -230,12 +230,12 @@ pub unsafe fn register_any<'p, T: 'static>(obj: T) -> &'p T {
.unwrap()
}

pub unsafe fn register_pointer(obj: *mut ffi::PyObject) {
pub unsafe fn register_pointer(obj: NonNullPyObject) {
let pool: &'static mut ReleasePool = &mut *POOL;

let mut v = pool.p.lock();
let pool: &'static mut Vec<*mut ffi::PyObject> = &mut *(*v);
pool.push(obj);
pool.push(obj.as_ptr());
}

pub unsafe fn register_owned(_py: Python, obj: *mut ffi::PyObject) -> &PyObjectRef {
Expand Down
1 change: 1 addition & 0 deletions src/typeob.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ impl PyRawObject {
}

impl<T: PyTypeInfo> AsRef<T> for PyRawObject {
#[inline]
fn as_ref(&self) -> &T {
// TODO: check is object initialized
unsafe {
Expand Down
1 change: 1 addition & 0 deletions src/types/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ macro_rules! pyobject_native_type_named (
impl<$($type_param,)*> $crate::PyNativeType for $name {}

impl<$($type_param,)*> ::std::convert::AsRef<$crate::types::PyObjectRef> for $name {
#[inline]
fn as_ref(&self) -> &$crate::types::PyObjectRef {
unsafe{&*(self as *const $name as *const $crate::types::PyObjectRef)}
}
Expand Down