Repository: zkat/miette
Pull request #221 (2022-11-03 02:23:48)

fix(graphical): Fix panic with span extending past end of line (#215)
This also changes the behavior with spans including a CRLF line-ending. Before the panic bug was introduced, these were rendered with the CRLF being two visual columns wide. Now, any span extending past the EOL is treated as including one extra visual column. See https://github.com/zkat/miette/issues/215#issuecomment-1301560478 for an example of this.

This PR notably does not fix the rendering bug with spans including the EOF. These were rendered incorrectly before the panic bug was introduced. @Boshen mentioned that they were seeing a panic with EOF in #215, but I haven't been able to reproduce that. We may want to wait for a response from them before merging this, since it seems like I'm missing something here.

Boshen: Yeah it was probably the bad behaviour.

To make sure everything is correct, I've tested your patch, a few points:
* It seems the graphic handler need to be updated as well, it crashed in the same spot. https://github.com/zkat/miette/blob/3e25fd5b86324e16b33dd7e9192faa8015ead562/src/handlers/graphical.rs#L620-L626 


Boshen: With the upgrade, I found another interesting case where I was pointing the span inside a unicode.


v5.3.0:

offset 5..7

```
  × Invalid Character `ⸯ`
    ╭─[language/identifiers/vertical-tilde-continue.js:15:1]
 15 │ 
 16 │ var aⸯ; // U+2E2F
    ·      ─┬
    ·       ╰── Invalid Character `ⸯ`
    ╰────
```

v5.4.0:
Span: 5..7 gives

'byte index 7 is not a char boundary; it is inside 'ⸯ' (bytes 5..8) of `var aⸯ;

I had to change the span to 5..8 to cover the unicode
```
  × Invalid Character `ⸯ`
   ╭─[main.ts:1:6]
 1 │ var aⸯ;
   ·      ┬
   ·      ╰── Invalid Character `ⸯ`
 2 │
   ╰────
```


Boshen: And to make things more annoying ...

Although v5.4.0 is more accurate when pointing to a unicode, but the graphical reporter will use the range pointer `┬`, it would be even nicer to get it to use the single char pointer `▲`.
Benjamin-L: > Yeah it was probably the bad behaviour.
> 
> To make sure everything is correct, I've tested your patch. It seems the graphic handler need to be updated as well, it crashed in the same spot.
> 
> https://github.com/zkat/miette/blob/3e25fd5b86324e16b33dd7e9192faa8015ead562/src/handlers/graphical.rs#L620-L626

To be clear, you tested input similar to the examples from #215 against this PR and are still seeing a panic? If so, could you send the input that you're seeing the panic on?

The code block linked in that comment is the same one that this PR is changing:

https://github.com/zkat/miette/pull/221/files#diff-a775e9b52316667cfcabc8d12a083cba8d18759534d48b7a5da6cd647bc6fe6dR620-R639

I tested the input from #215 during development of this. I was able to reproduce the panic with the `main` branch, but did not reproduce the panic with the code from this PR.
Benjamin-L: > With the upgrade, I found another interesting case where I was pointing the span inside a unicode.
> 
> v5.3.0:
> 
> offset 5..7
> 
> ```
>   × Invalid Character `ⸯ`
>     ╭─[language/identifiers/vertical-tilde-continue.js:15:1]
>  15 │ 
>  16 │ var aⸯ; // U+2E2F
>     ·      ─┬
>     ·       ╰── Invalid Character `ⸯ`
>     ╰────
> ```
> 
> v5.4.0: Span: 5..7 gives
> 
> 'byte index 7 is not a char boundary; it is inside 'ⸯ' (bytes 5..8) of `var aⸯ;
> 
> I had to change the span to 5..8 to cover the unicode
> 
> ```
>   × Invalid Character `ⸯ`
>    ╭─[main.ts:1:6]
>  1 │ var aⸯ;
>    ·      ┬
>    ·      ╰── Invalid Character `ⸯ`
>  2 │
>    ╰────
> ```

I've created a separate issue for this: #223.

> And to make things more annoying ...
> 
> Although v5.4.0 is more accurate when pointing to a unicode, but the graphical reporter will use the range pointer `┬`, it would be even nicer to get it to use the single char pointer `▲`.

I don't think this is specific to multi-byte unicode characters. Miette currently only uses the `▲` pointer when the span has length 0 (it points to a specific index, but doesn't include the following character in the span). The `┬` pointer is used for any single-line span with nonzero length. This includes multi-byte characters that take a single visual column (length > 1), but also includes single-byte characters with a single visual column (length = 1).

