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

LazyStaticType::get_or_init returns an *mut instead of a & ref #994

Merged
merged 1 commit into from
Jun 23, 2020

Conversation

scalexm
Copy link
Contributor

@scalexm scalexm commented Jun 22, 2020

Previously, we were returning a &ffi::PyTypeObject. However, we were often casting this immutable reference to a *mut ffi::PyTypeObject and then passing that pointer to FFI functions.

But we don't know whether the FFI functions mutate the PyTypeObject behind our back or not, and if they do, I believe that this is Undefined Behavior because the pointer comes from a shared reference.

Unfortunately, these things are very badly documented: I'm only aware of this because I followed many Unsafe Code Guideline discussions. Best source that I could get is the following comment from @RalfJung, who is very knowledgeable on the subject. The details are probably written in the Stacked Borrows paper, to be fair.

Even if it's not UB right now because of ill-specification or because of the compiler not taking advantage of it, we'd better be safe.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

This seems like a sensible idea to me!

@davidhewitt
Copy link
Member

I wonder about a possible further refactoring: PyTypeInfo could be PyTypeInfo: PyTypeObject, and we could add an .as_raw() method to pyo3::types::PyType which returns *mut ffi::PyTypeObject.

@kngwyu
Copy link
Member

kngwyu commented Jun 23, 2020

I wonder about a possible further refactoring: PyTypeInfo could be PyTypeInfo: PyTypeObject, and we could add an .as_raw() method to pyo3::types::PyType which returns *mut ffi::PyTypeObject.

Since impl<T: PyTypeInfo> PyTypeObject for T uses type_object_raw, I don't think it's possible.

@kngwyu
Copy link
Member

kngwyu commented Jun 23, 2020

Thanks!

@kngwyu kngwyu merged commit f757c99 into PyO3:master Jun 23, 2020
@RalfJung
Copy link

RalfJung commented Jun 23, 2020

Unfortunately, these things are very badly documented: I'm only aware of this because I followed many Unsafe Code Guideline discussions. Best source that I could get is the following comment from @RalfJung, who is very knowledgeable on the subject. The details are probably written in the Stacked Borrows paper, to be fair.

Yeah, the details of our reference rules are pretty fuzzy, and Stacked Borrows is a promising next step but it is not an official model yet.

However, this page is clear at least about some things. It is UB to do the following:

Mutating immutable data. All data inside a const item is immutable. Moreover, all data reached through a shared reference or data owned by an immutable binding is immutable, unless that data is contained within an UnsafeCell.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants