elaborate error macro and apply at various callsites

Signed-off-by: Jason Volk <jason@zemos.net>

src/core/error/err.rs

@@ -1,3 +1,36 @@
+//! Error construction macros
+//!
+//! These are specialized macros specific to this project's patterns for
+//! throwing Errors; they make Error construction succinct and reduce clutter.
+//! They are developed from folding existing patterns into the macro while
+//! fixing several anti-patterns in the codebase.
+//!
+//! - The primary macros `Err!` and `err!` are provided. `Err!` simply wraps
+//!   `err!` in the Result variant to reduce `Err(err!(...))` boilerplate, thus
+//!   `err!` can be used in any case.
+//!
+//! 1. The macro makes the general Error construction easy: `return
+//!    Err!("something went wrong")` replaces the prior `return
+//!    Err(Error::Err("something went wrong".to_owned()))`.
+//!
+//! 2. The macro integrates format strings automatically: `return
+//!    Err!("something bad: {msg}")` replaces the prior `return
+//!    Err(Error::Err(format!("something bad: {msg}")))`.
+//!
+//! 3. The macro scopes variants of Error: `return Err!(Database("problem with
+//!    bad database."))` replaces the prior `return Err(Error::Database("problem
+//!    with bad database."))`.
+//!
+//! 4. The macro matches and scopes some special-case sub-variants, for example
+//!    with ruma ErrorKind: `return Err!(Request(MissingToken("you must provide
+//!    an access token")))`.
+//!
+//! 5. The macro fixes the anti-pattern of repeating messages in an error! log
+//!    and then again in an Error construction, often slightly different due to
+//!    the Error variant not supporting a format string. Instead `return
+//!    Err(Database(error!("problem with db: {msg}")))` logs the error at the
+//!    callsite and then returns the error with the same string. Caller has the
+//!    option of replacing `error!` with `debug_error!`.
 #[macro_export]
 macro_rules! Err {
 	($($args:tt)*) => {
@@ -7,36 +40,56 @@ macro_rules! Err {
 
 #[macro_export]
 macro_rules! err {
-	(error!($($args:tt),+)) => {{
-		$crate::error!($($args),+);
-		$crate::error::Error::Err(std::format!($($args),+))
+	(Config($item:literal, $($args:expr),*)) => {{
+		$crate::error!(config = %$item, $($args),*);
+		$crate::error::Error::Config($item, $crate::format_maybe!($($args),*))
 	}};
 
-	(debug_error!($($args:tt),+)) => {{
-		$crate::debug_error!($($args),+);
-		$crate::error::Error::Err(std::format!($($args),+))
+	(Request(Forbidden($level:ident!($($args:expr),*)))) => {{
+		$crate::$level!($($args),*);
+		$crate::error::Error::Request(
+			::ruma::api::client::error::ErrorKind::forbidden(),
+			$crate::format_maybe!($($args),*)
+		)
 	}};
 
-	($variant:ident(error!($($args:tt),+))) => {{
-		$crate::error!($($args),+);
-		$crate::error::Error::$variant(std::format!($($args),+))
-	}};
-
-	($variant:ident(debug_error!($($args:tt),+))) => {{
-		$crate::debug_error!($($args),+);
-		$crate::error::Error::$variant(std::format!($($args),+))
-	}};
-
-	(Config($item:literal, $($args:tt),+)) => {{
-		$crate::error!(config = %$item, $($args),+);
-		$crate::error::Error::Config($item, std::format!($($args),+))
-	}};
-
-	($variant:ident($($args:tt),+)) => {
-		$crate::error::Error::$variant(std::format!($($args),+))
+	(Request(Forbidden($($args:expr),*))) => {
+		$crate::error::Error::Request(
+			::ruma::api::client::error::ErrorKind::forbidden(),
+			$crate::format_maybe!($($args),*)
+		)
 	};
 
-	($string:literal$(,)? $($args:tt),*) => {
-		$crate::error::Error::Err(std::format!($string, $($args),*))
+	(Request($variant:ident($level:ident!($($args:expr),*)))) => {{
+		$crate::$level!($($args),*);
+		$crate::error::Error::Request(
+			::ruma::api::client::error::ErrorKind::$variant,
+			$crate::format_maybe!($($args),*)
+		)
+	}};
+
+	(Request($variant:ident($($args:expr),*))) => {
+		$crate::error::Error::Request(
+			::ruma::api::client::error::ErrorKind::$variant,
+			$crate::format_maybe!($($args),*)
+		)
+	};
+
+	($variant:ident($level:ident!($($args:expr),*))) => {{
+		$crate::$level!($($args),*);
+		$crate::error::Error::$variant($crate::format_maybe!($($args),*))
+	}};
+
+	($variant:ident($($args:expr),*)) => {
+		$crate::error::Error::$variant($crate::format_maybe!($($args),*))
+	};
+
+	($level:ident!($($args:expr),*)) => {{
+		$crate::$level!($($args),*);
+		$crate::error::Error::Err($crate::format_maybe!($($args),*))
+	}};
+
+	($($args:expr),*) => {
+		$crate::error::Error::Err($crate::format_maybe!($($args),*))
 	};
 }

src/core/error/mod.rs

@@ -3,7 +3,7 @@ mod log;
 mod panic;
 mod response;
 
-use std::{any::Any, convert::Infallible, fmt};
+use std::{any::Any, borrow::Cow, convert::Infallible, fmt};
 
 pub use log::*;
 
@@ -64,7 +64,9 @@ pub enum Error {
 	#[error("{0}")]
 	Mxid(#[from] ruma::IdParseError),
 	#[error("{0}: {1}")]
-	BadRequest(ruma::api::client::error::ErrorKind, &'static str),
+	BadRequest(ruma::api::client::error::ErrorKind, &'static str), //TODO: remove
+	#[error("{0}: {1}")]
+	Request(ruma::api::client::error::ErrorKind, Cow<'static, str>),
 	#[error("from {0}: {1}")]
 	Redaction(ruma::OwnedServerName, ruma::canonical_json::RedactionError),
 	#[error("Remote server {0} responded with: {1}")]
@@ -76,11 +78,9 @@ pub enum Error {
 	#[error("Arithmetic operation failed: {0}")]
 	Arithmetic(&'static str),
 	#[error("There was a problem with the '{0}' directive in your configuration: {1}")]
-	Config(&'static str, String),
+	Config(&'static str, Cow<'static, str>),
 	#[error("{0}")]
-	BadDatabase(&'static str),
-	#[error("{0}")]
-	Database(String),
+	Database(Cow<'static, str>),
 	#[error("{0}")]
 	BadServerResponse(&'static str),
 	#[error("{0}")]
@@ -88,14 +88,11 @@ pub enum Error {
 
 	// unique / untyped
 	#[error("{0}")]
-	Err(String),
+	Err(Cow<'static, str>),
 }
 
 impl Error {
-	pub fn bad_database(message: &'static str) -> Self {
-		error!("BadDatabase: {}", message);
-		Self::BadDatabase(message)
-	}
+	pub fn bad_database(message: &'static str) -> Self { crate::err!(Database(error!("{message}"))) }
 
 	/// Sanitizes public-facing errors that can leak sensitive information.
 	pub fn sanitized_string(&self) -> String {
@@ -121,7 +118,7 @@ impl Error {
 
 		match self {
 			Self::Federation(_, error) => response::ruma_error_kind(error).clone(),
-			Self::BadRequest(kind, _) => kind.clone(),
+			Self::BadRequest(kind, _) | Self::Request(kind, _) => kind.clone(),
 			_ => Unknown,
 		}
 	}
@@ -129,7 +126,7 @@ impl Error {
 	pub fn status_code(&self) -> http::StatusCode {
 		match self {
 			Self::Federation(_, ref error) | Self::RumaError(ref error) => error.status_code,
-			Self::BadRequest(ref kind, _) => response::bad_request_code(kind),
+			Self::BadRequest(ref kind, _) | Self::Request(ref kind, _) => response::bad_request_code(kind),
 			Self::Conflict(_) => http::StatusCode::CONFLICT,
 			_ => http::StatusCode::INTERNAL_SERVER_ERROR,
 		}