IMO, it could go either way as to whether or not this behavior is correct. The one thing that I think is definitely _not_ correct is that multi-byte spans with zero visual columns (for example, spanning a zero-width-space or the `\r` of a CRLF with the code in this PR) are rendered as `┬` rather than `▲`, which is actively confusing. I'll make an issue for this one later today.
Benjamin-L: @Boshen I created an issue for the `┬` usage on spans pointing at zero-width nonempty spans: #224. That issue is _probably_ the right place to discuss whether or not `▲` should be used for 1-wide spans.
Boshen: > The code block linked in that comment is the same one that this PR is changing:

Oh I was looking at the wrong place again, I got dizzy from testing all these difference branches and versions 😵 

Anyway, this PR passed all my tests (I have a JavaScript / TypeScript compiler running with all Test262 / Babel / TypeScript conformance tests).



Patch:
diff --git a/miette-derive/src/code.rs b/miette-derive/src/code.rs
index 59aa436..22dc795 100644
--- a/miette-derive/src/code.rs
+++ b/miette-derive/src/code.rs
@@ -72,7 +72,7 @@ impl Code {
     pub(crate) fn gen_struct(&self) -> Option<TokenStream> {
         let code = &self.0;
         Some(quote! {
-            fn code<'a>(&'a self) -> std::option::Option<std::boxed::Box<dyn std::fmt::Display + 'a>> {
+            fn code(&self) -> std::option::Option<std::boxed::Box<dyn std::fmt::Display + '_>> {
                 std::option::Option::Some(std::boxed::Box::new(#code))
             }
         })
diff --git a/miette-derive/src/forward.rs b/miette-derive/src/forward.rs
index c8757b2..171019a 100644
--- a/miette-derive/src/forward.rs
+++ b/miette-derive/src/forward.rs
@@ -58,13 +58,13 @@ impl WhichFn {
     pub fn signature(&self) -> TokenStream {
         match self {
             Self::Code => quote! {
-                fn code<'a>(&'a self) -> std::option::Option<std::boxed::Box<dyn std::fmt::Display + 'a>>
+                fn code(& self) -> std::option::Option<std::boxed::Box<dyn std::fmt::Display + '_>>
             },
             Self::Help => quote! {
-                fn help<'a>(&'a self) -> std::option::Option<std::boxed::Box<dyn std::fmt::Display + 'a>>
+                fn help(& self) -> std::option::Option<std::boxed::Box<dyn std::fmt::Display + '_>>
             },
             Self::Url => quote! {
-                fn url<'a>(&'a self) -> std::option::Option<std::boxed::Box<dyn std::fmt::Display + 'a>>
+                fn url(& self) -> std::option::Option<std::boxed::Box<dyn std::fmt::Display + '_>>
             },
             Self::Severity => quote! {
                 fn severity(&self) -> std::option::Option<miette::Severity>
diff --git a/miette-derive/src/help.rs b/miette-derive/src/help.rs
index c5eedd1..ae2c5d8 100644
--- a/miette-derive/src/help.rs
+++ b/miette-derive/src/help.rs
@@ -108,7 +108,7 @@ impl Help {
                         Some(quote! {
                             Self::#ident #display_pat => {
                                 use miette::macro_helpers::ToOption;
-                                miette::macro_helpers::OptionalWrapper::<#ty>::new().to_option(&#help).as_ref().map(|#var| -> std::boxed::Box<dyn std::fmt::Display + 'a> { std::boxed::Box::new(format!("{}", #var)) })
+                                miette::macro_helpers::OptionalWrapper::<#ty>::new().to_option(&#help).as_ref().map(|#var| -> std::boxed::Box<dyn std::fmt::Display + '_> { std::boxed::Box::new(format!("{}", #var)) })
                             },
                         })
                     }
@@ -123,7 +123,7 @@ impl Help {
             Help::Display(display) => {
                 let (fmt, args) = display.expand_shorthand_cloned(&display_members);
                 Some(quote! {
-                    fn help<'a>(&'a self) -> std::option::Option<std::boxed::Box<dyn std::fmt::Display + 'a>> {
+                    fn help(&self) -> std::option::Option<std::boxed::Box<dyn std::fmt::Display + '_>> {
                         #[allow(unused_variables, deprecated)]
                         let Self #display_pat = self;
                         std::option::Option::Some(std::boxed::Box::new(format!(#fmt #args)))
@@ -133,11 +133,11 @@ impl Help {
             Help::Field(member, ty) => {
                 let var = quote! { __miette_internal_var };
                 Some(quote! {
-                    fn help<'a>(&'a self) -> std::option::Option<std::boxed::Box<dyn std::fmt::Display + 'a>> {
+                    fn help(&self) -> std::option::Option<std::boxed::Box<dyn std::fmt::Display + '_>> {
                         #[allow(unused_variables, deprecated)]
                         let Self #display_pat = self;
                         use miette::macro_helpers::ToOption;
-                        miette::macro_helpers::OptionalWrapper::<#ty>::new().to_option(&self.#member).as_ref().map(|#var| -> std::boxed::Box<dyn std::fmt::Display + 'a> { std::boxed::Box::new(format!("{}", #var)) })
+                        miette::macro_helpers::OptionalWrapper::<#ty>::new().to_option(&self.#member).as_ref().map(|#var| -> std::boxed::Box<dyn std::fmt::Display + '_> { std::boxed::Box::new(format!("{}", #var)) })
                     }
                 })
             }
diff --git a/miette-derive/src/url.rs b/miette-derive/src/url.rs
index d2e7adf..734d1a4 100644
--- a/miette-derive/src/url.rs
+++ b/miette-derive/src/url.rs
@@ -129,7 +129,7 @@ impl Url {
             }
         };
         Some(quote! {
-            fn url<'a>(&'a self) -> std::option::Option<std::boxed::Box<dyn std::fmt::Display + 'a>> {
+            fn url(&self) -> std::option::Option<std::boxed::Box<dyn std::fmt::Display + '_>> {
                 #[allow(unused_variables, deprecated)]
                 let Self #pat = self;
                 std::option::Option::Some(std::boxed::Box::new(format!(#fmt #args)))
diff --git a/src/handlers/graphical.rs b/src/handlers/graphical.rs
index 66760ff..15d2980 100644
--- a/src/handlers/graphical.rs
+++ b/src/handlers/graphical.rs
@@ -621,8 +621,22 @@ impl GraphicalReportHandler {
         let line_range = line.offset..=(line.offset + line.length);
         assert!(line_range.contains(&offset));
 
-        let text = &line.text[..offset - line.offset];
-        self.line_visual_char_width(text).sum()
+        let text_index = offset - line.offset;
+        let text = &line.text[..text_index.min(line.text.len())];
+        let text_width = self.line_visual_char_width(text).sum();
+        if text_index > line.text.len() {
+            // Spans extending past the end of the line are always rendered as
+            // one column past the end of the visible line.
+            //
+            // This doesn't necessarily correspond to a specific byte-offset,
+            // since a span extending past the end of the line could contain:
+            //  - an actual \n character (1 byte)
+            //  - a CRLF (2 bytes)
+            //  - EOF (0 bytes)
+            text_width + 1
+        } else {
+            text_width
+        }
     }
 
     /// Renders a line to the output formatter, replacing tabs with spaces.
diff --git a/src/source_impls.rs b/src/source_impls.rs
index 14fe782..52187cb 100644
--- a/src/source_impls.rs
+++ b/src/source_impls.rs
@@ -119,6 +119,17 @@ impl<'src> SourceCode for &'src [u8] {
     }
 }
 
+impl SourceCode for Vec<u8> {
+    fn read_span<'a>(
+        &'a self,
+        span: &SourceSpan,
+        context_lines_before: usize,
+        context_lines_after: usize,
+    ) -> Result<Box<dyn SpanContents<'a> + 'a>, MietteError> {
+        <[u8] as SourceCode>::read_span(self, span, context_lines_before, context_lines_after)
+    }
+}
+
 impl SourceCode for str {
     fn read_span<'a>(
         &'a self,


Test patch:
diff --git a/tests/derive.rs b/tests/derive.rs
index e0ee980..7faae42 100644
--- a/tests/derive.rs
+++ b/tests/derive.rs
@@ -185,7 +185,7 @@ fn fmt_help() {
     #[derive(Debug, Diagnostic, Error)]
     #[error("welp")]
     #[diagnostic(code(foo::bar::baz), help("{} x {0} x {:?}", 1, "2"))]
-    struct FooStruct(String);
+    struct FooStruct<'a>(&'a str);
 
     assert_eq!(
         "1 x hello x \"2\"".to_string(),
@@ -195,8 +195,8 @@ fn fmt_help() {
     #[derive(Debug, Diagnostic, Error)]
     #[error("welp")]
     #[diagnostic(code(foo::bar::baz), help("{} x {my_field} x {:?}", 1, "2"))]
-    struct BarStruct {
-        my_field: String,
+    struct BarStruct<'a> {
+        my_field: &'a str,
     }
 
     assert_eq!(
@@ -211,9 +211,9 @@ fn fmt_help() {
 
     #[derive(Debug, Diagnostic, Error)]
     #[error("welp")]
-    enum FooEnum {
+    enum FooEnum<'a> {
         #[diagnostic(code(foo::x), help("{} x {0} x {:?}", 1, "2"))]
-        X(String),
+        X(&'a str),
 
         #[diagnostic(code(foo::x), help("{} x {len} x {:?}", 1, "2"))]
         Y { len: usize },
@@ -243,9 +243,9 @@ fn help_field() {
     #[derive(Debug, Diagnostic, Error)]
     #[error("welp")]
     #[diagnostic()]
-    struct Foo {
+    struct Foo<'a> {
         #[help]
-        do_this: Option<String>,
+        do_this: Option<&'a str>,
     }
 
     assert_eq!(
@@ -261,11 +261,11 @@ fn help_field() {
     #[derive(Debug, Diagnostic, Error)]
     #[error("welp")]
     #[diagnostic()]
-    enum Bar {
-        A(#[help] Option<String>),
+    enum Bar<'a> {
+        A(#[help] Option<&'a str>),
         B {
             #[help]
-            do_this: Option<String>,
+            do_this: Option<&'a str>,
         },
     }
 
@@ -286,7 +286,7 @@ fn help_field() {
     #[derive(Debug, Diagnostic, Error)]
     #[error("welp")]
     #[diagnostic()]
-    struct Baz(#[help] Option<String>);
+    struct Baz<'a>(#[help] Option<&'a str>);
 
     assert_eq!(
         "x".to_string(),
@@ -296,7 +296,7 @@ fn help_field() {
     #[derive(Debug, Diagnostic, Error)]
     #[error("welp")]
     #[diagnostic()]
-    struct Quux(#[help] String);
+    struct Quux<'a>(#[help] &'a str);
 
     assert_eq!(
         "x".to_string(),
@@ -309,9 +309,9 @@ fn test_snippet_named_struct() {
     #[derive(Debug, Diagnostic, Error)]
     #[error("welp")]
     #[diagnostic(code(foo::bar::baz))]
-    struct Foo {
+    struct Foo<'a> {
         #[source_code]
-        src: String,
+        src: &'a str,
         #[label("var 1")]
         var1: SourceSpan,
         #[label = "var 2"]
@@ -331,8 +331,8 @@ fn test_snippet_unnamed_struct() {
     #[derive(Debug, Diagnostic, Error)]
     #[error("welp")]
     #[diagnostic(code(foo::bar::baz))]
-    struct Foo(
-        #[source_code] String,
+    struct Foo<'a>(
+        #[source_code] &'a str,
         #[label("{0}")] SourceSpan,
         #[label = "idk"] SourceSpan,
         #[label] SourceSpan,
@@ -346,11 +346,11 @@ fn test_snippet_enum() {
     #[derive(Debug, Diagnostic, Error)]
     #[error("welp")]
     #[allow(dead_code)]
-    enum Foo {
+    enum Foo<'a> {
         #[diagnostic(code(foo::a))]
         A {
             #[source_code]
-            src: String,
+            src: &'a str,
             msg: String,
             #[label("hi this is where the thing went wrong ({msg})")]
             var0: SourceSpan,
@@ -516,9 +516,9 @@ fn test_forward_struct_named() {
         help("{help}"),
         forward(span)
     )]
-    struct Struct {
+    struct Struct<'a> {
         span: ForwardsTo,
-        help: &'static str,
+        help: &'a str,
     }
     // Also check the From impl here
     let diag = Struct {
@@ -535,7 +535,7 @@ fn test_forward_struct_unnamed() {
     #[derive(Debug, Diagnostic, Error)]
     #[error("display")]
     #[diagnostic(code(foo::bar::overridden), url("{1}"), forward(0))]
-    struct Struct(ForwardsTo, &'static str);
+    struct Struct<'a>(ForwardsTo, &'a str);
 
     // Also check the From impl here
     let diag = Struct(ForwardsTo::new(), "url here");
@@ -546,12 +546,12 @@ fn test_forward_struct_unnamed() {
 #[test]
 fn test_forward_enum_named() {
     #[derive(Debug, Diagnostic, Error)]
-    enum Enum {
+    enum Enum<'a> {
         #[error("help: {help_text}")]
         #[diagnostic(code(foo::bar::overridden), help("{help_text}"), forward(span))]
         Variant {
             span: ForwardsTo,
-            help_text: &'static str,
+            help_text: &'a str,
         },
     }
     // Also check the From impl here
@@ -569,10 +569,10 @@ fn test_forward_enum_named() {
 #[test]
 fn test_forward_enum_unnamed() {
     #[derive(Debug, Diagnostic, Error)]
-    enum ForwardEnumUnnamed {
+    enum ForwardEnumUnnamed<'a> {
         #[error("help: {1}")]
         #[diagnostic(code(foo::bar::overridden), help("{1}"), forward(0))]
-        Variant(ForwardsTo, &'static str),
+        Variant(ForwardsTo, &'a str),
     }
     // Also check the From impl here
     let variant = ForwardEnumUnnamed::Variant(ForwardsTo::new(), "overridden help please");
diff --git a/tests/graphical.rs b/tests/graphical.rs
index dd18449..69f5bff 100644
--- a/tests/graphical.rs
+++ b/tests/graphical.rs
@@ -358,6 +358,80 @@ fn single_line_higlight_offset_end_of_line() -> Result<(), MietteError> {
     Ok(())
 }
 
+#[test]
+fn single_line_higlight_include_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: (9, 5).into(),
+    };
+    let out = fmt_report(err.into());
+    println!("Error: {}", out);
+    let expected = r#"oops::my::bad
+
+  × oops!
+   ╭─[bad_file.rs:1:1]
+ 1 │ source
+ 2 │   text
+   ·   ──┬──
+   ·     ╰── this bit here
+ 3 │     here
+   ╰────
+  help: try doing it better next time?
+"#
+    .trim_start()
+    .to_string();
+    assert_eq!(expected, out);
+    Ok(())
+}
+
+#[test]
+fn single_line_higlight_include_end_of_line_crlf() -> 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\r\n  text\r\n    here".to_string();
+    let err = MyBad {
+        src: NamedSource::new("bad_file.rs", src),
+        highlight: (10, 6).into(),
+    };
+    let out = fmt_report(err.into());
+    println!("Error: {}", out);
+    let expected = r#"oops::my::bad
+
+  × oops!
+   ╭─[bad_file.rs:1:1]
+ 1 │ source
+ 2 │   text
+   ·   ──┬──
+   ·     ╰── this bit here
+ 3 │     here
+   ╰────
+  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)]