Some safe functions are marked unsafe?

[nw0]

When I was looking at the remaining unsafe functions without safety notes for #698, I noticed that a couple of functions didn't seem to be unsafe at all.

This issue is about whether these functions should be safe to call (i.e. no need to be marked unsafe) because they wrap up the unsafety, or aren't actually unsafe.

1. PyLayout::py_init. This function takes ownership of a value: T, which could be leaked in the worst case, but leaking memory is considered safe Rust. For reference, both implementations don't use any unsafe code.
   

   pyo3/src/type_object.rs

   Line 23 in b6f595b

   unsafe fn py_init(&mut self, _value: T) {}

2. PyClassDict::clear_dict. In this case the implementation calls the FFI function PyDict_Clear, which seems to be safe as it operates solely on the refcounted contents of the class dict.
   

   pyo3/src/pyclass_slots.rs

   Line 9 in 3de51d5

   unsafe fn clear_dict(&mut self, _py: Python) {}

3. PyClassAlloc::new. This feels like implementations should encapsulate any unsafety, but I'm not sure.
   

   pyo3/src/pyclass.rs

   Line 75 in 3de51d5

   pub trait PyClassAlloc: PyTypeInfo + Sized {

Opening this issue as suggested in #1388 (comment), even if it's not obvious that we want to change the unsafe marking on any of the functions. I realise that most of these APIs are probably only used internally.

[davidhewitt]

I agree with 1 + 2 being needlessly unsafe, good catch. I'm in favour of removing unsafe from these APIs.

PyClassAlloc::new should continue to be unsafe to call, because it takes a raw pointer and so has to derefence it, which is unsafe.

[nw0]

PyClassAlloc::new should continue to be unsafe to call, because it takes a raw pointer and so has to derefence it, which is unsafe.

Oops, yes, of course.

4. opt_to_pyobj only gets the pointers to a PyObj, or None, which is itself safe (as it doesn't actually use the pointers).

   pyo3/src/types/datetime.rs

   Line 374 in 3de51d5

   unsafe fn opt_to_pyobj(py: Python, opt: Option<&PyObject>) -> *mut ffi::PyObject {

I've briefly looked at all the unsafe fns in PyO3, and the rest seem bona fide unsafe as they are currently defined. I can file a PR to change 1 and 2 (and 4?), if you'd like.

[davidhewitt]

Agreed that opt_to_pyobj is also safe.

I can file a PR to change 1 and 2 (and 4?), if you'd like.

If you're willing, that'd be very much appreciated! It'd technically be breaking so I probably wouldn't be able to merge the PR for a little bit until we're ready to merge all the breaking changes that will go into 0.14.

[nw0]

Sorry to post on an old issue: I think clear_weakrefs is another of these actually safe functions.

https://github.com/PyO3/pyo3/blob/master/src/pyclass_slots.rs#L69

PyObject_ClearWeakRefs clears the remaining weak references to the object. A weak reference (another PyObject)

[...] will allow access to the referenced object if it hasn't been collected and to determine if the object still exists in memory. Retrieving the referent is done by calling the reference object. If the referent is no longer alive, this will return None instead. (PEP 205)

I believe it's safe to clear weak references, as their being cleared "unexpectedly" is explicitly why weak references exist. If not, I suppose I'm about to learn what remark should be put into the "Safety" docstring... (#698)

[birkenfeld]

I don't think this can be safe as long as it takes a raw pointer, since there is no guarantee that it points to a valid PyObject.

[nw0]

Ah, of course -- my bad