error-stack: deprecate Frame::location()

packages/libs/error-stack/CHANGELOG.md

@@ -11,6 +11,7 @@ All notable changes to `error-stack` will be documented in this file.
 
 - The output of [`Location`](https://doc.rust-lang.org/std/panic/struct.Location.html) is no longer hard-coded and can now be adjusted through hooks. ([#1237](https://github.com/hashintel/hash/pull/1237))
 - The `TypeId` of a value contained in a `Frame` can now be accessed via `Frame::type_id` ([#1289](https://github.com/hashintel/hash/pull/1289))
+- Deprecate `Frame::location` in favor of an additional attachment on context change/creation ([#1311](https://github.com/hashintel/hash/pull/1311))
 
 ## [0.2.3](https://github.com/hashintel/hash/tree/error-stack%400.2.3/packages/libs/error-stack) - 2022-10-12
 

packages/libs/error-stack/src/fmt/hook.rs

@@ -11,13 +11,12 @@ use alloc::{boxed::Box, collections::BTreeMap, string::String, vec::Vec};
 use core::{
     any::{Any, TypeId},
     marker::PhantomData,
+    mem,
 };
-use std::mem;
 
-#[cfg(feature = "std")]
 pub(crate) use default::install_builtin_hooks;
 
-use crate::{fmt::Frame, FrameKind};
+use crate::fmt::Frame;
 
 type Storage = BTreeMap<TypeId, BTreeMap<TypeId, Box<dyn Any>>>;
 
@@ -671,7 +670,7 @@ impl<T: 'static> HookContext<T> {
     }
 }
 
-type BoxedHook = Box<dyn Fn(&Frame, &mut HookContext<Frame>) -> Option<()> + Send + Sync>;
+type BoxedHook = Box<dyn Fn(&Frame, &mut HookContext<Frame>) -> bool + Send + Sync>;
 
 fn into_boxed_hook<T: Send + Sync + 'static>(
     hook: impl Fn(&T, &mut HookContext<T>) + Send + Sync + 'static,
@@ -687,6 +686,7 @@ fn into_boxed_hook<T: Send + Sync + 'static>(
                         .request_value::<T>()
                         .map(|ref value| hook(value, context.cast()))
                 })
+                .is_some()
         }
 
         // emulate the behavior from nightly by searching for
@@ -697,6 +697,7 @@ fn into_boxed_hook<T: Send + Sync + 'static>(
             .then_some(frame)
             .and_then(Frame::downcast_ref::<T>)
             .map(|value| hook(value, context.cast()))
+            .is_some()
     })
 }
 
@@ -741,33 +742,14 @@ impl Hooks {
         self.inner.push((type_id, into_boxed_hook(hook)));
     }
 
-    /// This compatability function is needed, as we need to special-case location a bit.
-    ///
-    /// [`Location`] is special, as it is present on every single frame, but is not present in the
-    /// chain of sources.
-    ///
-    /// This invokes all hooks on the [`Location`] object by creating a fake frame, which is used to
-    /// invoke any hooks related to location.
-    ///
-    /// [`Location`]: core::panic::Location
-    fn call_location(&self, frame: &Frame, context: &mut HookContext<Frame>) {
-        if !matches!(frame.kind(), FrameKind::Context(_)) {
-            return;
-        }
-
-        // note: this will issue recursion, but this won't ever cause infinite recursion, as
-        // this code is conditional on it being a context.
-        let fake = Frame::from_attachment(*frame.location(), frame.location(), Box::new([]));
-
-        self.call(&fake, context);
-    }
-
-    pub(crate) fn call(&self, frame: &Frame, context: &mut HookContext<Frame>) {
-        self.call_location(frame, context);
+    pub(crate) fn call(&self, frame: &Frame, context: &mut HookContext<Frame>) -> bool {
+        let mut hit = false;
 
         for (_, hook) in &self.inner {
-            hook(frame, context);
+            hit = hook(frame, context) || hit;
         }
+
+        hit
     }
 }
 
