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

Conform to the recommended pattern for sealed traits #1194

Closed
Tracked by #120
dtolnay opened this issue May 31, 2017 · 3 comments
Closed
Tracked by #120

Conform to the recommended pattern for sealed traits #1194

dtolnay opened this issue May 31, 2017 · 3 comments

Comments

@dtolnay
Copy link

dtolnay commented May 31, 2017

The header module contains these two traits:

pub trait Header: HeaderClone + GetType + Send + Sync {
    fn header_name() -> &'static str where Self: Sized;
    fn parse_header(raw: &Raw) -> ::Result<Self> where Self: Sized;
    fn fmt_header(&self, f: &mut Formatter) -> fmt::Result;
}

#[doc(hidden)]
pub trait HeaderClone {
    fn clone_box(&self) -> Box<Header + Send + Sync>;
}

As discussed in the libz blitz evaluation, we would like a recognizable pattern for sealed traits that are impossible to implement outside of the current crate. We would like this pattern to be independent of whether certain functionality on a trait is a doc(hidden) implementation detail.

As such, we would like to:

  • Move clone_box (which is an implementation detail not relevant to users of hyper) from HeaderClone into a doc(hidden) method in Header
  • Rename the now-empty HeaderClone to Sealed
  • Mention in the documentation of Header that this trait is sealed and designed to be implemented only within the hyper codebase
@seanmonstar
Copy link
Member

There may have been a misunderstanding of the usage of this trait. The Header (and HeaderFormat) trait is meant to be implemented by users outside of hyper. The HeaderClone trait is an implementation detail that is only used inside hyper, which guarantees that T: Header is also T: Clone, and since the headers are put into a boxed trait object, Box<Header>, that a clone_box() method exists so that hyper can clone them internally.

The HeaderClone trait has a blanket impl inside hyper for all T: Header + Clone...

@dtolnay
Copy link
Author

dtolnay commented May 31, 2017

You are right. I misunderstood these traits.

To tease this apart:

  • There is a trait Header that users are meant to implement. They are also free to call the Header trait methods if they want.
  • There is a trait HeaderClone that users are not meant to implement, nor call its trait methods.

How would you feel about these changes?

  • Add an empty private supertrait Sealed to HeaderClone.
  • Put doc(hidden) on the clone_box method.
  • Publicly expose HeaderClone.

I like this because now Header has all public supertraits so it is clear that you can implement it. Previously Header had a private supertrait which suggests that you will not be able to implement it.

Also now rustdoc can show the HeaderClone for T: Header + Clone impl, so users can tell from rustdoc what types they are allowed to implement Header for. Previously they would have to dive through some hidden types to find this out.

@seanmonstar
Copy link
Member

This has been done, and and is viewable in https://docs.rs/hyper/0.10.12.

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

No branches or pull requests

2 participants