duesee / imap-codec

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

question: Remove `unvalidated` feature in `imap-types`? #512

Open HenningHolmDE opened 1 week ago

HenningHolmDE commented 1 week ago

Currently, imap-codec depends on the unvalidated feature of imap-types. As the crate re-exports imap-types, these can be used without additional opt-in:

#!/usr/bin/env rust-script
//! Create unvalidated type
//!
//! ```cargo
//! [dependencies]
//! imap-codec = { git = "https://github.com/duesee/imap-codec.git" }
//! ```

use imap_codec::imap_types::core::Text;

fn main() {
    let text = Text::unvalidated("Hello, 🦀!");
    println!("{text:?}")
}
$ rust-script create_unvalidated_type.ers
Text("Hello, 🦀!")

Is this already known to you? For me, this is not really a large problem, as the name and the documentation of the functions do provide sufficient information on how and when to use it safely.

However, does feature gating unvalidated in imap-types still make much sense? (Without it, I would not have been suprised about the re-export.)

duesee commented 1 week ago

Haha, nothing can hide from you :D This is great! But yes, I'm aware ...

If we look at imap-types in isolation, the feature is fine. But as soon as you implement a codec, there is a very high chance you need functions to not go through try_from(...).unwrap().

In the past, unvalidated was marked unsafe and there was no unvalidated feature. But... I felt like stretching the term unsafe, and forcing people to #[allow(unsafe)].

So... the alternative was to feature-gate it. But... I don't think it makes much sense. When we say that after types comes a codec (that needs unvalidated), all higher layers will have it. This is because imap-types should only be in the dependency tree once (and features will add up.)

Long story short: We should remove the feature if we feel that there is no good use case for imap-types w/o unvalidated.

Another thought: Maybe someone could write a codec w/o using unvalidated. Example for a tag: Read until space, and then call Tag::try_from.

To clarify:

As the crate re-exports imap-types, these can be used without additional opt-in:

Here could be a misunderstanding: It should also work by useing imap-types directly (w/o the re-export). This is because of Rust unifying dependencies accumulating features.

duesee commented 4 days ago

Related https://github.com/duesee/imap-codec/issues/398

duesee commented 1 day ago

Hello, again! I renamed this issue because there is a similar one asking about the imap-types re-export -- I hope this was in your sense! :-)

I feel that I probably want to un-gate the unvalidated methods, i.e., remove the unvalidated feature. Re-export seems like an orthogonal question. What do you think?

HenningHolmDE commented 1 day ago

Removing the unvalidated feature gate from imap-types would remove the basis for my initial confusion. So yes, I'm fine with renaming the issue.