From f0f4138499f1ecc0dd6f1d16cafe00f00541dd33 Mon Sep 17 00:00:00 2001 From: "Charles \"Demurgos\" Samborski" Date: Thu, 14 Dec 2023 11:31:04 +0000 Subject: [PATCH 1/6] Support `ErrorRef` in `#` error formatter This commit adds support for error references for the `#` error formatter from the `kv` macro. If the passed error is an owned error value it uses `ErrorValue`; otherwise if it can be dereferenced into an error then it uses `ErrorRef`. The way to detect if the value implements `Error` directly or if it's an error reference is based on auto-deref coercion as described in the following articles: - - I believe it to be fully backwards compatible with existing uses of `#`. This provides a short-hand for the `ErrorRef` wrapper in most common cases. However, it does not support temporary lifetime extension. This means that the following example needs to introduce a temporary binding to use the shorthand syntax: ```rust // explicit ref with lifetime extension info!(logger, "not found"; "error" => ErrorRef(&std::io::Error::from(std::io::ErrorKind::NotFound))); // `#` shorthand with temporary binding let error = std::io::Error::from(std::io::ErrorKind::NotFound); info!(logger, "not found"; "error" => #&error); ``` In practice, this use case is not an issue. Such a temporary can instead be passed by value (the reference is useless) through `ErrorValue`. There are [discussions in Zulip](https://rust-lang.zulipchat.com/#narrow/stream/403629-t-lang.2Ftemporary-lifetimes-2024) to support explicit lifetime extensions and support this use case in the future. --- CHANGELOG.md | 1 + src/lib.rs | 122 ++++++++++++++++++++++++++++++++++++++++++++++++--- src/tests.rs | 47 +++++++++++++++----- 3 files changed, 153 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0bfd60d9..45c1ceb6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] * Add `ErrorRef` wrapper to enable logging error references. + * The `#` error formatter in macros was updated to automatically select `ErrorValue` or `ErrorRef`. ### 2.8.0-beta.1 - 2023-09-09 diff --git a/src/lib.rs b/src/lib.rs index bc7c0569..ee79ec3e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -405,10 +405,10 @@ macro_rules! kv( kv!(@ ($crate::SingleKV::from(($k, __slog_builtin!(@format_args "{:#?}", $v))), $args_ready); $($args)* ) }; (@ $args_ready:expr; $k:expr => #$v:expr) => { - kv!(@ ($crate::SingleKV::from(($k, $crate::ErrorValue($v))), $args_ready); ) + kv!(@ ($crate::SingleKV::from(($k, { use $crate::{ErrorRefKind, ErrorValueKind}; let v = $v; let w = $crate::ErrorTagWrapper(v); (&&w).slog_error_kind().wrap(w.0) } )), $args_ready); ) }; (@ $args_ready:expr; $k:expr => #$v:expr, $($args:tt)* ) => { - kv!(@ ($crate::SingleKV::from(($k, $crate::ErrorValue($v))), $args_ready); $($args)* ) + kv!(@ ($crate::SingleKV::from(($k, { use $crate::{ErrorRefKind, ErrorValueKind}; let v = $v; let w = $crate::ErrorTagWrapper(v); (&&w).slog_error_kind().wrap(w.0) } )), $args_ready); $($args)* ) }; (@ $args_ready:expr; $k:expr => $v:expr) => { kv!(@ ($crate::SingleKV::from(($k, $v)), $args_ready); ) @@ -462,10 +462,10 @@ macro_rules! slog_kv( slog_kv!(@ ($crate::SingleKV::from(($k, __slog_builtin!(@format_args "{:#?}", $v))), $args_ready); $($args)* ) }; (@ $args_ready:expr; $k:expr => #$v:expr) => { - slog_kv!(@ ($crate::SingleKV::from(($k, $crate::ErrorValue($v))), $args_ready); ) + slog_kv!(@ ($crate::SingleKV::from(($k, { use $crate::{ErrorRefKind, ErrorValueKind}; let v = $v; let w = $crate::ErrorTagWrapper(v); (&&w).slog_error_kind().wrap(w.0) } )), $args_ready); ) }; (@ $args_ready:expr; $k:expr => #$v:expr, $($args:tt)* ) => { - slog_kv!(@ ($crate::SingleKV::from(($k, $crate::ErrorValue($v))), $args_ready); $($args)* ) + slog_kv!(@ ($crate::SingleKV::from(($k, { use $crate::{ErrorRefKind, ErrorValueKind}; let v = $v; let w = $crate::ErrorTagWrapper(v); (&&w).slog_error_kind().wrap(w.0) } )), $args_ready); $($args)* ) }; (@ $args_ready:expr; $k:expr => $v:expr) => { slog_kv!(@ ($crate::SingleKV::from(($k, $v)), $args_ready); ) @@ -730,7 +730,8 @@ macro_rules! slog_record( /// Similarly to use `std::fmt::Debug` value can be prefixed with `?`, /// or pretty-printed with `#?`. /// -/// Errors can be prefixed with `#` as a shorthand for wrapping with `ErrorValue`. +/// Errors can be prefixed with `#` as a shorthand. Error values will be wrapped +/// with [`ErrorValue`]. Error references will be wrapped in [`ErrorRef`]. /// /// Full list of supported formats can always be inspected by looking at /// [`kv` macro](macro.log.html) @@ -3349,6 +3350,115 @@ where } } +/// Wrapper for auto-deref specialization for error values and references. +/// +/// See +/// and for +/// details about the technique. +/// +/// This type allows to detect if it received an error reference or value. +/// To test the kind of `e`, you have to call it as `(&&ErrorTagWrapper(e)).slog_error_kind()`. +/// It will return [`ErrorValueTag`] for values and [`ErrorRefTag`] for references. +/// +/// This is an internal implementation detail of the `kv!` macro and should not +/// be used directly. +#[cfg(feature = "std")] +#[doc(hidden)] +pub struct ErrorTagWrapper(E); + +#[cfg(feature = "std")] +#[test] +fn test_error_tag_wrapper() { + #[derive(Debug, Clone, Copy)] + struct MyError(&'static str); + impl core::fmt::Display for MyError { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + f.write_str(self.0) + } + } + impl std::error::Error for MyError {} + let e = MyError("everything is on fire"); + assert_eq!((&&ErrorTagWrapper(e)).slog_error_kind(), ErrorValueTag); + let e = &e; + assert_eq!((&&ErrorTagWrapper(e)).slog_error_kind(), ErrorRefTag); +} + +/// Unit struct indicating that the content of `ErrorTagWrapper` was a value. +/// +/// This is an internal implementation detail of the `kv!` macro and should not +/// be used directly. +#[cfg(feature = "std")] +#[doc(hidden)] +#[derive(Debug, PartialEq, Eq)] +pub struct ErrorValueTag; +#[cfg(feature = "std")] +impl ErrorValueTag { + /// Create a [`Value`] wrapper for an owned error value. + pub fn wrap(self, e: E) -> ErrorValue + where + E: std::error::Error, + { + ErrorValue(e) + } +} + +/// Auxiliary trait for auto-deref dispatch of owned error values +/// +/// This is an internal implementation detail of the `kv!` macro and should not +/// be used directly. +#[cfg(feature = "std")] +#[doc(hidden)] +pub trait ErrorValueKind { + #[inline] + fn slog_error_kind(&self) -> ErrorValueTag { + ErrorValueTag + } +} +#[cfg(feature = "std")] +impl ErrorValueKind for ErrorTagWrapper {} + +/// Unit struct indicating that the content of `ErrorTagWrapper` was a reference. +/// +/// This is an internal implementation detail of the `kv!` macro and should not +/// be used directly. +#[cfg(feature = "std")] +#[doc(hidden)] +#[derive(Debug, PartialEq, Eq)] +pub struct ErrorRefTag; +#[cfg(feature = "std")] +impl ErrorRefTag { + /// Create a [`Value`] wrapper for an error reference. + pub fn wrap<'a, E>(self, e: &'a E) -> ErrorRef<'a, E> + where + E: ?Sized + 'static + std::error::Error, + { + ErrorRef(e) + } +} + +/// Auxiliary trait for auto-deref dispatch of error references +/// +/// This is an internal implementation detail of the `kv!` macro and should not +/// be used directly. +#[cfg(feature = "std")] +#[doc(hidden)] +pub trait ErrorRefKind { + #[inline] + fn slog_error_kind(self) -> ErrorRefTag + where + Self: Sized, + { + ErrorRefTag + } +} +#[cfg(feature = "std")] +impl ErrorRefKind for &&ErrorTagWrapper +where + ERef: core::ops::Deref, + ERef::Target: std::error::Error, +{ +} + /// A wrapper struct for serializing errors /// /// This struct can be used to wrap types that don't implement `slog::Value` but @@ -3388,7 +3498,7 @@ where /// /// Use [`ErrorValue`] if you want to move ownership of the error value. #[cfg(feature = "std")] -pub struct ErrorRef<'a, E: std::error::Error>(pub &'a E); +pub struct ErrorRef<'a, E: ?Sized + std::error::Error>(pub &'a E); #[cfg(feature = "std")] impl<'a, E> Value for ErrorRef<'a, E> diff --git a/src/tests.rs b/src/tests.rs index ff6a66b1..03f265ff 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -132,6 +132,30 @@ mod std_only { info!(log, "(d2, d1, c, b2, b1, a)"); } + #[test] + fn error_ref() { + let error = TestError::new("foo"); + let error = &error; + let logger = Logger::root(CheckError, o!()); + info!(logger, "foo"; "error" => #error); + } + + #[test] + fn error_box_ref() { + let error = TestError::new("foo"); + let error = Box::new(error); + let logger = Logger::root(CheckError, o!()); + info!(logger, "foo"; "error" => #&error); + } + + #[test] + fn error_arc_ref() { + let error = TestError::new("foo"); + let error = Arc::new(error); + let logger = Logger::root(CheckError, o!()); + info!(logger, "foo"; "error" => #&error); + } + #[test] fn error_fmt_no_source() { let logger = @@ -145,8 +169,8 @@ mod std_only { let error = TestError::new("foo"); let error = &error; let logger = Logger::root(CheckError, o!()); - info!(logger, "foo"; "error" => ErrorRef(error)); - slog_info!(logger, "foo"; "error" => ErrorRef(error)); + info!(logger, "foo"; "error" => #error); + slog_info!(logger, "foo"; "error" => #error); } #[test] @@ -164,8 +188,8 @@ mod std_only { let error = TestError::new("foo"); let error = &error; let logger = Logger::root(CheckError, o!()); - info!(logger, "not-error: not-error; foo"; "error" => ErrorRef(error), "not-error" => "not-error"); - slog_info!(logger, "not-error: not-error; foo"; "error" => ErrorRef(error), "not-error" => "not-error"); + info!(logger, "not-error: not-error; foo"; "error" => #error, "not-error" => "not-error"); + slog_info!(logger, "not-error: not-error; foo"; "error" => #error, "not-error" => "not-error"); } #[test] @@ -183,8 +207,8 @@ mod std_only { let error = TestError::new("foo"); let error = &error; let logger = Logger::root(CheckError, o!()); - info!(logger, "foonot-error: not-error; "; "not-error" => "not-error", "error" => ErrorRef(error)); - slog_info!(logger, "foonot-error: not-error; "; "not-error" => "not-error", "error" => ErrorRef(error)); + info!(logger, "foonot-error: not-error; "; "not-error" => "not-error", "error" => #error); + slog_info!(logger, "foonot-error: not-error; "; "not-error" => "not-error", "error" => #error); } #[test] @@ -202,8 +226,8 @@ mod std_only { let error = TestError("foo", Some(TestError::new("bar"))); let error = &error; let logger = Logger::root(CheckError, o!()); - info!(logger, "foo: bar"; "error" => ErrorRef(error)); - slog_info!(logger, "foo: bar"; "error" => ErrorRef(error)); + info!(logger, "foo: bar"; "error" => #error); + slog_info!(logger, "foo: bar"; "error" => #error); } #[test] @@ -224,8 +248,8 @@ mod std_only { ); let error = &error; let logger = Logger::root(CheckError, o!()); - info!(logger, "foo: bar: baz"; "error" => ErrorRef(error)); - slog_info!(logger, "foo: bar: baz"; "error" => ErrorRef(error)); + info!(logger, "foo: bar: baz"; "error" => #error); + slog_info!(logger, "foo: bar: baz"; "error" => #error); } #[test] @@ -234,7 +258,8 @@ mod std_only { info!(logger, "not found"; "error" => std::io::Error::from(std::io::ErrorKind::NotFound)); // compiles? info!(logger, "not found"; "error" => #std::io::Error::from(std::io::ErrorKind::NotFound)); - info!(logger, "not found"; "error" => ErrorRef(&std::io::Error::from(std::io::ErrorKind::NotFound))); + let error = std::io::Error::from(std::io::ErrorKind::NotFound); + info!(logger, "not found"; "error" => #&error); } } From a9b50a6cecd95542cba04b147a5a1ed65652a719 Mon Sep 17 00:00:00 2001 From: Techcable Date: Tue, 2 Jan 2024 20:47:40 -0700 Subject: [PATCH 2/6] Move ErrorRef wrapping logic to __slog_builtin! Avoids duplicating multiple times across multiple macros. Explicitly suppress unused_import warning (needed with clippy deny warnings) --- src/lib.rs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index ee79ec3e..60830bb4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -405,10 +405,10 @@ macro_rules! kv( kv!(@ ($crate::SingleKV::from(($k, __slog_builtin!(@format_args "{:#?}", $v))), $args_ready); $($args)* ) }; (@ $args_ready:expr; $k:expr => #$v:expr) => { - kv!(@ ($crate::SingleKV::from(($k, { use $crate::{ErrorRefKind, ErrorValueKind}; let v = $v; let w = $crate::ErrorTagWrapper(v); (&&w).slog_error_kind().wrap(w.0) } )), $args_ready); ) + kv!(@ ($crate::SingleKV::from(($k, __slog_builtin!(@wrap_error $v) )), $args_ready); ) }; (@ $args_ready:expr; $k:expr => #$v:expr, $($args:tt)* ) => { - kv!(@ ($crate::SingleKV::from(($k, { use $crate::{ErrorRefKind, ErrorValueKind}; let v = $v; let w = $crate::ErrorTagWrapper(v); (&&w).slog_error_kind().wrap(w.0) } )), $args_ready); $($args)* ) + kv!(@ ($crate::SingleKV::from(($k, __slog_builtin!(@wrap_error $v))), $args_ready); $($args)* ) }; (@ $args_ready:expr; $k:expr => $v:expr) => { kv!(@ ($crate::SingleKV::from(($k, $v)), $args_ready); ) @@ -462,10 +462,10 @@ macro_rules! slog_kv( slog_kv!(@ ($crate::SingleKV::from(($k, __slog_builtin!(@format_args "{:#?}", $v))), $args_ready); $($args)* ) }; (@ $args_ready:expr; $k:expr => #$v:expr) => { - slog_kv!(@ ($crate::SingleKV::from(($k, { use $crate::{ErrorRefKind, ErrorValueKind}; let v = $v; let w = $crate::ErrorTagWrapper(v); (&&w).slog_error_kind().wrap(w.0) } )), $args_ready); ) + slog_kv!(@ ($crate::SingleKV::from(($k, __slog_builtin!(@wrap_error $v) )), $args_ready); ) }; (@ $args_ready:expr; $k:expr => #$v:expr, $($args:tt)* ) => { - slog_kv!(@ ($crate::SingleKV::from(($k, { use $crate::{ErrorRefKind, ErrorValueKind}; let v = $v; let w = $crate::ErrorTagWrapper(v); (&&w).slog_error_kind().wrap(w.0) } )), $args_ready); $($args)* ) + slog_kv!(@ ($crate::SingleKV::from(($k, __slog_builtin!(@wrap_error $v) )), $args_ready); $($args)* ) }; (@ $args_ready:expr; $k:expr => $v:expr) => { slog_kv!(@ ($crate::SingleKV::from(($k, $v)), $args_ready); ) @@ -1068,6 +1068,16 @@ macro_rules! __slog_builtin { (@line) => ( line!() ); (@column) => ( column!() ); (@module_path) => ( module_path!() ); + (@wrap_error $v:expr) => ({ + // this magical sequence of code is used to wrap either with + // slog::ErrorValue or slog::ErrorRef as appropriate + // See PR #328 for details + #[allow(unused_imports)] + use $crate::{ErrorRefKind, ErrorValueKind}; + let v = $v; + let w = $crate::ErrorTagWrapper(v); + (&&w).slog_error_kind().wrap(w.0) + }); } // }}} From 12a3a95f84b9d677a26d1f916cadd8ef5cbc5201 Mon Sep 17 00:00:00 2001 From: Techcable Date: Tue, 2 Jan 2024 20:56:17 -0700 Subject: [PATCH 3/6] Elidle lifetimes in ErrorRefTag::wrap Clippy gives a warning here --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index 60830bb4..a58f9cfd 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -3438,7 +3438,7 @@ pub struct ErrorRefTag; #[cfg(feature = "std")] impl ErrorRefTag { /// Create a [`Value`] wrapper for an error reference. - pub fn wrap<'a, E>(self, e: &'a E) -> ErrorRef<'a, E> + pub fn wrap(self, e: &E) -> ErrorRef<'_, E> where E: ?Sized + 'static + std::error::Error, { From cc5e8e83d9c598f320b83dfa3b3082811d4279e0 Mon Sep 17 00:00:00 2001 From: Techcable Date: Tue, 2 Jan 2024 20:57:19 -0700 Subject: [PATCH 4/6] clippy: Suppress borrow warning in ErrorRef test The weird borrowing magic is part of the point (PR #328) --- src/lib.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index a58f9cfd..e5fd15b2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -3388,7 +3388,14 @@ fn test_error_tag_wrapper() { } impl std::error::Error for MyError {} let e = MyError("everything is on fire"); - assert_eq!((&&ErrorTagWrapper(e)).slog_error_kind(), ErrorValueTag); + assert_eq!( + { + #[allow(clippy::needless_borrow)] + // The "needless" borrow is part of the point + (&&ErrorTagWrapper(e)).slog_error_kind() + }, + ErrorValueTag + ); let e = &e; assert_eq!((&&ErrorTagWrapper(e)).slog_error_kind(), ErrorRefTag); } From 467bfdace05a38bdfd6dd758949d59a6b0c71efe Mon Sep 17 00:00:00 2001 From: Techcable Date: Wed, 3 Jan 2024 02:00:45 -0700 Subject: [PATCH 5/6] Test error_arc_ref requires Rust 1.52 Requires `impl Error for Arc`, which only exists on recent versions. Uses rustversion to conditionally disable. --- Cargo.toml | 9 +++++---- src/tests.rs | 2 ++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 70fa1b95..60d853f1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -69,14 +69,15 @@ release_max_level_trace = [] erased-serde = { version = "0.3", optional = true } serde = { version = "1", optional = true } serde_derive = { version = "1", optional = true } + +[dev-dependencies] # NOTE: This is just a macro (not a runtime dependency) # # It is used to conditionally enable use of newer rust language # features depending on the compiler features. -# Currently, this is not needed because our MSRV is sufficient -# for all the features we use. -# However, in the future we may need to re-add it. -# rustversion = "1" +# +# For the time being, this is only needed for tests. +rustversion = "1" [[example]] name = "singlethread" diff --git a/src/tests.rs b/src/tests.rs index 03f265ff..dce4456e 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -148,6 +148,8 @@ mod std_only { info!(logger, "foo"; "error" => #&error); } + // requires `impl Error for Arc` - since 1.52 + #[rustversion::since(1.52)] #[test] fn error_arc_ref() { let error = TestError::new("foo"); From 1a70db7c0ec5640d0b4a619014a77a142ac0c2c0 Mon Sep 17 00:00:00 2001 From: Techcable Date: Wed, 3 Jan 2024 02:13:22 -0700 Subject: [PATCH 6/6] changelog: Link ErrorRef PR changes to @demurgos --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 45c1ceb6..a98f6589 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,8 +6,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] -* Add `ErrorRef` wrapper to enable logging error references. - * The `#` error formatter in macros was updated to automatically select `ErrorValue` or `ErrorRef`. +* Add `ErrorRef` wrapper to enable logging error references (PR #327) + * The `#` error formatter in macros was updated to automatically select `ErrorValue` or `ErrorRef` (PR #328) ### 2.8.0-beta.1 - 2023-09-09