From 75fea0935e495d0215518c80d32dd820910982e3 Mon Sep 17 00:00:00 2001 From: Nahor Date: Sat, 17 Feb 2024 13:46:24 -0800 Subject: [PATCH] fix(label-collections): Label collection fixes and cleanup (#343) * feat(collection): add label(collection) documentation to lib.rs * feat(collection): remove repeated formatting of label text Because of a typo, the label text was being formatted multiple times per label in a collection. With the fix, the text is formatted only once per collection * feat(collection): chain iterators rather than extend vector Since we are going to iterate anyway, instead of growing the label vector, chain the iterators. This should be more efficient. In some cases, this also remove a compilation warning about the label vector being unnecessarily mutable. * feat(collection): remove unnecessary `OptionalWrapper` - In a label collection, there is no need for a `None` label. One should just not add that label - Code wise it is also incorrect. The wrapper will be for a vector of spans, not for individual spans. It happens to work anyway, but this was not the intended use. --- miette-derive/src/label.rs | 74 +++++++++++++++++++------------------- src/lib.rs | 52 +++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 38 deletions(-) diff --git a/miette-derive/src/label.rs b/miette-derive/src/label.rs index cd6994ab..ab2ceac1 100644 --- a/miette-derive/src/label.rs +++ b/miette-derive/src/label.rs @@ -162,7 +162,6 @@ impl Labels { pub(crate) fn gen_struct(&self, fields: &syn::Fields) -> Option { let (display_pat, display_members) = display_pat_members(fields); - let labels_gen_var = quote! { labels }; let labels = self.0.iter().filter_map(|highlight| { let Label { span, @@ -194,11 +193,11 @@ impl Labels { )) }) }); - let collections = self.0.iter().filter_map(|label| { + let collections_chain = self.0.iter().filter_map(|label| { let Label { span, label, - ty, + ty: _, lbl_ty, } = label; if *lbl_ty != LabelType::Collection { @@ -211,18 +210,17 @@ impl Labels { quote! { std::option::Option::None } }; Some(quote! { - let display = #display; - #labels_gen_var.extend(self.#span.iter().map(|label| { - miette::macro_helpers::OptionalWrapper::<#ty>::new().to_option(label) - .map(|span| { - use miette::macro_helpers::{ToLabelSpanWrapper,ToLabeledSpan}; - let mut labeled_span = ToLabelSpanWrapper::to_labeled_span(span.clone()); - if #display.is_some() && labeled_span.label().is_none() { - labeled_span.set_label(#display) - } - labeled_span - }) - })); + .chain({ + let display = #display; + self.#span.iter().map(move |span| { + use miette::macro_helpers::{ToLabelSpanWrapper,ToLabeledSpan}; + let mut labeled_span = ToLabelSpanWrapper::to_labeled_span(span.clone()); + if display.is_some() && labeled_span.label().is_none() { + labeled_span.set_label(display.clone()) + } + Some(labeled_span) + }) + }) }) }); @@ -232,12 +230,13 @@ impl Labels { use miette::macro_helpers::ToOption; let Self #display_pat = self; - let mut #labels_gen_var = vec![ + let labels_iter = vec![ #(#labels),* - ]; - #(#collections)* + ] + .into_iter() + #(#collections_chain)*; - std::option::Option::Some(Box::new(#labels_gen_var.into_iter().filter(Option::is_some).map(Option::unwrap))) + std::option::Option::Some(Box::new(labels_iter.filter(Option::is_some).map(Option::unwrap))) } }) } @@ -249,7 +248,6 @@ impl Labels { |ident, fields, DiagnosticConcreteArgs { labels, .. }| { let (display_pat, display_members) = display_pat_members(fields); labels.as_ref().and_then(|labels| { - let labels_gen_var = quote! { labels }; let variant_labels = labels.0.iter().filter_map(|label| { let Label { span, label, ty, lbl_ty } = label; if *lbl_ty == LabelType::Collection { @@ -282,8 +280,8 @@ impl Labels { )) }) }); - let collections = labels.0.iter().filter_map(|label| { - let Label { span, label, ty, lbl_ty } = label; + let collections_chain = labels.0.iter().filter_map(|label| { + let Label { span, label, ty: _, lbl_ty } = label; if *lbl_ty != LabelType::Collection { return None; } @@ -300,18 +298,17 @@ impl Labels { quote! { std::option::Option::None } }; Some(quote! { - let display = #display; - #labels_gen_var.extend(#field.iter().map(|label| { - miette::macro_helpers::OptionalWrapper::<#ty>::new().to_option(label) - .map(|span| { - use miette::macro_helpers::{ToLabelSpanWrapper,ToLabeledSpan}; - let mut labeled_span = ToLabelSpanWrapper::to_labeled_span(span.clone()); - if #display.is_some() && labeled_span.label().is_none() { - labeled_span.set_label(#display) - } - labeled_span - }) - })); + .chain({ + let display = #display; + #field.iter().map(move |span| { + use miette::macro_helpers::{ToLabelSpanWrapper,ToLabeledSpan}; + let mut labeled_span = ToLabelSpanWrapper::to_labeled_span(span.clone()); + if display.is_some() && labeled_span.label().is_none() { + labeled_span.set_label(display.clone()); + } + Some(labeled_span) + }) + }) }) }); let variant_name = ident.clone(); @@ -320,11 +317,12 @@ impl Labels { _ => Some(quote! { Self::#variant_name #display_pat => { use miette::macro_helpers::ToOption; - let mut #labels_gen_var = vec![ + let labels_iter = vec![ #(#variant_labels),* - ]; - #(#collections)* - std::option::Option::Some(std::boxed::Box::new(#labels_gen_var.into_iter().filter(Option::is_some).map(Option::unwrap))) + ] + .into_iter() + #(#collections_chain)*; + std::option::Option::Some(std::boxed::Box::new(labels_iter.filter(Option::is_some).map(Option::unwrap))) } }), } diff --git a/src/lib.rs b/src/lib.rs index 7674f2be..7891511f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -48,6 +48,7 @@ //! - [... handler options](#-handler-options) //! - [... dynamic diagnostics](#-dynamic-diagnostics) //! - [... syntax highlighting](#-syntax-highlighting) +//! - [... collection of labels](#-collection-of-labels) //! - [Acknowledgements](#acknowledgements) //! - [License](#license) //! @@ -672,6 +673,57 @@ //! [`with_syntax_highlighting`](MietteHandlerOpts::with_syntax_highlighting) //! method. See the [`highlighters`] module docs for more details. //! +//! ### ... collection of labels +//! +//! When the number of labels is unknown, you can use a collection of `SourceSpan` +//! (or any type convertible into `SourceSpan`). For this, add the `collection` +//! parameter to `label` and use any type than can be iterated over for the field. +//! +//! ```rust,ignore +//! #[derive(Debug, Diagnostic, Error)] +//! #[error("oops!")] +//! struct MyError { +//! #[label("main issue")] +//! primary_span: SourceSpan, +//! +//! #[label(collection, "related to this")] +//! other_spans: Vec>, +//! } +//! +//! let report: miette::Report = MyError { +//! primary_span: (6, 9).into(), +//! other_spans: vec![19..26, 30..41], +//! }.into(); +//! +//! println!("{:?}", report.with_source_code("About something or another or yet another ...".to_string())); +//! ``` +//! +//! A collection can also be of `LabeledSpan` if you want to have different text +//! for different labels. Labels with no text will use the one from the `label` +//! attribute +//! +//! ```rust,ignore +//! #[derive(Debug, Diagnostic, Error)] +//! #[error("oops!")] +//! struct MyError { +//! #[label("main issue")] +//! primary_span: SourceSpan, +//! +//! #[label(collection, "related to this")] +//! other_spans: Vec, // LabeledSpan +//! } +//! +//! let report: miette::Report = MyError { +//! primary_span: (6, 9).into(), +//! other_spans: vec![ +//! LabeledSpan::new(None, 19, 7), // Use default text `related to this` +//! LabeledSpan::new(Some("and also this".to_string()), 30, 11), // Use specific text +//! ], +//! }.into(); +//! +//! println!("{:?}", report.with_source_code("About something or another or yet another ...".to_string())); +//! ``` +//! //! ## MSRV //! //! This crate requires rustc 1.70.0 or later.