Even nicer logging: Add ok_or_log() and more (#2284)

Co-authored-by: Floris Bruynooghe <flub@devork.be>

deltachat-ffi/src/lib.rs

@@ -24,7 +24,6 @@ use std::time::{Duration, SystemTime};
 use async_std::task::{block_on, spawn};
 use num_traits::{FromPrimitive, ToPrimitive};
 
-use deltachat::accounts::Accounts;
 use deltachat::chat::{ChatId, ChatVisibility, MuteDuration, ProtectionStatus};
 use deltachat::constants::DC_MSG_ID_LAST_SPECIAL;
 use deltachat::contact::{Contact, Origin};
@@ -34,6 +33,7 @@ use deltachat::key::DcKey;
 use deltachat::message::MsgId;
 use deltachat::stock_str::StockMessage;
 use deltachat::*;
+use deltachat::{accounts::Accounts, log::LogExt};
 
 mod dc_array;
 
@@ -3451,15 +3451,11 @@ pub unsafe extern "C" fn dc_str_unref(s: *mut libc::c_char) {
 }
 
 trait ResultExt<T, E> {
+    /// Like `log_err()`, but:
+    /// - returns the default value instead of an Err value.
+    /// - emits an error instead of a warning for an [Err] result. This means
+    /// that the error will be shown to the user in a small pop-up.
     fn unwrap_or_log_default(self, context: &context::Context, message: &str) -> T;
-
-    /// Log a warning to a [ContextWrapper] for an [Err] result.
-    ///
-    /// Does nothing for an [Ok].
-    ///
-    /// You can do this as soon as the wrapper exists, it does not
-    /// have to be open (which is required for the `warn!()` macro).
-    fn log_err(self, wrapper: &Context, message: &str) -> Result<T, E>;
 }
 
 impl<T: Default, E: std::fmt::Display> ResultExt<T, E> for Result<T, E> {
@@ -3472,14 +3468,6 @@ impl<T: Default, E: std::fmt::Display> ResultExt<T, E> for Result<T, E> {
             }
         }
     }
