Repository: duesee/imap-codec (language: python)

Issue:
Review `Flag` for correctness
* [ ] `Flag::NameAttribute(Noinferiors)` vs. `Flag::Extension(Atom("Noinferiors"))]`
* [ ] How to treat `Extension(Atom("Recent"))`

---

```rust
	Data(
	    Fetch {
	        seq_or_uid: 1398555393,
	        attributes: NonEmptyVec {
	            inner: [
	                Flags(
	                    [
	                        Permanent,
	                        Deleted,
	                        Seen,
	                    ],
	                ),
	            ],
	        },
	    },
	)
```

... results in ...

```imap
* 1398555393 FETCH (FLAGS (\\* \\Deleted \\Seen))\r\n
```

... which can't be parsed.


Discussion / hints:

Fix patch:
diff --git a/fuzz/fuzz_targets/command_to_bytes_and_back.rs b/fuzz/fuzz_targets/command_to_bytes_and_back.rs
index fb9914b4..93d97805 100644
--- a/fuzz/fuzz_targets/command_to_bytes_and_back.rs
+++ b/fuzz/fuzz_targets/command_to_bytes_and_back.rs
@@ -7,23 +7,9 @@ use imap_codec::utils::escape_byte_string;
 use imap_codec::{
     codec::{Decode, Encode},
     command::{search::SearchKey, Command, CommandBody},
-    message::Flag,
 };
 use libfuzzer_sys::fuzz_target;
 
