Rollup merge of #97798 - WaffleLapkin:allow_for_suggestions_that_are_quite_far_away_from_each_other, r=estebank

Hide irrelevant lines in suggestions to allow for suggestions that are far from each other to be shown

This is an attempt to fix suggestions one part of which is 6 lines or more far from the first. I've noticed "the problem" (of not showing some parts of the suggestion) here: https://github.com/rust-lang/rust/pull/97759#discussion_r889689230.

I'm not sure about the implementation (this big closure is just bad and makes already complicated code even more so), but I want to at least discuss the result.

Here is an example of how this changes the output:

Before:
```text
help: consider enclosing expression in a block
  |
3 ~     'l: { match () { () => break 'l,
4 |
5 |
6 |
7 |
8 |
...
```

After:
```text
help: consider enclosing expression in a block
  |
3 ~     'l: { match () { () => break 'l,
4 |
...
31|
32~ } };
  |
```

r? `@estebank`
`@rustbot` label +A-diagnostics +A-suggestion-diagnostics

compiler/rustc_errors/src/emitter.rs

@@ -10,7 +10,7 @@
 use Destination::*;
 
 use rustc_span::source_map::SourceMap;
-use rustc_span::{SourceFile, Span};
+use rustc_span::{FileLines, SourceFile, Span};
 
 use crate::snippet::{Annotation, AnnotationType, Line, MultilineAnnotation, Style, StyledString};
 use crate::styled_buffer::StyledBuffer;
@@ -1761,12 +1761,6 @@ impl EmitterWriter {
             let has_deletion = parts.iter().any(|p| p.is_deletion());
             let is_multiline = complete.lines().count() > 1;
 
-            enum DisplaySuggestion {
-                Underline,
-                Diff,
-                None,
-            }
-
             if let Some(span) = span.primary_span() {
                 // Compare the primary span of the diagnostic with the span of the suggestion
                 // being emitted.  If they belong to the same file, we don't *need* to show the
@@ -1839,79 +1833,94 @@ impl EmitterWriter {
                 }
                 row_num += line_end - line_start;
             }
-            for (line_pos, (line, highlight_parts)) in
-                lines.by_ref().zip(highlights).take(MAX_SUGGESTION_HIGHLIGHT_LINES).enumerate()
-            {
-                // Print the span column to avoid confusion
-                buffer.puts(
-                    row_num,
-                    0,
-                    &self.maybe_anonymized(line_start + line_pos),
-                    Style::LineNumber,
-                );
-                if let DisplaySuggestion::Diff = show_code_change {
-                    // Add the line number for both addition and removal to drive the point home.
+            let mut unhighlighted_lines = Vec::new();
+            for (line_pos, (line, highlight_parts)) in lines.by_ref().zip(highlights).enumerate() {
+                debug!(%line_pos, %line, ?highlight_parts);
+
+                // Remember lines that are not highlighted to hide them if needed
+                if highlight_parts.is_empty() {
+                    unhighlighted_lines.push((line_pos, line));
+                    continue;
+                }
+
+                match unhighlighted_lines.len() {
+                    0 => (),
+                    // Since we show first line, "..." line and last line,
+                    // There is no reason to hide if there are 3 or less lines
+                    // (because then we just replace a line with ... which is
+                    // not helpful)
+                    n if n <= 3 => unhighlighted_lines.drain(..).for_each(|(p, l)| {
+                        self.draw_code_line(
+                            &mut buffer,
+                            &mut row_num,
+                            &Vec::new(),
+                            p,
+                            l,
+                            line_start,
+                            show_code_change,
+                            max_line_num_len,
+                            &file_lines,
+                            is_multiline,
+                        )
+                    }),
+                    // Print first unhighlighted line, "..." and last unhighlighted line, like so:
                     //
-                    // 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,
-                        &normalize_whitespace(
-                            &*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 &highlight_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);
-                        }
+                    // LL | this line was highlighted
+                    // LL | this line is just for context
+                    //   ...
+                    // LL | this line is just for context
+                    // LL | this line was highlighted
+                    _ => {
+                        let last_line = unhighlighted_lines.pop();
+                        let first_line = unhighlighted_lines.drain(..).next();
+
+                        first_line.map(|(p, l)| {
+                            self.draw_code_line(
+                                &mut buffer,
+                                &mut row_num,
+                                &Vec::new(),
+                                p,
+                                l,
+                                line_start,
+                                show_code_change,
+                                max_line_num_len,
+                                &file_lines,
+                                is_multiline,
+                            )
+                        });
+
+                        buffer.puts(row_num, max_line_num_len - 1, "...", Style::LineNumber);
+                        row_num += 1;
+
+                        last_line.map(|(p, l)| {
+                            self.draw_code_line(
+                                &mut buffer,
+                                &mut row_num,
+                                &Vec::new(),
+                                p,
+                                l,
+                                line_start,
+                                show_code_change,
+                                max_line_num_len,
+                                &file_lines,
+                                is_multiline,
+                            )
+                        });
                     }
-                } else {
-                    draw_col_separator(&mut buffer, row_num, max_line_num_len + 1);
                 }
 
-                // print the suggestion
-                buffer.append(row_num, &normalize_whitespace(line), Style::NoStyle);
-
-                // Colorize addition/replacements with green.
-                for &SubstitutionHighlight { start, end } in highlight_parts {
-                    // Account for tabs when highlighting (#87972).
-                    let tabs: usize = line
-                        .chars()
-                        .take(start)
-                        .map(|ch| match ch {
-                            '\t' => 3,
-                            _ => 0,
-                        })
-                        .sum();
-                    buffer.set_style_range(
-                        row_num,
-                        max_line_num_len + 3 + start + tabs,
-                        max_line_num_len + 3 + end + tabs,
-                        Style::Addition,
-                        true,
-                    );
-                }
-                row_num += 1;
+                self.draw_code_line(
+                    &mut buffer,
+                    &mut row_num,
+                    highlight_parts,
+                    line_pos,
+                    line,
+                    line_start,
+                    show_code_change,
+                    max_line_num_len,
+                    &file_lines,
+                    is_multiline,
+                )
             }
 
             // This offset and the ones below need to be signed to account for replacement code
@@ -2096,6 +2105,90 @@ impl EmitterWriter {
             }
         }
     }
