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

seals PyAnyMethods and friends #3909

Merged
merged 2 commits into from
Feb 28, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
11 changes: 1 addition & 10 deletions src/ffi_ptr_ext.rs
Original file line number Diff line number Diff line change
@@ -1,19 +1,10 @@
use crate::sealed::Sealed;
use crate::{
ffi,
instance::{Borrowed, Bound},
PyAny, PyResult, Python,
};

mod sealed {
use super::*;

pub trait Sealed {}

impl Sealed for *mut ffi::PyObject {}
}

use sealed::Sealed;

pub(crate) trait FfiPtrExt: Sealed {
unsafe fn assume_owned_or_err(self, py: Python<'_>) -> PyResult<Bound<'_, PyAny>>;
unsafe fn assume_owned_or_opt(self, py: Python<'_>) -> Option<Bound<'_, PyAny>>;
Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,7 @@ pub use crate::version::PythonVersionInfo;

pub(crate) mod ffi_ptr_ext;
pub(crate) mod py_result_ext;
pub(crate) mod sealed;

/// Old module which contained some implementation details of the `#[pyproto]` module.
///
Expand Down
31 changes: 31 additions & 0 deletions src/sealed.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
use crate::types::{
PyBool, PyByteArray, PyBytes, PyCapsule, PyComplex, PyDict, PyFloat, PyFrozenSet, PyList,
PyMapping, PyModule, PySequence, PySet, PySlice, PyString, PyTraceback, PyTuple, PyType,
};
use crate::{ffi, Bound, PyAny};

pub trait Sealed {}
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to have a single Sealed trait for the whole crate, or should we have multiple Sealed traits each only relevant for one or more types?

We have Sealed for PyResultExt trait too, which could be merged in I guess, if it makes sense to do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't think of an advantage of using multiple Sealed traits. The only relevant property is that it is a public trait, but not publicly nameable. If you can think of a good reason to split, I'm happy to do so, otherwise a central location is easy for reuse and might be easier to track changes in the future.

Hmm, now that I wrote that, maybe one disadvantage is that the sealed module is not completely private anymore, so implementation of the Sealed trait could be added anywhere in the crate instead of only in that module... How do you feel about that?

Copy link
Member

@davidhewitt davidhewitt Feb 27, 2024

Choose a reason for hiding this comment

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

I think I'm not worried about us implementing from anywhere in the crate, we can prevent that in review.

I think what I was wondering is, e.g. imagine we have a trait Foo which we only ever want implemented for Bound<T>. We seal it and we implement sealed for Bound<T>.

By adding that sealing for generic T now users can implement PyDictMethods for their own Bound<MyClass>.

If the sealing was done by separate traits then we could guarantee that the PyDictMethods seal would only ever allow Bound<PyDict> and not accidentally gave this mistake.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I am putting the cart before the horse, and for now we should just live with one Sealed trait because orphan rules will stop users adding further implementations to any of the types we've sealed here.

If there's a need to seal for a generic, we can think carefully about that at the time 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, sealing a generic type will require more (careful) thoughts. I agree that we can think about that, once we have a use case for it.


// for FfiPtrExt
impl Sealed for *mut ffi::PyObject {}

// for Py(...)Methods
impl Sealed for Bound<'_, PyAny> {}
impl Sealed for Bound<'_, PyBool> {}
impl Sealed for Bound<'_, PyByteArray> {}
impl Sealed for Bound<'_, PyBytes> {}
impl Sealed for Bound<'_, PyCapsule> {}
impl Sealed for Bound<'_, PyComplex> {}
impl Sealed for Bound<'_, PyDict> {}
impl Sealed for Bound<'_, PyFloat> {}
impl Sealed for Bound<'_, PyFrozenSet> {}
impl Sealed for Bound<'_, PyList> {}
impl Sealed for Bound<'_, PyMapping> {}
impl Sealed for Bound<'_, PyModule> {}
impl Sealed for Bound<'_, PySequence> {}
impl Sealed for Bound<'_, PySet> {}
impl Sealed for Bound<'_, PySlice> {}
impl Sealed for Bound<'_, PyString> {}
impl Sealed for Bound<'_, PyTraceback> {}
impl Sealed for Bound<'_, PyTuple> {}
impl Sealed for Bound<'_, PyType> {}
2 changes: 1 addition & 1 deletion src/types/any.rs
Original file line number Diff line number Diff line change
Expand Up @@ -939,7 +939,7 @@ impl PyAny {
/// It is recommended you import this trait via `use pyo3::prelude::*` rather than
/// by importing this trait directly.
#[doc(alias = "PyAny")]
pub trait PyAnyMethods<'py> {
pub trait PyAnyMethods<'py>: crate::sealed::Sealed {
/// Returns whether `self` and `other` point to the same object. To compare
/// the equality of two objects (the `==` operator), use [`eq`](PyAny::eq).
///
Expand Down
2 changes: 1 addition & 1 deletion src/types/boolobject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ impl PyBool {
/// syntax these methods are separated into a trait, because stable Rust does not yet support
/// `arbitrary_self_types`.
#[doc(alias = "PyBool")]
pub trait PyBoolMethods<'py> {
pub trait PyBoolMethods<'py>: crate::sealed::Sealed {
Copy link
Member

@davidhewitt davidhewitt Feb 27, 2024

Choose a reason for hiding this comment

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

Does rustdoc render the bound as Sealed or crate::sealed::Sealed? That might affect whether we use the trait first or go for the full path like here (if you think it matters).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
It seems it does not make a difference, this looks fine to me.

/// Gets whether this boolean is `true`.
fn is_true(&self) -> bool;
}
Expand Down
2 changes: 1 addition & 1 deletion src/types/bytearray.rs
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ impl PyByteArray {
/// syntax these methods are separated into a trait, because stable Rust does not yet support
/// `arbitrary_self_types`.
#[doc(alias = "PyByteArray")]
pub trait PyByteArrayMethods<'py> {
pub trait PyByteArrayMethods<'py>: crate::sealed::Sealed {
/// Gets the length of the bytearray.
fn len(&self) -> usize;

Expand Down
2 changes: 1 addition & 1 deletion src/types/bytes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ impl PyBytes {
/// syntax these methods are separated into a trait, because stable Rust does not yet support
/// `arbitrary_self_types`.
#[doc(alias = "PyBytes")]
pub trait PyBytesMethods<'py> {
pub trait PyBytesMethods<'py>: crate::sealed::Sealed {
/// Gets the Python string as a byte slice.
fn as_bytes(&self) -> &[u8];
}
Expand Down
2 changes: 1 addition & 1 deletion src/types/capsule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ impl PyCapsule {
/// syntax these methods are separated into a trait, because stable Rust does not yet support
/// `arbitrary_self_types`.
#[doc(alias = "PyCapsule")]
pub trait PyCapsuleMethods<'py> {
pub trait PyCapsuleMethods<'py>: crate::sealed::Sealed {
/// Sets the context pointer in the capsule.
///
/// Returns an error if this capsule is not valid.
Expand Down
2 changes: 1 addition & 1 deletion src/types/complex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ mod not_limited_impls {
/// syntax these methods are separated into a trait, because stable Rust does not yet support
/// `arbitrary_self_types`.
#[doc(alias = "PyComplex")]
pub trait PyComplexMethods<'py> {
pub trait PyComplexMethods<'py>: crate::sealed::Sealed {
/// Returns the real part of the complex number.
fn real(&self) -> c_double;
/// Returns the imaginary part of the complex number.
Expand Down
2 changes: 1 addition & 1 deletion src/types/dict.rs
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ impl PyDict {
/// syntax these methods are separated into a trait, because stable Rust does not yet support
/// `arbitrary_self_types`.
#[doc(alias = "PyDict")]
pub trait PyDictMethods<'py> {
pub trait PyDictMethods<'py>: crate::sealed::Sealed {
/// Returns a new dictionary that contains the same key-value pairs as self.
///
/// This is equivalent to the Python expression `self.copy()`.
Expand Down
2 changes: 1 addition & 1 deletion src/types/float.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ impl PyFloat {
/// syntax these methods are separated into a trait, because stable Rust does not yet support
/// `arbitrary_self_types`.
#[doc(alias = "PyFloat")]
pub trait PyFloatMethods<'py> {
pub trait PyFloatMethods<'py>: crate::sealed::Sealed {
/// Gets the value of this float.
fn value(&self) -> c_double;
}
Expand Down
2 changes: 1 addition & 1 deletion src/types/frozenset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ impl PyFrozenSet {
/// syntax these methods are separated into a trait, because stable Rust does not yet support
/// `arbitrary_self_types`.
#[doc(alias = "PyFrozenSet")]
pub trait PyFrozenSetMethods<'py> {
pub trait PyFrozenSetMethods<'py>: crate::sealed::Sealed {
/// Returns the number of items in the set.
///
/// This is equivalent to the Python expression `len(self)`.
Expand Down
2 changes: 1 addition & 1 deletion src/types/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ index_impls!(PyList, "list", PyList::len, PyList::get_slice);
/// syntax these methods are separated into a trait, because stable Rust does not yet support
/// `arbitrary_self_types`.
#[doc(alias = "PyList")]
pub trait PyListMethods<'py> {
pub trait PyListMethods<'py>: crate::sealed::Sealed {
/// Returns the length of the list.
fn len(&self) -> usize;

Expand Down
2 changes: 1 addition & 1 deletion src/types/mapping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ impl PyMapping {
/// syntax these methods are separated into a trait, because stable Rust does not yet support
/// `arbitrary_self_types`.
#[doc(alias = "PyMapping")]
pub trait PyMappingMethods<'py> {
pub trait PyMappingMethods<'py>: crate::sealed::Sealed {
/// Returns the number of objects in the mapping.
///
/// This is equivalent to the Python expression `len(self)`.
Expand Down
2 changes: 1 addition & 1 deletion src/types/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ impl PyModule {
/// syntax these methods are separated into a trait, because stable Rust does not yet support
/// `arbitrary_self_types`.
#[doc(alias = "PyModule")]
pub trait PyModuleMethods<'py> {
pub trait PyModuleMethods<'py>: crate::sealed::Sealed {
/// Returns the module's `__dict__` attribute, which contains the module's symbol table.
fn dict(&self) -> Bound<'py, PyDict>;

Expand Down
2 changes: 1 addition & 1 deletion src/types/sequence.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ impl PySequence {
/// syntax these methods are separated into a trait, because stable Rust does not yet support
/// `arbitrary_self_types`.
#[doc(alias = "PySequence")]
pub trait PySequenceMethods<'py> {
pub trait PySequenceMethods<'py>: crate::sealed::Sealed {
/// Returns the number of objects in sequence.
///
/// This is equivalent to the Python expression `len(self)`.
Expand Down
2 changes: 1 addition & 1 deletion src/types/set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ impl PySet {
/// syntax these methods are separated into a trait, because stable Rust does not yet support
/// `arbitrary_self_types`.
#[doc(alias = "PySet")]
pub trait PySetMethods<'py> {
pub trait PySetMethods<'py>: crate::sealed::Sealed {
/// Removes all elements from the set.
fn clear(&self);

Expand Down
2 changes: 1 addition & 1 deletion src/types/slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ impl PySlice {
/// syntax these methods are separated into a trait, because stable Rust does not yet support
/// `arbitrary_self_types`.
#[doc(alias = "PySlice")]
pub trait PySliceMethods<'py> {
pub trait PySliceMethods<'py>: crate::sealed::Sealed {
/// Retrieves the start, stop, and step indices from the slice object,
/// assuming a sequence of length `length`, and stores the length of the
/// slice in its `slicelength` member.
Expand Down
2 changes: 1 addition & 1 deletion src/types/string.rs
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ impl PyString {
/// syntax these methods are separated into a trait, because stable Rust does not yet support
/// `arbitrary_self_types`.
#[doc(alias = "PyString")]
pub trait PyStringMethods<'py> {
pub trait PyStringMethods<'py>: crate::sealed::Sealed {
/// Gets the Python string as a Rust UTF-8 string slice.
///
/// Returns a `UnicodeEncodeError` if the input is not valid unicode
Expand Down
2 changes: 1 addition & 1 deletion src/types/traceback.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ impl PyTraceback {
/// syntax these methods are separated into a trait, because stable Rust does not yet support
/// `arbitrary_self_types`.
#[doc(alias = "PyTraceback")]
pub trait PyTracebackMethods<'py> {
pub trait PyTracebackMethods<'py>: crate::sealed::Sealed {
/// Formats the traceback as a string.
///
/// This does not include the exception type and value. The exception type and value can be
Expand Down
2 changes: 1 addition & 1 deletion src/types/tuple.rs
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ index_impls!(PyTuple, "tuple", PyTuple::len, PyTuple::get_slice);
/// syntax these methods are separated into a trait, because stable Rust does not yet support
/// `arbitrary_self_types`.
#[doc(alias = "PyTuple")]
pub trait PyTupleMethods<'py> {
pub trait PyTupleMethods<'py>: crate::sealed::Sealed {
/// Gets the length of the tuple.
fn len(&self) -> usize;

Expand Down
2 changes: 1 addition & 1 deletion src/types/typeobject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ impl PyType {
/// syntax these methods are separated into a trait, because stable Rust does not yet support
/// `arbitrary_self_types`.
#[doc(alias = "PyType")]
pub trait PyTypeMethods<'py> {
pub trait PyTypeMethods<'py>: crate::sealed::Sealed {
/// Retrieves the underlying FFI pointer associated with this Python object.
fn as_type_ptr(&self) -> *mut ffi::PyTypeObject;

Expand Down
Loading