From 0580056b62e56f09652a74c9895891a501ebc94e Mon Sep 17 00:00:00 2001 From: Hocuri Date: Mon, 27 Apr 2026 11:09:01 +0200 Subject: [PATCH] refactor: Use regular functions rather than FromStr impls (#8178) Implementing `FromStr` and then calling `parse()` creates an indirection, which is hard to follow for people who are not familiar with Rust. @r10s recently voiced this problem when we were pair-programming, and I agree. We can decide what exactly to call the new function. I didn't remove all `FromStr` implementations yet; `FromStr for Fingerprint`, `FromStr for Params`, `FromStr for MozConfigTag` and `FromStr for EphemeralTimer` are still left. --- src/aheader.rs | 28 ++++++++++++---------------- src/mimeparser.rs | 2 +- src/reaction.rs | 41 ++++++++++++++++++----------------------- src/receive_imf.rs | 5 +++-- 4 files changed, 34 insertions(+), 42 deletions(-) diff --git a/src/aheader.rs b/src/aheader.rs index c82ce545b..218d08a58 100644 --- a/src/aheader.rs +++ b/src/aheader.rs @@ -4,9 +4,8 @@ use std::collections::BTreeMap; use std::fmt; -use std::str::FromStr; -use anyhow::{Context as _, Error, Result, bail}; +use anyhow::{Context as _, Result, bail}; use crate::key::{DcKey, SignedPublicKey}; @@ -28,10 +27,8 @@ impl fmt::Display for EncryptPreference { } } -impl FromStr for EncryptPreference { - type Err = Error; - - fn from_str(s: &str) -> Result { +impl EncryptPreference { + fn new(s: &str) -> Result { match s { "mutual" => Ok(EncryptPreference::Mutual), "nopreference" => Ok(EncryptPreference::NoPreference), @@ -85,10 +82,8 @@ impl fmt::Display for Aheader { } } -impl FromStr for Aheader { - type Err = Error; - - fn from_str(s: &str) -> Result { +impl Aheader { + pub(crate) fn from_str(s: &str) -> Result { let mut attributes: BTreeMap = s .split(';') .filter_map(|a| { @@ -116,7 +111,7 @@ impl FromStr for Aheader { let prefer_encrypt = attributes .remove("prefer-encrypt") - .and_then(|raw| raw.parse().ok()) + .and_then(|raw| EncryptPreference::new(&raw).ok()) .unwrap_or_default(); let verified = attributes.remove("_verified").is_some(); @@ -144,8 +139,9 @@ mod tests { #[test] fn test_from_str() -> Result<()> { - let h: Aheader = - format!("addr=me@mail.com; prefer-encrypt=mutual; keydata={RAWKEY}").parse()?; + let h = Aheader::from_str(&format!( + "addr=me@mail.com; prefer-encrypt=mutual; keydata={RAWKEY}" + ))?; assert_eq!(h.addr, "me@mail.com"); assert_eq!(h.prefer_encrypt, EncryptPreference::Mutual); @@ -157,7 +153,7 @@ mod tests { #[test] fn test_from_str_reset() -> Result<()> { let raw = format!("addr=reset@example.com; prefer-encrypt=reset; keydata={RAWKEY}"); - let h: Aheader = raw.parse()?; + let h = Aheader::from_str(&raw)?; assert_eq!(h.addr, "reset@example.com"); assert_eq!(h.prefer_encrypt, EncryptPreference::NoPreference); @@ -167,7 +163,7 @@ mod tests { #[test] fn test_from_str_non_critical() -> Result<()> { let raw = format!("addr=me@mail.com; _foo=one; _bar=two; keydata={RAWKEY}"); - let h: Aheader = raw.parse()?; + let h = Aheader::from_str(&raw)?; assert_eq!(h.addr, "me@mail.com"); assert_eq!(h.prefer_encrypt, EncryptPreference::NoPreference); @@ -177,7 +173,7 @@ mod tests { #[test] fn test_from_str_superflous_critical() { let raw = format!("addr=me@mail.com; _foo=one; _bar=two; other=me; keydata={RAWKEY}"); - assert!(raw.parse::().is_err()); + assert!(Aheader::from_str(&raw).is_err()); } #[test] diff --git a/src/mimeparser.rs b/src/mimeparser.rs index 04e618a85..36dfc2f78 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -2127,7 +2127,7 @@ async fn parse_gossip_headers( let mut gossiped_keys: BTreeMap = Default::default(); for value in &gossip_headers { - let header = match value.parse::() { + let header = match Aheader::from_str(value) { Ok(header) => header, Err(err) => { warn!(context, "Failed parsing Autocrypt-Gossip header: {}", err); diff --git a/src/reaction.rs b/src/reaction.rs index 515a468e3..b459a132e 100644 --- a/src/reaction.rs +++ b/src/reaction.rs @@ -36,10 +36,7 @@ pub struct Reaction { reaction: String, } -// We implement From<&str> instead of std::str::FromStr, because -// FromStr requires error type and reaction parsing never returns an -// error. -impl From<&str> for Reaction { +impl Reaction { /// Convert a `&str` into a `Reaction`. /// Everything after the first whitespace is ignored. /// @@ -51,7 +48,7 @@ impl From<&str> for Reaction { /// reactions is not different from other kinds of spam attacks /// such as sending large numbers of large messages, and should be /// dealt with the same way, e.g. by blocking the user. - fn from(reaction: &str) -> Self { + pub fn new(reaction: &str) -> Self { let reaction: &str = reaction .split_ascii_whitespace() .next() @@ -61,9 +58,7 @@ impl From<&str> for Reaction { reaction: reaction.to_string(), } } -} -impl Reaction { /// Returns true if reaction contains no emoji. pub fn is_empty(&self) -> bool { self.reaction.is_empty() @@ -212,7 +207,7 @@ pub async fn send_reaction(context: &Context, msg_id: MsgId, reaction: &str) -> let msg = Message::load_from_db(context, msg_id).await?; let chat_id = msg.chat_id; - let reaction: Reaction = reaction.into(); + let reaction = Reaction::new(reaction); let mut reaction_msg = Message::new_text(reaction.as_str().to_string()); reaction_msg.set_reaction(); reaction_msg.in_reply_to = Some(msg.rfc724_mid); @@ -282,7 +277,7 @@ pub async fn get_msg_reactions(context: &Context, msg_id: MsgId) -> Result panic!("Unexpected event {event:?}."), } diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 33e8ad5be..ebad8036d 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -3,6 +3,7 @@ use std::cmp; use std::collections::{BTreeMap, BTreeSet}; use std::iter; +use std::str::FromStr as _; use std::sync::LazyLock; use anyhow::{Context as _, Result, ensure}; @@ -1828,7 +1829,7 @@ async fn add_parts( // Extract ephemeral timer from the message let mut ephemeral_timer = if let Some(value) = mime_parser.get_header(HeaderDef::EphemeralTimer) { - match value.parse::() { + match EphemeralTimer::from_str(value) { Ok(timer) => timer, Err(err) => { warn!(context, "Can't parse ephemeral timer \"{value}\": {err:#}."); @@ -2106,7 +2107,7 @@ async fn add_parts( chat_id, from_id, sort_timestamp, - Reaction::from(reaction_str.as_str()), + Reaction::new(reaction_str.as_str()), is_incoming_fresh, ) .await?;