From d4f30ec50163ead1b3a35623b9b3db65f86ac625 Mon Sep 17 00:00:00 2001 From: Lucas Holten Date: Wed, 13 Dec 2023 12:31:57 +0100 Subject: [PATCH 1/7] Clean up render_snippets --- src/handlers/graphical.rs | 113 +++++++++++++++++++------------------- 1 file changed, 55 insertions(+), 58 deletions(-) diff --git a/src/handlers/graphical.rs b/src/handlers/graphical.rs index 31934724..e9e84b84 100644 --- a/src/handlers/graphical.rs +++ b/src/handlers/graphical.rs @@ -6,7 +6,7 @@ use unicode_width::UnicodeWidthChar; use crate::diagnostic_chain::{DiagnosticChain, ErrorKind}; use crate::handlers::theme::*; use crate::protocol::{Diagnostic, Severity}; -use crate::{LabeledSpan, MietteError, ReportHandler, SourceCode, SourceSpan, SpanContents}; +use crate::{LabeledSpan, ReportHandler, SourceCode, SourceSpan, SpanContents}; /** A [`ReportHandler`] that displays a given [`Report`](crate::Report) in a @@ -377,66 +377,63 @@ impl GraphicalReportHandler { diagnostic: &(dyn Diagnostic), opt_source: Option<&dyn SourceCode>, ) -> fmt::Result { - if let Some(source) = opt_source { - if let Some(labels) = diagnostic.labels() { - let mut labels = labels.collect::>(); - labels.sort_unstable_by_key(|l| l.inner().offset()); - if !labels.is_empty() { - let contents = labels - .iter() - .map(|label| { - source.read_span(label.inner(), self.context_lines, self.context_lines) - }) - .collect::>>, MietteError>>() - .map_err(|_| fmt::Error)?; - let mut contexts = Vec::with_capacity(contents.len()); - for (right, right_conts) in labels.iter().cloned().zip(contents.iter()) { - if contexts.is_empty() { - contexts.push((right, right_conts)); - } else { - let (left, left_conts) = contexts.last().unwrap().clone(); - let left_end = left.offset() + left.len(); - let right_end = right.offset() + right.len(); - if left_conts.line() + left_conts.line_count() >= right_conts.line() { - // The snippets will overlap, so we create one Big Chunky Boi - let new_span = LabeledSpan::new( - left.label().map(String::from), - left.offset(), - if right_end >= left_end { - // Right end goes past left end - right_end - left.offset() - } else { - // right is contained inside left - left.len() - }, - ); - if source - .read_span( - new_span.inner(), - self.context_lines, - self.context_lines, - ) - .is_ok() - { - contexts.pop(); - contexts.push(( - // We'll throw this away later - new_span, left_conts, - )); - } else { - contexts.push((right, right_conts)); - } - } else { - contexts.push((right, right_conts)); - } - } - } - for (ctx, _) in contexts { - self.render_context(f, source, &ctx, &labels[..])?; - } + let Some(source) = opt_source else { + return Ok(()); + }; + let Some(labels) = diagnostic.labels() else { + return Ok(()); + }; + + let mut labels = labels.collect::>(); + labels.sort_unstable_by_key(|l| l.inner().offset()); + + let mut contexts = Vec::with_capacity(labels.len()); + for right in labels.iter().cloned() { + let right_conts = source + .read_span(right.inner(), self.context_lines, self.context_lines) + .map_err(|_| fmt::Error)?; + + if contexts.is_empty() { + contexts.push((right, (right_conts.line(), right_conts.line_count()))); + continue; + } + + let (left, (left_line, left_line_count)) = contexts.last().unwrap().clone(); + if left_line + left_line_count >= right_conts.line() { + // The snippets will overlap, so we create one Big Chunky Boi + let left_end = left.offset() + left.len(); + let right_end = right.offset() + right.len(); + let new_span = LabeledSpan::new( + left.label().map(String::from), + left.offset(), + if right_end >= left_end { + // Right end goes past left end + right_end - left.offset() + } else { + // right is contained inside left + left.len() + }, + ); + // Check that the two contexts can be combined + if let Ok(new_conts) = + source.read_span(new_span.inner(), self.context_lines, self.context_lines) + { + contexts.pop(); + contexts.push(( + // We'll throw this away later + new_span, + (new_conts.line(), new_conts.line_count()), + )); + continue; } } + + contexts.push((right, (right_conts.line(), right_conts.line_count()))); } + for (ctx, _) in contexts { + self.render_context(f, source, &ctx, &labels[..])?; + } + Ok(()) } From 997a3ea7820cf7b19a98ce1a4641d25302da5a05 Mon Sep 17 00:00:00 2001 From: Lucas Holten Date: Wed, 13 Dec 2023 12:55:05 +0100 Subject: [PATCH 2/7] Choose primary label from inside the context --- src/handlers/graphical.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/handlers/graphical.rs b/src/handlers/graphical.rs index e9e84b84..9bba215c 100644 --- a/src/handlers/graphical.rs +++ b/src/handlers/graphical.rs @@ -446,10 +446,16 @@ impl GraphicalReportHandler { ) -> fmt::Result { let (contents, lines) = self.get_lines(source, context.inner())?; - let primary_label = labels - .iter() + // only consider labels from the context as primary label + let ctx_labels = labels.iter().filter(|l| { + context.inner().offset() <= l.inner().offset() + && l.inner().offset() + l.inner().len() + <= context.inner().offset() + context.inner().len() + }); + let primary_label = ctx_labels + .clone() .find(|label| label.primary()) - .or_else(|| labels.first()); + .or_else(|| ctx_labels.clone().next()); // sorting is your friend let labels = labels From 0bf3daebc236f0d736ec429e732a8c486bc05f80 Mon Sep 17 00:00:00 2001 From: Lucas Holten Date: Thu, 11 Jan 2024 10:11:42 +0100 Subject: [PATCH 3/7] Revert to passing around SpanContents --- src/handlers/graphical.rs | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/handlers/graphical.rs b/src/handlers/graphical.rs index 9bba215c..a1d06169 100644 --- a/src/handlers/graphical.rs +++ b/src/handlers/graphical.rs @@ -377,11 +377,13 @@ impl GraphicalReportHandler { diagnostic: &(dyn Diagnostic), opt_source: Option<&dyn SourceCode>, ) -> fmt::Result { - let Some(source) = opt_source else { - return Ok(()); + let source = match opt_source { + Some(source) => source, + None => return Ok(()), }; - let Some(labels) = diagnostic.labels() else { - return Ok(()); + let labels = match diagnostic.labels() { + Some(labels) => labels, + None => return Ok(()), }; let mut labels = labels.collect::>(); @@ -394,12 +396,12 @@ impl GraphicalReportHandler { .map_err(|_| fmt::Error)?; if contexts.is_empty() { - contexts.push((right, (right_conts.line(), right_conts.line_count()))); + contexts.push((right, right_conts)); continue; } - let (left, (left_line, left_line_count)) = contexts.last().unwrap().clone(); - if left_line + left_line_count >= right_conts.line() { + let (left, left_conts) = contexts.last().unwrap(); + if left_conts.line() + left_conts.line_count() >= right_conts.line() { // The snippets will overlap, so we create one Big Chunky Boi let left_end = left.offset() + left.len(); let right_end = right.offset() + right.len(); @@ -419,16 +421,13 @@ impl GraphicalReportHandler { source.read_span(new_span.inner(), self.context_lines, self.context_lines) { contexts.pop(); - contexts.push(( - // We'll throw this away later - new_span, - (new_conts.line(), new_conts.line_count()), - )); + // We'll throw the contents away later + contexts.push((new_span, new_conts)); continue; } } - contexts.push((right, (right_conts.line(), right_conts.line_count()))); + contexts.push((right, right_conts)); } for (ctx, _) in contexts { self.render_context(f, source, &ctx, &labels[..])?; From 36f7f65f8795ed8c2a363c4233a21158929f2f41 Mon Sep 17 00:00:00 2001 From: Lucas Holten Date: Thu, 11 Jan 2024 11:28:32 +0100 Subject: [PATCH 4/7] Add test for triple label context --- tests/graphical.rs | 47 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/tests/graphical.rs b/tests/graphical.rs index aabf167f..14178bf9 100644 --- a/tests/graphical.rs +++ b/tests/graphical.rs @@ -1713,3 +1713,50 @@ fn single_line_with_wide_char_unaligned_span_empty() -> Result<(), MietteError> assert_eq!(expected, out); Ok(()) } + +#[test] +fn triple_adjacent_highlight() -> Result<(), MietteError> { + #[derive(Debug, Diagnostic, Error)] + #[error("oops!")] + #[diagnostic(code(oops::my::bad), help("try doing it better next time?"))] + struct MyBad { + #[source_code] + src: NamedSource, + #[label = "this bit here"] + highlight1: SourceSpan, + #[label = "also this bit"] + highlight2: SourceSpan, + #[label = "finally we got"] + highlight3: SourceSpan, + } + + let src = "source\n text\n here".to_string(); + let err = MyBad { + src: NamedSource::new("bad_file.rs", src), + highlight1: (0, 6).into(), + highlight2: (9, 4).into(), + highlight3: (18, 4).into(), + }; + let out = fmt_report(err.into()); + println!("Error: {}", out); + let expected = "oops::my::bad + + × oops! + ╭─[bad_file.rs:1:1] + 1 │ source + · ───┬── + · ╰── this bit here + 2 │ text + · ──┬─ + · ╰── also this bit + 3 │ here + · ──┬─ + · ╰── finally we got + ╰──── + help: try doing it better next time? +" + .trim_start() + .to_string(); + assert_eq!(expected, out); + Ok(()) +} From b1e2bd2563d63d2b212f93f411f085c899aa8c27 Mon Sep 17 00:00:00 2001 From: Lucas Holten Date: Thu, 11 Jan 2024 11:34:25 +0100 Subject: [PATCH 5/7] use std::cmp::max instead of manual calculation --- src/handlers/graphical.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/handlers/graphical.rs b/src/handlers/graphical.rs index a1d06169..f742af0c 100644 --- a/src/handlers/graphical.rs +++ b/src/handlers/graphical.rs @@ -405,16 +405,12 @@ impl GraphicalReportHandler { // The snippets will overlap, so we create one Big Chunky Boi let left_end = left.offset() + left.len(); let right_end = right.offset() + right.len(); + let new_end = std::cmp::max(left_end, right_end); + let new_span = LabeledSpan::new( left.label().map(String::from), left.offset(), - if right_end >= left_end { - // Right end goes past left end - right_end - left.offset() - } else { - // right is contained inside left - left.len() - }, + new_end - left.offset(), ); // Check that the two contexts can be combined if let Ok(new_conts) = From 5fda9b0e5ba06a7b95f4a5e936f114df1ad2e7ea Mon Sep 17 00:00:00 2001 From: Lucas Holten Date: Thu, 11 Jan 2024 13:08:55 +0100 Subject: [PATCH 6/7] Update test so that it finds the bug. The previous test would still have overlap between the first and last context because of contexts are extended with `context_lines`. --- tests/graphical.rs | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/tests/graphical.rs b/tests/graphical.rs index 14178bf9..5114d43a 100644 --- a/tests/graphical.rs +++ b/tests/graphical.rs @@ -1730,12 +1730,12 @@ fn triple_adjacent_highlight() -> Result<(), MietteError> { highlight3: SourceSpan, } - let src = "source\n text\n here".to_string(); + let src = "source\n\n\n text\n\n\n here".to_string(); let err = MyBad { src: NamedSource::new("bad_file.rs", src), highlight1: (0, 6).into(), - highlight2: (9, 4).into(), - highlight3: (18, 4).into(), + highlight2: (11, 4).into(), + highlight3: (22, 4).into(), }; let out = fmt_report(err.into()); println!("Error: {}", out); @@ -1746,17 +1746,19 @@ fn triple_adjacent_highlight() -> Result<(), MietteError> { 1 │ source · ───┬── · ╰── this bit here - 2 │ text + 2 │ + 3 │ + 4 │ text · ──┬─ · ╰── also this bit - 3 │ here + 5 │ + 6 │ + 7 │ here · ──┬─ · ╰── finally we got ╰──── help: try doing it better next time? -" - .trim_start() - .to_string(); - assert_eq!(expected, out); +"; + assert_eq!(expected, &out); Ok(()) } From e16326ac58f5be7f36df5f8f68fdf4c9c20e436f Mon Sep 17 00:00:00 2001 From: Lucas Holten Date: Thu, 11 Jan 2024 13:12:44 +0100 Subject: [PATCH 7/7] Add test for non adjacent contexts --- tests/graphical.rs | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/tests/graphical.rs b/tests/graphical.rs index 5114d43a..7e2d19ae 100644 --- a/tests/graphical.rs +++ b/tests/graphical.rs @@ -1762,3 +1762,46 @@ fn triple_adjacent_highlight() -> Result<(), MietteError> { assert_eq!(expected, &out); Ok(()) } + +#[test] +fn non_adjacent_highlight() -> Result<(), MietteError> { + #[derive(Debug, Diagnostic, Error)] + #[error("oops!")] + #[diagnostic(code(oops::my::bad), help("try doing it better next time?"))] + struct MyBad { + #[source_code] + src: NamedSource, + #[label = "this bit here"] + highlight1: SourceSpan, + #[label = "also this bit"] + highlight2: SourceSpan, + } + + let src = "source\n\n\n\n text here".to_string(); + let err = MyBad { + src: NamedSource::new("bad_file.rs", src), + highlight1: (0, 6).into(), + highlight2: (12, 4).into(), + }; + let out = fmt_report(err.into()); + println!("Error: {}", out); + let expected = "oops::my::bad + + × oops! + ╭─[bad_file.rs:1:1] + 1 │ source + · ───┬── + · ╰── this bit here + 2 │ + ╰──── + ╭─[bad_file.rs:5:3] + 4 │ + 5 │ text here + · ──┬─ + · ╰── also this bit + ╰──── + help: try doing it better next time? +"; + assert_eq!(expected, &out); + Ok(()) +}