-
Notifications
You must be signed in to change notification settings - Fork 747
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
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, PyResult}; | ||
|
||
pub trait Sealed {} | ||
|
||
// for FfiPtrExt | ||
impl Sealed for *mut ffi::PyObject {} | ||
|
||
// for PyResultExt | ||
impl Sealed for PyResult<Bound<'_, PyAny>> {} | ||
|
||
// 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> {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does rustdoc render the bound as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
/// Gets whether this boolean is `true`. | ||
fn is_true(&self) -> bool; | ||
} | ||
|
There was a problem hiding this comment.
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 multipleSealed
traits each only relevant for one or more types?We have
Sealed
forPyResultExt
trait too, which could be merged in I guess, if it makes sense to do so.There was a problem hiding this comment.
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 theSealed
trait could be added anywhere in the crate instead of only in that module... How do you feel about that?There was a problem hiding this comment.
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 forBound<T>
. We seal it and we implement sealed forBound<T>
.By adding that sealing for generic
T
now users can implementPyDictMethods
for their ownBound<MyClass>
.If the sealing was done by separate traits then we could guarantee that the
PyDictMethods
seal would only ever allowBound<PyDict>
and not accidentally gave this mistake.There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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.