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

Moving PyObjectInit into impl_ and sealing the trait #4611

Merged
merged 5 commits into from
Oct 13, 2024

Conversation

Cheukting
Copy link
Contributor

Potentially closes #4552

PyNativeTypeInitializer and PyObjectInit are moved into impl_ but not PyClassInitializer as it is used in the prelude and used in the lib so I assume that is available to the public.

PyObjectInit is now a Sealed trait

Copy link
Contributor

@Icxolu Icxolu left a comment

Choose a reason for hiding this comment

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

Thanks! Just some small comments, otherwise looks good 🚀

#[doc(hidden)]
fn can_be_subclassed(&self) -> bool;

private_decl! {}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a different method of sealing, it can be removed now, together with private_impl!() below.

Copy link
Member

Choose a reason for hiding this comment

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

It would actually be a nice change in this PR (or a follow up) to replace all uses of private_decl! / private_impl! with sealing, and remove that older method.

Copy link
Contributor Author

@Cheukting Cheukting Oct 12, 2024

Choose a reason for hiding this comment

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

Sure, that means some of the traits in pyclass need to be Sealed, which is what I plan to do next. I think I will do it in another PR

src/sealed.rs Outdated
@@ -46,3 +49,7 @@ impl<T> Sealed for AddTypeToModule<T> {}
impl<T> Sealed for AddClassToModule<T> {}
impl Sealed for PyMethodDef {}
impl Sealed for ModuleDef {}

impl<T: PyObjectInit<T>> Sealed for T {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
impl<T: PyObjectInit<T>> Sealed for T {}

I don't think we should add a blanket implementation for Sealed. If we implement it for types that we don't control we have to be careful to not open the seal. In this case it seems we don't need that impl at all, so we should just remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that, I have no idea why I added that. Me just being silly I guess

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.

Thanks, agreed with the above comments 👍

Comment on lines +28 to +31
/// Initializer for Python native types, like `PyDict`.
pub struct PyNativeTypeInitializer<T: PyTypeInfo>(pub PhantomData<T>);

impl<T: PyTypeInfo> PyObjectInit<T> for PyNativeTypeInitializer<T> {
Copy link
Member

Choose a reason for hiding this comment

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

I'm happy with moving this to impl_ for now; I think there is a good chance we need to come up with a new way of initializing native super-types, but it won't stick around in this current form 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hope this got merged before further development 😅

Copy link
Contributor

@Icxolu Icxolu left a comment

Choose a reason for hiding this comment

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

Perfect! Thanks a lot 💯

@Icxolu Icxolu added this pull request to the merge queue Oct 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 13, 2024
@davidhewitt
Copy link
Member

Looks like there's still some freethreaded flake to investigate another time

@davidhewitt davidhewitt added this pull request to the merge queue Oct 13, 2024
Merged via the queue into PyO3:main with commit 12cac55 Oct 13, 2024
42 of 43 checks passed
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.

PyObjectInit should be private (and sealed)
3 participants