Repository: zkat/miette (rust)

Issue:
Indentation with tabs throws off labels
(Sorry to not provide a proper repro. I’ll provide one later if you want, just wanted to get the issue opened before I forget).

If I have code indented with tabs, it seems that something goes wrong with the labelled spans:
```ts
function lol(): void {
	const x: i32 = 1;
};
```

<img width="522" alt="Screenshot 2021-10-10 at 14 35 29" src="https://user-images.githubusercontent.com/234957/136698061-2f7787cb-4d4b-4af3-9426-9599131bffea.png">

If I instead indent the code with spaces, it looks correct:

<img width="533" alt="Screenshot 2021-10-10 at 14 35 35" src="https://user-images.githubusercontent.com/234957/136698070-f6dbe452-5a47-4e78-a5c5-48a76398ad49.png">

I’m assuming that this might be due to terminal configuration how to display tabs?


Pull request:
Fix graphical label alignment issues 
Fixes #87 and #97.

Turns out that reliably determining the width of a string when displayed in a terminal is impossible without using ANSI escapes to get the current cursor position. Relying on the ANSI escapes is an option, but won't work if your output isn't a TTY. Supporting non-TTY output is important for things like redirecting error messages to a file, so I implemented a _mostly correct_ label alignment process that makes some specific tradeoffs. In the future, we could also add the escape-based option, and just fall back to this behavior on non-TTY output.

Back in https://github.com/zkat/miette/issues/87#issuecomment-1137930566, I had an idea for a hack to fix tab alignment while still respecting the terminal's tabstop setting. Unfortunately, I couldn't figure out a way to make this work for underlines, so the remaining option is to give up and replace tabs with spaces. This allows us to determine the visual width of the output reliably, but it does mean that we're ignoring the user's tabstop preferences. The original default behavior, where tabs were emitted verbatim and highlight alignment was broken, is pretty much never desirable, so I ended up just removing that option.

One of the issues with replacing tabs with a fixed number of spaces is that it will produce bad results if tabs are used for alignment in the middle of a line. To avoid this, we can emulate the behavior of the terminal, where tabs shift the cursor to the next tabstop rather than by a fixed width. Existing compilers that I checked seem to go both ways on this one. Gcc, clang, and rustc all emulate tabstops, while ghc uses a fixed width. I implemented the tabstop emulation option, which is a second breaking change, but this should be easy enough to swap out if we don't want it.

The other tradeoff here is that `unicode_width` isn't actually going to be able to predict the width of characters displayed on an arbitrary terminal. For example, 🏳️‍🌈 could show up as 1, 2, 3, or 4 columns. Predicting this is, in the general case, not possible. `unicode_width` is still better than assuming every character is 1 column though, so yay for incremental improvement.

Computers are a mess lol.

Patch:
diff --git a/src/handlers/graphical.rs b/src/handlers/graphical.rs
index 0034a8b..795aa1a 100644
--- a/src/handlers/graphical.rs
+++ b/src/handlers/graphical.rs
@@ -1,6 +1,7 @@
 use std::fmt::{self, Write};
 
 use owo_colors::{OwoColorize, Style};
+use unicode_width::UnicodeWidthChar;
 
 use crate::diagnostic_chain::DiagnosticChain;
 use crate::handlers::theme::*;
@@ -27,7 +28,7 @@ pub struct GraphicalReportHandler {
     pub(crate) theme: GraphicalTheme,
     pub(crate) footer: Option<String>,
     pub(crate) context_lines: usize,
-    pub(crate) tab_width: Option<usize>,
+    pub(crate) tab_width: usize,
     pub(crate) with_cause_chain: bool,
 }
 
@@ -48,7 +49,7 @@ impl GraphicalReportHandler {
             theme: GraphicalTheme::default(),
             footer: None,
             context_lines: 1,
-            tab_width: None,
+            tab_width: 4,
             with_cause_chain: true,
         }
     }
@@ -61,14 +62,14 @@ impl GraphicalReportHandler {
             theme,
             footer: None,
             context_lines: 1,
-            tab_width: None,
+            tab_width: 4,
             with_cause_chain: true,
         }
     }
 
     /// Set the displayed tab width in spaces.
     pub fn tab_width(mut self, width: usize) -> Self {
-        self.tab_width = Some(width);
+        self.tab_width = width;
         self
     }
 
@@ -441,12 +442,7 @@ impl GraphicalReportHandler {
             self.render_line_gutter(f, max_gutter, line, &labels)?;
 
             // And _now_ we can print out the line text itself!
-            if let Some(w) = self.tab_width {
-                let text = line.text.replace('\t', " ".repeat(w).as_str());
-                writeln!(f, "{}", text)?;
-            } else {
-                writeln!(f, "{}", line.text)?;
-            };
+            self.render_line_text(f, &line.text)?;
 
             // Next, we write all the highlights that apply to this particular line.
             let (single_line, multi_line): (Vec<_>, Vec<_>) = labels
@@ -604,6 +600,46 @@ impl GraphicalReportHandler {
         Ok(())
     }
 
+    /// Returns an iterator over the visual width of each character in a line.
+    fn line_visual_char_width<'a>(&self, text: &'a str) -> impl Iterator<Item = usize> + 'a {
+        let mut column = 0;
+        let tab_width = self.tab_width;
+        text.chars().map(move |c| {
+            let width = if c == '\t' {
+                // Round up to the next multiple of tab_width
+                tab_width - column % tab_width
+            } else {
+                c.width().unwrap_or(0)
+            };
+            column += width;
+            width
+        })
+    }
+
+    /// 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);
+        assert!(line_range.contains(&offset));
+
+        let text = &line.text[..offset - line.offset];
+        self.line_visual_char_width(text).sum()
+    }
+
+    /// Renders a line to the output formatter, replacing tabs with spaces.
+    fn render_line_text(&self, f: &mut impl fmt::Write, text: &str) -> fmt::Result {
+        for (c, width) in text.chars().zip(self.line_visual_char_width(text)) {
+            if c == '\t' {
+                for _ in 0..width {
+                    f.write_char(' ')?
+                }
+            } else {
+                f.write_char(c)?
+            }
+        }
+        f.write_char('\n')?;
+        Ok(())
+    }
+
     fn render_single_line_highlights(
         &self,
         f: &mut impl fmt::Write,
@@ -617,63 +653,44 @@ impl GraphicalReportHandler {
         let mut highest = 0;
 
         let chars = &self.theme.characters;
-        for hl in single_liners {
-            let hl_len = std::cmp::max(1, hl.len());
-
-            let local_offset = if let Some(w) = self.tab_width {
-                // Only count tabs that affect the position of the highlighted
-                // line and ignore tabs past the span.
-                let tab_count = &line.text[..hl.offset() - line.offset].matches('\t').count();
-                let tabs_as_spaces = tab_count * w - tab_count;
-                hl.offset() - line.offset + tabs_as_spaces
-            } else {
-                hl.offset() - line.offset
-            };
-
-            let vbar_offset = local_offset + (hl_len / 2);
-            let num_left = vbar_offset - local_offset;
-            let num_right = local_offset + hl_len - vbar_offset - 1;
-            let start = std::cmp::max(local_offset, highest);
-            let end = local_offset + hl_len;
-            if start < end {
-                underlines.push_str(
-                    &format!(
-                        "{:width$}{}{}{}",
-                        "",
-                        chars.underline.to_string().repeat(num_left),
-                        if hl.len() == 0 {
-                            chars.uarrow
-                        } else if hl.label().is_some() {
-                            chars.underbar
-                        } else {
-                            chars.underline
-                        },
-                        chars.underline.to_string().repeat(num_right),
-                        width = local_offset.saturating_sub(highest),
-                    )
-                    .style(hl.style)
-                    .to_string(),
-                );
-            }
-            highest = std::cmp::max(highest, end);
-        }
-        writeln!(f, "{}", underlines)?;
-
         let vbar_offsets: Vec<_> = single_liners
             .iter()
             .map(|hl| {
-                let local_offset = if let Some(w) = self.tab_width {
-                    // Only count tabs that affect the position of the
-                    // highlighted line and ignore tabs past the span.
-                    let tab_count = &line.text[..hl.offset() - line.offset].matches('\t').count();
-                    let tabs_as_spaces = tab_count * w - tab_count;
-                    hl.offset() - line.offset + tabs_as_spaces
-                } else {
-                    hl.offset() - line.offset
-                };
-                (hl, local_offset + (std::cmp::max(1, hl.len()) / 2))
+                let byte_start = hl.offset();
+                let byte_end = hl.offset() + hl.len().max(1);
+                let start = self.visual_offset(line, byte_start).max(highest);
+                let end = self.visual_offset(line, byte_end);
+
+                let vbar_offset = (start + end) / 2;
+                let num_left = vbar_offset - start;
+                let num_right = end - vbar_offset - 1;
+                if start < end {
+                    underlines.push_str(
+                        &format!(
+                            "{:width$}{}{}{}",
+                            "",
+                            chars.underline.to_string().repeat(num_left),
+                            if hl.len() == 0 {
+                                chars.uarrow
+                            } else if hl.label().is_some() {
+                                chars.underbar
+                            } else {
+                                chars.underline
+                            },
+                            chars.underline.to_string().repeat(num_right),
+                            width = start.saturating_sub(highest),
+                        )
+                        .style(hl.style)
+                        .to_string(),
+                    );
+                }
+                highest = std::cmp::max(highest, end);
+
+                (hl, vbar_offset)
             })
             .collect();
+        writeln!(f, "{}", underlines)?;
+
         for hl in single_liners.iter().rev() {
             if let Some(label) = hl.label() {
                 self.write_no_linum(f, linum_width)?;


Test patch:
diff --git a/tests/graphical.rs b/tests/graphical.rs
index 2c486b9..420c9ae 100644
--- a/tests/graphical.rs
+++ b/tests/graphical.rs
@@ -82,7 +82,7 @@ fn single_line_with_wide_char() -> Result<(), MietteError> {
     let src = "source\n  👼🏼text\n    here".to_string();
     let err = MyBad {
         src: NamedSource::new("bad_file.rs", src),
-        highlight: (9, 6).into(),
+        highlight: (13, 8).into(),
     };
     let out = fmt_report(err.into());
     println!("Error: {}", out);
@@ -92,8 +92,8 @@ fn single_line_with_wide_char() -> Result<(), MietteError> {
    ╭─[bad_file.rs:1:1]
  1 │ source
  2 │   👼🏼text
-   ·   ───┬──
-   ·      ╰── this bit here
+   ·     ───┬──
+   ·        ╰── this bit here
  3 │     here
    ╰────
   help: try doing it better next time?
@@ -157,10 +157,10 @@ fn single_line_with_tab_in_middle() -> Result<(), MietteError> {
 
     std::env::set_var("REPLACE_TABS", "4");
 
-    let src = "source\ntext\ttext\n    here".to_string();
+    let src = "source\ntext =\ttext\n    here".to_string();
     let err = MyBad {
         src: NamedSource::new("bad_file.rs", src),
-        highlight: (12, 4).into(),
+        highlight: (14, 4).into(),
     };
     let out = fmt_report(err.into());
     println!("Error: {}", out);
@@ -169,7 +169,7 @@ fn single_line_with_tab_in_middle() -> Result<(), MietteError> {
   × oops!
    ╭─[bad_file.rs:1:1]
  1 │ source
- 2 │ text    text
+ 2 │ text =  text
    ·         ──┬─
    ·           ╰── this bit here
  3 │     here