diff --git a/CHANGELOG.md b/CHANGELOG.md index a503de4..3e1d96c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,10 +13,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Added a compatibility module for error-stack v0.7 [#169](https://github.com/rootcause-rs/rootcause/pull/169). - Implement `Deref` and `AsRef` for `Report`, `ReportRef` and `ReportMut`; `Report<_, _, SendSync>` targets `dyn Error + Send + Sync`. May break type inference at some `Report::new*` call sites. [#147](https://github.com/rootcause-rs/rootcause/pull/147), [#149](https://github.com/rootcause-rs/rootcause/pull/149) - Doc examples for nearly all public functions [#136](https://github.com/rootcause-rs/rootcause/pull/136). +- `ReportAttachmentRef::format_inner_with_parent` for formatting an attachment with parent-report context. [#163](https://github.com/rootcause-rs/rootcause/pull/163). ### Changed - Moved the preformat functionality out of `rootcause` into the new `rootcause-preformat` companion crate. The `preformatted` module, the `preformat*` methods, and the `display_preformatted`/`debug_preformatted` formatter-hook methods are removed; use the `Preformat*Ext` and `ContextTransformNestedExt` traits from the new crate. `AttachmentFormatterHook::preferred_formatting_style` and `ContextFormatterHook::preferred_context_formatting_style` now take a concrete type instead of `Dynamic`. [#148](https://github.com/rootcause-rs/rootcause/pull/148) +- `DefaultReportFormatter` now populates the `AttachmentParent` argument when invoking `AttachmentFormatterHook::display`/`debug` (previously always `None`), exposing the parent report and the attachment's pre-sort index. [#163](https://github.com/rootcause-rs/rootcause/pull/163). ### Fixed diff --git a/examples/formatting_hooks.rs b/examples/formatting_hooks.rs index c1e085d..9d174c8 100644 --- a/examples/formatting_hooks.rs +++ b/examples/formatting_hooks.rs @@ -16,7 +16,8 @@ use rootcause::{ ReportRef, handlers::{AttachmentFormattingPlacement, AttachmentFormattingStyle, FormattingFunction}, hooks::{ - Hooks, attachment_formatter::AttachmentFormatterHook, + Hooks, + attachment_formatter::{AttachmentFormatterHook, AttachmentParent}, context_formatter::ContextFormatterHook, }, markers::{Local, Uncloneable}, @@ -144,6 +145,49 @@ impl ContextFormatterHook for ValidationErrorFormatter { } } +// Example 4: Parent-aware formatting +// +// When the default report formatter walks an error tree, it hands every +// attachment formatter hook an `AttachmentParent` providing access to the parent report +// and the attachment's position in that report's original attachment +// list. +// This lets a hook produce output that's contextually aware of where it +// sits in the tree. + +#[derive(Debug)] +struct RequestId(usize); + +impl core::fmt::Display for RequestId { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + write!(f, "{}", self.0) + } +} + +struct RequestIdFormatter; + +impl AttachmentFormatterHook for RequestIdFormatter { + fn display( + &self, + attachment: ReportAttachmentRef<'_, RequestId>, + parent: Option>, + f: &mut core::fmt::Formatter<'_>, + ) -> core::fmt::Result { + let id = attachment.inner().0; + // The `parent` is `Some` when the hook is called from a `ReportFormatter` + // and `None` if the attachment is being formatted in isolation - + // such as `attachment.format_inner()` or `println!("{attachment}")` + match parent { + Some(parent) => write!( + f, + "request_id[{id}] (attachment #{} on Report<{}>)", + parent.attachment_index, + parent.report.current_context_type_name(), + ), + None => write!(f, "request {id}"), + } + } +} + // Example 1: Control attachment placement in output // Demonstrates placing verbose diagnostic data in the appendix section instead // of inline @@ -177,11 +221,19 @@ fn demo_context_formatting() -> Result<(), Report> { Err(report!(validation).into_dynamic()) } +fn demo_parent_aware_formatting() -> Result<(), Report> { + Err(report!("Outer failure") + .attach(RequestId(2)) + .attach("internal note") + .attach(RequestId(1))) +} + fn main() { // Install formatting hooks Hooks::new() .attachment_formatter::(DatabaseQueryFormatter) .attachment_formatter::(ActionRequiredFormatter) + .attachment_formatter::(RequestIdFormatter) .context_formatter::(ValidationErrorFormatter) .install() .expect("failed to install hooks"); @@ -200,6 +252,12 @@ fn main() { println!("Example 3: Context formatting\n"); match demo_context_formatting() { + Ok(()) => println!("Success"), + Err(error) => eprintln!("{error}\n"), + } + + println!("Example 4: Parent-aware attachment formatting\n"); + match demo_parent_aware_formatting() { Ok(()) => println!("Success"), Err(error) => eprintln!("{error}"), } diff --git a/src/hooks/attachment_formatter.rs b/src/hooks/attachment_formatter.rs index bea2e12..6d92d7f 100644 --- a/src/hooks/attachment_formatter.rs +++ b/src/hooks/attachment_formatter.rs @@ -256,6 +256,17 @@ where /// to understand the attachment's position or relationship to its containing /// report. /// +/// An [`AttachmentFormatterHook`] receives `Some(AttachmentParent)` when the +/// attachment is being formatted via +/// [`ReportAttachmentRef::format_inner_with_parent`], which the built-in +/// [`DefaultReportFormatter`](crate::hooks::builtin_hooks::report_formatter::DefaultReportFormatter) +/// uses when walking an error tree. It is `None` when the attachment is being +/// formatted in isolation via [`ReportAttachmentRef::format_inner`] (for +/// example, `attachment.format_inner().to_string()`). +/// +/// [`ReportAttachmentRef::format_inner`]: crate::report_attachment::ReportAttachmentRef::format_inner +/// [`ReportAttachmentRef::format_inner_with_parent`]: crate::report_attachment::ReportAttachmentRef::format_inner_with_parent +/// /// # Examples /// /// ``` @@ -292,7 +303,14 @@ where pub struct AttachmentParent<'a> { /// Reference to the report that contains this attachment pub report: ReportRef<'a, Dynamic, Uncloneable, Local>, - /// Index of this attachment within the parent report's attachment list + /// Index of this attachment within the parent report's attachment list. + /// + /// By convention this is the attachment's stable position in + /// `report.attachments()`, independent of any priority sorting or + /// placement filtering the formatter applies for display purposes. The + /// built-in [`DefaultReportFormatter`](crate::hooks::builtin_hooks::report_formatter::DefaultReportFormatter) + /// upholds this; custom [`ReportFormatter`](crate::hooks::report_formatter::ReportFormatter) + /// implementations are encouraged to do the same. pub attachment_index: usize, } @@ -402,7 +420,8 @@ pub trait AttachmentFormatterHook: 'static + Send + Sync { /// # Arguments /// /// * `attachment` - Reference to the attachment being formatted - /// * `attachment_parent` - Optional context about the parent report + /// * `attachment_parent` - Optional context about the parent report. See + /// [`AttachmentParent`] for when this is `Some` vs `None`. /// * `formatter` - The formatter to write output to /// /// # Examples @@ -448,7 +467,8 @@ pub trait AttachmentFormatterHook: 'static + Send + Sync { /// # Arguments /// /// * `attachment` - Reference to the attachment being formatted - /// * `attachment_parent` - Optional context about the parent report + /// * `attachment_parent` - Optional context about the parent report. See + /// [`AttachmentParent`] for when this is `Some` vs `None`. /// * `formatter` - The formatter to write output to /// /// # Examples diff --git a/src/hooks/builtin_hooks/report_formatter.rs b/src/hooks/builtin_hooks/report_formatter.rs index 6c29e5b..8aeeb15 100644 --- a/src/hooks/builtin_hooks/report_formatter.rs +++ b/src/hooks/builtin_hooks/report_formatter.rs @@ -64,7 +64,7 @@ use rootcause_internals::handlers::{ use crate::{ ReportRef, - hooks::report_formatter::ReportFormatter, + hooks::{attachment_formatter::AttachmentParent, report_formatter::ReportFormatter}, markers::{Dynamic, Local, Uncloneable}, report_attachment::ReportAttachmentRef, }; @@ -815,7 +815,11 @@ impl NodeConfig { } type Appendices<'a> = IndexMap< &'static str, - Vec<(ReportAttachmentRef<'a, Dynamic>, FormattingFunction)>, + Vec<( + ReportAttachmentRef<'a, Dynamic>, + AttachmentParent<'a>, + FormattingFunction, + )>, rustc_hash::FxBuildHasher, >; @@ -841,7 +845,11 @@ impl ReportFormatter for DefaultReportFormatter { } type TmpValueBuffer = String; -type TmpAttachmentsBuffer<'a> = Vec<(AttachmentFormattingStyle, ReportAttachmentRef<'a, Dynamic>)>; +type TmpAttachmentsBuffer<'a> = Vec<( + AttachmentFormattingStyle, + ReportAttachmentRef<'a, Dynamic>, + usize, +)>; impl<'a, 'b> DefaultFormatterState<'a, 'b> { fn new( @@ -1012,33 +1020,44 @@ impl<'a, 'b> DefaultFormatterState<'a, 'b> { report .attachments() .iter() - .map(|attachment| { + .enumerate() + .map(|(original_index, attachment)| { ( attachment.preferred_formatting_style(self.report_formatting_function), attachment, + original_index, ) }) - .filter( - |(formatting_style, _attachment)| match formatting_style.placement { + .filter(|(formatting_style, _attachment, _index)| { + match formatting_style.placement { AttachmentFormattingPlacement::Opaque => { opaque_attachment_count += 1; false } AttachmentFormattingPlacement::Hidden => false, _ => true, - }, - ), + } + }), ); tmp_attachments_buffer - .sort_by_key(|(style1, _attachment)| core::cmp::Reverse(style1.priority)); - for (attachment_index, &(attachment_formatting_style, attachment)) in + .sort_by_key(|(style, _attachment, _index)| core::cmp::Reverse(style.priority)); + for (display_index, &(attachment_formatting_style, attachment, original_index)) in tmp_attachments_buffer.iter().enumerate() { - let is_last_attachment = attachment_index + 1 == tmp_attachments_buffer.len(); + let is_last_attachment = display_index + 1 == tmp_attachments_buffer.len(); + // `original_index` reflects an attachment's position in the parent report's original + // attachment list. + // It is independent of any sorting or filtering the formatter may apply for display + // purposes. + let parent = AttachmentParent { + report, + attachment_index: original_index, + }; self.format_attachment( tmp_value_buffer, attachment_formatting_style, attachment, + parent, is_last_attachment && !has_children, )?; } @@ -1098,6 +1117,7 @@ impl<'a, 'b> DefaultFormatterState<'a, 'b> { tmp_value_buffer: &mut TmpValueBuffer, attachment_formatting_style: AttachmentFormattingStyle, attachment: ReportAttachmentRef<'a, Dynamic>, + attachment_parent: AttachmentParent<'a>, is_last: bool, ) -> fmt::Result { match attachment_formatting_style.placement { @@ -1110,7 +1130,7 @@ impl<'a, 'b> DefaultFormatterState<'a, 'b> { self.format_item( tmp_value_buffer, formatting, - attachment.format_inner(), + attachment.format_inner_with_parent(attachment_parent), attachment_formatting_style.function, )?; } @@ -1136,7 +1156,7 @@ impl<'a, 'b> DefaultFormatterState<'a, 'b> { this.format_item( tmp_value_buffer, &self.config.attachment_headered_formatting_data, - attachment.format_inner(), + attachment.format_inner_with_parent(attachment_parent), attachment_formatting_style.function, )?; if let Some(headered_attachment_data_suffix) = @@ -1151,7 +1171,11 @@ impl<'a, 'b> DefaultFormatterState<'a, 'b> { } AttachmentFormattingPlacement::Appendix { appendix_name } => { let appendices = self.appendices.entry(appendix_name).or_default(); - appendices.push((attachment, attachment_formatting_style.function)); + appendices.push(( + attachment, + attachment_parent, + attachment_formatting_style.function, + )); let formatting = if is_last { &self.config.notice_see_also_last_formatting } else { @@ -1270,7 +1294,7 @@ impl<'a, 'b> DefaultFormatterState<'a, 'b> { let mut is_first = true; for (appendix_name, appendices) in &appendices { - for (appendix_index, &(attachment, formatting_function)) in + for (appendix_index, &(attachment, attachment_parent, formatting_function)) in appendices.iter().enumerate() { if is_first { @@ -1285,7 +1309,7 @@ impl<'a, 'b> DefaultFormatterState<'a, 'b> { self.format_item( tmp_value_buffer, &self.config.appendix_body, - attachment.format_inner(), + attachment.format_inner_with_parent(attachment_parent), formatting_function, )?; } diff --git a/src/report_attachment/ref_.rs b/src/report_attachment/ref_.rs index a0924e4..401c494 100644 --- a/src/report_attachment/ref_.rs +++ b/src/report_attachment/ref_.rs @@ -2,7 +2,7 @@ use core::any::{Any, TypeId}; use rootcause_internals::handlers::{AttachmentFormattingStyle, FormattingFunction}; -use crate::{markers::Dynamic, util::format_helper}; +use crate::{hooks::attachment_formatter::AttachmentParent, markers::Dynamic, util::format_helper}; /// FIXME: Once rust-lang/rust#132922 gets resolved, we can make the `raw` field /// an unsafe field and remove this module. @@ -267,6 +267,60 @@ impl<'a, A: ?Sized> ReportAttachmentRef<'a, A> { ) } + /// Formats the inner attachment data with formatting hooks applied, + /// passing the supplied [`AttachmentParent`] to the hook. + /// + /// This is the variant of [`format_inner`] used by report formatters + /// while walking an error tree: it lets the hook see the parent report + /// and the attachment's index in that report's attachment list. Use + /// [`format_inner`] instead when there is no parent context (for + /// example, formatting an attachment in isolation). + /// + /// [`format_inner`]: Self::format_inner + /// + /// # Examples + /// + /// ``` + /// # use rootcause::{ + /// # hooks::attachment_formatter::AttachmentParent, + /// # prelude::*, + /// # report_attachment::ReportAttachment, + /// # }; + /// let report: Report = report!("outer"); + /// let attachment = ReportAttachment::new_sendsync(42i32); + /// let parent = AttachmentParent { + /// report: report.as_ref().into_dynamic().into_uncloneable().into_local(), + /// attachment_index: 0, + /// }; + /// assert_eq!( + /// attachment.as_ref().format_inner_with_parent(parent).to_string(), + /// "42", + /// ); + /// ``` + #[must_use] + pub fn format_inner_with_parent( + self, + parent: AttachmentParent<'_>, + ) -> impl core::fmt::Display + core::fmt::Debug { + format_helper( + (self.into_dynamic(), parent), + |(attachment, parent), formatter| { + crate::hooks::attachment_formatter::display_attachment( + attachment, + Some(parent), + formatter, + ) + }, + |(attachment, parent), formatter| { + crate::hooks::attachment_formatter::debug_attachment( + attachment, + Some(parent), + formatter, + ) + }, + ) + } + /// Formats the inner attachment data without applying any formatting hooks. /// /// This method provides direct access to the attachment's formatting diff --git a/tests/attachment_parent.rs b/tests/attachment_parent.rs new file mode 100644 index 0000000..8af5f91 --- /dev/null +++ b/tests/attachment_parent.rs @@ -0,0 +1,188 @@ +//! Locks down `AttachmentParent.attachment_index` semantics in +//! `DefaultReportFormatter`. +//! +//! The index reported to an `AttachmentFormatterHook` must reflect the +//! attachment's position in `report.attachments()`, not the position the +//! formatter ends up displaying it at after priority-sorting, and this must +//! hold for both `Inline` and `Appendix` placements. + +use rootcause::{ + handlers::{AttachmentFormattingPlacement, AttachmentFormattingStyle, FormattingFunction}, + hooks::{ + Hooks, + attachment_formatter::{AttachmentFormatterHook, AttachmentParent}, + }, + prelude::*, + report_attachment::ReportAttachmentRef, +}; + +#[derive(Debug)] +struct Probe { + tag: &'static str, + priority: i32, +} + +impl core::fmt::Display for Probe { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + write!(f, "{}", self.tag) + } +} + +struct ProbeFormatter; + +impl AttachmentFormatterHook for ProbeFormatter { + fn display( + &self, + attachment: ReportAttachmentRef<'_, Probe>, + parent: Option>, + f: &mut core::fmt::Formatter<'_>, + ) -> core::fmt::Result { + let tag = attachment.inner().tag; + match parent { + Some(parent) => write!(f, "{tag}@{}", parent.attachment_index), + None => write!(f, "{tag}@NONE"), + } + } + + fn preferred_formatting_style( + &self, + attachment: ReportAttachmentRef<'_, Probe>, + _report_formatting_function: FormattingFunction, + ) -> AttachmentFormattingStyle { + AttachmentFormattingStyle { + placement: AttachmentFormattingPlacement::Inline, + function: FormattingFunction::Display, + priority: attachment.inner().priority, + } + } +} + +#[derive(Debug)] +struct AppendixProbe { + tag: &'static str, + priority: i32, +} + +impl core::fmt::Display for AppendixProbe { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + write!(f, "{}", self.tag) + } +} + +struct AppendixProbeFormatter; + +impl AttachmentFormatterHook for AppendixProbeFormatter { + fn display( + &self, + attachment: ReportAttachmentRef<'_, AppendixProbe>, + parent: Option>, + f: &mut core::fmt::Formatter<'_>, + ) -> core::fmt::Result { + let tag = attachment.inner().tag; + match parent { + Some(parent) => write!(f, "{tag}@{}", parent.attachment_index), + None => write!(f, "{tag}@NONE"), + } + } + + fn preferred_formatting_style( + &self, + attachment: ReportAttachmentRef<'_, AppendixProbe>, + _report_formatting_function: FormattingFunction, + ) -> AttachmentFormattingStyle { + AttachmentFormattingStyle { + placement: AttachmentFormattingPlacement::Appendix { + appendix_name: "Diagnostics", + }, + function: FormattingFunction::Display, + priority: attachment.inner().priority, + } + } +} + +#[test] +fn attachment_index_is_pre_sort_position() { + // `new_without_locations` keeps attachment indices simple: 0, 1, 2, ... — no + // auto-Location attachment slipping in at index 0. + Hooks::new_without_locations() + .attachment_formatter::(ProbeFormatter) + .attachment_formatter::(AppendixProbeFormatter) + .install() + .expect("hooks should not already be installed"); + + // Priorities chosen so the display order differs from the insertion order: + // sorted descending → "second" (10), "appendix_first" (5), "first" (0), + // "appendix_second" (-2), "third" (-10). + let report: Report = report!("outer") + .attach(Probe { + tag: "first", + priority: 0, + }) + .attach(Probe { + tag: "second", + priority: 10, + }) + .attach(Probe { + tag: "third", + priority: -10, + }) + .attach(AppendixProbe { + tag: "appendix_first", + priority: 5, + }) + .attach(AppendixProbe { + tag: "appendix_second", + priority: -2, + }); + + let rendered = format!("{report}"); + + // Core invariant: each probe sees its ORIGINAL index, not its display + // index. Holds for both Inline and Appendix placements. + for needle in [ + "first@0", + "second@1", + "third@2", + "appendix_first@3", + "appendix_second@4", + ] { + assert!( + rendered.contains(needle), + "expected `{needle}` in output:\n{rendered}" + ); + } + + // Sanity: confirm the formatter actually re-sorted. Without this check the + // index-equality assertions above would still pass if sorting were a no-op, + // which would mean we weren't really testing the pre-sort property. + let pos = |needle: &str| { + rendered + .find(needle) + .unwrap_or_else(|| panic!("`{needle}` not found in:\n{rendered}")) + }; + let second = pos("second@1"); + let first = pos("first@0"); + let third = pos("third@2"); + assert!( + second < first && first < third, + "expected priority-sorted inline display order second,first,third but got:\n{rendered}" + ); + let appendix_first = pos("appendix_first@3"); + let appendix_second = pos("appendix_second@4"); + assert!( + appendix_first < appendix_second, + "expected priority-sorted appendix display order appendix_first,appendix_second but got:\n{rendered}" + ); + + // Bonus: `format_inner()` (no parent) reports `NONE`. + let isolated = format!( + "{}", + rootcause::report_attachment::ReportAttachment::new_local(Probe { + tag: "iso", + priority: 0, + }) + .as_ref() + .format_inner() + ); + assert_eq!(isolated, "iso@NONE"); +}