Repository: zkat/miette
Pull request #170 (2022-05-06 08:46:15)

Render inner diagnostics
Previously `#[diagnostic_source]` did not correctly render the inner diagnostic. This PR fixes that.

There's a change here that can be considered controversial:

- Only print newlines when there is an actual header (in ad9d0378ddd6dd4a6b9e650dd6aa62116da65910)

I am fine with removing it, but then the nested diagnostic source will be offset by one line and appear floating when it does not print a header.


PS: This PR also includes things from #169. I will rebase once that is merged (hopefully?)

TheNeikos: > I'm a little concerned about all the extra whitespace, and the potential complexity of displaying multiple errors in their entirety (as opposed to just the header, which I think was ideal).

I am not sure why `miette` would need to be aware of this? If a user wishes to not have it displayed, they can choose not to use the features that `miette` gives them. Since with an empty impl of `Diagnostic` it looks exactly how the `Error` output.

>  I'm not sure this matches up with what I'd like to see.

I absolutely understand that this is what I'd call a 'controversial' change, so I def want this to evolve to help everyone using `miette`. Could you elaborate a bit more on what parts you think do not fit? (Unless they are those you've already mentioned)

I'd love to come up with some more examples/combinations of how this could look like in some situations. (I've also gone one step further regarding the `related` questions in #171 but I'd say that is even more controversial and can wait :P)
zkat: >  If a user wishes to not have it displayed, they can choose not to use the features that miette gives them.

At scale, a dev does not necessarily have control over whether the sources they display are Diagnostics or not, and they might be surprised when they suddenly get a ton more output.

