Repository: brendanzab/codespan (rust)

Issue:
Fix tabulation columns in diagnostic output
Currently codespan-reporting treats tabs as being a constant width, but this is incorrect, as @legendiguess [notes](https://github.com/rust-lang/annotate-snippets-rs/issues/25#issuecomment-604805594):

> > The other would be to only adjust the underline calculation to account for tabs in line, but I'm not sure if display of `"\tt"` is always 5 ascii characters.
> 
> `\t` move next characters to the next tabulation column. so basically it's adds from 1 to 4 ascii characters, dependent on caret position off last tabulation column

We'll need to change the [source writer](https://github.com/brendanzab/codespan/blob/a77e646944765dfb338185b0ba9504500afb7304/codespan-reporting/src/term/config.rs#L56-L84) and the [width calculuation](https://github.com/brendanzab/codespan/blob/a77e646944765dfb338185b0ba9504500afb7304/codespan-reporting/src/term/config.rs#L35-L45) to properly support tabulation columns.

Pull request:
Fix rendering of tabulation columns
Closes #220.

Patch:
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index bff3b32..eef3843 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -35,11 +35,6 @@ jobs:
         with:
           command: check
           args: --manifest-path "codespan/Cargo.toml" --features "serialization"
-      - name: Run cargo test for codespan without codespan-reporting
-        uses: actions-rs/cargo@v1
-        with:
-          command: check
-          args: --manifest-path "codespan/Cargo.toml" --no-default-features
       - name: Run cargo test for codespan-lsp
         uses: actions-rs/cargo@v1
         with:
diff --git a/codespan-reporting/src/files.rs b/codespan-reporting/src/files.rs
index 41691b9..a717555 100644
--- a/codespan-reporting/src/files.rs
+++ b/codespan-reporting/src/files.rs
@@ -191,7 +191,6 @@ pub fn column_index(source: &str, line_range: Range<usize>, byte_index: usize) -
 ///
 /// assert_eq!(line_index(&line_starts, 5), Some(1));
 /// ```
-// NOTE: this is copied in `codespan::file::line_starts` and should be kept in sync.
 pub fn line_starts<'source>(source: &'source str) -> impl 'source + Iterator<Item = usize> {
     std::iter::once(0).chain(source.match_indices('\n').map(|(i, _)| i + 1))
 }
diff --git a/codespan-reporting/src/term/config.rs b/codespan-reporting/src/term/config.rs
index 8c68ac7..7b834d7 100644
--- a/codespan-reporting/src/term/config.rs
+++ b/codespan-reporting/src/term/config.rs
@@ -1,4 +1,3 @@
-use std::io;
 use termcolor::{Color, ColorSpec};
 
 use crate::diagnostic::{LabelStyle, Severity};
@@ -31,56 +30,6 @@ impl Default for Config {
     }
 }
 
-impl Config {
-    /// Measure the unicode width of a character, taking into account the tab width.
-    pub fn width(&self, ch: char) -> usize {
-        use unicode_width::UnicodeWidthChar;
-
-        match ch {
-            '\t' => self.tab_width,
-            _ => ch.width().unwrap_or(0),
-        }
-    }
-
-    /// Construct a source writer using the current config.
-    pub fn source<'a, W: ?Sized>(&self, writer: &'a mut W) -> SourceWriter<&'a mut W> {
-        SourceWriter {
-            upstream: writer,
-            tab_width: self.tab_width,
-        }
-    }
-}
-
-/// Writer that replaces tab characters with the configured number of spaces.
-pub struct SourceWriter<W> {
-    upstream: W,
-    tab_width: usize,
-}
-
-impl<W: io::Write> io::Write for SourceWriter<W> {
-    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
-        let mut last_term = 0usize;
-        for (i, ch) in buf.iter().enumerate() {
-            if *ch == b'\t' {
-                self.upstream.write_all(&buf[last_term..i])?;
-                last_term = i + 1;
-                write!(
-                    self.upstream,
-                    "{space: >width$}",
-                    space = "",
-                    width = self.tab_width,
-                )?;
-            }
-        }
-        self.upstream.write_all(&buf[last_term..])?;
-        Ok(buf.len())
-    }
-
-    fn flush(&mut self) -> io::Result<()> {
-        self.upstream.flush()
-    }
-}
-
 /// The display style to use when rendering diagnostics.
 #[derive(Clone, Debug)]
 pub enum DisplayStyle {
diff --git a/codespan-reporting/src/term/renderer.rs b/codespan-reporting/src/term/renderer.rs
index fb4029b..c288955 100644
--- a/codespan-reporting/src/term/renderer.rs
+++ b/codespan-reporting/src/term/renderer.rs
@@ -71,7 +71,6 @@ type Underline = (LabelStyle, VerticalBound);
 ///                     │ │ │   │                                                                │
 ///                  ┌────────────────────────────────────────────────────────────────────────────
 ///        header ── │ error[0001]: oh noes, a cupcake has occurred!
-///         empty ── │
 /// snippet start ── │    ┌─ test:9:0
 /// snippet empty ── │    │
 ///  snippet line ── │  9 │   ╭ Cupcake ipsum dolor. Sit amet marshmallow topping cheesecake
@@ -265,8 +264,14 @@ impl<'writer, 'config> Renderer<'writer, 'config> {
                 }
             }
 
