djc / tokio-imap

Tokio-based IMAP implementation
Apache License 2.0
123 stars 42 forks source link

`quoted_data` should return `Needed::Unknown` until no unescaped " found #27

Closed duesee closed 4 years ago

duesee commented 6 years ago

I probably noticed a bug in the quoted / quoted_data parser. Because I am just becoming acquainted with your code and with nom, I am not sure if I misunderstand something. Here is my failing test:

#[test]
fn test_quoted() {
    // This is fine.
    assert!(quoted(b"\"I am finished!\"").is_ok());

    // Here, the parser can't know if more data will follow...
    assert_eq!(
        quoted(b"\"I am fini..."), // ...because there is no closing quote.
        Err(Err::Incomplete(Needed::Unknown))
    );
}
djc commented 6 years ago

Oh, yeah, you're probably right. It's likely that all the parsers written as normal Rust functions instead of using nom combinators have problems like this. So yes, I agree with your assessment. Are you in a position to fix it yourself?

duesee commented 6 years ago

Currently, I am trying to find out why some fetch responses are not parsed correctly. I could commit the test first so that we don't forget about it and fix it later when I'm done with the fetch issue.

By the way: I also did some minor refactorings to group related parsers more closely together so that I can find them better. I also find it useful to distinguish between "core primitives" like atom, astring, etc. and the "real imap types". If your interested in those changes as well, I will open a pull request.

djc commented 6 years ago

Sure, you can open a PR and we can discuss the best way forward!

djc commented 5 years ago

@duesee how did you fare with this? If you still have some unfinished stuff somewhere, I'd like to be able to look at it!

duesee commented 5 years ago

Hey, I started implementing some missing parsers, then implemented an ABNF parser in Rust/Nom in the hope to be able to automate the manual process of typing the Nom parsers. To cut the long story short, I got distracted by many other stuff and didn't focused on it for a while :see_no_evil: I will take a look into it the next days and see if I can at least fix this issue.

djc commented 5 years ago

Hah, sounds like a proper yak shaving session! No pressure if you no longer want to work on this, was just wondering about the status.

duesee commented 5 years ago

I opened a pull request to merge some of the refactoring and the tests. BTW, what do you think about the idea to generate the parsers? Do you think this can be accomplished?

I tested it with Pest and was basically able to generate the whole spec. Unfortunately, Pest is not a streaming parser and processing the parse tree into nice structures remains tbd.