Auto merge of #86532 - estebank:delete-suggestion-underline, r=petroc…

…henkov

Make deleted code in a suggestion clearer

Show suggestions that include deletions in a way similar to `diff`'s output.

<img width="628" alt="" src="https://user-images.githubusercontent.com/1606434/123350316-9078e580-d50f-11eb-89b9-78431b85e23f.png">

For changes that do not have deletions, use `+` as an underline for additions and `~` as an underline for replacements.

<img width="631" alt="" src="https://user-images.githubusercontent.com/1606434/123701745-1ac68f80-d817-11eb-950b-09e5afd7532f.png">

For multiline suggestions, we now use `~` in the gutter to signal replacements and `+` to signal whole line replacements/additions.

<img width="834" alt="" src="https://user-images.githubusercontent.com/1606434/123701331-8eb46800-d816-11eb-9dcd-ef9098071afb.png">

In all cases we now use color to highlight the specific spans and snippets.

compiler/rustc_errors/src/emitter.rs

@@ -14,7 +14,10 @@ use rustc_span::{MultiSpan, SourceFile, Span};
 
 use crate::snippet::{Annotation, AnnotationType, Line, MultilineAnnotation, Style, StyledString};
 use crate::styled_buffer::StyledBuffer;
-use crate::{CodeSuggestion, Diagnostic, DiagnosticId, Level, SubDiagnostic, SuggestionStyle};
+use crate::{
+    CodeSuggestion, Diagnostic, DiagnosticId, Level, SubDiagnostic, SubstitutionHighlight,
+    SuggestionStyle,
+};
 
 use rustc_lint_defs::pluralize;
 
@@ -1590,25 +1593,38 @@ impl EmitterWriter {
         );
 
         let mut row_num = 2;
+        draw_col_separator_no_space(&mut buffer, 1, max_line_num_len + 1);
         let mut notice_capitalization = false;
