From 6f8fbf836d33f51e4f02cde19dc25d85a6ae4503 Mon Sep 17 00:00:00 2001 From: Nahor Date: Mon, 4 Mar 2024 13:03:02 -0800 Subject: [PATCH 1/6] refactor(get_lines): Simplify `get_lines` --- src/handlers/graphical.rs | 68 ++++++++++++----------------------- src/handlers/narratable.rs | 72 ++++++++++++++------------------------ 2 files changed, 48 insertions(+), 92 deletions(-) diff --git a/src/handlers/graphical.rs b/src/handlers/graphical.rs index a4474f90..1eb915e4 100644 --- a/src/handlers/graphical.rs +++ b/src/handlers/graphical.rs @@ -1189,54 +1189,30 @@ impl GraphicalReportHandler { .read_span(context_span, self.context_lines, self.context_lines) .map_err(|_| fmt::Error)?; let context = std::str::from_utf8(context_data.data()).expect("Bad utf8 detected"); - let mut line = context_data.line(); - let mut column = context_data.column(); - let mut offset = context_data.span().offset(); - let mut line_offset = offset; - let mut iter = context.chars().peekable(); - let mut line_str = String::new(); - let mut lines = Vec::new(); - while let Some(char) = iter.next() { - offset += char.len_utf8(); - let mut at_end_of_file = false; - match char { - '\r' => { - if iter.next_if_eq(&'\n').is_some() { - offset += 1; - line += 1; - column = 0; - } else { - line_str.push(char); - column += 1; - } - at_end_of_file = iter.peek().is_none(); - } - '\n' => { - at_end_of_file = iter.peek().is_none(); - line += 1; - column = 0; + let lines = context + .split_inclusive('\n') + .enumerate() + .map(|(line_number, line)| { + let length = line.len(); + // Strip the newline chars + let line = line + .strip_suffix('\n') + .and_then(|line| line.strip_suffix('\r').or(Some(line))) + .unwrap_or(line); + // SAFETY: + // - it is safe to use `offset_from` on slices of an array per Rus design (max array size) + // (https://doc.rust-lang.org/stable/reference/types/numeric.html#machine-dependent-integer-types) + // - since `line` is a slice of `context`, the offset cannot be negative either + Line { + line_number: context_data.line() + line_number + 1, + offset: context_data.span().offset() + + unsafe { line.as_ptr().offset_from(context.as_ptr()) } as usize, + length, + text: line.to_string(), } - _ => { - line_str.push(char); - column += 1; - } - } - - if iter.peek().is_none() && !at_end_of_file { - line += 1; - } + }) + .collect::>(); - if column == 0 || iter.peek().is_none() { - lines.push(Line { - line_number: line, - offset: line_offset, - length: offset - line_offset, - text: line_str.clone(), - }); - line_str.clear(); - line_offset = offset; - } - } Ok((context_data, lines)) } } diff --git a/src/handlers/narratable.rs b/src/handlers/narratable.rs index c8091249..418c899b 100644 --- a/src/handlers/narratable.rs +++ b/src/handlers/narratable.rs @@ -291,54 +291,34 @@ impl NarratableReportHandler { .read_span(context_span, self.context_lines, self.context_lines) .map_err(|_| fmt::Error)?; let context = std::str::from_utf8(context_data.data()).expect("Bad utf8 detected"); - let mut line = context_data.line(); - let mut column = context_data.column(); - let mut offset = context_data.span().offset(); - let mut line_offset = offset; - let mut iter = context.chars().peekable(); - let mut line_str = String::new(); - let mut lines = Vec::new(); - while let Some(char) = iter.next() { - offset += char.len_utf8(); - let mut at_end_of_file = false; - match char { - '\r' => { - if iter.next_if_eq(&'\n').is_some() { - offset += 1; - line += 1; - column = 0; - } else { - line_str.push(char); - column += 1; - } - at_end_of_file = iter.peek().is_none(); - } - '\n' => { - at_end_of_file = iter.peek().is_none(); - line += 1; - column = 0; - } - _ => { - line_str.push(char); - column += 1; - } - } - if iter.peek().is_none() && !at_end_of_file { - line += 1; - } - - if column == 0 || iter.peek().is_none() { - lines.push(Line { - line_number: line, - offset: line_offset, - text: line_str.clone(), + let lines = context + .split_inclusive('\n') + .enumerate() + .map(|(line_number, line)| { + // SAFETY: + // - it is safe to use `offset_from` on slices of an array per Rus design (max array size) + // (https://doc.rust-lang.org/stable/reference/types/numeric.html#machine-dependent-integer-types) + // - since `line` is a slice of `context`, the offset cannot be negative either + let offset = unsafe { line.as_ptr().offset_from(context.as_ptr()) } as usize; + let length = line.len(); + // Strip the newline chars + let line = line + .strip_suffix('\n') + .and_then(|line| line.strip_suffix('\r').or(Some(line))) + .unwrap_or(line); + // End of the "file" if the end of the line is also the end of + // the context and we removed some characters (newline) + let at_end_of_file = (offset + length == context.len()) && (length != line.len()); + Line { + line_number: context_data.line() + line_number + 1, + offset: context_data.span().offset() + offset, + text: line.to_string(), at_end_of_file, - }); - line_str.clear(); - line_offset = offset; - } - } + } + }) + .collect::>(); + Ok((context_data, lines)) } } From a37328a29982d497919486817e89acddd1a155f0 Mon Sep 17 00:00:00 2001 From: Nahor Date: Mon, 4 Mar 2024 13:49:15 -0800 Subject: [PATCH 2/6] refactor(span): Simplify checks how a span applies on a line --- src/handlers/graphical.rs | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/src/handlers/graphical.rs b/src/handlers/graphical.rs index 1eb915e4..f3561de8 100644 --- a/src/handlers/graphical.rs +++ b/src/handlers/graphical.rs @@ -1257,28 +1257,27 @@ impl Line { /// Returns whether `span` should be visible on this line, either in the gutter or under the /// text on this line fn span_applies(&self, span: &FancySpan) -> bool { - let spanlen = if span.len() == 0 { 1 } else { span.len() }; - // Span starts in this line - - (span.offset() >= self.offset && span.offset() < self.offset + self.length) - // Span passes through this line - || (span.offset() < self.offset && span.offset() + spanlen > self.offset + self.length) //todo - // Span ends on this line - || (span.offset() + spanlen > self.offset && span.offset() + spanlen <= self.offset + self.length) + // A span applies if its start is strictly before the line's end, + // i.e. the span is not after the line, and its end is strictly after + // the line's start, i.e. the span is not before the line. + // + // One corner case: if the span length is 0, then the span also applies + // if its end is *at* the line's start, not just strictly after. + (span.offset() < self.offset + self.length) + && match span.len() == 0 { + true => (span.offset() + span.len()) >= self.offset, + false => (span.offset() + span.len()) > self.offset, + } } /// Returns whether `span` should be visible on this line in the gutter (so this excludes spans /// that are only visible on this line and do not span multiple lines) fn span_applies_gutter(&self, span: &FancySpan) -> bool { - let spanlen = if span.len() == 0 { 1 } else { span.len() }; - // Span starts in this line + // The span must covers this line and at least one of its ends must be + // on another line self.span_applies(span) - && !( - // as long as it doesn't start *and* end on this line - (span.offset() >= self.offset && span.offset() < self.offset + self.length) - && (span.offset() + spanlen > self.offset - && span.offset() + spanlen <= self.offset + self.length) - ) + && ((span.offset() < self.offset) + || ((span.offset() + span.len()) >= (self.offset + self.length))) } // A 'flyby' is a multi-line span that technically covers this line, but @@ -1301,8 +1300,7 @@ impl Line { // Does this line contain the *end* of this multiline span? // This assumes self.span_applies() is true already. fn span_ends(&self, span: &FancySpan) -> bool { - span.offset() + span.len() >= self.offset - && span.offset() + span.len() <= self.offset + self.length + span.offset() + span.len() <= self.offset + self.length } } From f7fa50d8de219d21d479d0658922e10fea905b93 Mon Sep 17 00:00:00 2001 From: Nahor Date: Sun, 3 Mar 2024 12:02:41 -0800 Subject: [PATCH 3/6] fix(SourceCode): Fix and improve default `SourceCode` implementation - Fix a panic when a 0 length span covers the end of a document - Fix incorrect `line_count` - Add new unit tests and complete existing ones - Improve readability --- src/source_impls.rs | 257 +++++++++++++++++++++++++++++++------------- 1 file changed, 185 insertions(+), 72 deletions(-) diff --git a/src/source_impls.rs b/src/source_impls.rs index e362b4a5..61a7ffb3 100644 --- a/src/source_impls.rs +++ b/src/source_impls.rs @@ -5,91 +5,103 @@ use std::{borrow::Cow, collections::VecDeque, fmt::Debug, sync::Arc}; use crate::{MietteError, MietteSpanContents, SourceCode, SourceSpan, SpanContents}; +#[derive(Clone, Copy)] +struct LineInfo { + line_no: usize, + start: usize, + end: usize, +} + fn context_info<'a>( input: &'a [u8], span: &SourceSpan, context_lines_before: usize, context_lines_after: usize, ) -> Result, MietteError> { - let mut offset = 0usize; - let mut line_count = 0usize; - let mut start_line = 0usize; - let mut start_column = 0usize; - let mut before_lines_starts = VecDeque::new(); - let mut current_line_start = 0usize; - let mut end_lines = 0usize; - let mut post_span = false; - let mut post_span_got_newline = false; - let mut iter = input.iter().copied().peekable(); - while let Some(char) = iter.next() { - if matches!(char, b'\r' | b'\n') { - line_count += 1; - if char == b'\r' && iter.next_if_eq(&b'\n').is_some() { - offset += 1; + let mut iter = input + .split_inclusive(|b| *b == b'\n') + .enumerate() + .map(|(line_no, line)| { + // SAFETY: + // - it is safe to use `offset_from` on slices of an array per Rust design (max array size) + // (https://doc.rust-lang.org/stable/reference/types/numeric.html#machine-dependent-integer-types) + // - since `line` is a slice of `input, the offset cannot be negative either + let offset = unsafe { line.as_ptr().offset_from(input.as_ptr()) } as usize; + LineInfo { + line_no, + start: offset, + end: offset + line.len(), } - if offset < span.offset() { - // We're before the start of the span. - start_column = 0; - before_lines_starts.push_back(current_line_start); - if before_lines_starts.len() > context_lines_before { - start_line += 1; - before_lines_starts.pop_front(); - } - } else if offset >= span.offset() + span.len().saturating_sub(1) { - // We're after the end of the span, but haven't necessarily - // started collecting end lines yet (we might still be - // collecting context lines). - if post_span { - start_column = 0; - if post_span_got_newline { - end_lines += 1; - } else { - post_span_got_newline = true; - } - if end_lines >= context_lines_after { - offset += 1; - break; - } - } - } - current_line_start = offset + 1; - } else if offset < span.offset() { - start_column += 1; - } + }); - if offset >= (span.offset() + span.len()).saturating_sub(1) { - post_span = true; - if end_lines >= context_lines_after { - offset += 1; - break; - } + // First line handled separately because otherwise, we can't distinguish + // between `None` from an empty document (which we still want to handle as + // a "single empty line"), and `None` from the end of the document. + let mut line_starts = VecDeque::new(); + let mut line_info = match iter.next() { + None => LineInfo { + line_no: 0, + start: 0, + end: 0, + }, + Some(info) => info, + }; + line_starts.push_back(line_info); + + // Get the "before" lines (including the line containing the start + // of the span) + while span.offset() >= line_info.end { + line_info = match iter.next() { + None => break, + Some(info) => info, + }; + + if line_starts.len() > context_lines_before { + line_starts.pop_front(); } + line_starts.push_back(line_info); + } + let (start_lineno, start_offset, start_column) = { + let start_info = line_starts.pop_front().unwrap(); + if context_lines_before > 0 { + (start_info.line_no, start_info.start, 0) + } else { + ( + start_info.line_no, + span.offset(), + span.offset() - start_info.start, + ) + } + }; - offset += 1; + // Find the end of the span + while span.offset() + span.len() > line_info.end { + line_info = match iter.next() { + None => break, + Some(info) => info, + }; } - if offset >= (span.offset() + span.len()).saturating_sub(1) { - let starting_offset = before_lines_starts.front().copied().unwrap_or_else(|| { - if context_lines_before == 0 { - span.offset() - } else { - 0 - } - }); - Ok(MietteSpanContents::new( - &input[starting_offset..offset], - (starting_offset, offset - starting_offset).into(), - start_line, - if context_lines_before == 0 { - start_column - } else { - 0 - }, - line_count, - )) - } else { - Err(MietteError::OutOfBounds) + // Get the "after" lines + if let Some(last) = iter.take(context_lines_after).last() { + line_info = last; + } + if span.offset() + span.len() > line_info.end { + return Err(MietteError::OutOfBounds); } + let (end_lineno, end_offset) = if context_lines_after > 0 { + (line_info.line_no, line_info.end) + } else { + (line_info.line_no, span.offset() + span.len()) + }; + + Ok(MietteSpanContents::new( + &input[start_offset..end_offset], + (start_offset..end_offset).into(), + start_lineno, + start_column, + end_lineno - start_lineno + 1, + )) } impl SourceCode for [u8] { @@ -206,8 +218,10 @@ mod tests { let src = String::from("foo\n"); let contents = src.read_span(&(0, 4).into(), 0, 0)?; assert_eq!("foo\n", std::str::from_utf8(contents.data()).unwrap()); + assert_eq!(SourceSpan::from((0, 4)), *contents.span()); assert_eq!(0, contents.line()); assert_eq!(0, contents.column()); + assert_eq!(1, contents.line_count()); Ok(()) } @@ -216,8 +230,10 @@ mod tests { let src = String::from("foobar"); let contents = src.read_span(&(3, 3).into(), 1, 1)?; assert_eq!("foobar", std::str::from_utf8(contents.data()).unwrap()); + assert_eq!(SourceSpan::from((0, 6)), *contents.span()); assert_eq!(0, contents.line()); assert_eq!(0, contents.column()); + assert_eq!(1, contents.line_count()); Ok(()) } @@ -226,8 +242,10 @@ mod tests { let src = String::from("foo\nbar\nbaz\n"); let contents = src.read_span(&(4, 4).into(), 0, 0)?; assert_eq!("bar\n", std::str::from_utf8(contents.data()).unwrap()); + assert_eq!(SourceSpan::from((4, 4)), *contents.span()); assert_eq!(1, contents.line()); assert_eq!(0, contents.column()); + assert_eq!(1, contents.line_count()); Ok(()) } @@ -236,8 +254,58 @@ mod tests { let src = String::from("foo\nbarbar\nbaz\n"); let contents = src.read_span(&(7, 4).into(), 0, 0)?; assert_eq!("bar\n", std::str::from_utf8(contents.data()).unwrap()); + assert_eq!(SourceSpan::from((7, 4)), *contents.span()); + assert_eq!(1, contents.line()); + assert_eq!(3, contents.column()); + assert_eq!(1, contents.line_count()); + Ok(()) + } + + #[test] + fn end_of_line_before_newline() -> Result<(), MietteError> { + let src = String::from("foo\nbar\nbaz\n"); + let contents = src.read_span(&(7, 0).into(), 0, 0)?; + assert_eq!("", std::str::from_utf8(contents.data()).unwrap()); + assert_eq!(SourceSpan::from((7, 0)), *contents.span()); assert_eq!(1, contents.line()); assert_eq!(3, contents.column()); + assert_eq!(1, contents.line_count()); + Ok(()) + } + + #[test] + fn end_of_line_after_newline() -> Result<(), MietteError> { + let src = String::from("foo\nbar\nbaz\n"); + let contents = src.read_span(&(8, 0).into(), 0, 0)?; + assert_eq!("", std::str::from_utf8(contents.data()).unwrap()); + assert_eq!(SourceSpan::from((8, 0)), *contents.span()); + assert_eq!(2, contents.line()); + assert_eq!(0, contents.column()); + assert_eq!(1, contents.line_count()); + Ok(()) + } + + #[test] + fn end_of_file_with_newline() -> Result<(), MietteError> { + let src = String::from("foo\nbar\nbaz\n"); + let contents = src.read_span(&(12, 0).into(), 0, 0)?; + assert_eq!("", std::str::from_utf8(contents.data()).unwrap()); + assert_eq!(SourceSpan::from((12, 0)), *contents.span()); + assert_eq!(2, contents.line()); + assert_eq!(4, contents.column()); + assert_eq!(1, contents.line_count()); + Ok(()) + } + + #[test] + fn end_of_file_without_newline() -> Result<(), MietteError> { + let src = String::from("foo\nbar\nbaz"); + let contents = src.read_span(&(11, 0).into(), 0, 0)?; + assert_eq!("", std::str::from_utf8(contents.data()).unwrap()); + assert_eq!(SourceSpan::from((11, 0)), *contents.span()); + assert_eq!(2, contents.line()); + assert_eq!(3, contents.column()); + assert_eq!(1, contents.line_count()); Ok(()) } @@ -246,8 +314,10 @@ mod tests { let src = String::from("foo\r\nbar\r\nbaz\r\n"); let contents = src.read_span(&(5, 5).into(), 0, 0)?; assert_eq!("bar\r\n", std::str::from_utf8(contents.data()).unwrap()); + assert_eq!(SourceSpan::from((5, 5)), *contents.span()); assert_eq!(1, contents.line()); assert_eq!(0, contents.column()); + assert_eq!(1, contents.line_count()); Ok(()) } @@ -259,8 +329,10 @@ mod tests { "foo\nbar\nbaz\n", std::str::from_utf8(contents.data()).unwrap() ); + assert_eq!(SourceSpan::from((4, 12)), *contents.span()); assert_eq!(1, contents.line()); assert_eq!(0, contents.column()); + assert_eq!(3, contents.line_count()); Ok(()) } @@ -272,8 +344,10 @@ mod tests { "\nfoo\nbar\nbaz\n\n", std::str::from_utf8(contents.data()).unwrap() ); + assert_eq!(SourceSpan::from((8, 14)), *contents.span()); assert_eq!(2, contents.line()); assert_eq!(0, contents.column()); + assert_eq!(5, contents.line_count()); let span: SourceSpan = (8, 14).into(); assert_eq!(&span, contents.span()); Ok(()) @@ -287,10 +361,49 @@ mod tests { "one\ntwo\n\n", std::str::from_utf8(contents.data()).unwrap() ); + assert_eq!(SourceSpan::from((0, 9)), *contents.span()); assert_eq!(0, contents.line()); assert_eq!(0, contents.column()); let span: SourceSpan = (0, 9).into(); assert_eq!(&span, contents.span()); + assert_eq!(3, contents.line_count()); Ok(()) } + + #[test] + fn empty_source() -> Result<(), MietteError> { + let src = String::from(""); + + let contents = src.read_span(&(0, 0).into(), 0, 0)?; + assert_eq!("", std::str::from_utf8(contents.data()).unwrap()); + assert_eq!(SourceSpan::from((0, 0)), *contents.span()); + assert_eq!(0, contents.line()); + assert_eq!(0, contents.column()); + assert_eq!(1, contents.line_count()); + + Ok(()) + } + + #[test] + fn empty_source_out_of_bounds() { + let src = String::from(""); + + let contents = src.read_span(&(0, 1).into(), 0, 0); + assert!(matches!(contents, Err(MietteError::OutOfBounds))); + + let contents = src.read_span(&(0, 2).into(), 0, 0); + assert!(matches!(contents, Err(MietteError::OutOfBounds))); + + let contents = src.read_span(&(1, 0).into(), 0, 0); + assert!(matches!(contents, Err(MietteError::OutOfBounds))); + + let contents = src.read_span(&(1, 1).into(), 0, 0); + assert!(matches!(contents, Err(MietteError::OutOfBounds))); + + let contents = src.read_span(&(2, 0).into(), 0, 0); + assert!(matches!(contents, Err(MietteError::OutOfBounds))); + + let contents = src.read_span(&(2, 1).into(), 0, 0); + assert!(matches!(contents, Err(MietteError::OutOfBounds))); + } } From d63b2bb638371ebd036951b1c2b4e2fc0cff676e Mon Sep 17 00:00:00 2001 From: Nahor Date: Mon, 11 Mar 2024 12:15:31 -0700 Subject: [PATCH 4/6] fix(zero length, no context): Add Option to distinguish between "current line" and "just the span" Fixes #355 Change the number of context lines from usize to Option to allow choosing between "just the span" (as implemented previously when using 0 context line) using `None`, and displaying the error line without extra context (not possible before) using `Some(0)`. --- src/eyreish/wrapper.rs | 2 +- src/handlers/graphical.rs | 27 +++- src/handlers/json.rs | 2 +- src/handlers/narratable.rs | 21 +++- src/named_source.rs | 4 +- src/protocol.rs | 9 +- src/source_impls.rs | 82 ++++++------ tests/graphical.rs | 251 +++++++++++++++++++++++++++++++++++++ tests/test_boxed.rs | 2 +- 9 files changed, 343 insertions(+), 57 deletions(-) diff --git a/src/eyreish/wrapper.rs b/src/eyreish/wrapper.rs index 6e65eb72..01f64f09 100644 --- a/src/eyreish/wrapper.rs +++ b/src/eyreish/wrapper.rs @@ -283,7 +283,7 @@ mod tests { report .source_code() .unwrap() - .read_span(&(0..5).into(), 0, 0) + .read_span(&(0..5).into(), None, None) .unwrap() .data() .to_vec(), diff --git a/src/handlers/graphical.rs b/src/handlers/graphical.rs index f3561de8..d4644027 100644 --- a/src/handlers/graphical.rs +++ b/src/handlers/graphical.rs @@ -28,7 +28,7 @@ pub struct GraphicalReportHandler { pub(crate) termwidth: usize, pub(crate) theme: GraphicalTheme, pub(crate) footer: Option, - pub(crate) context_lines: usize, + pub(crate) context_lines: Option, pub(crate) tab_width: usize, pub(crate) with_cause_chain: bool, pub(crate) wrap_lines: bool, @@ -55,7 +55,7 @@ impl GraphicalReportHandler { termwidth: 200, theme: GraphicalTheme::default(), footer: None, - context_lines: 1, + context_lines: Some(1), tab_width: 4, with_cause_chain: true, wrap_lines: true, @@ -74,7 +74,7 @@ impl GraphicalReportHandler { termwidth: 200, theme, footer: None, - context_lines: 1, + context_lines: Some(1), tab_width: 4, wrap_lines: true, with_cause_chain: true, @@ -159,7 +159,7 @@ impl GraphicalReportHandler { self } - /// Sets the word splitter to usewhen wrapping. + /// Sets the word splitter to use when wrapping. pub fn with_word_splitter(mut self, word_splitter: textwrap::WordSplitter) -> Self { self.word_splitter = Some(word_splitter); self @@ -172,7 +172,22 @@ impl GraphicalReportHandler { } /// Sets the number of lines of context to show around each error. - pub fn with_context_lines(mut self, lines: usize) -> Self { + /// + /// If `0`, then only the span content will be shown (equivalent to + /// `with_opt_context_lines(None)`).\ + /// Use `with_opt_context_lines(Some(0))` if you want the whole line + /// containing the error without extra context. + pub fn with_context_lines(self, lines: usize) -> Self { + self.with_opt_context_lines((lines != 0).then_some(lines)) + } + + /// Sets the number of lines of context to show around each error. + /// + /// `None` means only the span content (and possibly the content in between + /// multiple adjacent labels) will be shown.\ + /// `Some(0)` will show the whole line containing the label.\ + /// `Some(n)` will show the whole line plus n line before and after the label. + pub fn with_opt_context_lines(mut self, lines: Option) -> Self { self.context_lines = lines; self } @@ -559,7 +574,7 @@ impl GraphicalReportHandler { // as the reference point for line/column information. let primary_contents = match primary_label { Some(label) => source - .read_span(label.inner(), 0, 0) + .read_span(label.inner(), None, None) .map_err(|_| fmt::Error)?, None => contents, }; diff --git a/src/handlers/json.rs b/src/handlers/json.rs index 0b4a405b..1ae06d35 100644 --- a/src/handlers/json.rs +++ b/src/handlers/json.rs @@ -159,7 +159,7 @@ impl JSONReportHandler { ) -> fmt::Result { if let Some(mut labels) = diagnostic.labels() { if let Some(label) = labels.next() { - if let Ok(span_content) = source.read_span(label.inner(), 0, 0) { + if let Ok(span_content) = source.read_span(label.inner(), None, None) { let filename = span_content.name().unwrap_or_default(); return write!(f, r#""filename": "{}","#, escape(filename)); } diff --git a/src/handlers/narratable.rs b/src/handlers/narratable.rs index 418c899b..ca2c0d84 100644 --- a/src/handlers/narratable.rs +++ b/src/handlers/narratable.rs @@ -13,7 +13,7 @@ non-graphical environments, such as non-TTY output. */ #[derive(Debug, Clone)] pub struct NarratableReportHandler { - context_lines: usize, + context_lines: Option, with_cause_chain: bool, footer: Option, } @@ -24,7 +24,7 @@ impl NarratableReportHandler { pub const fn new() -> Self { Self { footer: None, - context_lines: 1, + context_lines: Some(1), with_cause_chain: true, } } @@ -49,7 +49,22 @@ impl NarratableReportHandler { } /// Sets the number of lines of context to show around each error. - pub const fn with_context_lines(mut self, lines: usize) -> Self { + /// + /// If `0`, then only the span content will be shown (equivalent to + /// `with_opt_context_lines(None)`).\ + /// Use `with_opt_context_lines(Some(0))` if you want the whole line + /// containing the error without extra context. + pub fn with_context_lines(self, lines: usize) -> Self { + self.with_opt_context_lines((lines != 0).then_some(lines)) + } + + /// Sets the number of lines of context to show around each error. + /// + /// `None` means only the span content (and possibly the content in between + /// multiple adjacent labels) will be shown.\ + /// `Some(0)` will show the whole line containing the label.\ + /// `Some(n)` will show the whole line plus n line before and after the label. + pub fn with_opt_context_lines(mut self, lines: Option) -> Self { self.context_lines = lines; self } diff --git a/src/named_source.rs b/src/named_source.rs index ea11cd2c..371a6706 100644 --- a/src/named_source.rs +++ b/src/named_source.rs @@ -56,8 +56,8 @@ impl SourceCode for NamedSource { fn read_span<'a>( &'a self, span: &crate::SourceSpan, - context_lines_before: usize, - context_lines_after: usize, + context_lines_before: Option, + context_lines_after: Option, ) -> Result + 'a>, MietteError> { let inner_contents = self.inner() diff --git a/src/protocol.rs b/src/protocol.rs index 589cd0b5..f02a4b55 100644 --- a/src/protocol.rs +++ b/src/protocol.rs @@ -247,11 +247,16 @@ gigabytes or larger in size. pub trait SourceCode: Send + Sync { /// Read the bytes for a specific span from this `SourceCode`, keeping a /// certain number of lines before and after the span as context. + /// When the `context_lines_before` (resp. `context_lines_after`) is set to + /// `None`, the span content must start (resp. stop) at the **span** boundary. + /// When set to `Some(0)`, the content should start (resp. stop) at the + /// **line** boundary. When set to `Some(n)`, the content should include the + /// _n_ lines before (resp. after) the span. fn read_span<'a>( &'a self, span: &SourceSpan, - context_lines_before: usize, - context_lines_after: usize, + context_lines_before: Option, + context_lines_after: Option, ) -> Result + 'a>, MietteError>; } diff --git a/src/source_impls.rs b/src/source_impls.rs index 61a7ffb3..ffada4db 100644 --- a/src/source_impls.rs +++ b/src/source_impls.rs @@ -15,8 +15,8 @@ struct LineInfo { fn context_info<'a>( input: &'a [u8], span: &SourceSpan, - context_lines_before: usize, - context_lines_after: usize, + context_lines_before: Option, + context_lines_after: Option, ) -> Result, MietteError> { let mut iter = input .split_inclusive(|b| *b == b'\n') @@ -56,14 +56,14 @@ fn context_info<'a>( Some(info) => info, }; - if line_starts.len() > context_lines_before { + if line_starts.len() > context_lines_before.unwrap_or(0) { line_starts.pop_front(); } line_starts.push_back(line_info); } let (start_lineno, start_offset, start_column) = { let start_info = line_starts.pop_front().unwrap(); - if context_lines_before > 0 { + if context_lines_before.is_some() { (start_info.line_no, start_info.start, 0) } else { ( @@ -83,13 +83,13 @@ fn context_info<'a>( } // Get the "after" lines - if let Some(last) = iter.take(context_lines_after).last() { + if let Some(last) = iter.take(context_lines_after.unwrap_or(0)).last() { line_info = last; } if span.offset() + span.len() > line_info.end { return Err(MietteError::OutOfBounds); } - let (end_lineno, end_offset) = if context_lines_after > 0 { + let (end_lineno, end_offset) = if context_lines_after.is_some() { (line_info.line_no, line_info.end) } else { (line_info.line_no, span.offset() + span.len()) @@ -108,8 +108,8 @@ impl SourceCode for [u8] { fn read_span<'a>( &'a self, span: &SourceSpan, - context_lines_before: usize, - context_lines_after: usize, + context_lines_before: Option, + context_lines_after: Option, ) -> Result + 'a>, MietteError> { let contents = context_info(self, span, context_lines_before, context_lines_after)?; Ok(Box::new(contents)) @@ -120,8 +120,8 @@ impl<'src> SourceCode for &'src [u8] { fn read_span<'a>( &'a self, span: &SourceSpan, - context_lines_before: usize, - context_lines_after: usize, + context_lines_before: Option, + context_lines_after: Option, ) -> Result + 'a>, MietteError> { <[u8] as SourceCode>::read_span(self, span, context_lines_before, context_lines_after) } @@ -131,8 +131,8 @@ impl SourceCode for Vec { fn read_span<'a>( &'a self, span: &SourceSpan, - context_lines_before: usize, - context_lines_after: usize, + context_lines_before: Option, + context_lines_after: Option, ) -> Result + 'a>, MietteError> { <[u8] as SourceCode>::read_span(self, span, context_lines_before, context_lines_after) } @@ -142,8 +142,8 @@ impl SourceCode for str { fn read_span<'a>( &'a self, span: &SourceSpan, - context_lines_before: usize, - context_lines_after: usize, + context_lines_before: Option, + context_lines_after: Option, ) -> Result + 'a>, MietteError> { <[u8] as SourceCode>::read_span( self.as_bytes(), @@ -159,8 +159,8 @@ impl<'s> SourceCode for &'s str { fn read_span<'a>( &'a self, span: &SourceSpan, - context_lines_before: usize, - context_lines_after: usize, + context_lines_before: Option, + context_lines_after: Option, ) -> Result + 'a>, MietteError> { ::read_span(self, span, context_lines_before, context_lines_after) } @@ -170,8 +170,8 @@ impl SourceCode for String { fn read_span<'a>( &'a self, span: &SourceSpan, - context_lines_before: usize, - context_lines_after: usize, + context_lines_before: Option, + context_lines_after: Option, ) -> Result + 'a>, MietteError> { ::read_span(self, span, context_lines_before, context_lines_after) } @@ -181,8 +181,8 @@ impl SourceCode for Arc { fn read_span<'a>( &'a self, span: &SourceSpan, - context_lines_before: usize, - context_lines_after: usize, + context_lines_before: Option, + context_lines_after: Option, ) -> Result + 'a>, MietteError> { self.as_ref() .read_span(span, context_lines_before, context_lines_after) @@ -201,8 +201,8 @@ where fn read_span<'a>( &'a self, span: &SourceSpan, - context_lines_before: usize, - context_lines_after: usize, + context_lines_before: Option, + context_lines_after: Option, ) -> Result + 'a>, MietteError> { self.as_ref() .read_span(span, context_lines_before, context_lines_after) @@ -216,7 +216,7 @@ mod tests { #[test] fn basic() -> Result<(), MietteError> { let src = String::from("foo\n"); - let contents = src.read_span(&(0, 4).into(), 0, 0)?; + let contents = src.read_span(&(0, 4).into(), None, None)?; assert_eq!("foo\n", std::str::from_utf8(contents.data()).unwrap()); assert_eq!(SourceSpan::from((0, 4)), *contents.span()); assert_eq!(0, contents.line()); @@ -228,7 +228,7 @@ mod tests { #[test] fn shifted() -> Result<(), MietteError> { let src = String::from("foobar"); - let contents = src.read_span(&(3, 3).into(), 1, 1)?; + let contents = src.read_span(&(3, 3).into(), Some(1), Some(1))?; assert_eq!("foobar", std::str::from_utf8(contents.data()).unwrap()); assert_eq!(SourceSpan::from((0, 6)), *contents.span()); assert_eq!(0, contents.line()); @@ -240,7 +240,7 @@ mod tests { #[test] fn middle() -> Result<(), MietteError> { let src = String::from("foo\nbar\nbaz\n"); - let contents = src.read_span(&(4, 4).into(), 0, 0)?; + let contents = src.read_span(&(4, 4).into(), None, None)?; assert_eq!("bar\n", std::str::from_utf8(contents.data()).unwrap()); assert_eq!(SourceSpan::from((4, 4)), *contents.span()); assert_eq!(1, contents.line()); @@ -252,7 +252,7 @@ mod tests { #[test] fn middle_of_line() -> Result<(), MietteError> { let src = String::from("foo\nbarbar\nbaz\n"); - let contents = src.read_span(&(7, 4).into(), 0, 0)?; + let contents = src.read_span(&(7, 4).into(), None, None)?; assert_eq!("bar\n", std::str::from_utf8(contents.data()).unwrap()); assert_eq!(SourceSpan::from((7, 4)), *contents.span()); assert_eq!(1, contents.line()); @@ -264,7 +264,7 @@ mod tests { #[test] fn end_of_line_before_newline() -> Result<(), MietteError> { let src = String::from("foo\nbar\nbaz\n"); - let contents = src.read_span(&(7, 0).into(), 0, 0)?; + let contents = src.read_span(&(7, 0).into(), None, None)?; assert_eq!("", std::str::from_utf8(contents.data()).unwrap()); assert_eq!(SourceSpan::from((7, 0)), *contents.span()); assert_eq!(1, contents.line()); @@ -276,7 +276,7 @@ mod tests { #[test] fn end_of_line_after_newline() -> Result<(), MietteError> { let src = String::from("foo\nbar\nbaz\n"); - let contents = src.read_span(&(8, 0).into(), 0, 0)?; + let contents = src.read_span(&(8, 0).into(), None, None)?; assert_eq!("", std::str::from_utf8(contents.data()).unwrap()); assert_eq!(SourceSpan::from((8, 0)), *contents.span()); assert_eq!(2, contents.line()); @@ -288,7 +288,7 @@ mod tests { #[test] fn end_of_file_with_newline() -> Result<(), MietteError> { let src = String::from("foo\nbar\nbaz\n"); - let contents = src.read_span(&(12, 0).into(), 0, 0)?; + let contents = src.read_span(&(12, 0).into(), None, None)?; assert_eq!("", std::str::from_utf8(contents.data()).unwrap()); assert_eq!(SourceSpan::from((12, 0)), *contents.span()); assert_eq!(2, contents.line()); @@ -300,7 +300,7 @@ mod tests { #[test] fn end_of_file_without_newline() -> Result<(), MietteError> { let src = String::from("foo\nbar\nbaz"); - let contents = src.read_span(&(11, 0).into(), 0, 0)?; + let contents = src.read_span(&(11, 0).into(), None, None)?; assert_eq!("", std::str::from_utf8(contents.data()).unwrap()); assert_eq!(SourceSpan::from((11, 0)), *contents.span()); assert_eq!(2, contents.line()); @@ -312,7 +312,7 @@ mod tests { #[test] fn with_crlf() -> Result<(), MietteError> { let src = String::from("foo\r\nbar\r\nbaz\r\n"); - let contents = src.read_span(&(5, 5).into(), 0, 0)?; + let contents = src.read_span(&(5, 5).into(), None, None)?; assert_eq!("bar\r\n", std::str::from_utf8(contents.data()).unwrap()); assert_eq!(SourceSpan::from((5, 5)), *contents.span()); assert_eq!(1, contents.line()); @@ -324,7 +324,7 @@ mod tests { #[test] fn with_context() -> Result<(), MietteError> { let src = String::from("xxx\nfoo\nbar\nbaz\n\nyyy\n"); - let contents = src.read_span(&(8, 3).into(), 1, 1)?; + let contents = src.read_span(&(8, 3).into(), Some(1), Some(1))?; assert_eq!( "foo\nbar\nbaz\n", std::str::from_utf8(contents.data()).unwrap() @@ -339,7 +339,7 @@ mod tests { #[test] fn multiline_with_context() -> Result<(), MietteError> { let src = String::from("aaa\nxxx\n\nfoo\nbar\nbaz\n\nyyy\nbbb\n"); - let contents = src.read_span(&(9, 11).into(), 1, 1)?; + let contents = src.read_span(&(9, 11).into(), Some(1), Some(1))?; assert_eq!( "\nfoo\nbar\nbaz\n\n", std::str::from_utf8(contents.data()).unwrap() @@ -356,7 +356,7 @@ mod tests { #[test] fn multiline_with_context_line_start() -> Result<(), MietteError> { let src = String::from("one\ntwo\n\nthree\nfour\nfive\n\nsix\nseven\n"); - let contents = src.read_span(&(2, 0).into(), 2, 2)?; + let contents = src.read_span(&(2, 0).into(), Some(2), Some(2))?; assert_eq!( "one\ntwo\n\n", std::str::from_utf8(contents.data()).unwrap() @@ -374,7 +374,7 @@ mod tests { fn empty_source() -> Result<(), MietteError> { let src = String::from(""); - let contents = src.read_span(&(0, 0).into(), 0, 0)?; + let contents = src.read_span(&(0, 0).into(), None, None)?; assert_eq!("", std::str::from_utf8(contents.data()).unwrap()); assert_eq!(SourceSpan::from((0, 0)), *contents.span()); assert_eq!(0, contents.line()); @@ -388,22 +388,22 @@ mod tests { fn empty_source_out_of_bounds() { let src = String::from(""); - let contents = src.read_span(&(0, 1).into(), 0, 0); + let contents = src.read_span(&(0, 1).into(), None, None); assert!(matches!(contents, Err(MietteError::OutOfBounds))); - let contents = src.read_span(&(0, 2).into(), 0, 0); + let contents = src.read_span(&(0, 2).into(), None, None); assert!(matches!(contents, Err(MietteError::OutOfBounds))); - let contents = src.read_span(&(1, 0).into(), 0, 0); + let contents = src.read_span(&(1, 0).into(), None, None); assert!(matches!(contents, Err(MietteError::OutOfBounds))); - let contents = src.read_span(&(1, 1).into(), 0, 0); + let contents = src.read_span(&(1, 1).into(), None, None); assert!(matches!(contents, Err(MietteError::OutOfBounds))); - let contents = src.read_span(&(2, 0).into(), 0, 0); + let contents = src.read_span(&(2, 0).into(), None, None); assert!(matches!(contents, Err(MietteError::OutOfBounds))); - let contents = src.read_span(&(2, 1).into(), 0, 0); + let contents = src.read_span(&(2, 1).into(), None, None); assert!(matches!(contents, Err(MietteError::OutOfBounds))); } } diff --git a/tests/graphical.rs b/tests/graphical.rs index 47631171..32b9c020 100644 --- a/tests/graphical.rs +++ b/tests/graphical.rs @@ -2196,3 +2196,254 @@ Error: oops::my::inner assert_eq!(expected, &out); Ok(()) } + +#[test] +fn zero_length_no_context() -> 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")] + highlight: SourceSpan, + } + + let src = "one\ntwoo\nthree".to_string(); + let err = MyBad { + src: NamedSource::new("bad_file.rs", src), + highlight: (6, 0).into(), + }; + + let out = fmt_report_with_settings(err.into(), |handler| { + handler + .with_opt_context_lines(None) + .without_syntax_highlighting() + }); + let expected = r#"oops::my::bad + + × oops! + ╭─[bad_file.rs:2:3] + ╰──── + help: try doing it better next time? +"# + .trim_start() + .to_string(); + assert_eq!(expected, out); + Ok(()) +} + +#[test] +fn multi_adjacent_zero_length_no_context() -> 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")] + highlight: SourceSpan, + #[label("and here")] + highlight2: SourceSpan, + } + + let src = "one\ntwoo\nthree\nfour".to_string(); + let err = MyBad { + src: NamedSource::new("bad_file.rs", src), + highlight: (6, 0).into(), + highlight2: (12, 0).into(), + }; + + let out = fmt_report_with_settings(err.into(), |handler| { + handler + .with_opt_context_lines(None) + .without_syntax_highlighting() + }); + let expected = r#"oops::my::bad + + × oops! + ╭─[bad_file.rs:2:3] + 2 │ oo + · ▲ + · ╰── this bit here + 3 │ thr + ╰──── + help: try doing it better next time? +"# + .trim_start() + .to_string(); + assert_eq!(expected, out); + Ok(()) +} + +#[test] +fn multi_separated_zero_length_no_context() -> 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")] + highlight: SourceSpan, + #[label("and here")] + highlight2: SourceSpan, + } + + let src = "one\ntwoo\nthree\nfour".to_string(); + let err = MyBad { + src: NamedSource::new("bad_file.rs", src), + highlight: (6, 0).into(), + highlight2: (17, 0).into(), + }; + + let out = fmt_report_with_settings(err.into(), |handler| { + handler + .with_opt_context_lines(None) + .without_syntax_highlighting() + }); + let expected = r#"oops::my::bad + + × oops! + ╭─[bad_file.rs:2:3] + ╰──── + ╭─[bad_file.rs:4:3] + ╰──── + help: try doing it better next time? +"# + .trim_start() + .to_string(); + assert_eq!(expected, out); + Ok(()) +} + +#[test] +fn zero_length_zero_context() -> 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")] + highlight: SourceSpan, + } + + let src = "one\ntwoo\nthree".to_string(); + let err = MyBad { + src: NamedSource::new("bad_file.rs", src), + highlight: (6, 0).into(), + }; + + let out = fmt_report_with_settings(err.into(), |handler| { + handler + .with_opt_context_lines(Some(0)) + .without_syntax_highlighting() + }); + let expected = r#"oops::my::bad + + × oops! + ╭─[bad_file.rs:2:3] + 2 │ twoo + · ▲ + · ╰── this bit here + ╰──── + help: try doing it better next time? +"# + .trim_start() + .to_string(); + assert_eq!(expected, out); + Ok(()) +} + +#[test] +fn multi_adjacent_zero_length_zero_context() -> 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")] + highlight: SourceSpan, + #[label("and here")] + highlight2: SourceSpan, + } + + let src = "one\ntwoo\nthree\nfour".to_string(); + let err = MyBad { + src: NamedSource::new("bad_file.rs", src), + highlight: (6, 0).into(), + highlight2: (12, 0).into(), + }; + + let out = fmt_report_with_settings(err.into(), |handler| { + handler + .with_opt_context_lines(Some(0)) + .without_syntax_highlighting() + }); + let expected = r#"oops::my::bad + + × oops! + ╭─[bad_file.rs:2:3] + 2 │ twoo + · ▲ + · ╰── this bit here + 3 │ three + · ▲ + · ╰── and here + ╰──── + help: try doing it better next time? +"# + .trim_start() + .to_string(); + assert_eq!(expected, out); + Ok(()) +} + +#[test] +fn multi_separated_zero_length_zero_context() -> 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")] + highlight: SourceSpan, + #[label("and here")] + highlight2: SourceSpan, + } + + let src = "one\ntwoo\nthree\nfour".to_string(); + let err = MyBad { + src: NamedSource::new("bad_file.rs", src), + highlight: (6, 0).into(), + highlight2: (17, 0).into(), + }; + + let out = fmt_report_with_settings(err.into(), |handler| { + handler + .with_opt_context_lines(Some(0)) + .without_syntax_highlighting() + }); + let expected = r#"oops::my::bad + + × oops! + ╭─[bad_file.rs:2:3] + 2 │ twoo + · ▲ + · ╰── this bit here + ╰──── + ╭─[bad_file.rs:4:3] + 4 │ four + · ▲ + · ╰── and here + ╰──── + help: try doing it better next time? +"# + .trim_start() + .to_string(); + assert_eq!(expected, out); + Ok(()) +} diff --git a/tests/test_boxed.rs b/tests/test_boxed.rs index d6fa6f80..edafefaa 100644 --- a/tests/test_boxed.rs +++ b/tests/test_boxed.rs @@ -188,7 +188,7 @@ fn test_boxed_custom_diagnostic() { let span = SourceSpan::from(0..CustomDiagnostic::SOURCE_CODE.len()); assert_eq!( report.source_code().map(|source_code| source_code - .read_span(&span, 0, 0) + .read_span(&span, None, None) .expect("read data from source code successfully") .data() .to_owned()), From d0c114311d802f0d22d0daeaf1e303366b25fae4 Mon Sep 17 00:00:00 2001 From: Nahor Date: Tue, 19 Mar 2024 10:49:17 -0700 Subject: [PATCH 5/6] fix(miri): remove or mark as dead_code unused internal structs - Mark as dead_code the structs uses in testing - Remove unused code inherited from Eyre for converting `Option` into `Result` --- src/eyreish/context.rs | 1 - src/eyreish/wrapper.rs | 41 ----------------------------------------- tests/derive.rs | 6 ++++++ 3 files changed, 6 insertions(+), 42 deletions(-) diff --git a/src/eyreish/context.rs b/src/eyreish/context.rs index 3d9238b2..e3d41a7b 100644 --- a/src/eyreish/context.rs +++ b/src/eyreish/context.rs @@ -213,5 +213,4 @@ pub(crate) mod private { pub trait Sealed {} impl Sealed for Result where E: ext::Diag {} - impl Sealed for Option {} } diff --git a/src/eyreish/wrapper.rs b/src/eyreish/wrapper.rs index 01f64f09..a95b7d95 100644 --- a/src/eyreish/wrapper.rs +++ b/src/eyreish/wrapper.rs @@ -6,35 +6,9 @@ use crate::{Diagnostic, LabeledSpan, Report, SourceCode}; use crate as miette; -#[repr(transparent)] -pub(crate) struct DisplayError(pub(crate) M); - #[repr(transparent)] pub(crate) struct MessageError(pub(crate) M); -pub(crate) struct NoneError; - -impl Debug for DisplayError -where - M: Display, -{ - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - Display::fmt(&self.0, f) - } -} - -impl Display for DisplayError -where - M: Display, -{ - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - Display::fmt(&self.0, f) - } -} - -impl StdError for DisplayError where M: Display + 'static {} -impl Diagnostic for DisplayError where M: Display + 'static {} - impl Debug for MessageError where M: Display + Debug, @@ -56,21 +30,6 @@ where impl StdError for MessageError where M: Display + Debug + 'static {} impl Diagnostic for MessageError where M: Display + Debug + 'static {} -impl Debug for NoneError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - Debug::fmt("Option was None", f) - } -} - -impl Display for NoneError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - Display::fmt("Option was None", f) - } -} - -impl StdError for NoneError {} -impl Diagnostic for NoneError {} - #[repr(transparent)] pub(crate) struct BoxedError(pub(crate) Box); diff --git a/tests/derive.rs b/tests/derive.rs index ac29eee6..aa631dc5 100644 --- a/tests/derive.rs +++ b/tests/derive.rs @@ -6,12 +6,14 @@ fn related() { #[derive(Error, Debug, Diagnostic)] #[error("welp")] #[diagnostic(code(foo::bar::baz))] + #[allow(dead_code)] struct Foo { #[related] related: Vec, } #[derive(Error, Debug, Diagnostic)] + #[allow(dead_code)] enum Bar { #[error("variant1")] #[diagnostic(code(foo::bar::baz))] @@ -29,6 +31,7 @@ fn related() { #[derive(Error, Debug, Diagnostic)] #[error("welp2")] + #[allow(dead_code)] struct Baz; } @@ -37,6 +40,7 @@ fn related_report() { #[derive(Error, Debug, Diagnostic)] #[error("welp")] #[diagnostic(code(foo::bar::baz))] + #[allow(dead_code)] struct Foo { #[related] related: Vec, @@ -288,6 +292,7 @@ fn test_snippet_named_struct() { #[derive(Debug, Diagnostic, Error)] #[error("welp")] #[diagnostic(code(foo::bar::baz))] + #[allow(dead_code)] struct Foo<'a> { #[source_code] src: &'a str, @@ -310,6 +315,7 @@ fn test_snippet_unnamed_struct() { #[derive(Debug, Diagnostic, Error)] #[error("welp")] #[diagnostic(code(foo::bar::baz))] + #[allow(dead_code)] struct Foo<'a>( #[source_code] &'a str, #[label("{0}")] SourceSpan, From c7cbb07d84ecb80bda86dcb2ab93db5703bd19c5 Mon Sep 17 00:00:00 2001 From: Nahor Date: Wed, 20 Mar 2024 17:06:49 -0700 Subject: [PATCH 6/6] fix(zero length): Improve rendering for zero-length error spans A zero-length span at a line boundary can be associated either with the previous or the next line. Current code prefer to use the next line, however there isn't always a "next line" (end of source, or "no context" configuration). There is also the extra-special case of an empty document which has no "next line" but also no "previous line". This commit adds an empty newline at the end of a document if appropriate so that there is a "next line", or use the "previous line" --- src/handlers/graphical.rs | 137 +++++++++++++++++++++++--------------- src/source_impls.rs | 14 +++- tests/graphical.rs | 87 ++++++++++++++++++++++-- 3 files changed, 178 insertions(+), 60 deletions(-) diff --git a/src/handlers/graphical.rs b/src/handlers/graphical.rs index d4644027..d79e5251 100644 --- a/src/handlers/graphical.rs +++ b/src/handlers/graphical.rs @@ -515,7 +515,19 @@ impl GraphicalReportHandler { context: &LabeledSpan, labels: &[LabeledSpan], ) -> fmt::Result { - let (contents, lines) = self.get_lines(source, context.inner())?; + let (contents, mut lines) = self.get_lines(source, context.inner())?; + + // If the number of lines doesn't match the content's line_count, it's + // because the content is either an empty source or because the last + // line finishes with a newline. In this case, add an empty line. + if lines.len() != contents.line_count() { + lines.push(Line { + line_number: contents.line() + contents.line_count(), + offset: contents.span().offset() + contents.span().len(), + length: 0, + text: String::new(), + }); + } // only consider labels from the context as primary label let ctx_labels = labels.iter().filter(|l| { @@ -570,6 +582,10 @@ impl GraphicalReportHandler { self.theme.characters.hbar, )?; + // Save that for later, since `content` might be moved before we need + // that info + let has_content = !contents.data().is_empty(); + // If there is a primary label, then use its span // as the reference point for line/column information. let primary_contents = match primary_label { @@ -600,48 +616,53 @@ impl GraphicalReportHandler { } // Now it's time for the fun part--actually rendering everything! - for line in &lines { - // Line number, appropriately padded. - self.write_linum(f, linum_width, line.line_number)?; - - // Then, we need to print the gutter, along with any fly-bys We - // have separate gutters depending on whether we're on the actual - // line, or on one of the "highlight lines" below it. - self.render_line_gutter(f, max_gutter, line, &labels)?; - - // And _now_ we can print out the line text itself! - let styled_text = - StyledList::from(highlighter_state.highlight_line(&line.text)).to_string(); - self.render_line_text(f, &styled_text)?; - - // Next, we write all the highlights that apply to this particular line. - let (single_line, multi_line): (Vec<_>, Vec<_>) = labels - .iter() - .filter(|hl| line.span_applies(hl)) - .partition(|hl| line.span_line_only(hl)); - if !single_line.is_empty() { - // no line number! - self.write_no_linum(f, linum_width)?; - // gutter _again_ - self.render_highlight_gutter( - f, - max_gutter, - line, - &labels, - LabelRenderMode::SingleLine, - )?; - self.render_single_line_highlights( - f, - line, - linum_width, - max_gutter, - &single_line, - &labels, - )?; - } - for hl in multi_line { - if hl.label().is_some() && line.span_ends(hl) && !line.span_starts(hl) { - self.render_multi_line_end(f, &labels, max_gutter, linum_width, line, hl)?; + // (but only if we have content or if we wanted content, to avoid + // pointless detailed rendering, e.g. arrows pointing to nothing in the + // middle of nothing) + if has_content || self.context_lines.is_some() { + for (line_no, line) in lines.iter().enumerate() { + // Line number, appropriately padded. + self.write_linum(f, linum_width, line.line_number)?; + + // Then, we need to print the gutter, along with any fly-bys We + // have separate gutters depending on whether we're on the actual + // line, or on one of the "highlight lines" below it. + self.render_line_gutter(f, max_gutter, line, &labels)?; + + // And _now_ we can print out the line text itself! + let styled_text = + StyledList::from(highlighter_state.highlight_line(&line.text)).to_string(); + self.render_line_text(f, &styled_text)?; + + // Next, we write all the highlights that apply to this particular line. + let (single_line, multi_line): (Vec<_>, Vec<_>) = labels + .iter() + .filter(|hl| line.span_applies(hl, line_no == (lines.len() - 1))) + .partition(|hl| line.span_line_only(hl)); + if !single_line.is_empty() { + // no line number! + self.write_no_linum(f, linum_width)?; + // gutter _again_ + self.render_highlight_gutter( + f, + max_gutter, + line, + &labels, + LabelRenderMode::SingleLine, + )?; + self.render_single_line_highlights( + f, + line, + linum_width, + max_gutter, + &single_line, + &labels, + )?; + } + for hl in multi_line { + if hl.label().is_some() && line.span_ends(hl) && !line.span_starts(hl) { + self.render_multi_line_end(f, &labels, max_gutter, linum_width, line, hl)?; + } } } } @@ -1271,18 +1292,30 @@ impl Line { /// Returns whether `span` should be visible on this line, either in the gutter or under the /// text on this line - fn span_applies(&self, span: &FancySpan) -> bool { + /// + /// An empty span at a line boundary will preferable apply to the start of + /// a line (i.e. the second/next line) rather than the end of one (i.e. the + /// first/previous line). However if there are no "second" line, the span + /// can only apply the "first". The `inclusive` parameter is there to + /// indicate that `self` is the last line, i.e. that there are no "second" + /// line. + fn span_applies(&self, span: &FancySpan, inclusive: bool) -> bool { // A span applies if its start is strictly before the line's end, // i.e. the span is not after the line, and its end is strictly after // the line's start, i.e. the span is not before the line. // - // One corner case: if the span length is 0, then the span also applies - // if its end is *at* the line's start, not just strictly after. - (span.offset() < self.offset + self.length) - && match span.len() == 0 { - true => (span.offset() + span.len()) >= self.offset, - false => (span.offset() + span.len()) > self.offset, - } + // Two corner cases: + // - if `inclusive` is true, then the span also applies if its start is + // *at* the line's end, not just strictly before. + // - if the span length is 0, then the span also applies if its end is + // *at* the line's start, not just strictly after. + (match inclusive { + true => span.offset() <= self.offset + self.length, + false => span.offset() < self.offset + self.length, + }) && match span.len() == 0 { + true => (span.offset() + span.len()) >= self.offset, + false => (span.offset() + span.len()) > self.offset, + } } /// Returns whether `span` should be visible on this line in the gutter (so this excludes spans @@ -1290,7 +1323,7 @@ impl Line { fn span_applies_gutter(&self, span: &FancySpan) -> bool { // The span must covers this line and at least one of its ends must be // on another line - self.span_applies(span) + self.span_applies(span, false) && ((span.offset() < self.offset) || ((span.offset() + span.len()) >= (self.offset + self.length))) } diff --git a/src/source_impls.rs b/src/source_impls.rs index ffada4db..125fc9f1 100644 --- a/src/source_impls.rs +++ b/src/source_impls.rs @@ -20,6 +20,16 @@ fn context_info<'a>( ) -> Result, MietteError> { let mut iter = input .split_inclusive(|b| *b == b'\n') + .chain( + // `split_inclusive()` does not generate a line if the input is + // empty or for the "last line" if it terminates with a new line. + // This `chain` fixes that. + match input.last() { + None => Some(&input[0..0]), + Some(b'\n') => Some(&input[input.len()..input.len()]), + _ => None, + }, + ) .enumerate() .map(|(line_no, line)| { // SAFETY: @@ -291,8 +301,8 @@ mod tests { let contents = src.read_span(&(12, 0).into(), None, None)?; assert_eq!("", std::str::from_utf8(contents.data()).unwrap()); assert_eq!(SourceSpan::from((12, 0)), *contents.span()); - assert_eq!(2, contents.line()); - assert_eq!(4, contents.column()); + assert_eq!(3, contents.line()); + assert_eq!(0, contents.column()); assert_eq!(1, contents.line_count()); Ok(()) } diff --git a/tests/graphical.rs b/tests/graphical.rs index 32b9c020..c23e81e5 100644 --- a/tests/graphical.rs +++ b/tests/graphical.rs @@ -288,6 +288,9 @@ fn empty_source() -> Result<(), MietteError> { × oops! ╭─[bad_file.rs:1:1] + 1 │ + · ▲ + · ╰── this bit here ╰──── help: try doing it better next time? "# @@ -633,6 +636,78 @@ fn single_line_highlight_offset_end_of_line() -> Result<(), MietteError> { Ok(()) } +#[test] +fn single_line_highlight_offset_end_of_file_no_newline() -> 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")] + highlight: SourceSpan, + } + + let src = "one\ntwo\nthree".to_string(); + let err = MyBad { + src: NamedSource::new("bad_file.rs", src), + highlight: (13, 0).into(), + }; + let out = fmt_report(err.into()); + println!("Error: {}", out); + let expected = r#"oops::my::bad + + × oops! + ╭─[bad_file.rs:3:6] + 2 │ two + 3 │ three + · ▲ + · ╰── this bit here + ╰──── + help: try doing it better next time? +"# + .trim_start() + .to_string(); + assert_eq!(expected, out); + Ok(()) +} + +#[test] +fn single_line_highlight_offset_end_of_file_with_newline() -> 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")] + highlight: SourceSpan, + } + + let src = "one\ntwo\nthree\n".to_string(); + let err = MyBad { + src: NamedSource::new("bad_file.rs", src), + highlight: (14, 0).into(), + }; + let out = fmt_report(err.into()); + println!("Error: {}", out); + let expected = r#"oops::my::bad + + × oops! + ╭─[bad_file.rs:4:1] + 3 │ three + 4 │ + · ▲ + · ╰── this bit here + ╰──── + help: try doing it better next time? +"# + .trim_start() + .to_string(); + assert_eq!(expected, out); + Ok(()) +} + #[test] fn single_line_highlight_include_end_of_line() -> Result<(), MietteError> { #[derive(Debug, Diagnostic, Error)] @@ -1088,9 +1163,8 @@ fn multiline_highlight_flyby() -> Result<(), MietteError> { line2 line3 line4 -line5 -"# - .to_string(); +line5"# + .to_string(); let len = src.len(); let err = MyBad { src: NamedSource::new("bad_file.rs", src), @@ -1147,9 +1221,8 @@ fn multiline_highlight_no_label() -> Result<(), MietteError> { line2 line3 line4 -line5 -"# - .to_string(); +line5"# + .to_string(); let len = src.len(); let err = MyBad { source: Inner(InnerInner), @@ -2267,6 +2340,8 @@ fn multi_adjacent_zero_length_no_context() -> Result<(), MietteError> { · ▲ · ╰── this bit here 3 │ thr + · ▲ + · ╰── and here ╰──── help: try doing it better next time? "#