From e8d926d6891426d4f4a6d5eac56eb13c0c371f1f Mon Sep 17 00:00:00 2001 From: Bilal Mahmoud Date: Wed, 30 Nov 2022 19:12:10 +0100 Subject: [PATCH 1/7] chore: introduce new dependency `spin` and feature --- packages/libs/error-stack/Cargo.toml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/libs/error-stack/Cargo.toml b/packages/libs/error-stack/Cargo.toml index 233e7730f93..96c0e5cc269 100644 --- a/packages/libs/error-stack/Cargo.toml +++ b/packages/libs/error-stack/Cargo.toml @@ -22,6 +22,7 @@ anyhow = { version = "1.0.65", default-features = false, optional = true } eyre = { version = "0.6", default-features = false, optional = true } owo-colors = { version = "3", default-features = false, optional = true, features = ['supports-colors'] } serde = { version = "1", default-features = false, optional = true } +spin = { version = "0.9.4", default-features = false, optional = true, features = ['rwlock', 'once'] } [dev-dependencies] serde = { version = "1.0.148", features = ["derive"] } @@ -47,6 +48,7 @@ spantrace = ["dep:tracing-error", "std"] std = ["anyhow?/std"] eyre = ["dep:eyre", "std"] serde = ["dep:serde"] +hooks = ['dep:spin'] [package.metadata.docs.rs] all-features = true From d9296cd5646d82cc761248c61d41b28ebbc7b5d3 Mon Sep 17 00:00:00 2001 From: Bilal Mahmoud Date: Wed, 30 Nov 2022 19:12:44 +0100 Subject: [PATCH 2/7] feat: conditionally switch from `std` to `spin` --- packages/libs/error-stack/src/hook.rs | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/packages/libs/error-stack/src/hook.rs b/packages/libs/error-stack/src/hook.rs index ab66d874013..c5af82d1eef 100644 --- a/packages/libs/error-stack/src/hook.rs +++ b/packages/libs/error-stack/src/hook.rs @@ -1,10 +1,18 @@ -use std::sync::RwLock; +use alloc::vec::Vec; use crate::{ fmt::{install_builtin_hooks, HookContext, Hooks}, Report, }; +#[cfg(feature = "std")] +type RwLock = std::sync::RwLock; + +// Generally the std mutex is faster than spin, so if both `std` and `hooks` is enabled we use the +// std variant. +#[cfg(all(not(feature = "std"), feature = "hooks"))] +type RwLock = spin::rwlock::RwLock; + static FMT_HOOK: RwLock = RwLock::new(Hooks { inner: Vec::new() }); impl Report<()> { @@ -139,24 +147,36 @@ impl Report<()> { /// /// /// [`Error::provide`]: std::error::Error::provide - #[cfg(feature = "std")] + #[cfg(any(feature = "std", feature = "hooks"))] pub fn install_debug_hook( hook: impl Fn(&T, &mut HookContext) + Send + Sync + 'static, ) { install_builtin_hooks(); + #[cfg(feature = "std")] let mut lock = FMT_HOOK.write().expect("should not be poisoned"); + + // The spin RwLock cannot panic + #[cfg(all(not(feature = "std"), feature = "hooks"))] + let mut lock = FMT_HOOK.write(); + lock.insert(hook); } /// Returns the hook that was previously set by [`install_debug_hook`] /// /// [`install_debug_hook`]: Self::install_debug_hook - #[cfg(feature = "std")] + #[cfg(any(feature = "std", feature = "hooks"))] pub(crate) fn invoke_debug_format_hook(closure: impl FnOnce(&Hooks) -> T) -> T { install_builtin_hooks(); + #[cfg(feature = "std")] let hook = FMT_HOOK.read().expect("should not be poisoned"); + + // The spin RwLock cannot panic + #[cfg(all(not(feature = "std"), feature = "hooks"))] + let hook = FMT_HOOK.read(); + closure(&hook) } } From f4f7f890ef82c33aa7f62ab040f7209cc61e3213 Mon Sep 17 00:00:00 2001 From: Bilal Mahmoud Date: Wed, 30 Nov 2022 19:12:59 +0100 Subject: [PATCH 3/7] feat: convert `fmt` from `std` to `std | hooks` --- packages/libs/error-stack/src/fmt/hook.rs | 16 ++++++--- packages/libs/error-stack/src/fmt/mod.rs | 43 ++++++++++++----------- 2 files changed, 34 insertions(+), 25 deletions(-) diff --git a/packages/libs/error-stack/src/fmt/hook.rs b/packages/libs/error-stack/src/fmt/hook.rs index dc6bd529f1a..7f5df911056 100644 --- a/packages/libs/error-stack/src/fmt/hook.rs +++ b/packages/libs/error-stack/src/fmt/hook.rs @@ -720,14 +720,14 @@ fn into_boxed_hook( /// [`Display`]: core::fmt::Display /// [`Debug`]: core::fmt::Debug /// [`.insert()`]: Hooks::insert -#[cfg(feature = "std")] +#[cfg(any(feature = "std", feature = "hooks"))] pub(crate) struct Hooks { // We use `Vec`, instead of `HashMap` or `BTreeMap`, so that ordering is consistent with the // insertion order of types. pub(crate) inner: Vec<(TypeId, BoxedHook)>, } -#[cfg(feature = "std")] +#[cfg(any(feature = "std", feature = "hooks"))] impl Hooks { pub(crate) fn insert( &mut self, @@ -763,12 +763,15 @@ mod default { panic::Location, sync::atomic::{AtomicBool, Ordering}, }; - #[cfg(rust_1_65)] + #[cfg(all(rust_1_65, feature = "std"))] use std::backtrace::Backtrace; + #[cfg(feature = "std")] use std::sync::Once; #[cfg(feature = "pretty-print")] use owo_colors::{OwoColorize, Stream}; + #[cfg(all(not(feature = "std"), feature = "hooks"))] + use spin::once::Once; #[cfg(feature = "spantrace")] use tracing_error::SpanTrace; @@ -787,6 +790,9 @@ mod default { // // > If the given closure recursively invokes call_once on the same Once instance the exact // > behavior is not specified, allowed outcomes are a panic or a deadlock. + // + // This limitation is not present for the implementation from the spin crate, but for + // simplicity and readability the extra guard is kept. static INSTALL_BUILTIN_RUNNING: AtomicBool = AtomicBool::new(false); // This has minimal overhead, as `Once::call_once` calls `.is_completed` as the short path @@ -800,7 +806,7 @@ mod default { Report::install_debug_hook::(location); - #[cfg(rust_1_65)] + #[cfg(all(feature = "std", rust_1_65))] Report::install_debug_hook::(backtrace); #[cfg(feature = "spantrace")] @@ -819,7 +825,7 @@ mod default { context.push_body(format!("at {location}")); } - #[cfg(rust_1_65)] + #[cfg(all(feature = "std", rust_1_65))] fn backtrace(backtrace: &Backtrace, context: &mut HookContext) { let idx = context.increment_counter(); diff --git a/packages/libs/error-stack/src/fmt/mod.rs b/packages/libs/error-stack/src/fmt/mod.rs index f329dd78f2f..9413dd4eac0 100644 --- a/packages/libs/error-stack/src/fmt/mod.rs +++ b/packages/libs/error-stack/src/fmt/mod.rs @@ -142,7 +142,7 @@ //! [`atomic`]: std::sync::atomic //! [`Error::provide`]: core::error::Error::provide -#[cfg(feature = "std")] +#[cfg(any(feature = "std", feature = "hooks"))] mod hook; use alloc::{ @@ -159,9 +159,9 @@ use core::{ mem, }; -#[cfg(feature = "std")] +#[cfg(any(feature = "std", feature = "hooks"))] pub use hook::HookContext; -#[cfg(feature = "std")] +#[cfg(any(feature = "std", feature = "hooks"))] pub(crate) use hook::{install_builtin_hooks, Hooks}; #[cfg(feature = "pretty-print")] use owo_colors::{OwoColorize, Stream, Style as OwOStyle}; @@ -670,37 +670,37 @@ impl Opaque { fn debug_attachments_invoke<'a>( frames: impl IntoIterator, - #[cfg(feature = "std")] context: &mut HookContext, + #[cfg(any(feature = "std", feature = "hooks"))] context: &mut HookContext, ) -> (Opaque, Vec) { let mut opaque = Opaque::new(); let body = frames .into_iter() .map(|frame| match frame.kind() { - #[cfg(feature = "std")] + #[cfg(any(feature = "std", feature = "hooks"))] FrameKind::Context(_) => Some( Report::invoke_debug_format_hook(|hooks| hooks.call(frame, context)) .then(|| context.take_body()) .unwrap_or_default(), ), - #[cfg(feature = "std")] + #[cfg(any(feature = "std", feature = "hooks"))] FrameKind::Attachment(AttachmentKind::Printable(attachment)) => Some( Report::invoke_debug_format_hook(|hooks| hooks.call(frame, context)) .then(|| context.take_body()) .unwrap_or_else(|| vec![attachment.to_string()]), ), - #[cfg(feature = "std")] + #[cfg(any(feature = "std", feature = "hooks"))] FrameKind::Attachment(AttachmentKind::Opaque(_)) => { Report::invoke_debug_format_hook(|hooks| hooks.call(frame, context)) .then(|| context.take_body()) } - #[cfg(not(feature = "std"))] + #[cfg(not(any(feature = "std", feature = "hooks")))] FrameKind::Context(_) => Some(vec![]), - #[cfg(not(feature = "std"))] + #[cfg(not(any(feature = "std", feature = "hooks")))] FrameKind::Attachment(AttachmentKind::Printable(attachment)) => { Some(vec![attachment.to_string()]) } - #[cfg(all(not(feature = "std"), feature = "pretty-print"))] + #[cfg(all(not(any(feature = "std", feature = "hooks")), feature = "pretty-print"))] FrameKind::Attachment(AttachmentKind::Opaque(_)) => frame .downcast_ref::>() .map(|location| { @@ -710,7 +710,10 @@ fn debug_attachments_invoke<'a>( .to_string(), ] }), - #[cfg(all(not(feature = "std"), not(feature = "pretty-print")))] + #[cfg(all( + not(any(feature = "std", feature = "hooks")), + not(feature = "pretty-print") + ))] FrameKind::Attachment(AttachmentKind::Opaque(_)) => frame .downcast_ref::() .map(|location| vec![format!("at {location}")]), @@ -731,13 +734,13 @@ fn debug_attachments_invoke<'a>( fn debug_attachments<'a>( position: Position, frames: impl IntoIterator, - #[cfg(feature = "std")] context: &mut HookContext, + #[cfg(any(feature = "std", feature = "hooks"))] context: &mut HookContext, ) -> Lines { let last = matches!(position, Position::Final); let (opaque, entries) = debug_attachments_invoke( frames, - #[cfg(feature = "std")] + #[cfg(any(feature = "std", feature = "hooks"))] context, ); let opaque = opaque.render(); @@ -863,7 +866,7 @@ fn debug_render(head: Lines, contexts: VecDeque, sources: Vec) -> fn debug_frame( root: &Frame, prefix: &[&Frame], - #[cfg(feature = "std")] context: &mut HookContext, + #[cfg(any(feature = "std", feature = "hooks"))] context: &mut HookContext, ) -> Vec { let (stack, sources) = collect(root, prefix); let (stack, prefix) = partition(&stack); @@ -904,7 +907,7 @@ fn debug_frame( Position::Inner }, once(head).chain(body), - #[cfg(feature = "std")] + #[cfg(any(feature = "std", feature = "hooks"))] context, ); head_context.then(body) @@ -920,7 +923,7 @@ fn debug_frame( debug_frame( source, &prefix, - #[cfg(feature = "std")] + #[cfg(any(feature = "std", feature = "hooks"))] context, ) }, @@ -943,10 +946,10 @@ fn debug_frame( impl Debug for Report { fn fmt(&self, fmt: &mut Formatter<'_>) -> fmt::Result { - #[cfg(feature = "std")] + #[cfg(any(feature = "std", feature = "hooks"))] let mut context = HookContext::new(fmt.alternate()); - #[cfg_attr(not(feature = "std"), allow(unused_mut))] + #[cfg_attr(not(any(feature = "std", feature = "hooks")), allow(unused_mut))] let mut lines = self .current_frames() .iter() @@ -954,7 +957,7 @@ impl Debug for Report { debug_frame( frame, &[], - #[cfg(feature = "std")] + #[cfg(any(feature = "std", feature = "hooks"))] &mut context, ) }) @@ -975,7 +978,7 @@ impl Debug for Report { .collect::>() .join("\n"); - #[cfg(feature = "std")] + #[cfg(any(feature = "std", feature = "hooks"))] { let appendix = context .appendix() From f1dbe8ddd9854f8abc2c5ac6f90c856d4c876aa1 Mon Sep 17 00:00:00 2001 From: Bilal Mahmoud Date: Wed, 30 Nov 2022 19:13:20 +0100 Subject: [PATCH 4/7] feat: expose/enable modules for `std | hooks` --- packages/libs/error-stack/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/libs/error-stack/src/lib.rs b/packages/libs/error-stack/src/lib.rs index c1571f505ef..680db04e09c 100644 --- a/packages/libs/error-stack/src/lib.rs +++ b/packages/libs/error-stack/src/lib.rs @@ -475,11 +475,11 @@ mod report; mod result; mod context; -#[cfg(feature = "std")] +#[cfg(any(feature = "std", feature = "hooks"))] pub mod fmt; -#[cfg(not(feature = "std"))] +#[cfg(not(any(feature = "std", feature = "hooks")))] mod fmt; -#[cfg(feature = "std")] +#[cfg(any(feature = "std", feature = "hooks"))] mod hook; #[cfg(feature = "serde")] mod serde; From d609831af52bdc1e39a2f2d1db1dce740d14fba7 Mon Sep 17 00:00:00 2001 From: Bilal Mahmoud Date: Wed, 30 Nov 2022 19:29:56 +0100 Subject: [PATCH 5/7] feat: docs + changelog --- packages/libs/error-stack/CHANGELOG.md | 1 + packages/libs/error-stack/src/lib.rs | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/libs/error-stack/CHANGELOG.md b/packages/libs/error-stack/CHANGELOG.md index 66fa0af7e44..f6a117dc49e 100644 --- a/packages/libs/error-stack/CHANGELOG.md +++ b/packages/libs/error-stack/CHANGELOG.md @@ -15,6 +15,7 @@ All notable changes to `error-stack` will be documented in this file. ### Features - Add serializing support using [`serde`](https://serde.rs) ([#1290](https://github.com/hashintel/hash/pull/1290)) +- Support `Debug` hooks on `no-std` platforms via the `hooks` feature ([#1556](https://github.com/hashintel/hash/pull/1556)) ## [0.2.4](https://github.com/hashintel/hash/tree/error-stack%400.2.4/packages/libs/error-stack) - 2022-11-04 diff --git a/packages/libs/error-stack/src/lib.rs b/packages/libs/error-stack/src/lib.rs index 680db04e09c..9292c737e72 100644 --- a/packages/libs/error-stack/src/lib.rs +++ b/packages/libs/error-stack/src/lib.rs @@ -415,9 +415,10 @@ //! //! Feature | Description | default //! ---------------|--------------------------------------------------------------------|---------- -//! `std` | Enables support for [`Error`] and, on nightly, [`Backtrace`] | enabled +//! `std` | Enables support for [`Error`], and, on Rust 1.65+, [`Backtrace`] | enabled //! `pretty-print` | Provide color[^color] and use of unicode in [`Debug`] output | enabled //! `spantrace` | Enables automatic capturing of [`SpanTrace`]s | disabled +//! `hooks` | Enables hooks on `no-std` platforms using spin locks | disabled //! `anyhow` | Provides `into_report` to convert [`anyhow::Error`] to [`Report`] | disabled //! `eyre` | Provides `into_report` to convert [`eyre::Report`] to [`Report`] | disabled //! From 8a872b9c6192fb2dd50e8ed8a67eac2d8dccfe35 Mon Sep 17 00:00:00 2001 From: Bilal Mahmoud Date: Wed, 30 Nov 2022 19:32:43 +0100 Subject: [PATCH 6/7] fix: import cfg --- packages/libs/error-stack/src/fmt/hook.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/libs/error-stack/src/fmt/hook.rs b/packages/libs/error-stack/src/fmt/hook.rs index 7f5df911056..31b98007b97 100644 --- a/packages/libs/error-stack/src/fmt/hook.rs +++ b/packages/libs/error-stack/src/fmt/hook.rs @@ -755,9 +755,7 @@ impl Hooks { mod default { #![allow(unused_imports)] - #[cfg(any(rust_1_65, feature = "spantrace"))] - use alloc::format; - use alloc::{vec, vec::Vec}; + use alloc::{format, vec, vec::Vec}; use core::{ any::TypeId, panic::Location, From 122a119a12021b17f17782443d0422b47826f166 Mon Sep 17 00:00:00 2001 From: Bilal Mahmoud Date: Wed, 30 Nov 2022 19:42:34 +0100 Subject: [PATCH 7/7] feat: implement suggestions from code review --- packages/libs/error-stack/Cargo.toml | 2 +- packages/libs/error-stack/Makefile.toml | 4 ++-- packages/libs/error-stack/tests/test_debug.rs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/libs/error-stack/Cargo.toml b/packages/libs/error-stack/Cargo.toml index 96c0e5cc269..69b59bc29cd 100644 --- a/packages/libs/error-stack/Cargo.toml +++ b/packages/libs/error-stack/Cargo.toml @@ -22,7 +22,7 @@ anyhow = { version = "1.0.65", default-features = false, optional = true } eyre = { version = "0.6", default-features = false, optional = true } owo-colors = { version = "3", default-features = false, optional = true, features = ['supports-colors'] } serde = { version = "1", default-features = false, optional = true } -spin = { version = "0.9.4", default-features = false, optional = true, features = ['rwlock', 'once'] } +spin = { version = "0.9", default-features = false, optional = true, features = ['rwlock', 'once'] } [dev-dependencies] serde = { version = "1.0.148", features = ["derive"] } diff --git a/packages/libs/error-stack/Makefile.toml b/packages/libs/error-stack/Makefile.toml index 144a2a47a7a..d04dc00fc79 100644 --- a/packages/libs/error-stack/Makefile.toml +++ b/packages/libs/error-stack/Makefile.toml @@ -6,9 +6,9 @@ CARGO_TEST_HACK_FLAGS = "--workspace --optional-deps --feature-powerset --exclud CARGO_INSTA_TEST_FLAGS = "--no-fail-fast" CARGO_RUSTDOC_HACK_FLAGS = "" CARGO_DOC_HACK_FLAGS = "" -# The only features that are of relevance are: spantrace, pretty-print, std +# The only features that are of relevance are: spantrace, pretty-print, std, hooks # all others do not change the output -CARGO_INSTA_TEST_HACK_FLAGS = "--keep-going --feature-powerset --include-features spantrace,pretty-print,std" +CARGO_INSTA_TEST_HACK_FLAGS = "--keep-going --feature-powerset --include-features spantrace,pretty-print,std,hooks" [tasks.test] diff --git a/packages/libs/error-stack/tests/test_debug.rs b/packages/libs/error-stack/tests/test_debug.rs index 5023390d206..eba48c864e5 100644 --- a/packages/libs/error-stack/tests/test_debug.rs +++ b/packages/libs/error-stack/tests/test_debug.rs @@ -238,7 +238,7 @@ fn sources_nested_alternate() { #[cfg(all( rust_1_65, - feature = "std", + any(feature = "std", feature = "hooks"), feature = "spantrace", feature = "pretty-print" ))]