-        for (complete, parts, only_capitalization) in suggestions.iter().take(MAX_SUGGESTIONS) {
+        for (complete, parts, highlights, only_capitalization) in
+            suggestions.iter().take(MAX_SUGGESTIONS)
+        {
             notice_capitalization |= only_capitalization;
             // Only show underline if the suggestion spans a single line and doesn't cover the
             // entirety of the code output. If you have multiple replacements in the same line
             // of code, show the underline.
             let show_underline = !(parts.len() == 1 && parts[0].snippet.trim() == complete.trim())
                 && complete.lines().count() == 1;
 
-            let lines = sm
+            let has_deletion = parts.iter().any(|p| p.is_deletion());
+            let is_multiline = complete.lines().count() > 1;
+
+            let show_diff = has_deletion && !is_multiline;
+
+            if show_diff {
+                row_num += 1;
+            }
+
+            let file_lines = sm
                 .span_to_lines(parts[0].span)
                 .expect("span_to_lines failed when emitting suggestion");
 
-            assert!(!lines.lines.is_empty() || parts[0].span.is_dummy());
+            assert!(!file_lines.lines.is_empty() || parts[0].span.is_dummy());
 
             let line_start = sm.lookup_char_pos(parts[0].span.lo()).line;
             draw_col_separator_no_space(&mut buffer, 1, max_line_num_len + 1);
             let mut lines = complete.lines();
-            for (line_pos, line) in lines.by_ref().take(MAX_SUGGESTION_HIGHLIGHT_LINES).enumerate()
+            for (line_pos, (line, parts)) in
+                lines.by_ref().zip(highlights).take(MAX_SUGGESTION_HIGHLIGHT_LINES).enumerate()
             {
                 // Print the span column to avoid confusion
                 buffer.puts(
@@ -1617,9 +1633,60 @@ impl EmitterWriter {
                     &self.maybe_anonymized(line_start + line_pos),
                     Style::LineNumber,
                 );
+                if show_diff {
+                    // Add the line number for both addition and removal to drive the point home.
+                    //
+                    // N - fn foo<A: T>(bar: A) {
+                    // N + fn foo(bar: impl T) {
+                    buffer.puts(
+                        row_num - 1,
+                        0,
+                        &self.maybe_anonymized(line_start + line_pos),
+                        Style::LineNumber,
+                    );
+                    buffer.puts(row_num - 1, max_line_num_len + 1, "- ", Style::Removal);
+                    buffer.puts(
+                        row_num - 1,
+                        max_line_num_len + 3,
+                        &replace_tabs(
+                            &*file_lines
+                                .file
+                                .get_line(file_lines.lines[line_pos].line_index)
+                                .unwrap(),
+                        ),
+                        Style::NoStyle,
+                    );
+                    buffer.puts(row_num, max_line_num_len + 1, "+ ", Style::Addition);
+                } else if is_multiline {
+                    match &parts[..] {
+                        [SubstitutionHighlight { start: 0, end }] if *end == line.len() => {
+                            buffer.puts(row_num, max_line_num_len + 1, "+ ", Style::Addition);
+                        }
+                        [] => {
+                            draw_col_separator(&mut buffer, row_num, max_line_num_len + 1);
+                        }
+                        _ => {
+                            buffer.puts(row_num, max_line_num_len + 1, "~ ", Style::Addition);
+                        }
+                    }
+                } else {
+                    draw_col_separator(&mut buffer, row_num, max_line_num_len + 1);
+                }
+
                 // print the suggestion
-                draw_col_separator(&mut buffer, row_num, max_line_num_len + 1);
                 buffer.append(row_num, &replace_tabs(line), Style::NoStyle);
+
+                if is_multiline {
+                    for SubstitutionHighlight { start, end } in parts {
+                        buffer.set_style_range(
+                            row_num,
+                            max_line_num_len + 3 + start,
+                            max_line_num_len + 3 + end,
+                            Style::Addition,
+                            true,
+                        );
+                    }
+                }
                 row_num += 1;
             }
 
@@ -1654,25 +1721,36 @@ impl EmitterWriter {
                     let underline_start = (span_start_pos + start) as isize + offset;
                     let underline_end = (span_start_pos + start + sub_len) as isize + offset;
                     assert!(underline_start >= 0 && underline_end >= 0);
+                    let padding: usize = max_line_num_len + 3;
                     for p in underline_start..underline_end {
-                        buffer.putc(
-                            row_num,
-                            ((max_line_num_len + 3) as isize + p) as usize,
-                            '^',
-                            Style::UnderlinePrimary,
+                        // Colorize addition/replacements with green.
+                        buffer.set_style(
+                            row_num - 1,
+                            (padding as isize + p) as usize,
+                            Style::Addition,
+                            true,
                         );
-                    }
-                    // underline removals too
-                    if underline_start == underline_end {
-                        for p in underline_start - 1..underline_start + 1 {
+                        if !show_diff {
+                            // If this is a replacement, underline with `^`, if this is an addition
+                            // underline with `+`.
                             buffer.putc(
                                 row_num,
-                                ((max_line_num_len + 3) as isize + p) as usize,
-                                '-',
-                                Style::UnderlineSecondary,
+                                (padding as isize + p) as usize,
+                                if part.is_addition(&sm) { '+' } else { '~' },
+                                Style::Addition,
                             );
                         }
                     }
+                    if show_diff {
+                        // Colorize removal with red in diff format.
+                        buffer.set_style_range(
+                            row_num - 2,
+                            (padding as isize + span_start_pos as isize) as usize,
+                            (padding as isize + span_end_pos as isize) as usize,
+                            Style::Removal,
+                            true,
+                        );
+                    }
 
                     // length of the code after substitution
                     let full_sub_len = part
@@ -2129,6 +2207,12 @@ impl<'a> WritableDst<'a> {
     fn apply_style(&mut self, lvl: Level, style: Style) -> io::Result<()> {
         let mut spec = ColorSpec::new();
         match style {
+            Style::Addition => {
+                spec.set_fg(Some(Color::Green)).set_intense(true);
+            }
+            Style::Removal => {
+                spec.set_fg(Some(Color::Red)).set_intense(true);
+            }
             Style::LineAndColumn => {}
             Style::LineNumber => {
                 spec.set_bold(true);

compiler/rustc_errors/src/lib.rs

@@ -160,32 +160,77 @@ pub struct SubstitutionPart {
     pub snippet: String,
 }
 
+/// Used to translate between `Span`s and byte positions within a single output line in highlighted
+/// code of structured suggestions.
+#[derive(Debug, Clone, Copy)]
+pub struct SubstitutionHighlight {
+    start: usize,
+    end: usize,
+}
+
+impl SubstitutionPart {
+    pub fn is_addition(&self, sm: &SourceMap) -> bool {
+        !self.snippet.is_empty()
+            && sm
+                .span_to_snippet(self.span)
+                .map_or(self.span.is_empty(), |snippet| snippet.trim().is_empty())
+    }
+
+    pub fn is_deletion(&self) -> bool {
+        self.snippet.trim().is_empty()
+    }
+
+    pub fn is_replacement(&self, sm: &SourceMap) -> bool {
+        !self.snippet.is_empty()
+            && sm
+                .span_to_snippet(self.span)
+                .map_or(!self.span.is_empty(), |snippet| !snippet.trim().is_empty())
+    }
+}
+
 impl CodeSuggestion {
     /// Returns the assembled code suggestions, whether they should be shown with an underline
     /// and whether the substitution only differs in capitalization.
-    pub fn splice_lines(&self, sm: &SourceMap) -> Vec<(String, Vec<SubstitutionPart>, bool)> {
+    pub fn splice_lines(
+        &self,
+        sm: &SourceMap,
+    ) -> Vec<(String, Vec<SubstitutionPart>, Vec<Vec<SubstitutionHighlight>>, bool)> {
+        // For the `Vec<Vec<SubstitutionHighlight>>` value, the first level of the vector
+        // corresponds to the output snippet's lines, while the second level corresponds to the
+        // substrings within that line that should be highlighted.
+
         use rustc_span::{CharPos, Pos};
 
+        /// Append to a buffer the remainder of the line of existing source code, and return the
+        /// count of lines that have been added for accurate highlighting.
         fn push_trailing(
             buf: &mut String,
             line_opt: Option<&Cow<'_, str>>,
             lo: &Loc,
             hi_opt: Option<&Loc>,
-        ) {
+        ) -> usize {
+            let mut line_count = 0;
             let (lo, hi_opt) = (lo.col.to_usize(), hi_opt.map(|hi| hi.col.to_usize()));
             if let Some(line) = line_opt {
                 if let Some(lo) = line.char_indices().map(|(i, _)| i).nth(lo) {
                     let hi_opt = hi_opt.and_then(|hi| line.char_indices().map(|(i, _)| i).nth(hi));
                     match hi_opt {
-                        Some(hi) if hi > lo => buf.push_str(&line[lo..hi]),
+                        Some(hi) if hi > lo => {
+                            line_count = line[lo..hi].matches('\n').count();
+                            buf.push_str(&line[lo..hi])
+                        }
                         Some(_) => (),
-                        None => buf.push_str(&line[lo..]),
+                        None => {
+                            line_count = line[lo..].matches('\n').count();
+                            buf.push_str(&line[lo..])
+                        }
                     }
                 }
                 if hi_opt.is_none() {
                     buf.push('\n');
                 }
             }
+            line_count
         }
 
         assert!(!self.substitutions.is_empty());
@@ -220,6 +265,7 @@ impl CodeSuggestion {
                     return None;
                 }
 
+                let mut highlights = vec![];
                 // To build up the result, we do this for each span:
                 // - push the line segment trailing the previous span
                 //   (at the beginning a "phantom" span pointing at the start of the line)
@@ -236,17 +282,29 @@ impl CodeSuggestion {
                     lines.lines.get(0).and_then(|line0| sf.get_line(line0.line_index));
                 let mut buf = String::new();
 
+                let mut line_highlight = vec![];
                 for part in &substitution.parts {
                     let cur_lo = sm.lookup_char_pos(part.span.lo());
                     if prev_hi.line == cur_lo.line {
-                        push_trailing(&mut buf, prev_line.as_ref(), &prev_hi, Some(&cur_lo));
+                        let mut count =
+                            push_trailing(&mut buf, prev_line.as_ref(), &prev_hi, Some(&cur_lo));
+                        while count > 0 {
+                            highlights.push(std::mem::take(&mut line_highlight));
+                            count -= 1;
+                        }
                     } else {
-                        push_trailing(&mut buf, prev_line.as_ref(), &prev_hi, None);
+                        highlights.push(std::mem::take(&mut line_highlight));
+                        let mut count = push_trailing(&mut buf, prev_line.as_ref(), &prev_hi, None);
+                        while count > 0 {
+                            highlights.push(std::mem::take(&mut line_highlight));
+                            count -= 1;
+                        }
                         // push lines between the previous and current span (if any)
                         for idx in prev_hi.line..(cur_lo.line - 1) {
                             if let Some(line) = sf.get_line(idx) {
                                 buf.push_str(line.as_ref());
                                 buf.push('\n');
+                                highlights.push(std::mem::take(&mut line_highlight));
                             }
                         }
                         if let Some(cur_line) = sf.get_line(cur_lo.line - 1) {
@@ -257,10 +315,21 @@ impl CodeSuggestion {
                             buf.push_str(&cur_line[..end]);
                         }
                     }
+                    // Add a whole line highlight per line in the snippet.
+                    line_highlight.push(SubstitutionHighlight {
+                        start: cur_lo.col.0,
+                        end: cur_lo.col.0
+                            + part.snippet.split('\n').next().unwrap_or(&part.snippet).len(),
+                    });
+                    for line in part.snippet.split('\n').skip(1) {
+                        highlights.push(std::mem::take(&mut line_highlight));
+                        line_highlight.push(SubstitutionHighlight { start: 0, end: line.len() });
+                    }
                     buf.push_str(&part.snippet);
                     prev_hi = sm.lookup_char_pos(part.span.hi());
                     prev_line = sf.get_line(prev_hi.line - 1);
                 }
+                highlights.push(std::mem::take(&mut line_highlight));
                 let only_capitalization = is_case_difference(sm, &buf, bounding_span);
                 // if the replacement already ends with a newline, don't print the next line
                 if !buf.ends_with('\n') {
@@ -270,7 +339,7 @@ impl CodeSuggestion {
                 while buf.ends_with('\n') {
                     buf.pop();
                 }
-                Some((buf, substitution.parts, only_capitalization))
+                Some((buf, substitution.parts, highlights, only_capitalization))
             })
             .collect()
     }

compiler/rustc_errors/src/snippet.rs

@@ -177,4 +177,6 @@ pub enum Style {
     NoStyle,
     Level(Level),
     Highlight,
+    Addition,
+    Removal,
 }

src/test/rustdoc-ui/infinite-recursive-type-impl-trait-return.stderr

@@ -10,7 +10,7 @@ LL |         This(E),
 help: insert some indirection (e.g., a `DEF_ID` representable
    |
 LL |         This(Box<E>),
-   |              ^^^^ ^
+   |              ++++ +
 
 error: aborting due to previous error
 

src/test/rustdoc-ui/infinite-recursive-type-impl-trait.stderr

@@ -10,7 +10,7 @@ LL |         V(E),
 help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `f::E` representable
    |
 LL |         V(Box<E>),
-   |           ^^^^ ^
+   |           ++++ +
 
 error: aborting due to previous error
 

src/test/rustdoc-ui/infinite-recursive-type.stderr

@@ -10,7 +10,7 @@ LL |     V(E),
 help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `E` representable
    |
 LL |     V(Box<E>),
-   |       ^^^^ ^
+   |       ++++ +
 
 error: aborting due to previous error