Merge pull request #380 from Muscraft/fix-patch-tabs-last-line

fix: correctly count tabs for last patch line

Author: epage

src/renderer/render.rs

@@ -1723,64 +1723,42 @@ fn emit_suggestion_default(
                     // logic to show the whole prior snippet, but the current output is not
                     // too bad to begin with, so we side-step that issue here.
                     for (i, line) in snippet.lines().enumerate() {
-                        let full_line = if i == 0 {
-                            // `snippet` starts at the first character that must be edited,
-                            // which often does not contain the indent of the line. Retrieve
-                            // the full line in order to avoid missing leading tabs.
-                            sm.get_line(span_start.line).unwrap_or_default()
-                        } else {
-                            line
-                        };
-
-                        let tabs: usize = full_line
-                            .chars()
-                            .take(span_start.char)
-                            .map(|ch| match ch {
-                                '\t' => 3,
-                                _ => 0,
-                            })
-                            .sum();
-                        let line = normalize_whitespace(line);
+                        let norm_line = normalize_whitespace(line);
                         // Going lower than buffer_offset (+ 1) would mean
                         // overwriting existing content in the buffer
                         let min_row = buffer_offset + usize::from(!matches_previous_suggestion);
                         let row = (row_num - 2 - (offset - i - 1)).max(min_row);
-                        // On the first line, we highlight between the start of the part
-                        // span, and the end of that line.
-                        // On the last line, we highlight between the start of the line, and
-                        // the column of the part span end.
-                        // On all others, we highlight the whole line.
-                        let start = if i == 0 {
-                            (padding as isize + (span_start.char + tabs) as isize) as usize
-                        } else {
-                            padding
-                        };
-                        let end = if i == 0 {
-                            (padding as isize
-                                + (span_start.char + tabs) as isize
-                                + line.chars().count() as isize)
-                                as usize
+                        let (start, end) = if i == 0 {
+                            // On the first line, we highlight between the start of the part
+                            // span, and the end of that line.
+                            let full_line = sm.get_line(span_start.line).unwrap_or_default();
+                            let extra_width = extra_width_from_tabs(full_line, span_start.char);
+                            let start = span_start.char + extra_width;
+                            (start, start + norm_line.chars().count())
                         } else if i == newlines - 1 {
-                            (padding as isize + (span_end.char + tabs) as isize) as usize
+                            // On the last line, we highlight between the start of the line, and
+                            // the column of the part span end.
+                            let extra_width = extra_width_from_tabs(line, span_end.char);
+                            (0, span_end.char + extra_width)
                         } else {
-                            (padding as isize + line.chars().count() as isize) as usize
+                            // On all others, we highlight the whole line.
+                            (0, norm_line.chars().count())
                         };
-                        buffer.set_style_range(row, start, end, ElementStyle::Removal, true);
+                        buffer.set_style_range(
+                            row,
+                            padding + start,
+                            padding + end,
+                            ElementStyle::Removal,
+                            true,
+                        );
                     }
                 } else {
-                    let tabs: usize = snippet
-                        .chars()
-                        .take(span_start.char)
-                        .map(|ch| match ch {
-                            '\t' => 3,
-                            _ => 0,
-                        })
-                        .sum();
+                    let extra_width: usize = extra_width_from_tabs(snippet, span_start.char);
                     // The removed code fits all in one line.
                     buffer.set_style_range(
                         row_num - 2,
-                        (padding as isize + (span_start.char + tabs) as isize) as usize,
-                        (padding as isize + (span_end.char + tabs) as isize) as usize,
+                        span_start.char + extra_width,
+                        span_end.char + extra_width,
                         ElementStyle::Removal,
                         true,
                     );
@@ -1992,18 +1970,11 @@ fn draw_code_line(
         // This is a no-op for empty ranges
         if start != end {
             // Account for tabs when highlighting (#87972).
-            let tabs: usize = line_to_add
-                .chars()
-                .take(start)
-                .map(|ch| match ch {
-                    '\t' => 3,
-                    _ => 0,
-                })
-                .sum();
+            let extra_width: usize = extra_width_from_tabs(line_to_add, start);
             buffer.set_style_range(
                 *row_num,
-                max_line_num_len + 3 + start + tabs,
-                max_line_num_len + 3 + end + tabs,
+                max_line_num_len + 3 + start + extra_width,
+                max_line_num_len + 3 + end + extra_width,
                 ElementStyle::Addition,
                 true,
             );
@@ -2311,6 +2282,12 @@ impl MessageOrTitle for Message<'_> {
     }
 }
 
+/// Count extra display columns from tabs in the first `n` chars of `s`.
+/// Each tab is displayed as 4 spaces, so the extra width per tab is 3.
+fn extra_width_from_tabs(s: &str, n: usize) -> usize {
+    s.chars().take(n).filter(|&ch| ch == '\t').count() * 3
+}
+
 // instead of taking the String length or dividing by 10 while > 0, we multiply a limit by 10 until
 // we're higher. If the loop isn't exited by the `return`, the last multiplication will wrap, which
 // is OK, because while we cannot fit a higher power of 10 in a usize, the loop will end anyway.

tests/color/main.rs

@@ -14,6 +14,7 @@ mod highlight_duplicated_diff_lines;
 mod highlight_first_line_tab_371;
 mod highlight_source;
 mod issue_9;
+mod multiline_removal_last_line_tabs;
 mod multiline_removal_suggestion;
 mod multiple_annotations;
 mod multiple_highlight_duplicated;

tests/color/multiline_removal_last_line_tabs.ascii.term.svg

@@ -0,0 +1,18 @@
+use annotate_snippets::{renderer::DecorStyle, Group, Level, Patch, Renderer, Snippet};
+
+use snapbox::{assert_data_eq, file};
+
+#[test]
+fn test() {
+    let report = &[Group::with_level(Level::ERROR).element(
+        Snippet::source("a.foo(|t| {\n\t\t\tb\n\t\t\t}).bar()\n").patch(Patch::new(1..22, "")),
+    )];
+
+    let expected_ascii = file!["multiline_removal_last_line_tabs.ascii.term.svg": TermSvg];
+    let renderer = Renderer::styled();
+    assert_data_eq!(renderer.render(report), expected_ascii);
+
+    let expected_unicode = file!["multiline_removal_last_line_tabs.unicode.term.svg": TermSvg];
+    let renderer = renderer.decor_style(DecorStyle::Unicode);
+    assert_data_eq!(renderer.render(report), expected_unicode);
+}