@@ -819,17 +801,18 @@ mod default {
         INSTALL_BUILTIN.call_once(|| {
             INSTALL_BUILTIN_RUNNING.store(true, Ordering::Release);
 
-            Report::install_debug_hook(location);
+            Report::install_debug_hook::<Location>(location);
 
             #[cfg(rust_1_65)]
-            Report::install_debug_hook(backtrace);
+            Report::install_debug_hook::<Backtrace>(backtrace);
 
             #[cfg(feature = "spantrace")]
-            Report::install_debug_hook(span_trace);
+            Report::install_debug_hook::<SpanTrace>(span_trace);
         });
     }
 
     fn location(location: &Location<'static>, context: &mut HookContext<Location<'static>>) {
+        println!("location");
         #[cfg(feature = "pretty-print")]
         context.push_body(format!(
             "{}",

packages/libs/error-stack/src/fmt/mod.rs

@@ -1,6 +1,8 @@
 //! Implementation of formatting, to enable colors and the use of box-drawing characters use the
 //! `pretty-print` feature.
 //!
+//! > **Note:** `error-stack` does not provide any stability guarantees for the [`Debug`] output.
+//!
 //! # Hooks
 //!
 //! The [`Debug`] implementation can be easily extended using hooks. Hooks are functions of the
@@ -152,8 +154,7 @@ use alloc::{
     vec::Vec,
 };
 use core::{
-    fmt,
-    fmt::{Debug, Display, Formatter},
+    fmt::{self, Debug, Display, Formatter},
     iter::once,
     mem,
 };
@@ -667,8 +668,8 @@ impl Opaque {
     }
 }
 
-fn debug_attachments_invoke(
-    frames: Vec<&Frame>,
+fn debug_attachments_invoke<'a>(
+    frames: impl IntoIterator<Item = &'a Frame>,
     #[cfg(feature = "std")] context: &mut HookContext<Frame>,
 ) -> (Opaque, Vec<String>) {
     let mut opaque = Opaque::new();
@@ -677,62 +678,59 @@ fn debug_attachments_invoke(
         .into_iter()
         .map(|frame| match frame.kind() {
             #[cfg(feature = "std")]
-            FrameKind::Attachment(AttachmentKind::Opaque(_)) | FrameKind::Context(_) => {
-                Report::invoke_debug_format_hook(|hooks| hooks.call(frame, context));
-                context.take_body()
-            }
-            #[cfg(all(not(feature = "std"), feature = "pretty-print"))]
-            FrameKind::Context(_) => {
-                let location = frame
-                    .location()
-                    .if_supports_color(Stream::Stdout, OwoColorize::bright_black);
-
-                vec![format!("{location}")]
-            }
-            #[cfg(all(not(feature = "std"), not(feature = "pretty-print")))]
-            FrameKind::Context(_) => {
-                let location = frame.location();
-
-                vec![format!("at {location}")]
-            }
-            #[cfg(not(feature = "std"))]
-            FrameKind::Attachment(AttachmentKind::Opaque(_)) => {
-                vec![]
-            }
+            FrameKind::Context(_) => Some(
+                Report::invoke_debug_format_hook(|hooks| hooks.call(frame, context))
+                    .then(|| context.take_body())
+                    .unwrap_or_default(),
+            ),
             #[cfg(feature = "std")]
-            FrameKind::Attachment(AttachmentKind::Printable(attachment)) => {
-                Report::invoke_debug_format_hook(|hooks| hooks.call(frame, context));
-                let mut body = context.take_body();
-
-                if body.is_empty() {
-                    body.push(attachment.to_string());
-                }
-
-                body
+            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")]
+            FrameKind::Attachment(AttachmentKind::Opaque(_)) => {
+                Report::invoke_debug_format_hook(|hooks| hooks.call(frame, context))
+                    .then(|| context.take_body())
             }
             #[cfg(not(feature = "std"))]
+            FrameKind::Context(_) => Some(vec![]),
+            #[cfg(not(feature = "std"))]
             FrameKind::Attachment(AttachmentKind::Printable(attachment)) => {
-                vec![attachment.to_string()]
+                Some(vec![attachment.to_string()])
             }
+            #[cfg(all(not(feature = "std"), feature = "pretty-print"))]
+            FrameKind::Attachment(AttachmentKind::Opaque(_)) => frame
+                .downcast_ref::<core::panic::Location<'static>>()
+                .map(|location| {
+                    vec![
+                        location
+                            .if_supports_color(Stream::Stdout, OwoColorize::bright_black)
+                            .to_string(),
+                    ]
+                }),
+            #[cfg(all(not(feature = "std"), not(feature = "pretty-print")))]
+            FrameKind::Attachment(AttachmentKind::Opaque(_)) => frame
+                .downcast_ref::<core::panic::Location>()
+                .map(|location| vec![format!("at {location}")]),
         })
-        .enumerate()
-        .flat_map(|(idx, body)| {
-            // increase the opaque counter, if we're unable to determine the actual value of the
-            // frame
-            if idx > 0 && body.is_empty() {
+        .flat_map(|body| {
+            body.unwrap_or_else(|| {
+                // increase the opaque counter, if we're unable to determine the actual value of
+                // the frame
                 opaque.increase();
-            }
-
-            body
+                Vec::new()
+            })
         })
         .collect();
 
     (opaque, body)
 }
 
-fn debug_attachments(
+fn debug_attachments<'a>(
     position: Position,
-    frames: Vec<&Frame>,
+    frames: impl IntoIterator<Item = &'a Frame>,
     #[cfg(feature = "std")] context: &mut HookContext<Frame>,
 ) -> Lines {
     let last = matches!(position, Position::Final);
@@ -905,7 +903,7 @@ fn debug_frame(
                 } else {
                     Position::Inner
                 },
-                once(head).chain(body).collect(),
+                once(head).chain(body),
                 #[cfg(feature = "std")]
                 context,
             );

packages/libs/error-stack/src/frame/mod.rs

@@ -29,6 +29,13 @@ pub struct Frame {
 impl Frame {
     /// Returns the location where this `Frame` was created.
     #[must_use]
+    #[deprecated(
+        since = "0.2.4",
+        note = "`location()` has been replaced with an additional attachment containing \
+                `Location<'static>` for each `Context`, similar to how `Backtrace` and \
+                `SpanTrace` are handled. Note: This means that once `location()` is removed you \
+                won't be able to get location of attachments anymore."
+    )]
     pub const fn location(&self) -> &'static Location<'static> {
         self.location
     }
@@ -128,18 +135,17 @@ impl Provider for Frame {
 impl fmt::Debug for Frame {
     fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
         let mut debug = fmt.debug_struct("Frame");
-        debug.field("location", self.location());
+
         match self.kind() {
             FrameKind::Context(context) => {
                 debug.field("context", &context);
+                debug.finish()
             }
             FrameKind::Attachment(AttachmentKind::Printable(attachment)) => {
                 debug.field("attachment", &attachment);
+                debug.finish()
             }
-            FrameKind::Attachment(AttachmentKind::Opaque(_)) => {
-                debug.field("attachment", &"opaque");
-            }
+            FrameKind::Attachment(AttachmentKind::Opaque(_)) => debug.finish_non_exhaustive(),
         }
-        debug.finish()
     }
 }

packages/libs/error-stack/src/report.rs

@@ -226,6 +226,14 @@ impl<C> Report<C> {
 
     #[track_caller]
     pub(crate) fn from_frame(frame: Frame) -> Self {
+        #[cfg(nightly)]
+        let location = core::any::request_ref::<Location>(&frame)
+            .is_none()
+            .then_some(Location::caller());
+
+        #[cfg(not(nightly))]
+        let location = Some(Location::caller());
+
         #[cfg(all(nightly, feature = "std"))]
         let backtrace = core::any::request_ref::<Backtrace>(&frame)
             .filter(|backtrace| backtrace.status() == BacktraceStatus::Captured)
@@ -250,6 +258,10 @@ impl<C> Report<C> {
             _context: PhantomData,
         };
 
+        if let Some(location) = location {
+            report = report.attach(*location);
+        }
+
         #[cfg(all(rust_1_65, feature = "std"))]
         if let Some(backtrace) =
             backtrace.filter(|bt| matches!(bt.status(), BacktraceStatus::Captured))
@@ -453,10 +465,15 @@ impl<C> Report<C> {
         T: Context,
     {
         let old_frames = mem::replace(self.frames.as_mut(), Vec::with_capacity(1));
-        self.frames.push(Frame::from_context(
+        let context_frame = vec![Frame::from_context(
             context,
             Location::caller(),
             old_frames.into_boxed_slice(),
+        )];
+        self.frames.push(Frame::from_attachment(
+            *Location::caller(),
+            Location::caller(),
+            context_frame.into_boxed_slice(),
         ));
         Report {
             frames: self.frames,