+
+    fn draw_code_line(
+        &self,
+        buffer: &mut StyledBuffer,
+        row_num: &mut usize,
+        highlight_parts: &Vec<SubstitutionHighlight>,
+        line_pos: usize,
+        line: &str,
+        line_start: usize,
+        show_code_change: DisplaySuggestion,
+        max_line_num_len: usize,
+        file_lines: &FileLines,
+        is_multiline: bool,
+    ) {
+        // Print the span column to avoid confusion
+        buffer.puts(*row_num, 0, &self.maybe_anonymized(line_start + line_pos), Style::LineNumber);
+        if let DisplaySuggestion::Diff = show_code_change {
+            // 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,
+                &normalize_whitespace(
+                    &*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 &highlight_parts[..] {
+                [SubstitutionHighlight { start: 0, end }] if *end == line.len() => {
+                    buffer.puts(*row_num, max_line_num_len + 1, "+ ", Style::Addition);
+                }
+                [] => {
+                    draw_col_separator(buffer, *row_num, max_line_num_len + 1);
+                }
+                _ => {
+                    buffer.puts(*row_num, max_line_num_len + 1, "~ ", Style::Addition);
+                }
+            }
+        } else {
+            draw_col_separator(buffer, *row_num, max_line_num_len + 1);
+        }
+
+        // print the suggestion
+        buffer.append(*row_num, &normalize_whitespace(line), Style::NoStyle);
+
+        // Colorize addition/replacements with green.
+        for &SubstitutionHighlight { start, end } in highlight_parts {
+            // Account for tabs when highlighting (#87972).
+            let tabs: usize = line
+                .chars()
+                .take(start)
+                .map(|ch| match ch {
+                    '\t' => 3,
+                    _ => 0,
+                })
+                .sum();
+            buffer.set_style_range(
+                *row_num,
+                max_line_num_len + 3 + start + tabs,
+                max_line_num_len + 3 + end + tabs,
+                Style::Addition,
+                true,
+            );
+        }
+        *row_num += 1;
+    }
+}
+
+#[derive(Clone, Copy)]
+enum DisplaySuggestion {
+    Underline,
+    Diff,
+    None,
 }
 
 impl FileWithAnnotatedLines {

compiler/rustc_errors/src/lib.rs

@@ -1174,7 +1174,7 @@ impl HandlerInner {
             !this.emitted_diagnostics.insert(diagnostic_hash)
         };
 
-        // Only emit the diagnostic if we've been asked to deduplicate and
+        // Only emit the diagnostic if we've been asked to deduplicate or
         // haven't already emitted an equivalent diagnostic.
         if !(self.flags.deduplicate_diagnostics && already_emitted(self)) {
             debug!(?diagnostic);

src/test/ui/associated-consts/issue-93835.stderr

@@ -19,11 +19,10 @@ help: you might have meant to write a `struct` literal
    |
 LL ~ fn e() { SomeStruct {
 LL |     p:a<p:p<e=6>>
-LL |
-LL |
-LL |
-LL |
  ...
+LL |
+LL ~ }}
+   |
 help: maybe you meant to write a path separator here
    |
 LL |     p::a<p:p<e=6>>

src/test/ui/closures/2229_closure_analysis/migrations/auto_traits.stderr

@@ -17,11 +17,10 @@ help: add a dummy let to cause `fptr` to be fully captured
    |
 LL ~     thread::spawn(move || { let _ = &fptr; unsafe {
 LL |
-LL |
-LL |
-LL |
-LL |         *fptr.0 = 20;
  ...
+LL |
+LL ~     } });
+   |
 
 error: changes to closure capture in Rust 2021 will affect which traits the closure implements
   --> $DIR/auto_traits.rs:42:19
@@ -39,12 +38,11 @@ LL |         *fptr.0.0 = 20;
 help: add a dummy let to cause `fptr` to be fully captured
    |
 LL ~     thread::spawn(move || { let _ = &fptr; unsafe {
-LL |
-LL |
-LL |
-LL |
 LL |
  ...
+LL |
+LL ~     } });
+   |
 
 error: changes to closure capture in Rust 2021 will affect drop order and which traits the closure implements
   --> $DIR/auto_traits.rs:67:13

src/test/ui/closures/2229_closure_analysis/migrations/multi_diagnostics.stderr

@@ -108,12 +108,11 @@ LL |         *fptr2.0 = 20;
 help: add a dummy let to cause `fptr1`, `fptr2` to be fully captured
    |
 LL ~     thread::spawn(move || { let _ = (&fptr1, &fptr2); unsafe {
-LL |
-LL |
-LL |
-LL |
 LL |
  ...
+LL |
+LL ~     } });
+   |
 
 error: aborting due to 5 previous errors
 

src/test/ui/imports/issue-59764.stderr

@@ -209,8 +209,7 @@ help: a macro with this name exists at the root of the crate
    |
 LL ~         issue_59764::{makro as foobar, 
 LL | 
-LL |             foobaz,
-LL | 
+ ...
 LL | 
 LL ~             foo::{baz}
    |

src/test/ui/issues/issue-22644.stderr

@@ -89,12 +89,11 @@ LL |                    5);
 help: try comparing the cast value
    |
 LL ~     println!("{}", (a
-LL | 
-LL | 
-LL |                    as
-LL | 
 LL | 
  ...
+LL | 
+LL ~                    usize)
+   |
 
 error: `<` is interpreted as a start of generic arguments for `usize`, not a shift
   --> $DIR/issue-22644.rs:32:31

src/test/ui/let-else/let-else-if.stderr

@@ -8,8 +8,7 @@ help: try placing this code inside a block
    |
 LL ~     let Some(_) = Some(()) else { if true {
 LL |
-LL |         return;
-LL |     } else {
+ ...
 LL |         return;
 LL ~     } };
    |

src/test/ui/parser/recover-labeled-non-block-expr.stderr

@@ -54,11 +54,10 @@ help: consider enclosing expression in a block
    |
 LL ~     let _i = 'label: { match x {
 LL |         0 => 42,
-LL |         1 if false => break 'label 17,
-LL |         1 => {
-LL |             if true {
-LL |                 break 'label 13
  ...
+LL |         _ => 1,
+LL ~     } };
+   |
 
 error: expected `while`, `for`, `loop` or `{` after a label
   --> $DIR/recover-labeled-non-block-expr.rs:26:24

src/test/ui/type/ascription/issue-34255-1.stderr

@@ -8,8 +8,7 @@ help: you might have meant to write a `struct` literal
    |
 LL ~     pub fn new() -> Self { SomeStruct {
 LL |         input_cells: Vec::new()
-LL |
-LL |
+ ...
 LL |
 LL ~     }}
    |

src/test/ui/unboxed-closures/unboxed-closure-sugar-lifetime-elision.stderr

@@ -9,11 +9,10 @@ help: consider introducing a named lifetime parameter
    |
 LL ~ fn main<'a>() {
 LL |     eq::< dyn for<'a> Foo<(&'a isize,), Output=&'a isize>,
-LL |           dyn Foo(&isize) -> &isize                                   >();
-LL |     eq::< dyn for<'a> Foo<(&'a isize,), Output=(&'a isize, &'a isize)>,
-LL |           dyn Foo(&isize) -> (&isize, &isize)                           >();
-LL | 
  ...
+LL | 
+LL ~     let _: dyn Foo(&'a isize, &'a usize) -> &'a usize;
+   |
 
 error: aborting due to previous error
 

src/tools/clippy/tests/ui/bind_instead_of_map_multipart.stderr

@@ -56,7 +56,25 @@ LL |             if s == "43" {
 LL ~                 return 43;
 LL |             }
 LL |             s == "42"
+LL |         } {
+LL ~             return 45;
+LL |         }
+LL |         match s.len() {
+LL ~             10 => 2,
+LL |             20 => {
  ...
+LL |                         if foo() {
+LL ~                             return 20;
+LL |                         }
+LL |                         println!("foo");
+LL ~                         3
+LL |                     };
+LL |                 }
+LL ~                 20
+LL |             },
+LL ~             40 => 30,
+LL ~             _ => 1,
+   |
 
 error: using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`
   --> $DIR/bind_instead_of_map_multipart.rs:61:13

src/tools/clippy/tests/ui/branches_sharing_code/shared_at_top_and_bottom.stderr

@@ -98,7 +98,8 @@ LL +         id: e_id,
 LL +         name: "Player 1".to_string(),
 LL +         some_data: vec![0x12, 0x34, 0x56, 0x78, 0x90],
 LL +     };
- ...
+LL +     process_data(pack);
+   |
 
 error: all if blocks contain the same code at both the start and the end
   --> $DIR/shared_at_top_and_bottom.rs:94:5

src/tools/clippy/tests/ui/entry.stderr

@@ -28,7 +28,8 @@ LL +             v
 LL +         } else {
 LL +             v2
 LL +         }
- ...
+LL +     });
+   |
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
   --> $DIR/entry.rs:38:5
@@ -50,7 +51,8 @@ LL +             v
 LL +         } else {
 LL +             v2
 LL +         }
- ...
+LL +     });
+   |
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
   --> $DIR/entry.rs:47:5
@@ -72,7 +74,9 @@ LL +             e.insert(v);
 LL +         } else {
 LL +             e.insert(v2);
 LL +             return;
- ...
+LL +         }
+LL +     }
+   |
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
   --> $DIR/entry.rs:57:5
@@ -111,7 +115,11 @@ LL +             1 if true => {
 LL +                 v
 LL +             },
 LL +             _ => {
- ...
+LL +                 v2
+LL +             },
+LL +         }
+LL +     });
+   |
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
   --> $DIR/entry.rs:75:5
@@ -133,7 +141,9 @@ LL +             0 => foo(),
 LL +             _ => {
 LL +                 e.insert(v2);
 LL +             },
- ...
+LL +         };
+LL +     }
+   |
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
   --> $DIR/entry.rs:85:5
@@ -155,7 +165,26 @@ LL +         match 0 {
 LL +             0 if false => {
 LL +                 v
 LL +             },
- ...
+LL +             1 => {
+LL +                 foo();
+LL +                 v
+LL +             },
+LL +             2 | 3 => {
+LL +                 for _ in 0..2 {
+LL +                     foo();
+LL +                 }
+LL +                 if true {
+LL +                     v
+LL +                 } else {
+LL +                     v2
+LL +                 }
+LL +             },
+LL +             _ => {
+LL +                 v2
+LL +             },
+LL +         }
+LL +     });
+   |
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
   --> $DIR/entry.rs:119:5

src/tools/clippy/tests/ui/entry_with_else.stderr

@@ -17,7 +17,9 @@ LL +             e.insert(v);
 LL +         }
 LL +         std::collections::hash_map::Entry::Occupied(mut e) => {
 LL +             e.insert(v2);
- ...
+LL +         }
+LL +     }
+   |
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
   --> $DIR/entry_with_else.rs:22:5
@@ -37,7 +39,9 @@ LL +             e.insert(v);
 LL +         }
 LL +         std::collections::hash_map::Entry::Vacant(e) => {
 LL +             e.insert(v2);
- ...
+LL +         }
+LL +     }
+   |
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
   --> $DIR/entry_with_else.rs:28:5
@@ -95,7 +99,9 @@ LL +             e.insert(v);
 LL +         }
 LL +         std::collections::hash_map::Entry::Occupied(mut e) => {
 LL +             e.insert(v2);
- ...
+LL +         }
+LL +     }
+   |
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
   --> $DIR/entry_with_else.rs:46:5
@@ -115,7 +121,10 @@ LL +             if true { Some(e.insert(v)) } else { Some(e.insert(v2)) }
 LL +         }
 LL +         std::collections::hash_map::Entry::Vacant(e) => {
 LL +             e.insert(v);
- ...
+LL +             None
+LL +         }
+LL ~     };
+   |
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
   --> $DIR/entry_with_else.rs:52:5

src/tools/clippy/tests/ui/let_unit.stderr

@@ -32,7 +32,8 @@ LL +         .map(|i| i * 2)
 LL +         .filter(|i| i % 2 == 0)
 LL +         .map(|_| ())
 LL +         .next()
- ...
+LL +         .unwrap();
+   |
 
 error: this let-binding has unit value
   --> $DIR/let_unit.rs:80:5

src/tools/clippy/tests/ui/manual_async_fn.stderr

@@ -122,7 +122,14 @@ LL +         let a = 42;
 LL +         let b = 21;
 LL +         if a < b {
 LL +             let c = 21;
- ...
+LL +             let d = 42;
+LL +             if c < d {
+LL +                 let _ = 42;
+LL +             }
+LL +         }
+LL +         42
+LL +     }
+   |
 
 error: this function can be simplified using the `async fn` syntax
   --> $DIR/manual_async_fn.rs:92:1

src/tools/clippy/tests/ui/needless_for_each_unfixable.stderr

@@ -19,7 +19,8 @@ LL +             return;
 LL +         } else {
 LL +             println!("{}", v);
 LL +         }
- ...
+LL +     }
+   |
 help: ...and replace `return` with `continue`
    |
 LL |             continue;

src/tools/clippy/tests/ui/new_without_default.stderr

@@ -117,7 +117,8 @@ LL +             Self::new()
 LL +         }
 LL +     }
 LL + 
- ...
+LL ~     impl<T> Foo<T> {
+   |
 
 error: aborting due to 7 previous errors
 

src/tools/clippy/tests/ui/ptr_arg.stderr

@@ -56,7 +56,8 @@ LL |     let f = e.clone(); // OK
 LL |     let g = x;
 LL ~     let h = g.to_owned();
 LL |     let i = (e).clone();
- ...
+LL ~     x.to_owned()
+   |
 
 error: writing `&String` instead of `&str` involves a new object where a slice will do
   --> $DIR/ptr_arg.rs:57:18

src/tools/clippy/tests/ui/significant_drop_in_scrutinee.stderr

@@ -188,7 +188,9 @@ LL +         _ => mutex2.lock().unwrap(),
 LL +     }
 LL +     .s
 LL +     .len()
- ...
+LL +         > 1;
+LL ~     match value
+   |
 
 error: temporary with significant drop in match scrutinee
   --> $DIR/significant_drop_in_scrutinee.rs:397:11
@@ -211,7 +213,10 @@ LL +     } else {
 LL +         mutex2.lock().unwrap()
 LL +     }
 LL +     .s
- ...
+LL +     .len()
+LL +         > 1;
+LL ~     match value
+   |
 
 error: temporary with significant drop in match scrutinee
   --> $DIR/significant_drop_in_scrutinee.rs:451:11

src/tools/clippy/tests/ui/unit_arg.stderr

@@ -137,7 +137,13 @@ LL +         foo(1);
 LL +     };
 LL +     {
 LL +         foo(2);
- ...
+LL +         foo(3);
+LL +     };
+LL +     taking_multiple_units(
+LL +         (),
+LL +         (),
+LL ~     );
+   |
 
 error: passing a unit value to a function
   --> $DIR/unit_arg.rs:85:13

src/tools/clippy/tests/ui/unnecessary_wraps.stderr

@@ -23,7 +23,8 @@ LL |     if a {
 LL |         Some(-1);
 LL ~         2
 LL |     } else {
- ...
+LL ~         return 1337;
+   |
 
 error: this function's return value is unnecessarily wrapped by `Option`
   --> $DIR/unnecessary_wraps.rs:21:1
@@ -122,7 +123,8 @@ LL |     if a {
 LL |         Some(());
 LL ~         
 LL |     } else {
- ...
+LL ~         return ;
+   |
 
 error: this function's return value is unnecessary
   --> $DIR/unnecessary_wraps.rs:117:1