Repository: zkat/miette
Pull request #204 (2022-09-12 02:22:25)

fix(graphical): Fix panic when span covers full line
Fixes: #203

A classic off-by-one error, sorry :)

zkat: Note: I haven't merged this yet because I want to load it up in `nushell` and give it a shot to make doubleplus sure they won't just run into the same issue on upgrade again (and that there's no other surprises), but I just haven't made time for that. If you want to do it yourself, that would speed up getting this merged, though.
Benjamin-L: I made a build pointed at the `v5.3.1` tag on git, and was able to reproduce the original behavior from #203:

```console
/home/benjamin/nushell> 〉blah
Error: nu::shell::external_command (link)

  × External command failed
   ╭─[entry #1:1:1]
 1 │ blah
   · thread 'main' panicked at 'assertion failed: line_range.contains(&offset)', /home/benjamin/.cargo/git/checkouts/miette-45ce4058eaf7b1b9/9d665d9/src/handlers/graphical.rs:622:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```

There's also a unit test failing against 5.3.1:

```
---- tests::test_parser::proper_missing_param stdout ----
stdout:
stderr: Error: nu::parser::missing_positional (link)

  × Missing required positional argument.
   ╭─[/tmp/nix-shell.YL7Iro/.tmpOWMQrK:1:1]
 1 │ def foo [x y z w] { }; foo a b c
   · thread 'main' panicked at 'assertion failed: line_range.contains(&offset)', /home/benjamin/.cargo/git/checkouts/miette-45ce4058eaf7b1b9/9d665d9/src/handlers/graphical.rs:622:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

thread 'tests::test_parser::proper_missing_param' panicked at 'assertion failed: !stderr.is_empty() && stderr.contains(expected)', src/tests.rs:121:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:---- tests::test_parser::proper_missing_param stdout ----
stdout:
stderr: Error: nu::parser::missing_positional (link)

  × Missing required positional argument.
   ╭─[/tmp/nix-shell.YL7Iro/.tmpOWMQrK:1:1]
 1 │ def foo [x y z w] { }; foo a b c
   · thread 'main' panicked at 'assertion failed: line_range.contains(&offset)', /home/benjamin/.cargo/git/checkouts/miette-45ce4058eaf7b1b9/9d665d9/src/handlers/graphical.rs:622:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

thread 'tests::test_parser::proper_missing_param' panicked at 'assertion failed: !stderr.is_empty() && stderr.contains(expected)', src/tests.rs:121:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    tests::test_parser::proper_missing_param

test result: FAILED. 335 passed; 1 failed; 5 ignored; 0 measured; 0 filtered out; finished in 5.08s
    tests::test_parser::proper_missing_param

test result: FAILED. 335 passed; 1 failed; 5 ignored; 0 measured; 0 filtered out; finished in 5.08s
```

Trying the same thing pointed at this PR's branch:

```console
/home/benjamin/nushell〉> blah
Error: nu::shell::external_command (link)

  × External command failed
   ╭─[entry #1:1:1]
 1 │ blah
   · ──┬─
   ·   ╰── can't run executable
   ╰────
  help: No such file or directory (os error 2)
```

However, the unit test is still failing, with a different error this time:

```
---- tests::test_parser::proper_missing_param stdout ----
stdout:
stderr: Error: nu::parser::missing_positional (link)

  × Missing required positional argument.
   ╭─[/tmp/nix-shell.YL7Iro/.tmpnakncE:1:1]
 1 │ def foo [x y z w] { }; foo a b c
   · thread 'main' panicked at 'byte index 33 is out of bounds of `def foo [x y z w] { }; foo a b c`', /home/benjamin/.cargo/git/checkouts/miette-79329ecf4b2054f7/dd256fd/src/handlers/graphical.rs:624:21
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

thread 'tests::test_parser::proper_missing_param' panicked at 'assertion failed: !stderr.is_empty() && stderr.contains(expected)', src/tests.rs:121:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    tests::test_parser::proper_missing_param
```

---

It might be a while before I have time to look at this, might be better to just revert #202 for now if you want to get another release out soon.
zkat: I’m not really in a rush to put a release out but I’ll keep this in mind and consider reverting. I might end up taking as look at some point too but I also can’t manage it right now
zkat: Also, thanks for doing this legwork for me, I appreciate it
Benjamin-L: Forgot to run `cargo fmt` on the first push :)

All the nushell tests are passing now. I also did some basic smoke testing in an interactive nushell session and nothing fell apart.

