Don't use private traits in public APIs #2051
Comments
|
The H2Exec bound in particular is a bit of a nightmare: |
|
Same for the |
|
It is possible - you just have to source dive and find the right blanket implementation you need to satisfy: https://github.com/hyperium/hyper/blob/master/src/client/connect/mod.rs#L203-L208 |
|
Line 12 in 0dc8968 but yeah we could improve documentation... |
|
So, hyper uses these "private trait aliases" to try to make the
I assumed that seeing less "noise" would be helpful, and it also means less duplication of bounds inside the hyper codebase. If it really seems to make things worse, we can remove these "aliases" and just make the bounds lists bigger. |
seanmonstar
added
the
B-rfc
|
It's fine to use the private traits bounds internally in hyper - the parts that matter to me are just the public interfaces, where it is important to be able to see what is in fact required to call the method. |
|
With the The propagation of the futures for the executor stuff is just horrible, but I don't know a better way while still allowing |
|
You'd need to further unwrap them until they don't show private types. Anyone writing code that deals with hyper types in a generic context has to figure this stuff out anyway! |
|
This is what I arrived at after ~1 hour of source diving into hyper and bouncing off of compiler errors as the trait bounds for Connection's Future implementation: impl<I, S, B> Future for Connection<I, S>
where
S: Service<Request<Body>, Response = Response<B>>,
S::Future: 'static + Send,
S::Error: Into<Box<dyn Error + Sync + Send>>,
I: AsyncRead + AsyncWrite + Unpin + 'static,
B: http_body::Body + 'static + Send + Unpin,
B::Data: Send + Unpin,
B::Error: Into<Box<dyn Error + Sync + Send>>, |
|
Those Unpins on B and B::Data come through like 8 layers out of H2Exec. |
|
Oh, we can probably remove those |
|
As I've done similar spelunking to remain generic over That you are considering the now visible |
|
Can we expose the traits publically but seal them so that they still need to be implemented via the real trait but can be used to make bounds match. I currently do this in tonic which isn't so bad but could be useful to get he httpservice trait out https://github.com/hyperium/tonic/blob/master/tonic/src/transport/server.rs#L215 The other idea is that maybe some of these more generic traits like |
That's how the current hyper release works with e.g. |
|
Sounds like this could be an improved compiler diagnostics thing. Where you have a sealed trait it should tell you the possible trait you need to implement but I don't think we have that. |
|
@dekellum the issue with large trait bounds is that they just genearlly look scary and harder to use. I think this problem is more of a documentation issue and edcuation issue over ergonomics or anything else. |
|
Why are these traits sealed in the first place? Why does hyper care if someone implements Connect via Service or directly? |
|
I think the concern is to build an ecosystem of reusable parts. If we have users implement connectors via the connect trait and some via the service trait then then we create a split. If we use trait aliases then every implementation has to derived from |
|
IMO private types in a public signature are even more "scary". At best they seem like an oversight or unfinished work. Could there be some separate public type aliases, with tests confirming compatibility with the internal, non-public types (which may span multiple crates)? Just rustdoc text is unlikely to improve confidence, particularly if it becomes out of date. |
I strongly agree with this. I was attempting to update https://github.com/ctz/hyper-rustls to the latest Hyper release on the master branch and the sealed types in trait bounds are not fun to work with or reconstruct. A big portion of this were the unimplementable trait aliases in public signatures. If Hyper is to continue exposing these trait aliases, I'd greatly prefer that they be unsealed despite them being a potential semver hazard.
Perhaps I don't fully understand, but if there is a blanket implementation of |
|
@dekellum what bounds did you use to be generic over |
This comment has been minimized.
This comment has been minimized.
|
@dekellum just wondering, does it need to be specifically |
|
It calls |
|
when using Although, I haven't finished going through all the compiler errors yet. |
|
It's slightly more involved to use a |
|
Edit: I see now that for my use case, just using hyper-tls is sufficient: https://docs.rs/hyper-tls/0.4.0/hyper_tls/struct.HttpsConnector.html . I’m not sure, but I feel like I had gone the generic route because of some example I found in the past. I still generally feel the pain of sealed traits though. so, I'm pretty stuck at this point. If I use If I use Since it was pretty straightforward to have a Really looking forward to upgrading to hyper 0.13, so help figuring this out is definitely appreciated. |
|
I ended up with this for a struct that allows hyper pub struct Requester<S>
where
S: Service<Uri> + Clone + Send + Sync + 'static,
S::Response: Connection + AsyncRead + AsyncWrite + Send + Unpin + 'static,
S::Future: Send + Unpin + 'static,
S::Error: Into<Box<dyn std::error::Error + Send + Sync>>,
{
client: hyper::Client<S, Body>,
} |
|
I ended up forking |
|
Thanks for the pointer @jbg, as it seems my upgrade to the last alpha (signature above) is drastically incomplete for the hyper 0.13 release. I additonally need to be generic over B: HttpBody, so thus far I have this: use hyper::service::Service;
use hyper::Uri;
use tokio::io::{AsyncRead, AsyncWrite};
use hyper::client::connect::Connection;
pub fn request_dialog<CN, B>(
client: &hyper::Client<CN, B>,
rr: RequestRecord<B>,
tune: &Tunables)
-> impl Future<Output=Result<Dialog, FutioError>> + Send + 'static
where CN: Service<Uri> + Clone + Send + Sync + 'static,
CN::Response: Connection + AsyncRead + AsyncWrite + Send + Unpin + 'static,
CN::Future: Send + Unpin + 'static,
CN::Error: Into<Box<dyn std::error::Error + Send + Sync + 'static>>,
B: hyper::body::HttpBody + Send + Unpin + 'static,
B::Data: Send + Unpin,
B::Error: Into<Box<dyn std::error::Error + Send + Sync + 'static>>Lots of |
|
@dekellum technically, asyncread + asyncwrite are agnostic to an executor, they are more connected to a reactor. You can easily shim them over to As for the Send + Sync bounds, you need to add them, this was a big reason why I am a proponet for trait aliases, as they allow applications to build ontop of the core traits and create easier trait bounds while creating easier error messages. They also make things like this much less scarier. Most types you pass in will most 95% of the bounds but we just need to be explicit. |
|
I only mentioned
Maybe client can also work if I duplicate these methods using a @seanmonstar bot: remove |
We should! Filed #2070
Perhaps the |
|
As for using a async fn fetch_reddit_new<S, ReqBody, ResBody>(
client: &mut S,
subreddit: &str,
posts: usize,
) -> Result<Vec<PostData>, YourError>
where
S: Service<
http::Request<ReqBody>,
Response = http::Response<ResBody>,
>,
ReqBody: Default,
ResBody: HttpBody,
YourError: From<S::Error>,
YourError: From<ResBody::Error>,
{
let url = /* ... */;
let req = Request::get(url).body(ReqBody::default())?;
let res = client.call(req).await?;
let bytes = hyper::body::to_bytes(res.into_body()).await?;
// ...
}We could reduce some of that by exposing the |
|
I just wanted to say +1 to making the Connect trait alias publicly visible again. Making the trait visible (even while still not allowing others to implement it) seems better in all ways than the current form. First it makes it possible to write generic bounds against it, second it can show up in docs which will also highlight the blanket impl for tower::Service along with an opportunity to write a comment that tower::Service is what should be implemented. Right now the only way to know you need to implement tower::Service is to read the source code. An issue when moving from a hyper::Client to a generic tower::Service<http::Request, Response=http::Response> is that the ownership semantics between hyper and tower are not the same. Hyper has internal synchronization so that you can invoke methods on shared references. A tower service requires a mutable reference to invoke call. This means that I now need to implement my own synchronization around the tower service, even though I really just want to support hyper::Clients that already provide their own. My code would end up being much more complicated and would be doing double synchronization for the trouble. |
|
OK, so there is #2073 up regarding the When exposing this, I remembered why it was hidden: I hope to be able to remove some of the required bounds on some of the associated types, but if the trait is public, you could use it to assume the same bounds (and thus when hyper removed the need for them, your code could break). So this PR exposes the So, again using a previous example, the code can now be: pub struct Requester<C>
where
C: Connect + Clone + Send + Sync + 'static,
{
client: hyper::Client<S, Body>,
} |
|
Thanks, with master (#2070) and #2073 my bounds reduce to the following, and I can avoid some manual/unsafe pub fn request_dialog<CN, B>(
client: &hyper::Client<CN, B>,
rr: RequestRecord<B>,
tune: &Tunables)
-> impl Future<Output=Result<Dialog, FutioError>> + Send + 'static
where CN: hyper::client::connect::Connect + Clone + Send + Sync + 'static,
B: hyper::body::HttpBody + Send + 'static,
B::Data: Send,
B::Error: Into<Flaw>Where |
|
Is really the only difference here not using the actual |
|
As opposed to my prior signature (an object of humor!) its 4 instead of 7 (lines of) bounds and all |
I've migrated my code to the new Service trait. |
This is normal, and how tower was designed to work with its |
|
Though this brings up an interesting thought, since |
|
FYI, v0.13.1 is just released that exposes |
|
I am trying to port some code to [email protected] which does not expose |
|
@dignifiedquire there is a trait alias bound that you can implement that version of basically where |
|
I'm trying to write a function that returns a Edit: Maybe the solution is to instead return |
|
Honestly after trying to mess around with the API for a bit, I can safely say that the lack of public types makes things a nightmare to deal with. Like, with all due respect, trying to fenangle the types for IMHO, if the types are too complicated to work with internally, then they're too complicated to work with externally too. Aliases that are helpful internally should be packaged and offered externally. I do think that using Mostly just my 2¢. Haven't read the full thread and haven't done a whole lot of work in Rust recently, so, may not be the most representative opinion of other users of the library. |
|
Any update on this? |
|
We are working on plans for hyper 1.0, which should greatly improve on all of this. |
…unds` (hyperium#3127) Define public trait aliases that are sealed, but can be named externally, and have documentation showing how to provide a Executor to match the bounds, and how to express the bounds in your own API. Closes hyperium#2051 Closes hyperium#3097
…unds` (hyperium#3127) Define public trait aliases that are sealed, but can be named externally, and have documentation showing how to provide a Executor to match the bounds, and how to express the bounds in your own API. Closes hyperium#2051 Closes hyperium#3097 Signed-off-by: Sven Pfennig <[email protected]>
For example, hyper master currently has this:
HttpServiceandH2Execare both private traits, though. This makes it a huge pain for code working generically with connections to express the proper bounds in their code. You basically have to try to reverse engineer the impl bounds by bouncing off the compiler :(The text was updated successfully, but these errors were encountered: