Repository: zkat/miette
Pull request #101 (2022-01-07 00:23:32)

Fix bugs in JSON handler
Currently JSON handler can produce invalid JSON.

There are string escaping bugfixes:
1. Things like `\n` are not doubly-escaped any more
2. The backslash `\` itself in the source is escaped

And also `related` array was not comma-delimited.

zkat: Can you fix the fmt/clippy stuff, and then I can merge this?
tailhook: Updated, although, I'm not sure what to do with:
```
error: field is never read: `state`
   --> src/eyreish/mod.rs:216:5
    |
216 |     state: crate::chain::ChainState<'a>,
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D dead-code` implied by `-D warnings`
```
Which is unrelated to this PR and show up even in docs PR.
zkat: oh huh. I guess we have an extra Chain definition that literally never gets used and for some reason I didn't remove it. Do you want to remove it in this PR? I can't really work on miette directly tonight cause I'm on some other stuff but I'm happy to approve/merge things for you!
tailhook: Submitted #102 

Patch:
diff --git a/src/chain.rs b/src/chain.rs
index fe4d336..7a66383 100644
--- a/src/chain.rs
+++ b/src/chain.rs
@@ -8,6 +8,25 @@ use std::vec;
 
 use ChainState::*;
 
+/// Iterator of a chain of source errors.
+///
+/// This type is the iterator returned by [`Report::chain`].
+///
+/// # Example
+///
+/// ```
+/// use miette::Report;
+/// use std::io;
+///
+/// pub fn underlying_io_error_kind(error: &Report) -> Option<io::ErrorKind> {
+///     for cause in error.chain() {
+///         if let Some(io_error) = cause.downcast_ref::<io::Error>() {
+///             return Some(io_error.kind());
+///         }
+///     }
+///     None
+/// }
+/// ```
 #[derive(Clone)]
 #[allow(missing_debug_implementations)]
 pub struct Chain<'a> {
@@ -25,7 +44,7 @@ pub(crate) enum ChainState<'a> {
 }
 
 impl<'a> Chain<'a> {
-    pub fn new(head: &'a (dyn StdError + 'static)) -> Self {
+    pub(crate) fn new(head: &'a (dyn StdError + 'static)) -> Self {
         Chain {
             state: ChainState::Linked { next: Some(head) },
         }
diff --git a/src/eyreish/mod.rs b/src/eyreish/mod.rs
index 26776c5..6082356 100644
--- a/src/eyreish/mod.rs
+++ b/src/eyreish/mod.rs
@@ -191,31 +191,6 @@ pub trait ReportHandler: core::any::Any + Send + Sync {
     fn track_caller(&mut self, location: &'static std::panic::Location<'static>) {}
 }
 
-/// Iterator of a chain of source errors.
-///
-/// This type is the iterator returned by [`Report::chain`].
-///
-/// # Example
-///
-/// ```
-/// use miette::Report;
-/// use std::io;
-///
-/// pub fn underlying_io_error_kind(error: &Report) -> Option<io::ErrorKind> {
-///     for cause in error.chain() {
-///         if let Some(io_error) = cause.downcast_ref::<io::Error>() {
-///             return Some(io_error.kind());
-///         }
-///     }
-///     None
-/// }
-/// ```
-#[derive(Clone)]
-#[allow(missing_debug_implementations)]
-pub struct Chain<'a> {
-    state: crate::chain::ChainState<'a>,
-}
-
 /// type alias for `Result<T, Report>`
 ///
 /// This is a reasonable return type to use throughout your application but also for `fn main`; if
diff --git a/src/handlers/json.rs b/src/handlers/json.rs
index 7508db0..eb47abc 100644
--- a/src/handlers/json.rs
+++ b/src/handlers/json.rs
@@ -1,4 +1,4 @@
-use std::fmt;
+use std::fmt::{self, Write};
 
 use crate::{protocol::Diagnostic, ReportHandler, Severity};
 
@@ -23,20 +23,34 @@ impl Default for JSONReportHandler {
     }
 }
 
