Repository: brendanzab/codespan (rust)

Issue:
Correctly handle tab indent widths
We need a test to check if we handle tab indentation properly when printing diagnostic labels. We may also need to add a 'tab width' display configuration.

See also: wycats/language-reporting#3

Pull request:
Print tabs properly
These were getting printed out of alignment because of how `unicode-width` was being used.

In order to get things to line up I replace the tabs in each source string with explicit spaces based on the emitter configuration.

Fixes #80

Patch:
diff --git a/codespan-reporting/src/emitter/mod.rs b/codespan-reporting/src/emitter/mod.rs
index c1ba1d5..df28bbb 100644
--- a/codespan-reporting/src/emitter/mod.rs
+++ b/codespan-reporting/src/emitter/mod.rs
@@ -9,6 +9,9 @@ mod views;
 /// Configures how a diagnostic is rendered.
 #[derive(Clone, Debug)]
 pub struct Config {
+    /// Column width of tabs.
+    /// Defaults to: `4`.
+    pub tab_width: usize,
     /// The color to use when rendering bugs.
     /// Defaults to `Color::Red`.
     pub bug_color: Color,
@@ -65,6 +68,7 @@ impl Default for Config {
         const BLUE: Color = Color::Blue;
 
         Config {
+            tab_width: 4,
             bug_color: Color::Red,
             error_color: Color::Red,
             warning_color: Color::Yellow,
@@ -95,6 +99,23 @@ impl Config {
             Severity::Help => self.help_color,
         }
     }
+
+    /// Measure the width of a string, taking into account the tab width.
+    pub fn width(&self, s: &str) -> usize {
+        use unicode_width::UnicodeWidthChar;
+
+        s.chars()
+            .map(|ch| match ch {
+                '\t' => self.tab_width,
+                _ => ch.width().unwrap_or(0),
+            })
+            .sum()
+    }
+
+    /// Get the amount of spaces we should use for printing tabs.
+    pub fn tab_padding(&self) -> String {
+        (0..self.tab_width).map(|_| ' ').collect()
+    }
 }
 
 pub fn emit(
@@ -116,79 +137,155 @@ mod tests {
     use crate::diagnostic::{Diagnostic, Label};
     use crate::termcolor::Buffer;
 
-    fn emit_fizz_buzz(writer: &mut impl WriteColor) {
-        let mut files = Files::new();
+    mod fizz_buzz {
+        use super::*;
 
-        let file_id = files.add(
-            "FizzBuzz.fun",
-            unindent::unindent(
-                r#"
-                    module FizzBuzz where
+        fn emit_test(writer: &mut impl WriteColor) {
+            let mut files = Files::new();
 
-                    fizz₁ : Nat → String
-                    fizz₁ num = case (mod num 5) (mod num 3) of
-                        0 0 => "FizzBuzz"
-                        0 _ => "Fizz"
-                        _ 0 => "Buzz"
-                        _ _ => num
+            let file_id = files.add(
+                "FizzBuzz.fun",
+                unindent::unindent(
+                    r#"
+                        module FizzBuzz where
 
-                    fizz₂ num =
-                        case (mod num 5) (mod num 3) of
+                        fizz₁ : Nat → String
+                        fizz₁ num = case (mod num 5) (mod num 3) of
                             0 0 => "FizzBuzz"
                             0 _ => "Fizz"
                             _ 0 => "Buzz"
                             _ _ => num
-                "#,
-            ),
-        );
-
-        let diagnostics = vec![
-            // Incompatible match clause error
-            Diagnostic::new_error(
-                "`case` clauses have incompatible types",
-                Label::new(file_id, 163..166, "expected `String`, found `Nat`"),
-            )
-            .with_code("E0308")
-            .with_notes(vec![unindent::unindent(
-                "
-                    expected type `String`
-                       found type `Nat`
-                ",
-            )])
-            .with_secondary_labels(vec![
-                Label::new(file_id, 62..166, "`case` clauses have incompatible types"),
-                Label::new(file_id, 41..47, "expected type `String` found here"),
-            ]),
-            // Incompatible match clause error
-            Diagnostic::new_error(
-                "`case` clauses have incompatible types",
-                Label::new(file_id, 303..306, "expected `String`, found `Nat`"),
-            )
-            .with_code("E0308")
-            .with_notes(vec![unindent::unindent(
-                "
-                    expected type `String`
-                       found type `Nat`
-                ",
-            )])
-            .with_secondary_labels(vec![
-                Label::new(file_id, 186..306, "`case` clauses have incompatible types"),
-                Label::new(file_id, 233..243, "this is found to be of type `String`"),
-                Label::new(file_id, 259..265, "this is found to be of type `String`"),
-                Label::new(file_id, 281..287, "this is found to be of type `String`"),
-            ]),
-        ];
-
-        for diagnostic in &diagnostics {
-            emit(writer, &Config::default(), &files, &diagnostic).unwrap();
+
+                        fizz₂ num =
+                            case (mod num 5) (mod num 3) of
+                                0 0 => "FizzBuzz"
+                                0 _ => "Fizz"
+                                _ 0 => "Buzz"
+                                _ _ => num
+                    "#,
+                ),
+            );
+
+            let diagnostics = vec![
+                // Incompatible match clause error
+                Diagnostic::new_error(
+                    "`case` clauses have incompatible types",
+                    Label::new(file_id, 163..166, "expected `String`, found `Nat`"),
+                )
+                .with_code("E0308")
+                .with_notes(vec![unindent::unindent(
+                    "
+                        expected type `String`
+                           found type `Nat`
+                    ",
+                )])
+                .with_secondary_labels(vec![
+                    Label::new(file_id, 62..166, "`case` clauses have incompatible types"),
+                    Label::new(file_id, 41..47, "expected type `String` found here"),
+                ]),
+                // Incompatible match clause error
+                Diagnostic::new_error(
+                    "`case` clauses have incompatible types",
+                    Label::new(file_id, 303..306, "expected `String`, found `Nat`"),
+                )
+                .with_code("E0308")
+                .with_notes(vec![unindent::unindent(
+                    "
+                        expected type `String`
+                           found type `Nat`
+                    ",
+                )])
+                .with_secondary_labels(vec![
+                    Label::new(file_id, 186..306, "`case` clauses have incompatible types"),
+                    Label::new(file_id, 233..243, "this is found to be of type `String`"),
+                    Label::new(file_id, 259..265, "this is found to be of type `String`"),
+                    Label::new(file_id, 281..287, "this is found to be of type `String`"),
+                ]),
+            ];
+
+            for diagnostic in &diagnostics {
+                emit(writer, &Config::default(), &files, &diagnostic).unwrap();
+            }
+        }
+
+        #[test]
+        fn no_color() {
+            let mut buffer = Buffer::no_color();
+            emit_test(&mut buffer);
+            let result = String::from_utf8_lossy(buffer.as_slice());
+            insta::assert_snapshot_matches!("no_color", result);
         }
     }
 
-    #[test]
-    fn fizz_buzz_no_color() {
-        let mut buffer = Buffer::no_color();
-        emit_fizz_buzz(&mut buffer);
-        let result = String::from_utf8_lossy(buffer.as_slice());
-        insta::assert_snapshot_matches!("fizz_buzz_no_color", result);
+    mod tabbed {
+        use super::*;
+
+        fn emit_test(writer: &mut impl WriteColor, config: &Config) {
+            let mut files = Files::new();
+
+            let file_id = files.add(
+                "tabbed",
+                [
+                    "Entity:",
+                    "\tArmament:",
+                    "\t\tWeapon: DogJaw",
+                    "\t\tReloadingCondition:\tattack-cooldown",
+                    "\tFoo: Bar",
+                ]
+                .join("\n"),
+            );
+
+            let diagnostics = vec![
+                Diagnostic::new_warning(
+                    "unknown weapon `DogJaw`",
+                    Label::new(file_id, 29..35, "the weapon"),
+                ),
+                Diagnostic::new_warning(
+                    "unknown condition `attack-cooldown`",
+                    Label::new(file_id, 58..73, "the condition"),
+                ),
+                Diagnostic::new_warning(
+                    "unknown field `Foo`",
+                    Label::new(file_id, 75..78, "the field"),
+                ),
+            ];
+
+            for diagnostic in &diagnostics {
+                emit(writer, config, &files, &diagnostic).unwrap();
+            }
+        }
+
+        #[test]
+        fn tab_width_default_no_color() {
+            let config = Config::default();
+            let mut buffer = Buffer::no_color();
+            emit_test(&mut buffer, &config);
+            let result = String::from_utf8_lossy(buffer.as_slice());
+            insta::assert_snapshot_matches!("tab_width_default_no_color", result);
+        }
+
+        #[test]
+        fn tab_width_3_no_color() {
+            let config = Config {
+                tab_width: 3,
+                ..Config::default()
+            };
+            let mut buffer = Buffer::no_color();
+            emit_test(&mut buffer, &config);
+            let result = String::from_utf8_lossy(buffer.as_slice());
+            insta::assert_snapshot_matches!("tab_width_3_no_color", result);
+        }
+
+        #[test]
+        fn tab_width_6_no_color() {
+            let config = Config {
+                tab_width: 6,
+                ..Config::default()
+            };
+            let mut buffer = Buffer::no_color();
+            emit_test(&mut buffer, &config);
+            let result = String::from_utf8_lossy(buffer.as_slice());
+            insta::assert_snapshot_matches!("tab_width_6_no_color", result);
+        }
     }
 }
diff --git a/codespan-reporting/src/emitter/snapshots/tabbed__tab_width_3_no_color.snap b/codespan-reporting/src/emitter/snapshots/tabbed__tab_width_3_no_color.snap
new file mode 100644
index 0000000..77f3dc3
--- /dev/null
+++ b/codespan-reporting/src/emitter/snapshots/tabbed__tab_width_3_no_color.snap
@@ -0,0 +1,31 @@
+---
+created: "2019-06-09T13:17:52.640671Z"
+creator: insta@0.8.1
+source: codespan-reporting/src/emitter/mod.rs
+expression: result
+---
+warning: unknown weapon `DogJaw`
+
+   ┌── tabbed:3:11 ───
+   │ 
+ 3 │       Weapon: DogJaw
+   │               ^^^^^^ the weapon
+   │ 
+
+warning: unknown condition `attack-cooldown`
+
+   ┌── tabbed:4:23 ───
+   │ 
+ 4 │       ReloadingCondition:   attack-cooldown
+   │                             ^^^^^^^^^^^^^^^ the condition
+   │ 
+
+warning: unknown field `Foo`
+
+   ┌── tabbed:5:2 ───
+   │ 
+ 5 │    Foo: Bar
+   │    ^^^ the field
+   │ 
+
+
diff --git a/codespan-reporting/src/emitter/snapshots/tabbed__tab_width_6_no_color.snap b/codespan-reporting/src/emitter/snapshots/tabbed__tab_width_6_no_color.snap
new file mode 100644
index 0000000..33fff7c
--- /dev/null
+++ b/codespan-reporting/src/emitter/snapshots/tabbed__tab_width_6_no_color.snap
@@ -0,0 +1,31 @@
+---
+created: "2019-06-09T13:17:52.640651Z"
+creator: insta@0.8.1
+source: codespan-reporting/src/emitter/mod.rs
+expression: result
+---
+warning: unknown weapon `DogJaw`
+
+   ┌── tabbed:3:11 ───
+   │ 
+ 3 │             Weapon: DogJaw
+   │                     ^^^^^^ the weapon
+   │ 
+
+warning: unknown condition `attack-cooldown`
+
+   ┌── tabbed:4:23 ───
+   │ 
+ 4 │             ReloadingCondition:      attack-cooldown
+   │                                      ^^^^^^^^^^^^^^^ the condition
+   │ 
+
+warning: unknown field `Foo`
+
+   ┌── tabbed:5:2 ───
+   │ 
+ 5 │       Foo: Bar
+   │       ^^^ the field
+   │ 
+
+
diff --git a/codespan-reporting/src/emitter/snapshots/tabbed__tab_width_default_no_color.snap b/codespan-reporting/src/emitter/snapshots/tabbed__tab_width_default_no_color.snap
new file mode 100644
index 0000000..2c0c510
--- /dev/null
+++ b/codespan-reporting/src/emitter/snapshots/tabbed__tab_width_default_no_color.snap
@@ -0,0 +1,31 @@
+---
+created: "2019-06-09T13:17:52.640651Z"
+creator: insta@0.8.1
+source: codespan-reporting/src/emitter/mod.rs
+expression: result
+---
+warning: unknown weapon `DogJaw`
+
+   ┌── tabbed:3:11 ───
+   │ 
+ 3 │         Weapon: DogJaw
+   │                 ^^^^^^ the weapon
+   │ 
+
+warning: unknown condition `attack-cooldown`
+
+   ┌── tabbed:4:23 ───
+   │ 
+ 4 │         ReloadingCondition:    attack-cooldown
+   │                                ^^^^^^^^^^^^^^^ the condition
+   │ 
+
+warning: unknown field `Foo`
+
+   ┌── tabbed:5:2 ───
+   │ 
+ 5 │     Foo: Bar
+   │     ^^^ the field
+   │ 
+
+
diff --git a/codespan-reporting/src/emitter/views/source_snippet.rs b/codespan-reporting/src/emitter/views/source_snippet.rs
index 195b6ad..413cfb8 100644
--- a/codespan-reporting/src/emitter/views/source_snippet.rs
+++ b/codespan-reporting/src/emitter/views/source_snippet.rs
@@ -1,7 +1,6 @@
 use codespan::{ByteIndex, Files, LineIndex, Location, Span};
 use std::io;
 use termcolor::{Color, ColorSpec, WriteColor};
-use unicode_width::UnicodeWidthStr;
 
 use crate::emitter::Config;
 use crate::{Diagnostic, Label, Severity};
@@ -70,8 +69,12 @@ impl<'a> SourceSnippet<'a> {
         self.files.location(self.label.file_id, byte_index)
     }
 
-    fn source_slice(&self, span: Span) -> Result<&'a str, impl std::error::Error> {
-        self.files.source_slice(self.label.file_id, span)
+    fn source_slice(&self, span: Span, tab: &'a str) -> Result<String, impl std::error::Error> {
+        // NOTE: Not sure if we can do this more efficiently? Perhaps a custom
+        // writer might be better?
+        self.files
+            .source_slice(self.label.file_id, span)
+            .map(|s| s.replace('\t', tab))
     }
 
     fn line_span(&self, line_index: LineIndex) -> Result<Span, impl std::error::Error> {
@@ -104,6 +107,8 @@ impl<'a> SourceSnippet<'a> {
 
         // Use the length of the last line number as the gutter padding
         let gutter_padding = format!("{}", end.line.number()).len();
+        // Cache the tabs we'll be using to pad the source strings.
+        let tab = config.tab_padding();
 
         // Top left border and locus.
         //
@@ -144,7 +149,7 @@ impl<'a> SourceSnippet<'a> {
 
         // Write source prefix before marked section
         let prefix_span = start_line_span.with_end(self.start());
-        let source_prefix = self.source_slice(prefix_span).expect("source_prefix");
+        let source_prefix = self.source_slice(prefix_span, &tab).expect("source_prefix");
         write!(writer, "{}", source_prefix)?;
 
         // Write marked section
@@ -152,17 +157,19 @@ impl<'a> SourceSnippet<'a> {
             // Single line
 
             // Write marked source section
-            let marked_source = self.source_slice(self.span()).expect("marked_source");
+            let marked_source = self.source_slice(self.span(), &tab).expect("marked_source");
             writer.set_color(&label_spec)?;
             write!(writer, "{}", marked_source)?;
             writer.reset()?;
-            marked_source.width()
+            config.width(&marked_source)
         } else {
             // Multiple lines
 
             // Write marked source section
             let marked_span = start_line_span.with_start(self.start());
-            let marked_source = self.source_slice(marked_span).expect("marked_source_1");
+            let marked_source = self
+                .source_slice(marked_span, &tab)
+                .expect("marked_source_1");
             writer.set_color(&label_spec)?;
             write!(writer, "{}", marked_source)?;
 
@@ -175,7 +182,9 @@ impl<'a> SourceSnippet<'a> {
 
                 // Write marked source section
                 let marked_span = self.line_span(line_index).expect("marked_span");
-                let marked_source = self.source_slice(marked_span).expect("marked_source_2");
+                let marked_source = self
+                    .source_slice(marked_span, &tab)
+                    .expect("marked_source_2");
                 writer.set_color(&label_spec)?;
                 write!(writer, "{}", marked_source.trim_end_matches(line_trimmer))?;
                 NewLine::new().emit(writer, config)?;
@@ -187,16 +196,18 @@ impl<'a> SourceSnippet<'a> {
 
             // Write marked source section
             let marked_span = end_line_span.with_end(self.end());
-            let marked_source = self.source_slice(marked_span).expect("marked_source_3");
+            let marked_source = self
+                .source_slice(marked_span, &tab)
+                .expect("marked_source_3");
             writer.set_color(&label_spec)?;
             write!(writer, "{}", marked_source)?;
             writer.reset()?;
-            marked_source.width()
+            config.width(&marked_source)
         };
 
         // Write source suffix after marked section
         let suffix_span = end_line_span.with_start(self.end());
-        let source_suffix = self.source_slice(suffix_span).expect("source_suffix");
+        let source_suffix = self.source_slice(suffix_span, &tab).expect("source_suffix");
         write!(writer, "{}", source_suffix.trim_end_matches(line_trimmer))?;
         NewLine::new().emit(writer, config)?;
 
@@ -210,7 +221,7 @@ impl<'a> SourceSnippet<'a> {
             writer,
             "{space: >width$}",
             space = "",
-            width = source_prefix.width(),
+            width = config.width(&source_prefix),
         )?;
         for _ in 0..mark_len {
             write!(writer, "{}", self.underline_char(config))?;


Test patch:
diff --git a/codespan-reporting/src/emitter/snapshots/tests__fizz_buzz_no_color.snap b/codespan-reporting/src/emitter/snapshots/fizz_buzz__no_color.snap
similarity index 97%
rename from codespan-reporting/src/emitter/snapshots/tests__fizz_buzz_no_color.snap
rename to codespan-reporting/src/emitter/snapshots/fizz_buzz__no_color.snap
index 7154da5..b66be2f 100644
--- a/codespan-reporting/src/emitter/snapshots/tests__fizz_buzz_no_color.snap
+++ b/codespan-reporting/src/emitter/snapshots/fizz_buzz__no_color.snap
@@ -1,5 +1,5 @@
 ---
-created: "2019-06-06T14:02:34.225144Z"
+created: "2019-06-09T12:57:51.853099Z"
 creator: insta@0.8.1
 source: codespan-reporting/src/emitter/mod.rs
 expression: result