> Could you elaborate a bit more on what parts you think do not fit? (Unless they are those you've already mentioned)

I just think we can do better than just rendering the entire diagnostic inline like this.

> I'd love to come up with some more examples/combinations of how this could look like in some situations.

Ultimately, this is what I really want (for this and for #171). I'm not sure I like how they look right now, and I'd like to explore some more ideas of how things look _before_ we move on to implementation. Just figure out the best end result, regardless of current implementation, and work backwards from there. Heck, we can even change the general structure of error printing right now if it makes sense!
TheNeikos: > At scale, a dev does not necessarily have control over whether the sources they display are Diagnostics or not, and they might be surprised when they suddenly get a ton more output.

Hm, I'd agree if `source` and `diagnostic_source` would be the same annotation, but if a dev puts `diagnostic_source` into their code then I do believe they should expect it to be printed as a diagnostic! (Otherwise, they could just use it as an `Error` & `source`)

> I just think we can do better than just rendering the entire diagnostic inline like this.
Alright! I am not exactly sure what the alternatives are haha, but I'd love to discuss it.

> Ultimately, this is what I really want (for this and for https://github.com/zkat/miette/pull/171). I'm not sure I like how they look right now, and I'd like to explore some more ideas of how things look before we move on to implementation. Just figure out the best end result, regardless of current implementation, and work backwards from there. Heck, we can even change the general structure of error printing right now if it makes sense!

Sounds good to me, I'll be able to get back to it probably next week, so I'll try to hash out some nice error structures then, so we can compare/discuss.
Porges: > if a dev puts `diagnostic_source` into their code then I do believe they should expect it to be printed as a diagnostic!

Yes, definitely agree here. It’s opt-in and I would expect (as seen in #172) the diagnostics to be included in this case.
zkat: Ok I think we can merge this. Do you mind rebasing/resolving conflicts?
TheNeikos: @zkat there we go!
TheNeikos: > One last thing: do you mind writing a quick test to show what these nested diagnostics actually look like?

Duh, totally forgot that one haha

It prints them indented as before, but simply prints them as a whole rather than just their debug info

Patch:
diff --git a/src/handlers/graphical.rs b/src/handlers/graphical.rs
index 1ab09fa..b5dd754 100644
--- a/src/handlers/graphical.rs
+++ b/src/handlers/graphical.rs
@@ -3,7 +3,7 @@ use std::fmt::{self, Write};
 use owo_colors::{OwoColorize, Style};
 use unicode_width::UnicodeWidthChar;
 
-use crate::diagnostic_chain::DiagnosticChain;
+use crate::diagnostic_chain::{DiagnosticChain, ErrorKind};
 use crate::handlers::theme::*;
 use crate::protocol::{Diagnostic, Severity};
 use crate::{LabeledSpan, MietteError, ReportHandler, SourceCode, SourceSpan, SpanContents};
@@ -151,7 +151,6 @@ impl GraphicalReportHandler {
         diagnostic: &(dyn Diagnostic),
     ) -> fmt::Result {
         self.render_header(f, diagnostic)?;
-        writeln!(f)?;
         self.render_causes(f, diagnostic)?;
         let src = diagnostic.source_code();
         self.render_snippets(f, diagnostic, src)?;
@@ -190,6 +189,7 @@ impl GraphicalReportHandler {
             );
             write!(header, "{}", link)?;
             writeln!(f, "{}", header)?;
+            writeln!(f)?;
         } else if let Some(code) = diagnostic.code() {
             write!(header, "{}", code.style(severity_style),)?;
             if self.links == LinkStyle::Text && diagnostic.url().is_some() {
@@ -197,6 +197,7 @@ impl GraphicalReportHandler {
                 write!(header, " ({})", url.style(self.theme.styles.link))?;
             }
             writeln!(f, "{}", header)?;
+            writeln!(f)?;
         }
         Ok(())
     }
@@ -253,7 +254,22 @@ impl GraphicalReportHandler {
                 let opts = textwrap::Options::new(width)
                     .initial_indent(&initial_indent)
                     .subsequent_indent(&rest_indent);
-                writeln!(f, "{}", textwrap::fill(&error.to_string(), opts))?;
+                match error {
+                    ErrorKind::Diagnostic(diag) => {
+                        let mut inner = String::new();
+
+                        // Don't print footer for inner errors
+                        let mut inner_renderer = self.clone();
+                        inner_renderer.footer = None;
+                        inner_renderer.with_cause_chain = false;
+                        inner_renderer.render_report(&mut inner, diag)?;
+
+                        writeln!(f, "{}", textwrap::fill(&inner, opts))?;
+                    }
+                    ErrorKind::StdError(err) => {
+                        writeln!(f, "{}", textwrap::fill(&err.to_string(), opts))?;
+                    }
+                }
             }
         }
 
@@ -287,7 +303,6 @@ impl GraphicalReportHandler {
                     Some(Severity::Advice) => write!(f, "Advice: ")?,
                 };
                 self.render_header(f, rel)?;
-                writeln!(f)?;
                 self.render_causes(f, rel)?;
                 let src = rel.source_code().or(parent_src);
                 self.render_snippets(f, rel, src)?;


Test patch:
diff --git a/tests/graphical.rs b/tests/graphical.rs
index f929884..0c69470 100644
--- a/tests/graphical.rs
+++ b/tests/graphical.rs
@@ -85,8 +85,7 @@ fn single_line_highlight_span_full_line() {
     let out = fmt_report(err.into());
     println!("Error: {}", out);
 
-    let expected = r#"
-  × oops!
+    let expected = r#"  × oops!
    ╭─[issue:1:1]
  1 │ source
  2 │ text
@@ -1201,8 +1200,7 @@ fn zero_length_eol_span() {
     let out = fmt_report(err.into());
     println!("Error: {}", out);
 
-    let expected = r#"
-  × oops!
+    let expected = r#"  × oops!
    ╭─[issue:1:1]
  1 │ this is the first line
  2 │ this is the second line
diff --git a/tests/test_diagnostic_source_macro.rs b/tests/test_diagnostic_source_macro.rs
index 8af2e88..536aedf 100644
--- a/tests/test_diagnostic_source_macro.rs
+++ b/tests/test_diagnostic_source_macro.rs
@@ -1,5 +1,16 @@
 use miette::Diagnostic;
 
+#[derive(Debug, miette::Diagnostic, thiserror::Error)]
+#[error("A complex error happened")]
+struct SourceError {
+    #[source_code]
+    code: String,
+    #[help]
+    help: String,
+    #[label("here")]
+    label: (usize, usize),
+}
+
 #[derive(Debug, miette::Diagnostic, thiserror::Error)]
 #[error("AnErr")]
 struct AnErr;
@@ -8,7 +19,7 @@ struct AnErr;
 #[error("TestError")]
 struct TestStructError {
     #[diagnostic_source]
-    asdf_inner_foo: AnErr,
+    asdf_inner_foo: SourceError,
 }
 
 #[derive(Debug, miette::Diagnostic, thiserror::Error)]
@@ -37,7 +48,11 @@ struct TestArcedError(#[diagnostic_source] std::sync::Arc<dyn Diagnostic>);
 #[test]
 fn test_diagnostic_source() {
     let error = TestStructError {
-        asdf_inner_foo: AnErr,
+        asdf_inner_foo: SourceError {
+            code: String::new(),
+            help: String::new(),
+            label: (0, 0),
+        },
     };
     assert!(error.diagnostic_source().is_some());
 
@@ -59,3 +74,120 @@ fn test_diagnostic_source() {
     let error = TestArcedError(std::sync::Arc::new(AnErr));
     assert!(error.diagnostic_source().is_some());
 }
+
+#[test]
+fn test_diagnostic_source_pass_extra_info() {
+    let diag = TestBoxedError(Box::new(SourceError {
+        code: String::from("Hello\nWorld!"),
+        help: format!("Have you tried turning it on and off again?"),
+        label: (1, 4),
+    }));
+    let mut out = String::new();
+    miette::GraphicalReportHandler::new_themed(miette::GraphicalTheme::unicode_nocolor())
+        .with_width(80)
+        .with_footer("this is a footer".into())
+        .render_report(&mut out, &diag)
+        .unwrap();
+    println!("Error: {}", out);
+    let expected = r#"  × TestError
+  ╰─▶   × A complex error happened
+         ╭─[1:1]
+       1 │ Hello
+         ·  ──┬─
+         ·    ╰── here
+       2 │ World!
+         ╰────
+        help: Have you tried turning it on and off again?
+      
+
+  this is a footer
+"#
+    .to_string();
+    assert_eq!(expected, out);
+}
+
+#[test]
+fn test_diagnostic_source_is_output() {
+    let diag = TestStructError {
+        asdf_inner_foo: SourceError {
+            code: String::from("right here"),
+            help: String::from("That's where the error is!"),
+            label: (6, 4),
+        },
+    };
+    let mut out = String::new();
+    miette::GraphicalReportHandler::new_themed(miette::GraphicalTheme::unicode_nocolor())
+        .with_width(80)
+        .render_report(&mut out, &diag)
+        .unwrap();
+    println!("{}", out);
+
+    let expected = r#"  × TestError
+  ╰─▶   × A complex error happened
+         ╭────
+       1 │ right here
+         ·       ──┬─
+         ·         ╰── here
+         ╰────
+        help: That's where the error is!
+      
+"#;
+
+    assert_eq!(expected, out);
+}
+
+#[derive(Debug, miette::Diagnostic, thiserror::Error)]
+#[error("A nested error happened")]
+struct NestedError {
+    #[source_code]
+    code: String,
+    #[label("here")]
+    label: (usize, usize),
+    #[diagnostic_source]
+    the_other_err: Box<dyn Diagnostic>,
+}
+
+#[test]
+fn test_nested_diagnostic_source_is_output() {
+    let inner_error = TestStructError {
+        asdf_inner_foo: SourceError {
+            code: String::from("This is another error"),
+            help: String::from("You should fix this"),
+            label: (3, 4),
+        },
+    };
+    let diag = NestedError {
+        code: String::from("right here"),
+        label: (6, 4),
+        the_other_err: Box::new(inner_error),
+    };
+    let mut out = String::new();
+    miette::GraphicalReportHandler::new_themed(miette::GraphicalTheme::unicode_nocolor())
+        .with_width(80)
+        .with_footer("Yooo, a footer".to_string())
+        .render_report(&mut out, &diag)
+        .unwrap();
+    println!("{}", out);
+
+    let expected = r#"  × A nested error happened
+  ├─▶   × TestError
+  │   
+  ╰─▶   × A complex error happened
+         ╭────
+       1 │ This is another error
+         ·    ──┬─
+         ·      ╰── here
+         ╰────
+        help: You should fix this
+      
+   ╭────
+ 1 │ right here
+   ·       ──┬─
+   ·         ╰── here
+   ╰────
+
+  Yooo, a footer
+"#;
+
+    assert_eq!(expected, out);
+}