-fn escape(input: &str) -> String {
-    input
-        .chars()
-        .map(|c| match c {
-            '"' => "\\\\\"".to_string(),
-            '\'' => "\\\\'".to_string(),
-            '\r' => "\\\\r".to_string(),
-            '\n' => "\\\\n".to_string(),
-            '\t' => "\\\\t".to_string(),
-            '\u{08}' => "\\\\b".to_string(),
-            '\u{0c}' => "\\\\f".to_string(),
-            c => format!("{}", c),
-        })
-        .collect()
+struct Escape<'a>(&'a str);
+
+impl fmt::Display for Escape<'_> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        for c in self.0.chars() {
+            let escape = match c {
+                '\\' => Some(r"\\"),
+                '"' => Some(r#"\""#),
+                '\'' => Some(r"\'"),
+                '\r' => Some(r"\r"),
+                '\n' => Some(r"\n"),
+                '\t' => Some(r"\t"),
+                '\u{08}' => Some(r"\b"),
+                '\u{0c}' => Some(r"\f"),
+                _ => None,
+            };
+            if let Some(escape) = escape {
+                f.write_str(escape)?;
+            } else {
+                f.write_char(c)?;
+            }
+        }
+        Ok(())
+    }
+}
+
+fn escape(input: &'_ str) -> Escape<'_> {
+    Escape(input)
 }
 
 impl JSONReportHandler {
@@ -93,7 +107,13 @@ impl JSONReportHandler {
         }
         if let Some(relateds) = diagnostic.related() {
             write!(f, r#""related": ["#)?;
+            let mut add_comma = false;
             for related in relateds {
+                if add_comma {
+                    write!(f, ",")?;
+                } else {
+                    add_comma = true;
+                }
                 self.render_report(f, related)?;
             }
             write!(f, "]")?;
@@ -127,3 +147,9 @@ impl ReportHandler for JSONReportHandler {
         self.render_report(f, diagnostic)
     }
 }
+
+#[test]
+fn test_escape() {
+    assert_eq!(escape("a\nb").to_string(), r"a\nb");
+    assert_eq!(escape("C:\\Miette").to_string(), r"C:\\Miette");
+}


Test patch:
diff --git a/tests/test_context.rs b/tests/test_context.rs
index 5f78572..9788f50 100644
--- a/tests/test_context.rs
+++ b/tests/test_context.rs
@@ -19,6 +19,7 @@ macro_rules! context_type {
         #[derive(Debug)]
         struct $name {
             message: &'static str,
+            #[allow(dead_code)]
             drop: DetectDrop,
         }
 
diff --git a/tests/test_json.rs b/tests/test_json.rs
index c56217c..f7ad3a6 100644
--- a/tests/test_json.rs
+++ b/tests/test_json.rs
@@ -497,7 +497,7 @@ mod json_report_handler {
         println!("Error: {}", out);
         let expected: String = r#"
         {
-            "message": "wtf?!\\nit broke :(",
+            "message": "wtf?!\nit broke :(",
             "code": "oops::my::bad",
             "severity": "error",
             "help": "try doing it better next time?",
@@ -737,11 +737,18 @@ mod json_report_handler {
         let err = MyBad {
             src: NamedSource::new("bad_file.rs", src.clone()),
             highlight: (9, 4).into(),
-            related: vec![MyBad {
-                src: NamedSource::new("bad_file2.rs", src),
-                highlight: (0, 6).into(),
-                related: vec![],
-            }],
+            related: vec![
+                MyBad {
+                    src: NamedSource::new("bad_file2.rs", src.clone()),
+                    highlight: (0, 6).into(),
+                    related: vec![],
+                },
+                MyBad {
+                    src: NamedSource::new("bad_file3.rs", src),
+                    highlight: (0, 6).into(),
+                    related: vec![],
+                },
+            ],
         };
         let out = fmt_report(err.into());
         println!("Error: {}", out);
@@ -777,6 +784,22 @@ mod json_report_handler {
                     }
                 ],
                 "related": []
+            },{
+                "message": "oops!",
+                "code": "oops::my::bad",
+                "severity": "error",
+                "help": "try doing it better next time?",
+                "filename": "bad_file3.rs",
+                "labels": [
+                    {
+                        "label": "this bit here",
+                        "span": {
+                            "offset": 0,
+                            "length": 6
+                        }
+                    }
+                ],
+                "related": []
             }]
         }"#
         .lines()