-fn ignore_flags(flags: &[Flag]) -> bool {
-    flags.iter().any(|flag| {
-        matches!(
-            flag,
-            Flag::Keyword(_)
-                | Flag::Extension(_)
-                | Flag::Permanent
-                | Flag::Recent
-                | Flag::NameAttribute(_)
-        )
-    })
-}
-
 #[cfg(feature = "ext_enable")]
 fn ignore_capabilities_enable(capabilities: &[CapabilityEnable]) -> bool {
     capabilities
@@ -47,14 +33,6 @@ fuzz_target!(|test: Command| {
     // TODO(#30): Skip certain generations for now as we know they need to be fixed.
     //            The goal is to not skip anything eventually.
     match test.body {
-        CommandBody::Store { ref flags, .. } if ignore_flags(flags) => {
-            // FIXME(#30)
-            return;
-        }
-        CommandBody::Append { ref flags, .. } if ignore_flags(flags) => {
-            // FIXME(#30)
-            return;
-        }
         CommandBody::Search { ref criteria, .. } if ignore_search_key_and(criteria) => {
             // FIXME(#30)
             return;
diff --git a/fuzz/fuzz_targets/greeting_to_bytes_and_back.rs b/fuzz/fuzz_targets/greeting_to_bytes_and_back.rs
index f71d34ba..9143bbd0 100644
--- a/fuzz/fuzz_targets/greeting_to_bytes_and_back.rs
+++ b/fuzz/fuzz_targets/greeting_to_bytes_and_back.rs
@@ -13,10 +13,6 @@ fuzz_target!(|test: Greeting| {
     //            The goal is to not skip anything eventually.
     if let Some(ref code) = test.code {
         match code {
-            Code::PermanentFlags(_) => {
-                // FIXME(#30)
-                return;
-            }
             #[cfg(any(feature = "ext_login_referrals", feature = "ext_mailbox_referrals"))]
             Code::Referral(_) => {
                 // FIXME(#30)
diff --git a/fuzz/fuzz_targets/response_to_bytes_and_back.rs b/fuzz/fuzz_targets/response_to_bytes_and_back.rs
index c055e522..18f5fb5e 100644
--- a/fuzz/fuzz_targets/response_to_bytes_and_back.rs
+++ b/fuzz/fuzz_targets/response_to_bytes_and_back.rs
@@ -1,11 +1,13 @@
 #![no_main]
 
 use base64::{engine::general_purpose::STANDARD as _base64, Engine};
+#[cfg(any(feature = "ext_login_referrals", feature = "ext_mailbox_referrals"))]
+use imap_codec::response::Code;
 #[cfg(feature = "debug")]
 use imap_codec::utils::escape_byte_string;
 use imap_codec::{
     codec::{Decode, Encode},
-    response::{data::FetchAttributeValue, Code, Continue, Data, Response},
+    response::{data::FetchAttributeValue, Continue, Data, Response},
 };
 use libfuzzer_sys::fuzz_target;
 
@@ -13,72 +15,45 @@ fuzz_target!(|test: Response| {
     // TODO(#30): Skip certain generations for now as we know they need to be fixed.
     //            The goal is to not skip anything eventually.
     match test {
-        Response::Data(ref data) => match data {
-            Data::Fetch { ref attributes, .. } => {
+        Response::Data(ref data) => {
+            if let Data::Fetch { ref attributes, .. } = data {
                 for attribute in attributes.as_ref().iter() {
                     match attribute {
                         FetchAttributeValue::Body(_) | FetchAttributeValue::BodyStructure(_) => {
                             // FIXME(#30): Body(Structure)
                             return;
                         }
-                        FetchAttributeValue::Flags(_) => {
-                            // FIXME(#30): Flag handling.
-                            return;
-                        }
                         _ => {}
                     }
                 }
             }
-            Data::List { items, .. } | Data::Lsub { items, .. } if !items.is_empty() => {
-                // FIXME(#30): Flag handling.
-                return;
-            }
-            Data::Flags(_) => {
-                // FIXME(#30): Flag handling.
-                return;
-            }
-            _ => {}
-        },
+        }
+        #[cfg(any(feature = "ext_login_referrals", feature = "ext_mailbox_referrals"))]
         Response::Status(ref status) => {
-            if let Some(ref code) = status.code() {
-                match code {
-                    Code::PermanentFlags(_) => {
-                        // FIXME(#30): Flag handling.
-                        return;
-                    }
-                    #[cfg(any(feature = "ext_login_referrals", feature = "ext_mailbox_referrals"))]
-                    Code::Referral(_) => {
-                        // FIXME(#30)
-                        return;
-                    }
-                    _ => {}
-                }
+            if let Some(Code::Referral(_)) = status.code() {
+                // FIXME(#30)
+                return;
             }
         }
         Response::Continue(ref continue_request) => match continue_request {
+            #[cfg(any(feature = "ext_login_referrals", feature = "ext_mailbox_referrals"))]
             Continue::Basic {
-                code: Some(code), ..
-            } => match code {
-                Code::PermanentFlags(_) => {
-                    // FIXME(#30): Flag handling.
-                    return;
-                }
-                #[cfg(any(feature = "ext_login_referrals", feature = "ext_mailbox_referrals"))]
-                Code::Referral(_) => {
-                    // FIXME(#30)
-                    return;
-                }
-                _ => {}
-            },
+                code: Some(Code::Referral(_)),
+                ..
+            } => {
+                // FIXME(#30)
+                return;
+            }
             // Oh, IMAP :-/
             Continue::Basic { code: None, text } => {
                 if _base64.decode(text.inner()).is_ok() {
-                    // FIXME(#30): Flag handling.
+                    // FIXME(#30)
                     return;
                 }
             }
             _ => {}
         },
+        _ => {}
     }
 
     #[cfg(feature = "debug")]
diff --git a/imap-types/src/arbitrary/mod.rs b/imap-types/src/arbitrary/mod.rs
index 05b798ba..57988cc2 100644
--- a/imap-types/src/arbitrary/mod.rs
+++ b/imap-types/src/arbitrary/mod.rs
@@ -8,7 +8,9 @@ use crate::extensions::rfc9208::ResourceOther;
 use crate::{
     command::{search::SearchKey, ListCharString, SequenceSet},
     core::{AString, Atom, AtomExt, Literal, NonEmptyVec, Quoted},
-    message::{AuthMechanismOther, Mailbox, MailboxOther, MyDateTime, MyNaiveDate, Tag},
+    message::{
+        AuthMechanismOther, FlagExtension, Mailbox, MailboxOther, MyDateTime, MyNaiveDate, Tag,
+    },
     response::{
         data::{Capability, QuotedChar},
         CodeOther, Text,
@@ -53,6 +55,7 @@ implement_tryfrom! { ListCharString<'a>, &str }
 implement_tryfrom! { QuotedChar, char }
 implement_tryfrom! { Mailbox<'a>, &str }
 implement_tryfrom! { Capability<'a>, Atom<'a> }
+implement_tryfrom! { FlagExtension<'a>, Atom<'a> }
 implement_tryfrom! { MailboxOther<'a>, AString<'a> }
 #[cfg(feature = "ext_quota")]
 implement_tryfrom! { ResourceOther<'a>, Atom<'a> }
diff --git a/imap-types/src/codec.rs b/imap-types/src/codec.rs
index fe625b40..2cfa2c94 100644
--- a/imap-types/src/codec.rs
+++ b/imap-types/src/codec.rs
@@ -36,8 +36,8 @@ use crate::{
     },
     core::{AString, Atom, AtomExt, IString, Literal, NString, Quoted},
     message::{
-        AuthMechanism, AuthMechanismOther, Charset, Flag, FlagNameAttribute, Mailbox, MailboxOther,
-        MyDateTime, MyNaiveDate, Part, Section, Tag,
+        AuthMechanism, AuthMechanismOther, Charset, Flag, FlagFetch, FlagNameAttribute, FlagPerm,
+        Mailbox, MailboxOther, MyDateTime, MyNaiveDate, Part, Section, Tag,
     },
     response::{
         data::{
@@ -47,6 +47,7 @@ use crate::{
         },
         Code, CodeOther, Continue, Data, Greeting, GreetingKind, Response, Status, Text,
     },
+    rfc3501::flag::FlagExtension,
     utils::escape_quoted,
 };
 
@@ -472,32 +473,42 @@ impl Encode for StatusAttribute {
 impl<'a> Encode for Flag<'a> {
     fn encode(&self, writer: &mut impl Write) -> std::io::Result<()> {
         match self {
-            // ----- System -----
             Flag::Seen => writer.write_all(b"\\Seen"),
             Flag::Answered => writer.write_all(b"\\Answered"),
             Flag::Flagged => writer.write_all(b"\\Flagged"),
             Flag::Deleted => writer.write_all(b"\\Deleted"),
             Flag::Draft => writer.write_all(b"\\Draft"),
-
-            // ----- Fetch -----
-            Flag::Recent => writer.write_all(b"\\Recent"),
-
-            // ----- Selectability -----
-            Flag::NameAttribute(flag) => flag.encode(writer),
-
-            // ----- Keyword -----
-            Flag::Permanent => writer.write_all(b"\\*"),
+            Flag::Extension(other) => other.encode(writer),
             Flag::Keyword(atom) => atom.encode(writer),
+        }
+    }
+}
 
-            // ----- Others -----
-            Flag::Extension(atom) => {
-                writer.write_all(b"\\")?;
-                atom.encode(writer)
-            }
+impl<'a> Encode for FlagFetch<'a> {
+    fn encode(&self, writer: &mut impl Write) -> std::io::Result<()> {
+        match self {
+            Self::Flag(flag) => flag.encode(writer),
+            Self::Recent => writer.write_all(b"\\Recent"),
         }
     }
 }
 
+impl<'a> Encode for FlagPerm<'a> {
+    fn encode(&self, writer: &mut impl Write) -> std::io::Result<()> {
+        match self {
+            Self::Flag(flag) => flag.encode(writer),
+            Self::AllowNewKeywords => writer.write_all(b"\\*"),
+        }
+    }
+}
+
+impl<'a> Encode for FlagExtension<'a> {
+    fn encode(&self, writer: &mut impl Write) -> std::io::Result<()> {
+        writer.write_all(b"\\")?;
+        self.0.encode(writer)
+    }
+}
+
 impl Encode for MyDateTime {
     fn encode(&self, writer: &mut impl Write) -> std::io::Result<()> {
         self.0.encode(writer)
diff --git a/imap-types/src/lib.rs b/imap-types/src/lib.rs
index 25dc20aa..199a88d2 100644
--- a/imap-types/src/lib.rs
+++ b/imap-types/src/lib.rs
@@ -204,7 +204,7 @@ pub mod message {
     pub use crate::rfc3501::{
         core::{Charset, Tag},
         datetime::{MyDateTime, MyNaiveDate},
-        flag::{Flag, FlagNameAttribute},
+        flag::{Flag, FlagError, FlagExtension, FlagFetch, FlagNameAttribute, FlagPerm},
         mailbox::{Mailbox, MailboxOther},
         section::{Part, PartSpecifier, Section},
         AuthMechanism, AuthMechanismOther,
diff --git a/imap-types/src/rfc3501/core.rs b/imap-types/src/rfc3501/core.rs
index c336413f..1026857f 100644
--- a/imap-types/src/rfc3501/core.rs
+++ b/imap-types/src/rfc3501/core.rs
@@ -33,7 +33,7 @@ pub(crate) use impl_try_from;
 /// "An atom consists of one or more non-special characters." ([RFC 3501](https://www.rfc-editor.org/rfc/rfc3501.html))
 #[cfg_attr(feature = "bounded-static", derive(ToStatic))]
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
-#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+#[derive(Debug, Clone, PartialEq, Eq, Ord, PartialOrd, Hash)]
 pub struct Atom<'a>(pub(crate) Cow<'a, str>);
 
 impl<'a> Atom<'a> {
diff --git a/imap-types/src/rfc3501/fetch_attributes.rs b/imap-types/src/rfc3501/fetch_attributes.rs
index 6a40e952..d3a6d62a 100644
--- a/imap-types/src/rfc3501/fetch_attributes.rs
+++ b/imap-types/src/rfc3501/fetch_attributes.rs
@@ -9,7 +9,7 @@ use serde::{Deserialize, Serialize};
 
 use crate::{
     core::NString,
-    message::{Flag, MyDateTime, Section},
+    message::{FlagFetch, MyDateTime, Section},
     response::data::{BodyStructure, Envelope},
 };
 
@@ -243,7 +243,7 @@ pub enum FetchAttributeValue<'a> {
     /// A parenthesized list of flags that are set for this message.
     ///
     /// `FLAGS`
-    Flags(Vec<Flag<'a>>),
+    Flags(Vec<FlagFetch<'a>>),
 
     /// A string representing the internal date of the message.
     ///
diff --git a/imap-types/src/rfc3501/flag.rs b/imap-types/src/rfc3501/flag.rs
index d24a6b16..a7f09fbf 100644
--- a/imap-types/src/rfc3501/flag.rs
+++ b/imap-types/src/rfc3501/flag.rs
@@ -1,64 +1,150 @@
 // ### 2.3.2. Flags Message Attribute
 
+use std::convert::{TryFrom, TryInto};
+
 #[cfg(feature = "arbitrary")]
 use arbitrary::Arbitrary;
 #[cfg(feature = "bounded-static")]
 use bounded_static::ToStatic;
 #[cfg(feature = "serde")]
 use serde::{Deserialize, Serialize};
+use thiserror::Error;
 
-use crate::core::Atom;
+use crate::{core::Atom, rfc3501::core::AtomError};
 
-/// A list of zero or more named tokens associated with the message.  A
-/// flag is set by its addition to this list, and is cleared by its
-/// removal.  There are two types of flags in IMAP4rev1. A flag of either
-/// type can be permanent or session-only.
-/// TODO(#7): this struct is not very usable currently...
+/// There are two types of flags in IMAP4rev1: System and keyword flags.
+///
+/// A system flag is a flag name that is pre-defined in RFC3501.
+/// All system flags begin with "\" and certain system flags (`\Deleted` and `\Seen`) have special semantics.
+/// Flags that begin with "\" but are not pre-defined system flags, are extension flags.
+/// Clients MUST accept them and servers MUST NOT send them except when defined by future standard or standards-track revisions.
+///
+/// A keyword is defined by the server implementation.
+/// Keywords do not begin with "\" and servers may permit the client to define new ones
+/// in the mailbox by sending the "\*" flag ([`FlagPerm::AllowNewKeyword`]) in the PERMANENTFLAGS response..
+///
+/// Note that a flag of either type can be permanent or session-only.
 #[cfg_attr(feature = "arbitrary", derive(Arbitrary))]
 #[cfg_attr(feature = "bounded-static", derive(ToStatic))]
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub enum Flag<'a> {
-    // ----- System -----
-    //
-    // A system flag is a flag name that is pre-defined in this
-    // specification.  All system flags begin with "\".  Certain system
-    // flags (\Deleted and \Seen) have special semantics described elsewhere.
-    /// Message has been read (`\Seen`)
+    /// Message has been read (`\Seen`).
     Seen,
-    /// Message has been answered (`\Answered`)
+    /// Message has been answered (`\Answered`).
     Answered,
-    /// Message is "flagged" for urgent/special attention (`\Flagged`)
+    /// Message is "flagged" for urgent/special attention (`\Flagged`).
     Flagged,
-    /// Message is "deleted" for removal by later EXPUNGE (`\Deleted`)
+    /// Message is "deleted" for removal by later EXPUNGE (`\Deleted`).
     Deleted,
-    /// Message has not completed composition (marked as a draft). (`\Draft`)
+    /// Message has not completed composition (marked as a draft) (`\Draft`).
     Draft,
+    /// A future expansion of a system flag.
+    Extension(FlagExtension<'a>),
+    /// A keyword.
+    Keyword(Atom<'a>),
+}
+
+impl<'a> Flag<'a> {
+    pub fn system<A>(value: A) -> Result<Self, FlagError<'a, A::Error>>
+    where
+        A: TryInto<Atom<'a>>,
+    {
+        let atom = value.try_into()?;
+
+        match atom.as_ref().to_ascii_lowercase().as_ref() {
+            "answered" => Ok(Self::Answered),
+            "flagged" => Ok(Self::Flagged),
+            "deleted" => Ok(Self::Deleted),
+            "seen" => Ok(Self::Seen),
+            "draft" => Ok(Self::Draft),
+            _ => Err(FlagError::IsAnExtensionFlag { candidate: atom }),
+        }
+    }
+
+    pub fn extension<A>(value: A) -> Result<Self, FlagError<'a, A::Error>>
+    where
+        A: TryInto<Atom<'a>>,
+    {
+        let atom = value.try_into()?;
+
+        match atom.as_ref().to_ascii_lowercase().as_ref() {
+            "answered" | "flagged" | "deleted" | "seen" | "draft" => {
+                Err(FlagError::IsASystemFlag { candidate: atom })
+            }
+            _ => Ok(Self::Extension(FlagExtension(atom))),
+        }
+    }
+
+    pub fn system_or_extension(atom: Atom<'a>) -> Self {
+        match Self::system(atom) {
+            Ok(system) => system,
+            Err(FlagError::Atom(_)) => unreachable!(),
+            Err(FlagError::IsASystemFlag { .. }) => unreachable!(),
+            Err(FlagError::IsAnExtensionFlag { candidate }) => {
+                Self::Extension(FlagExtension(candidate))
+            }
+        }
+    }
+}
+
+#[cfg_attr(feature = "arbitrary", derive(Arbitrary))]
+#[cfg_attr(feature = "bounded-static", derive(ToStatic))]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub enum FlagFetch<'a> {
+    Flag(Flag<'a>),
 
-    // ----- Fetch -----
     /// Message is "recently" arrived in this mailbox. (`\Recent`)
     ///
-    /// This session is the first session to have been notified about this
-    /// message; if the session is read-write, subsequent sessions
-    /// will not see \Recent set for this message.  This flag can not
-    /// be altered by the client.
+    /// This session is the first session to have been notified about this message; if the session
+    /// is read-write, subsequent sessions will not see \Recent set for this message.
+    ///
+    /// Note: This flag can not be altered by the client.
     Recent,
+}
 
-    // ----- Selectability -----
-    NameAttribute(FlagNameAttribute<'a>),
+#[cfg_attr(feature = "arbitrary", derive(Arbitrary))]
+#[cfg_attr(feature = "bounded-static", derive(ToStatic))]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub enum FlagPerm<'a> {
+    Flag(Flag<'a>),
 
-    // ----- Keyword -----
     /// Indicates that it is possible to create new keywords by
-    /// attempting to store those flags in the mailbox. (`\*`)
-    Permanent,
-    /// A keyword is defined by the server implementation.  Keywords do not
-    /// begin with "\".  Servers MAY permit the client to define new keywords
-    /// in the mailbox (see the description of the PERMANENTFLAGS response
-    /// code for more information).
-    Keyword(Atom<'a>),
+    /// attempting to store those flags in the mailbox (`\*`).
+    AllowNewKeywords,
+}
+
+/// Client implementations MUST accept flag-extension flags.
+/// Server implementations MUST NOT generate flag-extension flags
+/// except as defined by future standard or standards-track revisions of this specification.
+#[cfg_attr(feature = "bounded-static", derive(ToStatic))]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub struct FlagExtension<'a>(pub(crate) Atom<'a>);
+
+impl<'a> TryFrom<Atom<'a>> for FlagExtension<'a> {
+    type Error = FlagError<'a, AtomError>;
+
+    fn try_from(value: Atom<'a>) -> Result<Self, Self::Error> {
+        match value.as_ref().to_ascii_lowercase().as_ref() {
+            "answered" | "flagged" | "deleted" | "seen" | "draft" => {
+                Err(FlagError::IsASystemFlag { candidate: value })
+            }
+            _ => Ok(Self(value)),
+        }
+    }
+}
 
-    // ----- Others -----
-    Extension(Atom<'a>), // FIXME(#32): How to treat Extension(Atom("Recent"))
+#[derive(Clone, Debug, Eq, Error, Hash, Ord, PartialEq, PartialOrd)]
+pub enum FlagError<'a, A> {
+    #[error(transparent)]
+    Atom(#[from] A),
+    #[error("Is a system flag.")]
+    IsASystemFlag { candidate: Atom<'a> },
+    #[error("Is an extension flag.")]
+    IsAnExtensionFlag { candidate: Atom<'a> },
 }
 
 /// Four name attributes are defined.
diff --git a/imap-types/src/rfc3501/response.rs b/imap-types/src/rfc3501/response.rs
index d9c002ff..89915736 100644
--- a/imap-types/src/rfc3501/response.rs
+++ b/imap-types/src/rfc3501/response.rs
@@ -27,7 +27,7 @@ use crate::{
 };
 use crate::{
     core::{Atom, NonEmptyVec},
-    message::{AuthMechanism, Charset, Flag, FlagNameAttribute, Mailbox, Tag},
+    message::{AuthMechanism, Charset, Flag, FlagNameAttribute, FlagPerm, Mailbox, Tag},
     response::{
         data::{FetchAttributeValue, QuotedChar, StatusAttributeValue},
         Text,
@@ -730,7 +730,7 @@ pub enum Code<'a> {
     /// The PERMANENTFLAGS list can also include the special flag \*,
     /// which indicates that it is possible to create new keywords by
     /// attempting to store those flags in the mailbox.
-    PermanentFlags(Vec<Flag<'a>>),
+    PermanentFlags(Vec<FlagPerm<'a>>),
 
     /// `READ-ONLY`
     ///
@@ -816,7 +816,7 @@ impl<'a> Code<'a> {
         Ok(Self::Capability(caps.try_into()?))
     }
 
-    pub fn permanentflags(flags: Vec<Flag<'a>>) -> Self {
+    pub fn permanentflags(flags: Vec<FlagPerm<'a>>) -> Self {
         Self::PermanentFlags(flags)
     }
 
diff --git a/src/rfc3501/flag.rs b/src/rfc3501/flag.rs
index 9911f146..6365591c 100644
--- a/src/rfc3501/flag.rs
+++ b/src/rfc3501/flag.rs
@@ -1,54 +1,54 @@
 use abnf_core::streaming::SP;
 use imap_types::{
     core::Atom,
-    message::{Flag, FlagNameAttribute},
+    message::{Flag, FlagFetch, FlagNameAttribute, FlagPerm},
 };
 use nom::{
     branch::alt,
     bytes::streaming::{tag, tag_no_case},
-    combinator::{map, value},
+    character::streaming::char,
+    combinator::{map, recognize, value},
     multi::{separated_list0, separated_list1},
-    sequence::{delimited, preceded},
+    sequence::{delimited, preceded, tuple},
     IResult,
 };
 
 use crate::rfc3501::core::atom;
 
-/// `flag = "\Answered" /
-///         "\Flagged" /
-///         "\Deleted" /
-///         "\Seen" /
-///         "\Draft" /
-///         flag-keyword /
-///         flag-extension`
+/// ```abnf
+/// flag = "\Answered" /
+///        "\Flagged" /
+///        "\Deleted" /
+///        "\Seen" /
+///        "\Draft" /
+///        flag-keyword /
+///        flag-extension
+/// ```
 ///
 /// Note: Does not include "\Recent"
 pub fn flag(input: &[u8]) -> IResult<&[u8], Flag> {
     alt((
-        value(Flag::Answered, tag_no_case(b"\\Answered")),
-        value(Flag::Flagged, tag_no_case(b"\\Flagged")),
-        value(Flag::Deleted, tag_no_case(b"\\Deleted")),
-        value(Flag::Seen, tag_no_case(b"\\Seen")),
-        value(Flag::Draft, tag_no_case(b"\\Draft")),
-        flag_keyword,
-        map(flag_extension, Flag::Extension),
+        map(preceded(char('\\'), atom), Flag::system_or_extension),
+        map(atom, Flag::Keyword),
     ))(input)
 }
 
-/// `flag-fetch = flag / "\Recent"`
-pub fn flag_fetch(input: &[u8]) -> IResult<&[u8], Flag> {
-    alt((flag, value(Flag::Recent, tag_no_case(b"\\Recent"))))(input)
-}
-
-/// `flag-perm = flag / "\*"`
-pub fn flag_perm(input: &[u8]) -> IResult<&[u8], Flag> {
-    alt((flag, value(Flag::Permanent, tag(b"\\*"))))(input)
-}
+// Note(duesee): This was inlined into [`flag`].
+// #[inline]
+// /// `flag-keyword = atom`
+// pub fn flag_keyword(input: &[u8]) -> IResult<&[u8], Flag> {
+//     map(atom, Flag::Keyword)(input)
+// }
 
-#[inline]
-/// `flag-keyword = atom`
-pub fn flag_keyword(input: &[u8]) -> IResult<&[u8], Flag> {
-    map(atom, Flag::Keyword)(input)
+/// `flag-extension = "\" atom`
+///
+/// Future expansion.
+///
+/// Client implementations MUST accept flag-extension flags.
+/// Server implementations MUST NOT generate flag-extension flags
+/// except as defined by future standard or standards-track revisions of this specification.
+pub fn flag_extension(input: &[u8]) -> IResult<&[u8], Atom> {
+    preceded(tag(b"\\"), atom)(input)
 }
 
 /// `flag-list = "(" [flag *(SP flag)] ")"`
@@ -56,6 +56,25 @@ pub fn flag_list(input: &[u8]) -> IResult<&[u8], Vec<Flag>> {
     delimited(tag(b"("), separated_list0(SP, flag), tag(b")"))(input)
 }
 
+/// `flag-fetch = flag / "\Recent"`
+pub fn flag_fetch(input: &[u8]) -> IResult<&[u8], FlagFetch> {
+    if let Ok((rem, peek)) = recognize(tuple((char('\\'), atom)))(input) {
+        if peek.to_ascii_lowercase() == b"\\recent" {
+            return Ok((rem, FlagFetch::Recent));
+        }
+    }
+
+    map(flag, FlagFetch::Flag)(input)
+}
+
+/// `flag-perm = flag / "\*"`
+pub fn flag_perm(input: &[u8]) -> IResult<&[u8], FlagPerm> {
+    alt((
+        value(FlagPerm::AllowNewKeywords, tag("\\*")),
+        map(flag, FlagPerm::Flag),
+    ))(input)
+}
+
 /// `mbx-list-flags = *(mbx-list-oflag SP) mbx-list-sflag *(SP mbx-list-oflag) /
 ///                                        mbx-list-oflag *(SP mbx-list-oflag)`
 ///
@@ -64,17 +83,18 @@ pub fn flag_list(input: &[u8]) -> IResult<&[u8], Vec<Flag>> {
 pub fn mbx_list_flags(input: &[u8]) -> IResult<&[u8], Vec<FlagNameAttribute>> {
     let (remaining, flags) = separated_list1(SP, alt((mbx_list_sflag, mbx_list_oflag)))(input)?;
 
-    let sflag_count = flags
-        .iter()
-        .filter(|&flag| FlagNameAttribute::is_selectability(flag))
-        .count();
-
-    if sflag_count > 1 {
-        return Err(nom::Err::Failure(nom::error::make_error(
-            input,
-            nom::error::ErrorKind::Verify,
-        )));
-    }
+    // TODO: Do we really want to enforce this?
+    // let sflag_count = flags
+    //     .iter()
+    //     .filter(|&flag| FlagNameAttribute::is_selectability(flag))
+    //     .count();
+    //
+    // if sflag_count > 1 {
+    //     return Err(nom::Err::Failure(nom::error::make_error(
+    //         input,
+    //         nom::error::ErrorKind::Verify,
+    //     )));
+    // }
 
     Ok((remaining, flags))
 }
@@ -103,13 +123,38 @@ pub fn mbx_list_sflag(input: &[u8]) -> IResult<&[u8], FlagNameAttribute> {
     ))(input)
 }
 
-/// `flag-extension = "\" atom`
-///
-/// Future expansion.
-///
-/// Client implementations MUST accept flag-extension flags.
-/// Server implementations MUST NOT generate flag-extension flags
-/// except as defined by future standard or standards-track revisions of this specification.
-pub fn flag_extension(input: &[u8]) -> IResult<&[u8], Atom> {
-    preceded(tag(b"\\"), atom)(input)
+#[cfg(test)]
+mod tests {
+    use std::convert::TryFrom;
+
+    use super::*;
+
+    #[test]
+    fn test_flag_fetch() {
+        let tests = [(
+            "iS)",
+            FlagFetch::Flag(Flag::Keyword(Atom::try_from("iS").unwrap())),
+        )];
+
+        for (test, expected) in tests {
+            let (rem, got) = flag_fetch(test.as_bytes()).unwrap();
+            assert_eq!(rem.len(), 1);
+            assert_eq!(expected, got);
+        }
+    }
+
+    #[test]
+    fn test_flag_perm() {
+        let tests = [
+            ("\\Deleted)", FlagPerm::Flag(Flag::Deleted)),
+            ("\\Seen ", FlagPerm::Flag(Flag::Seen)),
+            ("\\*)", FlagPerm::AllowNewKeywords),
+        ];
+
+        for (test, expected) in tests {
+            let (rem, got) = flag_perm(test.as_bytes()).unwrap();
+            assert_eq!(rem.len(), 1);
+            assert_eq!(expected, got);
+        }
+    }
 }


Test patch:
diff --git a/tests/trace.rs b/tests/trace.rs
index 094772f6..00b15464 100644
--- a/tests/trace.rs
+++ b/tests/trace.rs
@@ -9,13 +9,13 @@ use imap_codec::{
         Command, CommandBody,
     },
     core::{AString, IString, Quoted},
-    message::{AuthMechanism, Flag, Section, Tag},
+    message::{AuthMechanism, Flag, FlagPerm, Section, Tag},
     response::{
         data::{Capability, FetchAttributeValue},
         Code, Data, Response, Status,
     },
 };
-use imap_types::{response::Greeting, security::Secret};
+use imap_types::{message::FlagFetch, response::Greeting, security::Secret};
 
 enum Who {
     Client,
@@ -289,7 +289,10 @@ fn test_from_noop() {
                 Message::Response(Response::Data(
                     Data::fetch(
                         14,
-                        vec![FetchAttributeValue::Flags(vec![Flag::Seen, Flag::Deleted])],
+                        vec![FetchAttributeValue::Flags(vec![
+                            FlagFetch::Flag(Flag::Seen),
+                            FlagFetch::Flag(Flag::Deleted),
+                        ])],
                     )
                     .unwrap(),
                 )),
@@ -503,9 +506,9 @@ fn test_from_select() {
                     Status::ok(
                         None,
                         Some(Code::PermanentFlags(vec![
-                            Flag::Deleted,
-                            Flag::Seen,
-                            Flag::Permanent,
+                            FlagPerm::Flag(Flag::Deleted),
+                            FlagPerm::Flag(Flag::Seen),
+                            FlagPerm::AllowNewKeywords,
                         ])),
                         "Limited",
                     )