-            // Write source
-            write!(self.config.source(self.writer), " {}", source)?;
+            // Write source text
+            write!(self, " ")?;
+            for (metrics, ch) in self.char_metrics(source.char_indices()) {
+                match ch {
+                    '\t' => (0..metrics.unicode_width).try_for_each(|_| write!(self, " "))?,
+                    _ => write!(self, "{}", ch)?,
+                }
+            }
             write!(self, "\n")?;
         }
 
@@ -366,8 +371,12 @@ impl<'writer, 'config> Renderer<'writer, 'config> {
             write!(self, " ")?;
 
             let mut previous_label_style = None;
-            for (byte_index, ch) in source
-                .char_indices()
+            let placeholder_metrics = Metrics {
+                byte_index: source.len(),
+                unicode_width: 1,
+            };
+            for (metrics, ch) in self
+                .char_metrics(source.char_indices())
                 // Add a placeholder source column at the end to allow for
                 // printing carets at the end of lines, eg:
                 //
@@ -375,13 +384,10 @@ impl<'writer, 'config> Renderer<'writer, 'config> {
                 // 1 │ Hello world!
                 //   │             ^
                 // ```
-                //
-                // ' ' is used because its unicode width is equal to 1 according
-                // to `unicode_width::UnicodeWidthChar`.
-                .chain(std::iter::once((source.len(), ' ')))
+                .chain(std::iter::once((placeholder_metrics, '\0')))
             {
                 // Find the current label style at this column
-                let column_range = byte_index..(byte_index + ch.len_utf8());
+                let column_range = metrics.byte_index..(metrics.byte_index + ch.len_utf8());
                 let current_label_style = single_labels
                     .iter()
                     .filter(|(_, range, _)| is_overlapping(range, &column_range))
@@ -402,12 +408,12 @@ impl<'writer, 'config> Renderer<'writer, 'config> {
                     Some(LabelStyle::Primary) => Some(self.chars().single_primary_caret),
                     Some(LabelStyle::Secondary) => Some(self.chars().single_secondary_caret),
                     // Only print padding if we are before the end of the last single line caret
-                    None if byte_index < max_label_end => Some(' '),
+                    None if metrics.byte_index < max_label_end => Some(' '),
                     None => None,
                 };
                 if let Some(caret_ch) = caret_ch {
                     // FIXME: improve rendering for carets that occurring within character boundaries
-                    for _ in 0..self.config.width(ch) {
+                    for _ in 0..metrics.unicode_width {
                         write!(self, "{}", caret_ch)?;
                     }
                 }
@@ -615,12 +621,39 @@ impl<'writer, 'config> Renderer<'writer, 'config> {
         Ok(())
     }
 
+    /// Adds tab-stop aware unicode-width computations to an iterator over
+    /// character indices. Assumes that the character indices begin at the start
+    /// of the line.
+    fn char_metrics(
+        &self,
+        char_indices: impl Iterator<Item = (usize, char)>,
+    ) -> impl Iterator<Item = (Metrics, char)> {
+        use unicode_width::UnicodeWidthChar;
+
+        let tab_width = self.config.tab_width;
+        let mut unicode_column = 0;
+
+        char_indices.map(move |(byte_index, ch)| {
+            let metrics = Metrics {
+                byte_index,
+                unicode_width: match (ch, tab_width) {
+                    ('\t', 0) => 0, // Guard divide-by-zero
+                    ('\t', _) => tab_width - (unicode_column % tab_width),
+                    (ch, _) => ch.width().unwrap_or(0),
+                },
+            };
+            unicode_column += metrics.unicode_width;
+
+            (metrics, ch)
+        })
+    }
+
     /// Location focus.
     fn snippet_locus(&mut self, locus: &Locus) -> io::Result<()> {
         write!(
             self,
-            "{origin}:{line_number}:{column_number}",
-            origin = locus.name,
+            "{name}:{line_number}:{column_number}",
+            name = locus.name,
             line_number = locus.location.line_number,
             column_number = locus.location.column_number,
         )
@@ -628,7 +661,7 @@ impl<'writer, 'config> Renderer<'writer, 'config> {
 
     /// The outer gutter of a source line.
     fn outer_gutter(&mut self, outer_padding: usize) -> io::Result<()> {
-        write!(self, "{space: >width$}", space = "", width = outer_padding,)?;
+        write!(self, "{space: >width$}", space = "", width = outer_padding)?;
         write!(self, " ")?;
         Ok(())
     }
@@ -672,27 +705,25 @@ impl<'writer, 'config> Renderer<'writer, 'config> {
         trailing_label: Option<(usize, &SingleLabel<'_>)>,
         char_indices: impl Iterator<Item = (usize, char)>,
     ) -> io::Result<()> {
-        for (byte_index, ch) in char_indices {
-            let column_range = byte_index..(byte_index + ch.len_utf8());
+        for (metrics, ch) in self.char_metrics(char_indices) {
+            let column_range = metrics.byte_index..(metrics.byte_index + ch.len_utf8());
             let label_style = hanging_labels(single_labels, trailing_label)
                 .filter(|(_, range, _)| column_range.contains(&range.start))
                 .map(|(label_style, _, _)| *label_style)
                 .max_by_key(label_priority_key);
 
-            let spaces = match label_style {
-                None => 0..self.config.width(ch),
+            let mut spaces = match label_style {
+                None => 0..metrics.unicode_width,
                 Some(label_style) => {
                     self.set_color(self.styles().label(severity, label_style))?;
                     write!(self, "{}", self.chars().pointer_left)?;
                     self.reset()?;
-                    1..self.config.width(ch)
+                    1..metrics.unicode_width
                 }
             };
             // Only print padding if we are before the end of the last single line caret
-            if byte_index <= max_label_start {
-                for _ in spaces {
-                    write!(self, " ")?;
-                }
+            if metrics.byte_index <= max_label_start {
+                spaces.try_for_each(|_| write!(self, " "))?;
             }
         }
 
@@ -773,12 +804,12 @@ impl<'writer, 'config> Renderer<'writer, 'config> {
     ) -> io::Result<()> {
         self.set_color(self.styles().label(severity, label_style))?;
 
-        for (_, ch) in source
-            .char_indices()
-            .take_while(|(byte_index, _)| *byte_index < range.end + 1)
+        for (metrics, _) in self
+            .char_metrics(source.char_indices())
+            .take_while(|(metrics, _)| metrics.byte_index < range.end + 1)
         {
             // FIXME: improve rendering for carets that occurring within character boundaries
-            for _ in 0..self.config.width(ch) {
+            for _ in 0..metrics.unicode_width {
                 write!(self, "{}", self.chars().multi_top)?;
             }
         }
@@ -808,12 +839,12 @@ impl<'writer, 'config> Renderer<'writer, 'config> {
     ) -> io::Result<()> {
         self.set_color(self.styles().label(severity, label_style))?;
 
-        for (_, ch) in source
-            .char_indices()
-            .take_while(|(byte_index, _)| *byte_index < range.end)
+        for (metrics, _) in self
+            .char_metrics(source.char_indices())
+            .take_while(|(metrics, _)| metrics.byte_index < range.end)
         {
             // FIXME: improve rendering for carets that occurring within character boundaries
-            for _ in 0..self.config.width(ch) {
+            for _ in 0..metrics.unicode_width {
                 write!(self, "{}", self.chars().multi_bottom)?;
             }
         }
@@ -915,6 +946,11 @@ impl<'writer, 'config> WriteColor for Renderer<'writer, 'config> {
     }
 }
 
+struct Metrics {
+    byte_index: usize,
+    unicode_width: usize,
+}
+
 /// Check if two ranges overlap
 fn is_overlapping(range0: &Range<usize>, range1: &Range<usize>) -> bool {
     let start = std::cmp::max(range0.start, range1.start);
diff --git a/codespan/src/file.rs b/codespan/src/file.rs
index 3150517..322a6d0 100644
--- a/codespan/src/file.rs
+++ b/codespan/src/file.rs
@@ -341,7 +341,7 @@ where
     Source: AsRef<str>,
 {
     fn new(name: OsString, source: Source) -> Self {
-        let line_starts = line_starts(source.as_ref())
+        let line_starts = codespan_reporting::files::line_starts(source.as_ref())
             .map(|i| ByteIndex::from(i as u32))
             .collect();
 
@@ -353,7 +353,7 @@ where
     }
 
     fn update(&mut self, source: Source) {
-        let line_starts = line_starts(source.as_ref())
+        let line_starts = codespan_reporting::files::line_starts(source.as_ref())
             .map(|i| ByteIndex::from(i as u32))
             .collect();
         self.source = source;
@@ -443,11 +443,6 @@ where
     }
 }
 
-// NOTE: this is copied from `codespan_reporting::files::line_starts` and should be kept in sync.
-fn line_starts<'source>(source: &'source str) -> impl 'source + Iterator<Item = usize> {
-    std::iter::once(0).chain(source.match_indices('\n').map(|(i, _)| i + 1))
-}
-
 #[cfg(test)]
 mod test {
     use super::*;


Test patch:
diff --git a/codespan-reporting/tests/snapshots/term__tab_columns__tab_width_2_no_color.snap b/codespan-reporting/tests/snapshots/term__tab_columns__tab_width_2_no_color.snap
new file mode 100644
index 0000000..0faa8d8
--- /dev/null
+++ b/codespan-reporting/tests/snapshots/term__tab_columns__tab_width_2_no_color.snap
@@ -0,0 +1,23 @@
+---
+source: codespan-reporting/tests/term.rs
+expression: TEST_DATA.emit_no_color(&config)
+---
+warning: tab test
+  ┌─ tab_columns:1:2
+  │
+1 │   hello
+  │   ^^^^^
+2 │ ∙ hello
+  │   ^^^^^
+3 │ ∙∙  hello
+  │     ^^^^^
+4 │ ∙∙∙ hello
+  │     ^^^^^
+5 │ ∙∙∙∙  hello
+  │       ^^^^^
+6 │ ∙∙∙∙∙ hello
+  │       ^^^^^
+7 │ ∙∙∙∙∙∙  hello
+  │         ^^^^^
+
+
diff --git a/codespan-reporting/tests/snapshots/term__tab_columns__tab_width_3_no_color.snap b/codespan-reporting/tests/snapshots/term__tab_columns__tab_width_3_no_color.snap
new file mode 100644
index 0000000..40f20a6
--- /dev/null
+++ b/codespan-reporting/tests/snapshots/term__tab_columns__tab_width_3_no_color.snap
@@ -0,0 +1,23 @@
+---
+source: codespan-reporting/tests/term.rs
+expression: TEST_DATA.emit_no_color(&config)
+---
+warning: tab test
+  ┌─ tab_columns:1:2
+  │
+1 │    hello
+  │    ^^^^^
+2 │ ∙  hello
+  │    ^^^^^
+3 │ ∙∙ hello
+  │    ^^^^^
+4 │ ∙∙∙   hello
+  │       ^^^^^
+5 │ ∙∙∙∙  hello
+  │       ^^^^^
+6 │ ∙∙∙∙∙ hello
+  │       ^^^^^
+7 │ ∙∙∙∙∙∙   hello
+  │          ^^^^^
+
+
diff --git a/codespan-reporting/tests/snapshots/term__tab_columns__tab_width_6_no_color.snap b/codespan-reporting/tests/snapshots/term__tab_columns__tab_width_6_no_color.snap
new file mode 100644
index 0000000..018a402
--- /dev/null
+++ b/codespan-reporting/tests/snapshots/term__tab_columns__tab_width_6_no_color.snap
@@ -0,0 +1,23 @@
+---
+source: codespan-reporting/tests/term.rs
+expression: TEST_DATA.emit_no_color(&config)
+---
+warning: tab test
+  ┌─ tab_columns:1:2
+  │
+1 │       hello
+  │       ^^^^^
+2 │ ∙     hello
+  │       ^^^^^
+3 │ ∙∙    hello
+  │       ^^^^^
+4 │ ∙∙∙   hello
+  │       ^^^^^
+5 │ ∙∙∙∙  hello
+  │       ^^^^^
+6 │ ∙∙∙∙∙ hello
+  │       ^^^^^
+7 │ ∙∙∙∙∙∙      hello
+  │             ^^^^^
+
+
diff --git a/codespan-reporting/tests/snapshots/term__tab_columns__tab_width_default_no_color.snap b/codespan-reporting/tests/snapshots/term__tab_columns__tab_width_default_no_color.snap
new file mode 100644
index 0000000..67cd187
--- /dev/null
+++ b/codespan-reporting/tests/snapshots/term__tab_columns__tab_width_default_no_color.snap
@@ -0,0 +1,23 @@
+---
+source: codespan-reporting/tests/term.rs
+expression: TEST_DATA.emit_no_color(&config)
+---
+warning: tab test
+  ┌─ tab_columns:1:2
+  │
+1 │     hello
+  │     ^^^^^
+2 │ ∙   hello
+  │     ^^^^^
+3 │ ∙∙  hello
+  │     ^^^^^
+4 │ ∙∙∙ hello
+  │     ^^^^^
+5 │ ∙∙∙∙    hello
+  │         ^^^^^
+6 │ ∙∙∙∙∙   hello
+  │         ^^^^^
+7 │ ∙∙∙∙∙∙  hello
+  │         ^^^^^
+
+
diff --git a/codespan-reporting/tests/snapshots/term__tabbed__tab_width_3_no_color.snap b/codespan-reporting/tests/snapshots/term__tabbed__tab_width_3_no_color.snap
index bb367ab..f7328c3 100644
--- a/codespan-reporting/tests/snapshots/term__tabbed__tab_width_3_no_color.snap
+++ b/codespan-reporting/tests/snapshots/term__tabbed__tab_width_3_no_color.snap
@@ -11,8 +11,8 @@ warning: unknown weapon `DogJaw`
 warning: unknown condition `attack-cooldown`
   ┌─ tabbed:4:23
   │
-4 │       ReloadingCondition:   attack-cooldown
-  │                             ^^^^^^^^^^^^^^^ the condition
+4 │       ReloadingCondition:  attack-cooldown
+  │                            ^^^^^^^^^^^^^^^ the condition
 
 warning: unknown field `Foo`
   ┌─ tabbed:5:2
diff --git a/codespan-reporting/tests/snapshots/term__tabbed__tab_width_6_no_color.snap b/codespan-reporting/tests/snapshots/term__tabbed__tab_width_6_no_color.snap
index 14e97a2..bcd1b2b 100644
--- a/codespan-reporting/tests/snapshots/term__tabbed__tab_width_6_no_color.snap
+++ b/codespan-reporting/tests/snapshots/term__tabbed__tab_width_6_no_color.snap
@@ -11,8 +11,8 @@ warning: unknown weapon `DogJaw`
 warning: unknown condition `attack-cooldown`
   ┌─ tabbed:4:23
   │
-4 │             ReloadingCondition:      attack-cooldown
-  │                                      ^^^^^^^^^^^^^^^ the condition
+4 │             ReloadingCondition:     attack-cooldown
+  │                                     ^^^^^^^^^^^^^^^ the condition
 
 warning: unknown field `Foo`
   ┌─ tabbed:5:2
diff --git a/codespan-reporting/tests/snapshots/term__tabbed__tab_width_default_no_color.snap b/codespan-reporting/tests/snapshots/term__tabbed__tab_width_default_no_color.snap
index 2ea4a27..d3a41a0 100644
--- a/codespan-reporting/tests/snapshots/term__tabbed__tab_width_default_no_color.snap
+++ b/codespan-reporting/tests/snapshots/term__tabbed__tab_width_default_no_color.snap
@@ -11,8 +11,8 @@ warning: unknown weapon `DogJaw`
 warning: unknown condition `attack-cooldown`
   ┌─ tabbed:4:23
   │
-4 │         ReloadingCondition:    attack-cooldown
-  │                                ^^^^^^^^^^^^^^^ the condition
+4 │         ReloadingCondition: attack-cooldown
+  │                             ^^^^^^^^^^^^^^^ the condition
 
 warning: unknown field `Foo`
   ┌─ tabbed:5:2
diff --git a/codespan-reporting/tests/term.rs b/codespan-reporting/tests/term.rs
index 600132a..ee02372 100644
--- a/codespan-reporting/tests/term.rs
+++ b/codespan-reporting/tests/term.rs
@@ -631,10 +631,83 @@ mod tabbed {
     fn tab_width_default_no_color() {
         let config = TEST_CONFIG.clone();
 
-        insta::assert_snapshot!(
-            "tab_width_default_no_color",
-            TEST_DATA.emit_no_color(&config)
-        );
+        insta::assert_snapshot!(TEST_DATA.emit_no_color(&config));
+    }
+
+    #[test]
+    fn tab_width_3_no_color() {
+        let config = Config {
+            tab_width: 3,
+            ..TEST_CONFIG.clone()
+        };
+
+        insta::assert_snapshot!(TEST_DATA.emit_no_color(&config));
+    }
+
+    #[test]
+    fn tab_width_6_no_color() {
+        let config = Config {
+            tab_width: 6,
+            ..TEST_CONFIG.clone()
+        };
+
+        insta::assert_snapshot!(TEST_DATA.emit_no_color(&config));
+    }
+}
+
+mod tab_columns {
+    use super::*;
+
+    lazy_static::lazy_static! {
+        static ref TEST_DATA: TestData<'static, SimpleFiles<&'static str, String>> = {
+            let mut files = SimpleFiles::new();
+
+            let source = [
+                "\thello",
+                "∙\thello",
+                "∙∙\thello",
+                "∙∙∙\thello",
+                "∙∙∙∙\thello",
+                "∙∙∙∙∙\thello",
+                "∙∙∙∙∙∙\thello",
+            ].join("\n");
+            let hello_ranges = source
+                .match_indices("hello")
+                .map(|(start, hello)| start..(start+hello.len()))
+                .collect::<Vec<_>>();
+
+            let file_id = files.add("tab_columns", source);
+
+            let diagnostics = vec![
+                Diagnostic::warning()
+                    .with_message("tab test")
+                    .with_labels(
+                        hello_ranges
+                            .into_iter()
+                            .map(|range| Label::primary(file_id, range))
+                            .collect(),
+                    ),
+            ];
+
+            TestData { files, diagnostics }
+        };
+    }
+
+    #[test]
+    fn tab_width_default_no_color() {
+        let config = TEST_CONFIG.clone();
+
+        insta::assert_snapshot!(TEST_DATA.emit_no_color(&config));
+    }
+
+    #[test]
+    fn tab_width_2_no_color() {
+        let config = Config {
+            tab_width: 2,
+            ..TEST_CONFIG.clone()
+        };
+
+        insta::assert_snapshot!(TEST_DATA.emit_no_color(&config));
     }
 
     #[test]