-
-    fn log_err(self, ctx: &Context, message: &str) -> Result<T, E> {
-        self.map_err(|err| {
-            // We are using Anyhow's .context() and to show the inner error, too, we need the {:#}:
-            warn!(ctx, "{}: {:#}", message, err);
-            err
-        })
-    }
 }
 
 trait ResultNullableExt<T> {

examples/repl/cmdline.rs

@@ -514,7 +514,7 @@ pub async fn cmdline(context: Context, line: &str, chat_id: &mut ChatId) -> Resu
             context.maybe_network().await;
         }
         "housekeeping" => {
-            sql::housekeeping(&context).await.log(&context);
+            sql::housekeeping(&context).await.ok_or_log(&context);
         }
         "listchats" | "listarchived" | "chats" => {
             let listflags = if arg0 == "listarchived" { 0x01 } else { 0 };

src/imex.rs

@@ -676,7 +676,7 @@ async fn export_backup(context: &Context, dir: impl AsRef<Path>) -> Result<()> {
         .sql
         .set_raw_config_int(context, "backup_time", now as i32)
         .await?;
-    sql::housekeeping(context).await.log(context);
+    sql::housekeeping(context).await.ok_or_log(context);
 
     context
         .sql

src/job.rs

@@ -1157,7 +1157,7 @@ async fn perform_job_action(
         Action::MoveMsg => job.move_msg(context, connection.inbox()).await,
         Action::FetchExistingMsgs => job.fetch_existing_msgs(context, connection.inbox()).await,
         Action::Housekeeping => {
-            sql::housekeeping(context).await.log(context);
+            sql::housekeeping(context).await.ok_or_log(context);
             Status::Finished(Ok(()))
         }
     };

src/log.rs

@@ -1,5 +1,4 @@
 //! # Logging
-
 use crate::context::Context;
 
 #[macro_export]
@@ -61,38 +60,94 @@ macro_rules! emit_event {
     };
 }
 
-pub trait LogExt<T> {
+pub trait LogExt<T, E>
+where
+    Self: std::marker::Sized,
+{
+    #[track_caller]
+    fn log_err_inner(self, context: &Context, msg: Option<&str>) -> Result<T, E>;
+
     /// Emits a warning if the receiver contains an Err value.
     ///
-    /// Returns an [`Option<T>`] with the `Ok(_)` value, if any:
-    /// - You won't get any warnings about unused results but can still use the value if you need it
-    /// - This prevents the same warning from being printed to the log multiple times
-    ///
     /// Thanks to the [track_caller](https://blog.rust-lang.org/2020/08/27/Rust-1.46.0.html#track_caller)
     /// feature, the location of the caller is printed to the log, just like with the warn!() macro.
+    ///
+    /// Unfortunately, the track_caller feature does not work on async functions (as of Rust 1.50).
+    /// Once it is, you can add `#[track_caller]` to helper functions that use one of the log helpers here
+    /// so that the location of the caller can be seen in the log. (this won't work with the macros,
+    /// like warn!(), since the file!() and line!() macros don't work with track_caller)  
+    /// See https://github.com/rust-lang/rust/issues/78840 for progress on this.
     #[track_caller]
-    fn log(self, context: &Context) -> Option<T>;
+    fn log_err(self, context: &Context, msg: &str) -> Result<T, E> {
-}
+        self.log_err_inner(context, Some(msg))
+    }
 
-impl<T> LogExt<T> for anyhow::Result<T> {
+    /// Emits a warning if the receiver contains an Err value and returns an [`Option<T>`].
+    ///
+    /// Example:
+    /// ```text
+    /// if let Err(e) = do_something() {
+    ///     warn!(context, "{:#}", e);
+    /// }
+    /// ```
+    /// is equivalent to:
+    /// ```text
+    /// do_something().ok_or_log(context);
+    /// ```
+    ///
+    /// For a note on the `track_caller` feature, see the doc comment on `log_err()`.
     #[track_caller]
-    fn log(self, context: &Context) -> Option<T> {
+    fn ok_or_log(self, context: &Context) -> Option<T> {
-        match self {
+        self.log_err_inner(context, None).ok()
-            Err(e) => {
+    }
-                let location = std::panic::Location::caller();
+
-                // We are using Anyhow's .context() and to show the inner error, too, we need the {:#}:
+    /// Like `ok_or_log()`, but you can pass an extra message that is prepended in the log.
-                let full = format!(
+    ///
-                    "{file}:{line}: {e:#}",
+    /// Example:
-                    file = location.file(),
+    /// ```text
-                    line = location.line(),
+    /// if let Err(e) = do_something() {
-                    e = e
+    ///     warn!(context, "Something went wrong: {:#}", e);
-                );
+    /// }
-                // We can't use the warn!() macro here as the file!() and line!() macros
+    /// ```
-                // don't work well with #[track_caller]
+    /// is equivalent to:
-                emit_event!(context, crate::EventType::Warning(full));
+    /// ```text
-                None
+    /// do_something().ok_or_log_msg(context, "Something went wrong");
-            }
+    /// ```
-            Ok(v) => Some(v),
+    /// and is also equivalent to:
-        }
+    /// ```text
+    /// use anyhow::Context as _;
+    /// do_something().context("Something went wrong").ok_or_log(context);
+    /// ```
+    ///
+    /// For a note on the `track_caller` feature, see the doc comment on `log_err()`.
+    #[track_caller]
+    fn ok_or_log_msg(self, context: &Context, msg: &'static str) -> Option<T> {
+        self.log_err_inner(context, Some(msg)).ok()
+    }
+}
+
+impl<T: Default, E: std::fmt::Display> LogExt<T, E> for Result<T, E> {
+    #[track_caller]
+    fn log_err_inner(self, context: &Context, msg: Option<&str>) -> Result<T, E> {
+        if let Err(e) = &self {
+            let location = std::panic::Location::caller();
+
+            let separator = if msg.is_none() { "" } else { ": " };
+            let msg = msg.unwrap_or_default();
+
+            // We are using Anyhow's .context() and to show the inner error, too, we need the {:#}:
+            let full = format!(
+                "{file}:{line}: {msg}{separator}{e:#}",
+                file = location.file(),
+                line = location.line(),
+                msg = msg,
+                separator = separator,
+                e = e
+            );
+            // We can't use the warn!() macro here as the file!() and line!() macros
+            // don't work with #[track_caller]
+            emit_event!(context, crate::EventType::Warning(full));
+        };
+        self
     }
 }