Use type args for default implementations of L2 traits#301
Conversation
PLeVasseur
left a comment
There was a problem hiding this comment.
Hey @sophokles73 👋 I really like the direction. When I thought about it and then threw Opus 4.6 and GPT-5.4 at it, we came up with some things that could be improved to help improve ergonomics of users.
Overall impact of moving bounds off the public struct definitions
In the current PR, the generic bounds live on the public type declarations themselves:
src/communication/default_notifier.rs:29src/communication/default_pubsub.rs:187src/communication/default_pubsub.rs:245src/communication/in_memory_rpc_client.rs:159src/communication/in_memory_rpc_server.rs:120
That means downstream users have to repeat those bounds anywhere they even name these types, even if their wrapper type is only storing the value and is not constructing it or calling any transport-dependent methods at that point.
What this looks like for downstream users today
I validated this with a tiny external crate against the PR exactly as authored.
The natural downstream wrapper code is:
use up_rust::communication::{
InMemoryRpcClient, InMemoryRpcServer, InMemorySubscriber, SimpleNotifier, SimplePublisher,
};
pub struct NotifierHolder<T, P> {
pub notifier: SimpleNotifier<T, P>,
}
pub struct PublisherHolder<T, P> {
pub publisher: SimplePublisher<T, P>,
}
pub struct SubscriberHolder<T, S, N> {
pub subscriber: InMemorySubscriber<T, S, N>,
}
pub struct RpcClientHolder<T, P> {
pub client: InMemoryRpcClient<T, P>,
}
pub struct RpcServerHolder<T, P> {
pub server: InMemoryRpcServer<T, P>,
}That does not compile against the current PR state. The compiler immediately asks the downstream crate to restate the bounds at the wrapper type definition, e.g.:
the trait bound `T: UTransport` is not satisfied
the trait bound `P: LocalUriProvider` is not satisfied
the trait bound `S: USubscription` is not satisfied
the trait bound `N: Notifier` is not satisfied
So the downstream user is forced to write this instead:
use up_rust::communication::Notifier;
use up_rust::communication::{
InMemoryRpcClient, InMemoryRpcServer, InMemorySubscriber, SimpleNotifier, SimplePublisher,
};
use up_rust::core::usubscription::USubscription;
use up_rust::{LocalUriProvider, UTransport};
pub struct NotifierHolder<T: UTransport, P: LocalUriProvider> {
pub notifier: SimpleNotifier<T, P>,
}
pub struct PublisherHolder<T: UTransport, P: LocalUriProvider> {
pub publisher: SimplePublisher<T, P>,
}
pub struct SubscriberHolder<T: UTransport, S: USubscription, N: Notifier> {
pub subscriber: InMemorySubscriber<T, S, N>,
}
pub struct RpcClientHolder<T: UTransport, P: LocalUriProvider> {
pub client: InMemoryRpcClient<T, P>,
}
pub struct RpcServerHolder<T: UTransport, P: LocalUriProvider> {
pub server: InMemoryRpcServer<T, P>,
}I also validated that this bound-included version does compile against the current PR state.
What changes after the suggested edit
I then applied the suggested changes in a separate worktree (/tmp/up-rust-pr-301-after):
src/communication/default_notifier.rs:29->pub struct SimpleNotifier<T, P>src/communication/default_pubsub.rs:187->pub struct SimplePublisher<T, P>src/communication/default_pubsub.rs:245->pub struct InMemorySubscriber<T, S, N>src/communication/default_pubsub.rs:287->Result<Self, RegistrationError>src/communication/in_memory_rpc_client.rs:159->pub struct InMemoryRpcClient<T, P>src/communication/in_memory_rpc_server.rs:31->struct RequestListener<T>src/communication/in_memory_rpc_server.rs:120->pub struct InMemoryRpcServer<T, P>
With those edits in place, the original lightweight downstream wrapper code compiles unchanged:
use up_rust::communication::{
InMemoryRpcClient, InMemoryRpcServer, InMemorySubscriber, SimpleNotifier, SimplePublisher,
};
pub struct NotifierHolder<T, P> {
pub notifier: SimpleNotifier<T, P>,
}
pub struct PublisherHolder<T, P> {
pub publisher: SimplePublisher<T, P>,
}
pub struct SubscriberHolder<T, S, N> {
pub subscriber: InMemorySubscriber<T, S, N>,
}
pub struct RpcClientHolder<T, P> {
pub client: InMemoryRpcClient<T, P>,
}
pub struct RpcServerHolder<T, P> {
pub server: InMemoryRpcServer<T, P>,
}I validated that this compiles successfully with the suggestions given.
Why this matters
The current PR makes downstream generic code noisier for no functional gain at the type declaration site. A user who only wants to store one of these communication-layer types inside a wrapper, service container, or builder has to pull in and restate all of the transport/provider trait bounds immediately, even though the real operational constraints only matter when the relevant impl methods are used.
Moving the bounds from the struct declarations to the impl blocks improves API ergonomics because it:
- removes unnecessary generic clutter from downstream wrapper types
- avoids forcing users to import traits just to name a field type
- keeps the real constraints enforced where behavior actually depends on them
- preserves behavior; this is an ergonomics improvement, not a semantic change
Validation summary
before: the lightweight downstream wrapper code fails to compile, and the downstream crate must repeatUTransport,LocalUriProvider,USubscription, andNotifierbounds to make the type names legalbefore: the bound-heavy version compilesafter: the lightweight wrapper code compiles without repeating those boundsafter: the library still passescargo fmt --all --checkandcargo check --workspace --all-targets --all-features
|
@PLeVasseur thanks for the review 👍 |
This allows us to avoid dynamic dispatch and use static dispatch instead, which can improve performance and reduce code size. The InMemorySubscriber::for_clients function no longer takes a LocalUriProvider parameter, because it was not used anyways.
c2ee94a to
1c0a348
Compare
PLeVasseur
left a comment
There was a problem hiding this comment.
This looks good. Sorry for the review delay 😅
This allows us to avoid dynamic dispatch and use static dispatch instead, which can improve performance and reduce code size.