seals PyAnyMethods and friends
#3909
Conversation
This seals these new traits, preventing downstream crates from implementing them on their types. These traits are mainly a workaround for arbitrary self receiver types, so this gives us more flexibility if these need to be changed in the future.
| }; | ||
| use crate::{ffi, Bound, PyAny}; | ||
|
|
||
| pub trait Sealed {} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
| @@ -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.
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).
There was a problem hiding this comment.
It seems it does not make a difference, this looks fine to me.
| }; | ||
| use crate::{ffi, Bound, PyAny}; | ||
|
|
||
| pub trait Sealed {} |
There was a problem hiding this comment.
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 👍
Part of #3684, following #3684 (comment)
This seals these new traits, preventing downstream crates from implementing them on their types. These traits are mainly a workaround for arbitrary self receiver types, so this gives us more flexibility if these need to be changed in the future.