From 3dbab881da0cc35ba1aaa9ddcc19497cdb5c3481 Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Wed, 27 Apr 2022 14:58:50 -0400 Subject: [PATCH] tamer: diagnose::report: Produce Report object Rather than writing to the provided `Write` object, this produces a `Report` object. While a lifetime still exists for the diagnostic data (labels, specifically), I was able to remove the other lifetime resulting from `ResolvedSpan` by transferring ownership of the data to the `Report` itself. Once actual source lines are integrated shortly, `Report` will include those as well. This has been a tedious process, but it's coming together. Hopefully these commits documenting the progressive and ugly refactoring are found useful by some reader in the future. DEV-12151 --- tamer/src/bin/tamec.rs | 2 +- tamer/src/bin/tameld.rs | 2 +- tamer/src/diagnose/report.rs | 109 ++++++++---------- tamer/src/diagnose/report/test/integration.rs | 4 +- 4 files changed, 51 insertions(+), 66 deletions(-) diff --git a/tamer/src/bin/tamec.rs b/tamer/src/bin/tamec.rs index df353576..6ff26e91 100644 --- a/tamer/src/bin/tamec.rs +++ b/tamer/src/bin/tamec.rs @@ -106,7 +106,7 @@ pub fn main() -> Result<(), TamecError> { // interleave output between processes, // but we ought to reuse a buffer when we support multiple // errors. - let report = reporter.render_to_string(&e)?; + let report = reporter.render(&e).to_string(); println!("{report}\nfatal: failed to link `{}`", output); std::process::exit(1); diff --git a/tamer/src/bin/tameld.rs b/tamer/src/bin/tameld.rs index 5d393c05..19d5451c 100644 --- a/tamer/src/bin/tameld.rs +++ b/tamer/src/bin/tameld.rs @@ -71,7 +71,7 @@ pub fn main() -> Result<(), TameldError> { // interleave output between processes, // but we ought to reuse a buffer when we support multiple // errors. - let report = reporter.render_to_string(&e)?; + let report = reporter.render(&e).to_string(); println!("{report}\nfatal: failed to link `{}`", output); std::process::exit(1); diff --git a/tamer/src/diagnose/report.rs b/tamer/src/diagnose/report.rs index a1bbfc1d..90864ae0 100644 --- a/tamer/src/diagnose/report.rs +++ b/tamer/src/diagnose/report.rs @@ -30,14 +30,17 @@ use super::{ }; use crate::span::{Context, Span, UNKNOWN_SPAN}; use std::{ - fmt::{self, Display, Write}, + fmt::{self, Display}, num::NonZeroU32, }; pub trait Reporter { /// Render diagnostic report. /// - /// Please be mindful of where this report is being rendered `to`. + /// The provided [`Report`] implements [`Display`]. + /// + /// Please be mindful of where this report is being rendered to + /// (via [`Display`]). /// For example, /// if rendering to standard out, /// it is a good idea to buffer the entire report before flushing to @@ -45,23 +48,17 @@ pub trait Reporter { /// otherwise the report may become interleaved with other /// concurrent processes /// (e.g. if TAMER is being invoked using `make -jN`). - fn render( - &mut self, - diagnostic: &impl Diagnostic, - to: &mut impl Write, - ) -> Result<(), fmt::Error>; - - /// Render a diagnostic report into an owned [`String`]. /// - /// This invokes [`Reporter::render`] on a newly allocated [`String`]. - fn render_to_string( - &mut self, - diagnostic: &impl Diagnostic, - ) -> Result { - let mut str = String::new(); - self.render(diagnostic, &mut str)?; - Ok(str) - } + /// It is also important to note that this method + /// _does not return [`Result`]_ and should never fail, + /// unless due to a panic in the standard library + /// (e.g. due to allocation failure). + /// The report absorbs errors during processing and renders those errors + /// to the report itself, + /// ensuring both that the user is made aware of the problem + /// and that we're not inadvertently suppressing the actual + /// diagnostic messages that were requested. + fn render<'l>(&mut self, diagnostic: &'l impl Diagnostic) -> Report<'l>; } /// Render diagnostic report in a highly visual way. @@ -84,12 +81,7 @@ impl VisualReporter { } impl Reporter for VisualReporter { - // _TODO: This is a proof-of-concept._ - fn render( - &mut self, - diagnostic: &impl Diagnostic, - to: &mut impl Write, - ) -> fmt::Result { + fn render<'l>(&mut self, diagnostic: &'l impl Diagnostic) -> Report<'l> { // TODO: Avoid duplicate lookups of the same span, // or at least adjacent ones. let mspans = diagnostic @@ -106,22 +98,21 @@ impl Reporter for VisualReporter { .collect::>(); let message = Message(diagnostic.to_string()); + let mut report = Report::empty(message); - - report.extend(mspans.iter().map(Into::into)); - - write!(to, "{}", report) + report.extend(mspans.into_iter().map(Into::into)); + report } } #[derive(Debug)] -struct Report<'s, 'l> { +pub struct Report<'l> { msg: Message, - secs: Vec>, + secs: Vec>, level: Level, } -impl<'s, 'l> Report<'s, 'l> { +impl<'l> Report<'l> { fn empty(msg: Message) -> Self { Self { msg, @@ -131,8 +122,8 @@ impl<'s, 'l> Report<'s, 'l> { } } -impl<'s, 'l> Extend> for Report<'s, 'l> { - fn extend>>(&mut self, secs: T) { +impl<'l> Extend> for Report<'l> { + fn extend>>(&mut self, secs: T) { for sec in secs { self.level = self.level.min(sec.level()); self.secs.push(sec.consider_squash(self.secs.last())); @@ -140,7 +131,7 @@ impl<'s, 'l> Extend> for Report<'s, 'l> { } } -impl<'s, 'l> Display for Report<'s, 'l> { +impl<'l> Display for Report<'l> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{level}: {msg}\n", level = self.level, msg = self.msg)?; self.secs.iter().try_for_each(|sec| sec.fmt(f)) @@ -157,18 +148,18 @@ impl Display for Message { } #[derive(Debug)] -enum Section<'s, 'l> { +enum Section<'l> { /// Section is delimited from surrounding sections with a heading. - Delimited(SpanHeading, SystemLabels, Option<&'s SpanLabel<'l>>, Span), + Delimited(SpanHeading, SystemLabels, Option>, Span), /// Section is squashed into the preceding section by eliding its /// heading. /// /// This term originates from `git rebase` for a similar operation. - Squashed(Option<&'s SpanLabel<'l>>, Span), + Squashed(Option>, Span), } -impl<'s, 'l> Section<'s, 'l> { +impl<'s, 'l> Section<'l> { fn level(&self) -> Level { match self { Self::Delimited(_, _, olabel, _) | Self::Squashed(olabel, _) => { @@ -198,21 +189,30 @@ impl<'s, 'l> Section<'s, 'l> { } } -impl<'s, 'l, 'a, S> From<&'s MaybeResolvedSpan<'l, S>> for Section<'s, 'l> +impl<'l, 'a, S> From> for Section<'l> where S: ResolvedSpanData, { - fn from(mspan: &'s MaybeResolvedSpan<'l, S>) -> Self { - Section::Delimited( - SpanHeading::from(mspan), - mspan.system_labels(), - mspan.label(), - mspan.unresolved_span(), - ) + fn from(mspan: MaybeResolvedSpan<'l, S>) -> Self { + let heading = SpanHeading::from(&mspan); + let syslabels = mspan.system_labels(); + + match mspan { + MaybeResolvedSpan::Resolved(rspan, label) => Section::Delimited( + heading, + syslabels, + label, + rspan.unresolved_span(), + ), + + MaybeResolvedSpan::Unresolved(span, label, _) => { + Section::Delimited(heading, syslabels, label, span) + } + } } } -impl<'s, 'l> Display for Section<'s, 'l> { +impl<'l> Display for Section<'l> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let olabel = match self { Self::Delimited(heading, syslabels, olabel, _) => { @@ -259,21 +259,6 @@ enum MaybeResolvedSpan<'l, S: ResolvedSpanData> { } impl<'l, S: ResolvedSpanData> MaybeResolvedSpan<'l, S> { - fn unresolved_span(&self) -> Span { - match self { - Self::Resolved(rspan, ..) => rspan.unresolved_span(), - Self::Unresolved(span, ..) => *span, - } - } - - fn label(&self) -> Option<&SpanLabel<'l>> { - match self { - Self::Resolved(_, olabel) | Self::Unresolved(_, olabel, _) => { - olabel.as_ref() - } - } - } - /// We should never mask an error with our own; /// the diagnostic system is supposed to _help_ the user in diagnosing /// problems, diff --git a/tamer/src/diagnose/report/test/integration.rs b/tamer/src/diagnose/report/test/integration.rs index 0450f9cd..77fd89d8 100644 --- a/tamer/src/diagnose/report/test/integration.rs +++ b/tamer/src/diagnose/report/test/integration.rs @@ -122,8 +122,8 @@ macro_rules! assert_report { let mut sut = VisualReporter::new(resolver); assert_eq!( - sut.render_to_string(&StubError($msg.into(), $aspans)), - Ok($expected.into()), + sut.render(&StubError($msg.into(), $aspans)).to_string(), + $expected, ); }; }