The issue turned out to be that I was conflating byte offsets with visual offsets in a way that only mattered with zero-width spans on the end of lines. I've added a test for this.
Benjamin-L: A possible future improvement would be to introduce newtype wrappers for visual and byte offsets, to avoid this kind of mistake at the type level. This would, however, add a fair amount of conversion boilerplate.
Benjamin-L: It looks like CI is busted for reasons unrelated to this PR (#209).

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index f22228e..ad3cbb9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,18 @@
 # `miette` Release Changelog
 
+<a name="5.4.0"></a>
+## 5.4.0 (2022-10-25)
+
+### Features
+
+* **version:** declare minimum supported rust version at 1.56.0 (#209) ([ac02a124](https://github.com/zkat/miette/commit/ac02a1242b1d6452a428846d2a84d2ac164fd914))
+* **report:** `Report::new_boxed` ([0660d2f4](https://github.com/zkat/miette/commit/0660d2f43c0a793b1e289b26bcca73c8733bdcff))
+* **error:** `impl AsRef<dyn StdError> for Report` ([1a27033d](https://github.com/zkat/miette/commit/1a27033d7afd0007907550b1fc9d589d6f658662))
+
+### Bug Fixes
+
+* **wrapper:** complete forwarding `Diagnostic` implementations ([3fc5c04c](https://github.com/zkat/miette/commit/3fc5c04cbbd4b92863290a488a23d5243c16fe60))
+
 <a name="5.3.1"></a>
 ## 5.3.1 (2022-09-10)
 
diff --git a/Cargo.toml b/Cargo.toml
index 65cec15..48fca46 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "miette"
-version = "5.3.2-alpha.0"
+version = "5.4.1-alpha.0"
 authors = ["Kat Marchán <kzm@zkat.tech>"]
 description = "Fancy diagnostic reporting library and protocol for us mere mortals who aren't compiler hackers."
 categories = ["rust-patterns"]
@@ -14,7 +14,7 @@ exclude = ["images/", "tests/", "miette-derive/"]
 
 [dependencies]
 thiserror = "1.0.26"
-miette-derive = { path = "miette-derive", version = "=5.3.2-alpha.0"}
+miette-derive = { path = "miette-derive", version = "=5.4.1-alpha.0"}
 once_cell = "1.8.0"
 unicode-width = "0.1.9"
 
diff --git a/README.md b/README.md
index 5376dd2..6f9dcfb 100644
--- a/README.md
+++ b/README.md
@@ -606,7 +606,7 @@ under the Apache License. Some code is taken from
 
 [`miette!`]: https://docs.rs/miette/latest/miette/macro.miette.html
 [`std::error::Error`]: https://doc.rust-lang.org/nightly/std/error/trait.Error.html
-[`Diagnostic`]: https://docs.rs/miette/latest/miette/struct.Diagnostic.html
+[`Diagnostic`]: https://docs.rs/miette/latest/miette/trait.Diagnostic.html
 [`IntoDiagnostic`]: https://docs.rs/miette/latest/miette/trait.IntoDiagnostic.html
 [`MietteHandlerOpts`]: https://docs.rs/miette/latest/miette/struct.MietteHandlerOpts.html
 [`MietteHandler`]: https://docs.rs/miette/latest/miette/struct.MietteHandler.html
diff --git a/miette-derive/Cargo.toml b/miette-derive/Cargo.toml
index e3ab44d..a26d6ce 100644
--- a/miette-derive/Cargo.toml
+++ b/miette-derive/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "miette-derive"
-version = "5.3.2-alpha.0"
+version = "5.4.1-alpha.0"
 authors = ["Kat Marchán <kzm@zkat.tech>"]
 edition = "2018"
 license = "Apache-2.0"
diff --git a/src/eyreish/error.rs b/src/eyreish/error.rs
index a7ed3bd..6b0dc34 100644
--- a/src/eyreish/error.rs
+++ b/src/eyreish/error.rs
@@ -73,6 +73,18 @@ impl Report {
         Report::from_adhoc(message)
     }
 
+    /// Create a new error object from a boxed [`Diagnostic`].
+    ///
+    /// The boxed type must be thread safe and 'static, so that the `Report`
+    /// will be as well.
+    ///
+    /// Boxed `Diagnostic`s don't implement `Diagnostic` themselves due to trait coherence issues.
+    /// This method allows you to create a `Report` from a boxed `Diagnostic`.
+    #[cfg_attr(track_caller, track_caller)]
+    pub fn new_boxed(error: Box<dyn Diagnostic + Send + Sync + 'static>) -> Self {
+        Report::from_boxed(error)
+    }
+
     #[cfg_attr(track_caller, track_caller)]
     pub(crate) fn from_std<E>(error: E) -> Self
     where
@@ -779,6 +791,18 @@ impl AsRef<dyn Diagnostic> for Report {
     }
 }
 
+impl AsRef<dyn StdError + Send + Sync> for Report {
+    fn as_ref(&self) -> &(dyn StdError + Send + Sync + 'static) {
+        unsafe { ErrorImpl::error(self.inner.by_ref()) }
+    }
+}
+
+impl AsRef<dyn StdError> for Report {
+    fn as_ref(&self) -> &(dyn StdError + 'static) {
+        unsafe { ErrorImpl::error(self.inner.by_ref()) }
+    }
+}
+
 impl std::borrow::Borrow<dyn Diagnostic> for Report {
     fn borrow(&self) -> &(dyn Diagnostic + 'static) {
         self.as_ref()
diff --git a/src/eyreish/wrapper.rs b/src/eyreish/wrapper.rs
index a5e67db..91a5ef3 100644
--- a/src/eyreish/wrapper.rs
+++ b/src/eyreish/wrapper.rs
@@ -102,6 +102,10 @@ impl Diagnostic for BoxedError {
     fn related<'a>(&'a self) -> Option<Box<dyn Iterator<Item = &'a dyn Diagnostic> + 'a>> {
         self.0.related()
     }
+
+    fn diagnostic_source(&self) -> Option<&dyn Diagnostic> {
+        self.0.diagnostic_source()
+    }
 }
 
 impl Debug for BoxedError {
@@ -116,7 +120,21 @@ impl Display for BoxedError {
     }
 }
 
-impl StdError for BoxedError {}
+impl StdError for BoxedError {
+    fn source(&self) -> Option<&(dyn StdError + 'static)> {
+        self.0.source()
+    }
+
+    fn description(&self) -> &str {
+        #[allow(deprecated)]
+        self.0.description()
+    }
+
+    fn cause(&self) -> Option<&dyn StdError> {
+        #[allow(deprecated)]
+        self.0.cause()
+    }
+}
 
 pub(crate) struct WithSourceCode<E, C> {
     pub(crate) error: E,
@@ -151,6 +169,10 @@ impl<E: Diagnostic, C: SourceCode> Diagnostic for WithSourceCode<E, C> {
     fn related<'a>(&'a self) -> Option<Box<dyn Iterator<Item = &'a dyn Diagnostic> + 'a>> {
         self.error.related()
     }
+
+    fn diagnostic_source(&self) -> Option<&dyn Diagnostic> {
+        self.error.diagnostic_source()
+    }
 }
 
 impl<C: SourceCode> Diagnostic for WithSourceCode<Report, C> {
@@ -181,6 +203,10 @@ impl<C: SourceCode> Diagnostic for WithSourceCode<Report, C> {
     fn related<'a>(&'a self) -> Option<Box<dyn Iterator<Item = &'a dyn Diagnostic> + 'a>> {
         self.error.related()
     }
+
+    fn diagnostic_source(&self) -> Option<&dyn Diagnostic> {
+        self.error.diagnostic_source()
+    }
 }
 
 impl<E: Debug, C> Debug for WithSourceCode<E, C> {
diff --git a/src/handlers/graphical.rs b/src/handlers/graphical.rs
index 795aa1a..66760ff 100644
--- a/src/handlers/graphical.rs
+++ b/src/handlers/graphical.rs
@@ -618,7 +618,7 @@ impl GraphicalReportHandler {
 
     /// Returns the visual column position of a byte offset on a specific line.
     fn visual_offset(&self, line: &Line, offset: usize) -> usize {
-        let line_range = line.offset..(line.offset + line.length);
+        let line_range = line.offset..=(line.offset + line.length);
         assert!(line_range.contains(&offset));
 
         let text = &line.text[..offset - line.offset];
@@ -657,9 +657,9 @@ impl GraphicalReportHandler {
             .iter()
             .map(|hl| {
                 let byte_start = hl.offset();
-                let byte_end = hl.offset() + hl.len().max(1);
+                let byte_end = hl.offset() + hl.len();
                 let start = self.visual_offset(line, byte_start).max(highest);
-                let end = self.visual_offset(line, byte_end);
+                let end = self.visual_offset(line, byte_end).max(start + 1);
 
                 let vbar_offset = (start + end) / 2;
                 let num_left = vbar_offset - start;


Test patch:
diff --git a/tests/graphical.rs b/tests/graphical.rs
index 420c9ae..dd18449 100644
--- a/tests/graphical.rs
+++ b/tests/graphical.rs
@@ -67,6 +67,38 @@ fn empty_source() -> Result<(), MietteError> {
     Ok(())
 }
 
+#[test]
+fn single_line_highlight_span_full_line() {
+    #[derive(Error, Debug, Diagnostic)]
+    #[error("oops!")]
+    #[diagnostic(severity(Error))]
+    struct MyBad {
+        #[source_code]
+        src: NamedSource,
+        #[label("This bit here")]
+        bad_bit: SourceSpan,
+    }
+    let err = MyBad {
+        src: NamedSource::new("issue", "source\ntext"),
+        bad_bit: (7, 4).into(),
+    };
+    let out = fmt_report(err.into());
+    println!("Error: {}", out);
+
+    let expected = r#"
+  × oops!
+   ╭─[issue:1:1]
+ 1 │ source
+ 2 │ text
+   · ──┬─
+   ·   ╰── This bit here
+   ╰────
+"#
+    .to_string();
+
+    assert_eq!(expected, out);
+}
+
 #[test]
 fn single_line_with_wide_char() -> Result<(), MietteError> {
     #[derive(Debug, Diagnostic, Error)]
@@ -290,6 +322,42 @@ fn single_line_highlight_offset_zero() -> Result<(), MietteError> {
     Ok(())
 }
 
+#[test]
+fn single_line_higlight_offset_end_of_line() -> 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 = "source\n  text\n    here".to_string();
+    let err = MyBad {
+        src: NamedSource::new("bad_file.rs", src),
+        highlight: (6, 0).into(),
+    };
+    let out = fmt_report(err.into());
+    println!("Error: {}", out);
+    let expected = r#"oops::my::bad
+
+  × oops!
+   ╭─[bad_file.rs:1:1]
+ 1 │ source
+   ·       ▲
+   ·       ╰── this bit here
+ 2 │   text
+   ╰────
+  help: try doing it better next time?
+"#
+    .trim_start()
+    .to_string();
+    assert_eq!(expected, out);
+    Ok(())
+}
+
 #[test]
 fn single_line_highlight_with_empty_span() -> Result<(), MietteError> {
     #[derive(Debug, Diagnostic, Error)]
diff --git a/tests/test_boxed.rs b/tests/test_boxed.rs
index af16db7..d6fa6f8 100644
--- a/tests/test_boxed.rs
+++ b/tests/test_boxed.rs
@@ -1,4 +1,4 @@
-use miette::{miette, Diagnostic, Report};
+use miette::{miette, Diagnostic, LabeledSpan, Report, SourceSpan};
 use std::error::Error as StdError;
 use std::io;
 use thiserror::Error;
@@ -43,8 +43,15 @@ fn test_boxed_thiserror() {
     let error = MyError {
         source: io::Error::new(io::ErrorKind::Other, "oh no!"),
     };
-    let error: Report = miette!(error);
-    assert_eq!("oh no!", error.source().unwrap().to_string());
+    let report: Report = miette!(error);
+    assert_eq!("oh no!", report.source().unwrap().to_string());
+
+    let error = MyError {
+        source: io::Error::new(io::ErrorKind::Other, "oh no!!!!"),
+    };
+    let error: Box<dyn Diagnostic + Send + Sync + 'static> = Box::new(error);
+    let report = Report::new_boxed(error);
+    assert_eq!("oh no!!!!", report.source().unwrap().to_string());
 }
 
 #[test]
@@ -54,6 +61,162 @@ fn test_boxed_miette() {
     assert_eq!("oh no!", error.source().unwrap().to_string());
 }
 
+#[derive(Debug)]
+struct CustomDiagnostic {
+    source: Option<Report>,
+    related: Vec<Box<dyn Diagnostic + Send + Sync>>,
+}
+
+impl CustomDiagnostic {
+    const DISPLAY: &'static str = "CustomDiagnostic display";
+    const DESCRIPTION: &'static str = "CustomDiagnostic description";
+    const CODE: &'static str = "A042";
+    const SEVERITY: miette::Severity = miette::Severity::Advice;
+    const HELP: &'static str = "CustomDiagnostic help";
+    const URL: &'static str = "https://custom-diagnostic-url";
+    const LABEL: &'static str = "CustomDiagnostic label";
+    const SOURCE_CODE: &'static str = "this-is-some-source-code";
+
+    fn new() -> Self {
+        Self {
+            source: None,
+            related: Vec::new(),
+        }
+    }
+
+    fn with_source<E: StdError + Send + Sync + 'static>(self, source: E) -> Self {
+        let source = miette!(source);
+        Self {
+            source: Some(source),
+            related: Vec::new(),
+        }
+    }
+
+    fn with_related<D: Diagnostic + Send + Sync + 'static>(mut self, diagnostic: D) -> Self {
+        self.related.push(Box::new(diagnostic));
+        self
+    }
+}
+
+impl std::fmt::Display for CustomDiagnostic {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.write_str(Self::DISPLAY)
+    }
+}
+
+impl StdError for CustomDiagnostic {
+    fn source(&self) -> Option<&(dyn StdError + 'static)> {
+        self.source.as_ref().map(|source| source.as_ref())
+    }
+
+    fn description(&self) -> &str {
+        Self::DESCRIPTION
+    }
+
+    fn cause(&self) -> Option<&dyn StdError> {
+        self.source.as_ref().map(|source| source.as_ref())
+    }
+}
+
+impl Diagnostic for CustomDiagnostic {
+    fn code<'a>(&'a self) -> Option<Box<dyn std::fmt::Display + 'a>> {
+        Some(Box::new(Self::CODE))
+    }
+
+    fn severity(&self) -> Option<miette::Severity> {
+        Some(miette::Severity::Advice)
+    }
+
+    fn help<'a>(&'a self) -> Option<Box<dyn std::fmt::Display + 'a>> {
+        Some(Box::new(Self::HELP))
+    }
+
+    fn url<'a>(&'a self) -> Option<Box<dyn std::fmt::Display + 'a>> {
+        Some(Box::new(Self::URL))
+    }
+
+    fn labels<'a>(&'a self) -> Option<Box<dyn Iterator<Item = miette::LabeledSpan> + 'a>> {
+        let labels = miette::LabeledSpan::new(Some(Self::LABEL.to_owned()), 0, 7);
+        Some(Box::new(std::iter::once(labels)))
+    }
+
+    fn source_code(&self) -> Option<&dyn miette::SourceCode> {
+        Some(&Self::SOURCE_CODE)
+    }
+
+    fn related<'a>(&'a self) -> Option<Box<dyn Iterator<Item = &'a dyn Diagnostic> + 'a>> {
+        Some(Box::new(
+            self.related.iter().map(|d| &**d as &'a dyn Diagnostic),
+        ))
+    }
+
+    fn diagnostic_source(&self) -> Option<&dyn Diagnostic> {
+        self.source
+            .as_ref()
+            .map(|source| &**source as &dyn Diagnostic)
+    }
+}
+
+#[test]
+fn test_boxed_custom_diagnostic() {
+    fn assert_report(report: &Report) {
+        assert_eq!(
+            report.source().map(|source| source.to_string()),
+            Some("oh no!".to_owned()),
+        );
+        assert_eq!(
+            report.code().map(|code| code.to_string()),
+            Some(CustomDiagnostic::CODE.to_owned())
+        );
+        assert_eq!(report.severity(), Some(CustomDiagnostic::SEVERITY));
+        assert_eq!(
+            report.help().map(|help| help.to_string()),
+            Some(CustomDiagnostic::HELP.to_owned())
+        );
+        assert_eq!(
+            report.url().map(|url| url.to_string()),
+            Some(CustomDiagnostic::URL.to_owned())
+        );
+        assert_eq!(
+            report.labels().map(|labels| labels.collect::<Vec<_>>()),
+            Some(vec![LabeledSpan::new(
+                Some(CustomDiagnostic::LABEL.to_owned()),
+                0,
+                7
+            )]),
+        );
+        let span = SourceSpan::from(0..CustomDiagnostic::SOURCE_CODE.len());
+        assert_eq!(
+            report.source_code().map(|source_code| source_code
+                .read_span(&span, 0, 0)
+                .expect("read data from source code successfully")
+                .data()
+                .to_owned()),
+            Some(CustomDiagnostic::SOURCE_CODE.to_owned().into_bytes())
+        );
+        assert_eq!(
+            report.diagnostic_source().map(|source| source.to_string()),
+            Some("oh no!".to_owned()),
+        );
+    }
+
+    let related = CustomDiagnostic::new();
+    let main_diagnostic = CustomDiagnostic::new()
+        .with_source(io::Error::new(io::ErrorKind::Other, "oh no!"))
+        .with_related(related);
+
+    let report = Report::new_boxed(Box::new(main_diagnostic));
+    assert_report(&report);
+
+    let related = CustomDiagnostic::new();
+    let main_diagnostic = CustomDiagnostic::new()
+        .with_source(io::Error::new(io::ErrorKind::Other, "oh no!"))
+        .with_related(related);
+    let main_diagnostic = Box::new(main_diagnostic) as Box<dyn Diagnostic + Send + Sync + 'static>;
+    let report = miette!(main_diagnostic);
+    assert_report(&report);
+}
+
 #[test]
 #[ignore = "I don't know why this isn't working but it needs fixing."]
 fn test_boxed_sources() {