Make ContactId::LAST_SPECIAL private

Also remove the Ord implementations, this makes ContactId more opaque
by no longer letting people deal with the fact this is ordered.

deltachat-ffi/src/lib.rs

@@ -1949,18 +1949,19 @@ pub unsafe extern "C" fn dc_block_contact(
     contact_id: u32,
     block: libc::c_int,
 ) {
-    if context.is_null() || contact_id <= ContactId::LAST_SPECIAL.to_u32() {
+    let contact_id = ContactId::new(contact_id);
+    if context.is_null() || contact_id.is_special() {
         eprintln!("ignoring careless call to dc_block_contact()");
         return;
     }
     let ctx = &*context;
     block_on(async move {
         if block == 0 {
-            Contact::unblock(ctx, ContactId::new(contact_id))
+            Contact::unblock(ctx, contact_id)
                 .await
                 .ok_or_log_msg(ctx, "Can't unblock contact");
         } else {
-            Contact::block(ctx, ContactId::new(contact_id))
+            Contact::block(ctx, contact_id)
                 .await
                 .ok_or_log_msg(ctx, "Can't block contact");
         }
@@ -1994,14 +1995,15 @@ pub unsafe extern "C" fn dc_delete_contact(
     context: *mut dc_context_t,
     contact_id: u32,
 ) -> libc::c_int {
-    if context.is_null() || contact_id <= ContactId::LAST_SPECIAL.to_u32() {
+    let contact_id = ContactId::new(contact_id);
+    if context.is_null() || contact_id.is_special() {
         eprintln!("ignoring careless call to dc_delete_contact()");
         return 0;
     }
     let ctx = &*context;
 
     block_on(async move {
-        match Contact::delete(ctx, ContactId::new(contact_id)).await {
+        match Contact::delete(ctx, contact_id).await {
             Ok(_) => 1,
             Err(_) => 0,
         }

src/chat.rs

@@ -860,7 +860,7 @@ impl ChatId {
         for contact_id in get_chat_contacts(context, self)
             .await?
             .iter()
-            .filter(|&contact_id| *contact_id > ContactId::LAST_SPECIAL)
+            .filter(|&contact_id| !contact_id.is_special())
         {
             let contact = Contact::load_from_db(context, *contact_id).await?;
             let addr = contact.get_addr();
@@ -1686,7 +1686,7 @@ impl ChatIdBlocked {
     ) -> Result<Option<Self>> {
         ensure!(context.sql.is_open().await, "Database not available");
         ensure!(
-            contact_id > ContactId::new(0),
+            contact_id != ContactId::UNDEFINED,
             "Invalid contact id requested"
         );
 
@@ -1722,7 +1722,7 @@ impl ChatIdBlocked {
     ) -> Result<Self> {
         ensure!(context.sql.is_open().await, "Database not available");
         ensure!(
-            contact_id > ContactId::new(0),
+            contact_id != ContactId::UNDEFINED,
             "Invalid contact id requested"
         );
 
@@ -2888,7 +2888,7 @@ pub async fn remove_contact_from_chat(
         chat_id
     );
     ensure!(
-        contact_id > ContactId::LAST_SPECIAL || contact_id == ContactId::SELF,
+        !contact_id.is_special() || contact_id == ContactId::SELF,
         "Cannot remove special contact"
     );
 

src/contact.rs

@@ -30,9 +30,7 @@ use crate::{chat, stock_str};
 ///
 /// Some contact IDs are reserved to identify special contacts.  This
 /// type can represent both the special as well as normal contacts.
-#[derive(
+#[derive(Debug, Copy, Clone, Default, PartialEq, Eq, Hash, Serialize, Deserialize)]
-    Debug, Copy, Clone, Default, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize,
-)]
 pub struct ContactId(u32);
 
 impl ContactId {
@@ -43,9 +41,9 @@ impl ContactId {
     pub const SELF: ContactId = ContactId::new(1);
     pub const INFO: ContactId = ContactId::new(2);
     pub const DEVICE: ContactId = ContactId::new(5);
-    pub const LAST_SPECIAL: ContactId = ContactId::new(9);
+    const LAST_SPECIAL: ContactId = ContactId::new(9);
 
-    /// Address to go with [`ContactId::Device`].
+    /// Address to go with [`ContactId::DEVICE`].
     ///
     /// This is used by APIs which need to return an email address for this contact.
     pub const DEVICE_ADDR: &'static str = "device@localhost";
@@ -55,6 +53,16 @@ impl ContactId {
         ContactId(id)
     }
 
+    /// Whether this is a special [`ContactId`].
+    ///
+    /// Some [`ContactId`]s are reserved for special contacts like [`ContactId::SELF`],
+    /// [`ContactId::INFO`] and [`ContactId::DEVICE`].  This function indicates whether this
+    /// [`ContactId`] is any of the reserved special [`ContactId`]s (`true`) or whether it
+    /// is the [`ContactId`] of a real contact (`false`).
+    pub fn is_special(&self) -> bool {
+        self.0 <= Self::LAST_SPECIAL.0
+    }
+
     /// Numerical representation of the [`ContactId`].
     ///
     /// Each contact ID has a unique numerical representation which is used in the database
@@ -75,7 +83,7 @@ impl fmt::Display for ContactId {
             write!(f, "Contact#Info")
         } else if *self == ContactId::DEVICE {
             write!(f, "Contact#Device")
-        } else if *self <= ContactId::LAST_SPECIAL {
+        } else if self.is_special() {
             write!(f, "Contact#Special{}", self.0)
         } else {
             write!(f, "Contact#{}", self.0)
@@ -856,7 +864,7 @@ impl Contact {
     /// fingerprints of the keys involved.
     pub async fn get_encrinfo(context: &Context, contact_id: ContactId) -> Result<String> {
         ensure!(
-            contact_id > ContactId::LAST_SPECIAL,
+            !contact_id.is_special(),
             "Can not provide encryption info for special contact"
         );
 
@@ -924,10 +932,7 @@ impl Contact {
     ///
     /// May result in a `#DC_EVENT_CONTACTS_CHANGED` event.
     pub async fn delete(context: &Context, contact_id: ContactId) -> Result<()> {
-        ensure!(
+        ensure!(!contact_id.is_special(), "Can not delete special contact");
-            contact_id > ContactId::LAST_SPECIAL,
-            "Can not delete special contact"
-        );
 
         let count_chats = context
             .sql
@@ -1157,7 +1162,7 @@ impl Contact {
     }
 
     pub async fn real_exists_by_id(context: &Context, contact_id: ContactId) -> Result<bool> {
-        if contact_id <= ContactId::LAST_SPECIAL {
+        if contact_id.is_special() {
             return Ok(false);
         }
 
@@ -1230,7 +1235,7 @@ async fn set_block_contact(
     new_blocking: bool,
 ) -> Result<()> {
     ensure!(
-        contact_id > ContactId::LAST_SPECIAL,
+        !contact_id.is_special(),
         "Can't block special contact {}",
         contact_id
     );
@@ -1370,7 +1375,7 @@ pub(crate) async fn update_last_seen(
     timestamp: i64,
 ) -> Result<()> {
     ensure!(
-        contact_id > ContactId::LAST_SPECIAL,
+        !contact_id.is_special(),
         "Can not update special contact last seen timestamp"
     );
 
@@ -1615,7 +1620,7 @@ mod tests {
             Contact::add_or_lookup(&t, "bla foo", "one@eins.org", Origin::IncomingUnknownTo)
                 .await
                 .unwrap();
-        assert!(contact_id > ContactId::LAST_SPECIAL);
+        assert!(!contact_id.is_special());
         assert_eq!(sth_modified, Modifier::Modified);
         let contact = Contact::load_from_db(&t, contact_id).await.unwrap();
         assert_eq!(contact.get_id(), contact_id);
@@ -1642,7 +1647,7 @@ mod tests {
             Contact::add_or_lookup(&t, "", "three@drei.sam", Origin::IncomingUnknownTo)
                 .await
                 .unwrap();
-        assert!(contact_id > ContactId::LAST_SPECIAL);
+        assert!(!contact_id.is_special());
         assert_eq!(sth_modified, Modifier::None);
         let contact = Contact::load_from_db(&t, contact_id).await.unwrap();
         assert_eq!(contact.get_name(), "");
@@ -1682,7 +1687,7 @@ mod tests {
             Contact::add_or_lookup(&t, "", "alice@w.de", Origin::IncomingUnknownTo)
                 .await
                 .unwrap();
-        assert!(contact_id > ContactId::LAST_SPECIAL);
+        assert!(!contact_id.is_special());
         assert_eq!(sth_modified, Modifier::None);
         let contact = Contact::load_from_db(&t, contact_id).await.unwrap();
         assert_eq!(contact.get_name(), "Wonderland, Alice");
@@ -1735,7 +1740,7 @@ mod tests {
             Contact::add_or_lookup(&t, "bob1", "bob@example.org", Origin::IncomingUnknownFrom)
                 .await
                 .unwrap();
-        assert!(contact_id > ContactId::LAST_SPECIAL);
+        assert!(!contact_id.is_special());
         assert_eq!(sth_modified, Modifier::Created);
         let contact = Contact::load_from_db(&t, contact_id).await.unwrap();
         assert_eq!(contact.get_authname(), "bob1");
@@ -1747,7 +1752,7 @@ mod tests {
             Contact::add_or_lookup(&t, "bob2", "bob@example.org", Origin::IncomingUnknownFrom)
                 .await
                 .unwrap();
-        assert!(contact_id > ContactId::LAST_SPECIAL);
+        assert!(!contact_id.is_special());
         assert_eq!(sth_modified, Modifier::Modified);
         let contact = Contact::load_from_db(&t, contact_id).await.unwrap();
         assert_eq!(contact.get_authname(), "bob2");
@@ -1758,7 +1763,7 @@ mod tests {
         let contact_id = Contact::create(&t, "bob3", "bob@example.org")
             .await
             .unwrap();
-        assert!(contact_id > ContactId::LAST_SPECIAL);
+        assert!(!contact_id.is_special());
         let contact = Contact::load_from_db(&t, contact_id).await.unwrap();
         assert_eq!(contact.get_authname(), "bob2");
         assert_eq!(contact.get_name(), "bob3");
@@ -1769,7 +1774,7 @@ mod tests {
             Contact::add_or_lookup(&t, "bob4", "bob@example.org", Origin::IncomingUnknownFrom)
                 .await
                 .unwrap();
-        assert!(contact_id > ContactId::LAST_SPECIAL);
+        assert!(!contact_id.is_special());
         assert_eq!(sth_modified, Modifier::Modified);
         let contact = Contact::load_from_db(&t, contact_id).await.unwrap();
         assert_eq!(contact.get_authname(), "bob4");
@@ -1783,7 +1788,7 @@ mod tests {
 
         // manually create "claire@example.org" without a given name
         let contact_id = Contact::create(&t, "", "claire@example.org").await.unwrap();
-        assert!(contact_id > ContactId::LAST_SPECIAL);
+        assert!(!contact_id.is_special());
         let contact = Contact::load_from_db(&t, contact_id).await.unwrap();
         assert_eq!(contact.get_authname(), "");
         assert_eq!(contact.get_name(), "");

src/dc_receive_imf.rs

@@ -1,7 +1,7 @@
 //! Internet Message Format reception pipeline.
 
 use std::cmp::min;
-use std::collections::BTreeSet;
+use std::collections::HashSet;
 use std::convert::TryFrom;
 
 use anyhow::{bail, ensure, Context as _, Result};
@@ -220,7 +220,7 @@ pub(crate) async fn dc_receive_imf_inner(
     .await
     .context("add_parts error")?;
 
-    if from_id > ContactId::LAST_SPECIAL {
+    if !from_id.is_special() {
         contact::update_last_seen(context, from_id, sent_timestamp).await?;
     }
 
@@ -1489,8 +1489,7 @@ async fn create_or_lookup_group(
 
         // Create initial member list.
         chat::add_to_chat_contacts_table(context, new_chat_id, ContactId::SELF).await?;
-        if from_id > ContactId::LAST_SPECIAL
+        if !from_id.is_special() && !chat::is_contact_in_chat(context, new_chat_id, from_id).await?
-            && !chat::is_contact_in_chat(context, new_chat_id, from_id).await?
         {
             chat::add_to_chat_contacts_table(context, new_chat_id, from_id).await?;
         }
@@ -1674,7 +1673,7 @@ async fn apply_group_changes(
                     chat::add_to_chat_contacts_table(context, chat_id, ContactId::SELF).await?;
                 }
             }
-            if from_id > ContactId::LAST_SPECIAL
+            if !from_id.is_special()
                 && !Contact::addr_equals_contact(context, &self_addr, from_id).await?
                 && !chat::is_contact_in_chat(context, chat_id, from_id).await?
                 && removed_id != Some(from_id)
@@ -2261,7 +2260,7 @@ async fn dc_add_or_lookup_contacts_by_address_list(
     origin: Origin,
     prevent_rename: bool,
 ) -> Result<Vec<ContactId>> {
-    let mut contact_ids = BTreeSet::new();
+    let mut contact_ids = HashSet::new();
     for info in address_list.iter() {
         let addr = &info.addr;
         if !may_be_valid_addr(addr) {

src/securejoin.rs

@@ -310,7 +310,7 @@ pub(crate) async fn handle_securejoin_handshake(
     mime_message: &MimeMessage,
     contact_id: ContactId,
 ) -> Result<HandshakeMessage> {
-    if contact_id <= ContactId::LAST_SPECIAL {
+    if contact_id.is_special() {
         return Err(Error::msg("Can not be called with special contact ID"));
     }
     let step = mime_message
@@ -573,7 +573,7 @@ pub(crate) async fn observe_securejoin_on_other_device(
     mime_message: &MimeMessage,
     contact_id: ContactId,
 ) -> Result<HandshakeMessage> {
-    if contact_id <= ContactId::LAST_SPECIAL {
+    if contact_id.is_special() {
         return Err(Error::msg("Can not be called with special contact ID"));
     }
     let step = mime_message