duesee / imap-codec

Rock-solid and complete codec for IMAP
Apache License 2.0
35 stars 13 forks source link

refactor: Replace trait `Decode` with `Decoder` #337

Closed jakoschiko closed 11 months ago

jakoschiko commented 11 months ago

Replaces the current trait Decode with the trait Decoder in order to improve lifetime issues.

I don't expect this PR to be merged, I just want to demonstrate my ideas. So, what's the problem with the current trait Decode?

pub trait Decode<'a>: Sized {
    fn decode(input: &'a [u8]) -> Result<(&'a [u8], Self), DecodeError>;
}

If I use this trait with type bounds, then I need to choose the lifetime in the wrong place:

fn foo<'a, T: Decode<'a>>() -> T { // The caller chooses a lifetime 'a
    let vec: Vec<u8> = b"foo".to_vec();
    let slice: &[u8] = &vec; // The slice's lifetime does not match lifetime 'a
    T::decode(slice); // Does not compile
}

This prevents me from writing useful abstractions based on Decode. So, here my proposed trait Decoder.

pub trait Decoder {
    type Item<'a>: Sized;

    fn decode<'a>(input: &'a [u8]) -> Result<(&'a [u8], Self::Item<'a>), DecodeError>;
}

It allows me to choose the lifetime at a later point in time:

fn foo<T: Decoder>() {
    let vec: Vec<u8> = b"foo".to_vec();
    let slice: &[u8] = &vec;
    T::decode(slice); // The lifetime is chosen here, now it compiles
}

List of changes:

If you like the changes, we could do even more:

coveralls commented 11 months ago

Pull Request Test Coverage Report for Build 5797418516


Changes Missing Coverage Covered Lines Changed/Added Lines %
imap-codec/src/codec.rs 6 12 50.0%
<!-- Total: 29 35 82.86% -->
Totals Coverage Status
Change from base Build 5796696502: -0.06%
Covered Lines: 8706
Relevant Lines: 9302

💛 - Coveralls
duesee commented 11 months ago

This makes so much more sense, thanks! :+1: (As discussed offline, let's talk about how, and when, to merge this later.)

duesee commented 11 months ago

As discussed yesterday, I moved the *Decoder structs into codec and renamed them to *Codec. As you proposed, I also removed the StaticDecoder by making it a default trait impl. Is there anything left we want to do in this PR?

Two possible paths to continue: 1) Continue with Encoder or 2) #333.

jakoschiko commented 11 months ago

I suggest this order: merge this, merge #333, add Encoder