Move Encodable generic param?

[JirkaVebr]

Currently, the Encodable trait is generic over E: Encoder, allowing the encodable to effectively choose which encoders it supports. I find that a bit strange since I'd say an encodable value shouldn't really care about what encoder will write its bytes (or where or how), and instead only concern itself about how to convert &self into said bytes. And indeed, as far as I can tell, all encodables appear to just do impl<E: Encoder> Encodable<E> for SomeEncodable anyway. At least all in this repository and the ones I've written so far.

Implementing just Encodable<SomeEncoder> and Encodable<OtherEncoder>, for instance, not only leaves out all the other encoders (which is a shame), but also introduces a hazard as the two implementations may accidentally differ.

I'm wondering… Why can't all encodables support all encoders? We could do that by moving the generic parameter onto the encode method, thus allowing the caller to choose the encoder. I haven't tested this but I think something like this could work:

pub trait Encodable {
    type Error<EncoderError>: From<EncoderError>;

    fn encode<E: Encoder>(&self, encoder: &mut E) -> Result<(), Self::Error<E::Error>>;
}

This would also completely remove the need for trait EncodableSize: Encodable<encoders::SizeEncoder> as all encodables would automatically support the SizeEncoder, which to me makes sense.

Note that serde's Serialize trait has a generic method like this as well which matches the sentiment here: a Serialize struct shouldn't have to know or worry about json/yaml/whatever.

If I were to contribute a pull request, would there be interest in merging it or is that already too disruptive a change?

[Altair-Bueno]

Hi, thanks for reaching out. I hope the library serves you well.

Indeed, the design of the Encodable trait differs from others within the ecosystem. However, it was designed that way intentionally. First off, the trait is object safe, allowing consumers to erase types by using an indirection. The snippet you provided wouldn't be object safe. See serde's documentation on the Serialize trait, where rustdoc points out dyn compatibility.

This snippet works and is object safe

use std::convert::Infallible;

use encode::Encodable;

fn main() {
    let encodable = "Hello world!";
    let mut encoder = Vec::new();
    let dyn_encodeable = &encodable as &dyn Encodable<Vec<u8>, Error = Infallible>;
    encodable.encode(&mut encoder);
}

Dynamically dispatching encoders proves to be useful in network protocols, as encodings heavily change as soon after protocol negotiations are finished (see HTTP 1 and 2 headers, for example). Also, dynamic dispatch is a must if you plan to build an encoder in Rust and consume it through FFI.

Secondly, you rise the point that "two implementations may accidentally differ", and that is a potential hazard. I totally understand your point, and after reading your comment, I believe the documentation should include a warning on the potential pitfalls someone might get into by poorly implementing the Encodable trait. However, the benefits of this design greatly surpass the downsides. By using generics on the type level rather than the method level, consumers can manually optimize each encoder implementation.

For example, picture an Encoder that is implemented using a native library. The compiler isn't able to see anything past the FFI boundary, so calling the expensive native function just to get the size would result in a waste of cycles. In the future, one would be able to write a default implementation and specialize it for specific encoders, but right now on stable different non-overlapping implementation blocks is the best we can do.

[JirkaVebr]

Hey, that's a valid point! I hadn't really properly considered dyn compatibility. Thanks for taking the time to explain your rationale!

I guess we can close this now. :)