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

Remove static mut from PyTypeInfo implementation #751

Merged
merged 3 commits into from
Feb 3, 2020

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Jan 30, 2020

Fixes #699 .

I looked at using lazy_static! and a couple other options, in the end I went for once_cell.

I modified the signature of PyTypeInfo::type_object() to return *mut instead of &mut. It's a minor thing but handing out &mut in this way is definitely breaking Rust's aliasing rules.

Also removed unsafe from the function because there's nothing unsafe about calling it - the user is responsible for using the returned *mut ffi::PyTypeInfo pointer safely.

Might need a changelog entry because the trait signature changed?

(As a note, I plan to follow up this PR with a second one which removes the need for init_type(). Let's start with this to keep the PR smaller and simpler.)

@programmerjake
Copy link
Contributor

the PyTypeInfo trait should be unsafe, because otherwise a user can cause UB by mis-implementing it.

@kngwyu
Copy link
Member

kngwyu commented Jan 30, 2020

I modified the signature of PyTypeInfo::type_object() to return *mut instead of &mut.

👍 for this change.

So... I think this PR is good but the problem is our pair of PyTypeInfo::type_object and PyTypeObject has really complex semantics(I don't remember correctly... but maybe to avoid specialization?).

I'm sorry but I'm going to merge this PR after understanding the problem clearer.

@davidhewitt
Copy link
Member Author

No rush. 👍

I spent some time thinking about PyTypeObject this morning. I experimented with merging PyTypeInfo and PyTypeObject, but it leads to refactorings across the codebase. Probably too big to land in a single PR. I'll share notes about it sometime.

@davidhewitt
Copy link
Member Author

davidhewitt commented Jan 30, 2020

I had some ideas to take this a little further:

  • Now type_object() always returns an initialized PyTypeObject.
  • type_object() now returns NonNull instead of *mut.
  • Removed PyTypeObject::init_type() - it's no longer needed.
  • Removed initialize_type<T>() - it's no longer possible to initialize the type object manually other than by calling type_object().
  • PyTypeInfo is now unsafe trait
  • Also removed use of static mut in all the exceptions.rs macros.

@kngwyu
Copy link
Member

kngwyu commented Feb 2, 2020

Sorry for the delay with the review..., but now I'm considering fn type_object() -> Pin<&'static ffi::PyTypeObject for this purpose and experimenting it.

@davidhewitt
Copy link
Member Author

davidhewitt commented Feb 2, 2020

Please do experiment with better designs! I'd rather we make these changes once and get them right than break stuff again and again :)

Let me know if you come up with something you prefer and want me to change this PR.

@kngwyu
Copy link
Member

kngwyu commented Feb 2, 2020

I tried to remove Box::new in create_type_object but this attempt just told me that ffi::PyType_Ready requires type object has a static address 🙄
So now I think the currently implementation is sufficiently good, except some minor things.

I also tried Pin<&'static ffi::PyTypeObject>, but finally I didn't like it since it requires too many pointer conversion.

@kngwyu kngwyu force-pushed the remove-static-mut branch from 5d13505 to 7531b9f Compare February 2, 2020 13:51
@kngwyu
Copy link
Member

kngwyu commented Feb 2, 2020

force-push is my failure... don't mind it.

pyo3-derive-backend/src/pyclass.rs Show resolved Hide resolved
src/pyclass.rs Show resolved Hide resolved
src/type_object.rs Outdated Show resolved Hide resolved
@kngwyu
Copy link
Member

kngwyu commented Feb 3, 2020

Thank you!

@kngwyu kngwyu merged commit 8fea23a into PyO3:master Feb 3, 2020
@davidhewitt davidhewitt deleted the remove-static-mut branch February 8, 2020 19:10
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.

static mut should not be used
3 participants