Rollup merge of rust-lang#136958 - compiler-errors:additive-replacmeent, r=estebank

Fix presentation of purely "additive" replacement suggestion parts

rust-lang#127541 changes replacement suggestions to use the "diff" view always, which I think is really verbose in cases where a replacement snippet is a "superset" of the snippet that is being replaced.

Consider:

```
LL -     Self::Baz: Clone,
LL +     Self::Baz: Clone, T: std::clone::Clone
```

In this code, we suggest replacing `", "` with `", T: std::clone::Clone"`. This is a consequence of how the snippet is constructed. I believe that since the string that is being replaced is a subset of the replacement string, it's not providing much value to present this as a diff. Users should be able to clearly understand what's being suggested here using the `~` underline view we've been suggesting for some time now.

Given that this affects ~100 tests out of the ~1000 UI tests affected, I expect this to be a pretty meaningful improvement of the fallout of rust-lang#127541.

---

In the last commit, this PR also "trims" replacement parts so that they are turned into their purely additive subset, if possible. See the diff for what this means.

---

r? estebank

Author: workingjubilee

compiler/rustc_errors/src/emitter.rs

@@ -1976,13 +1976,16 @@ impl HumanEmitter {
             Some(Style::HeaderMsg),
         );
 
+        let other_suggestions = suggestions.len().saturating_sub(MAX_SUGGESTIONS);
+
         let mut row_num = 2;
         for (i, (complete, parts, highlights, _)) in
-            suggestions.iter().enumerate().take(MAX_SUGGESTIONS)
+            suggestions.into_iter().enumerate().take(MAX_SUGGESTIONS)
         {
             debug!(?complete, ?parts, ?highlights);
 
-            let has_deletion = parts.iter().any(|p| p.is_deletion(sm) || p.is_replacement(sm));
+            let has_deletion =
+                parts.iter().any(|p| p.is_deletion(sm) || p.is_destructive_replacement(sm));
             let is_multiline = complete.lines().count() > 1;
 
             if i == 0 {
@@ -2167,7 +2170,7 @@ impl HumanEmitter {
                 self.draw_code_line(
                     &mut buffer,
                     &mut row_num,
-                    highlight_parts,
+                    &highlight_parts,
                     line_pos + line_start,
                     line,
                     show_code_change,
@@ -2213,7 +2216,12 @@ impl HumanEmitter {
             if let DisplaySuggestion::Diff | DisplaySuggestion::Underline | DisplaySuggestion::Add =
                 show_code_change
             {
-                for part in parts {
+                for mut part in parts {
+                    // If this is a replacement of, e.g. `"a"` into `"ab"`, adjust the
+                    // suggestion and snippet to look as if we just suggested to add
+                    // `"b"`, which is typically much easier for the user to understand.
+                    part.trim_trivial_replacements(sm);
+
                     let snippet = if let Ok(snippet) = sm.span_to_snippet(part.span) {
                         snippet
                     } else {
@@ -2376,9 +2384,12 @@ impl HumanEmitter {
                 row_num = row + 1;
             }
         }
-        if suggestions.len() > MAX_SUGGESTIONS {
-            let others = suggestions.len() - MAX_SUGGESTIONS;
-            let msg = format!("and {} other candidate{}", others, pluralize!(others));
+        if other_suggestions > 0 {
+            let msg = format!(
+                "and {} other candidate{}",
+                other_suggestions,
+                pluralize!(other_suggestions)
+            );
             buffer.puts(row_num, max_line_num_len + 3, &msg, Style::NoStyle);
         }
 

compiler/rustc_errors/src/lib.rs

@@ -230,10 +230,40 @@ impl SubstitutionPart {
         !self.snippet.is_empty() && self.replaces_meaningful_content(sm)
     }
 
+    /// Whether this is a replacement that overwrites source with a snippet
+    /// in a way that isn't a superset of the original string. For example,
+    /// replacing "abc" with "abcde" is not destructive, but replacing it
+    /// it with "abx" is, since the "c" character is lost.
+    pub fn is_destructive_replacement(&self, sm: &SourceMap) -> bool {
+        self.is_replacement(sm)
+            && !sm.span_to_snippet(self.span).is_ok_and(|snippet| {
+                self.snippet.trim_start().starts_with(snippet.trim_start())
+                    || self.snippet.trim_end().ends_with(snippet.trim_end())
+            })
+    }
+
     fn replaces_meaningful_content(&self, sm: &SourceMap) -> bool {
         sm.span_to_snippet(self.span)
             .map_or(!self.span.is_empty(), |snippet| !snippet.trim().is_empty())
     }
+
+    /// Try to turn a replacement into an addition when the span that is being
+    /// overwritten matches either the prefix or suffix of the replacement.
+    fn trim_trivial_replacements(&mut self, sm: &SourceMap) {
+        if self.snippet.is_empty() {
+            return;
+        }
+        let Ok(snippet) = sm.span_to_snippet(self.span) else {
+            return;
+        };
+        if self.snippet.starts_with(&snippet) {
+            self.span = self.span.shrink_to_hi();
+            self.snippet = self.snippet[snippet.len()..].to_string();
+        } else if self.snippet.ends_with(&snippet) {
+            self.span = self.span.shrink_to_lo();
+            self.snippet = self.snippet[..self.snippet.len() - snippet.len()].to_string();
+        }
+    }
 }
 
 impl CodeSuggestion {

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

@@ -8,8 +8,7 @@ LL |     true
    = help: to override `-D warnings` add `#[allow(clippy::implicit_return)]`
 help: add `return` as shown
    |
-LL -     true
-LL +     return true
+LL |     return true
    |
 
 error: missing `return` statement
@@ -20,9 +19,8 @@ LL |     if true { true } else { false }
    |
 help: add `return` as shown
    |
-LL -     if true { true } else { false }
-LL +     if true { return true } else { false }
-   |
+LL |     if true { return true } else { false }
+   |               ++++++
 
 error: missing `return` statement
   --> tests/ui/implicit_return.rs:19:29
@@ -32,9 +30,8 @@ LL |     if true { true } else { false }
    |
 help: add `return` as shown
    |
-LL -     if true { true } else { false }
-LL +     if true { true } else { return false }
-   |
+LL |     if true { true } else { return false }
+   |                             ++++++
 
 error: missing `return` statement
   --> tests/ui/implicit_return.rs:25:17
@@ -44,9 +41,8 @@ LL |         true => false,
    |
 help: add `return` as shown
    |
-LL -         true => false,
-LL +         true => return false,
-   |
+LL |         true => return false,
+   |                 ++++++
 
 error: missing `return` statement
   --> tests/ui/implicit_return.rs:26:20
@@ -56,9 +52,8 @@ LL |         false => { true },
    |
 help: add `return` as shown
    |
-LL -         false => { true },
-LL +         false => { return true },
-   |
+LL |         false => { return true },
+   |                    ++++++
 
 error: missing `return` statement
   --> tests/ui/implicit_return.rs:39:9
@@ -104,9 +99,8 @@ LL |     let _ = || { true };
    |
 help: add `return` as shown
    |
-LL -     let _ = || { true };
-LL +     let _ = || { return true };
-   |
+LL |     let _ = || { return true };
+   |                  ++++++
 
 error: missing `return` statement
   --> tests/ui/implicit_return.rs:73:16
@@ -116,9 +110,8 @@ LL |     let _ = || true;
    |
 help: add `return` as shown
    |
-LL -     let _ = || true;
-LL +     let _ = || return true;
-   |
+LL |     let _ = || return true;
+   |                ++++++
 
 error: missing `return` statement
   --> tests/ui/implicit_return.rs:81:5
@@ -128,8 +121,7 @@ LL |     format!("test {}", "test")
    |
 help: add `return` as shown
    |
-LL -     format!("test {}", "test")
-LL +     return format!("test {}", "test")
+LL |     return format!("test {}", "test")
    |
 
 error: missing `return` statement
@@ -140,8 +132,7 @@ LL |     m!(true, false)
    |
 help: add `return` as shown
    |
-LL -     m!(true, false)
-LL +     return m!(true, false)
+LL |     return m!(true, false)
    |
 
 error: missing `return` statement
@@ -191,8 +182,7 @@ LL |     true
    |
 help: add `return` as shown
    |
-LL -     true
-LL +     return true
+LL |     return true
    |
 
 error: aborting due to 16 previous errors

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

@@ -68,9 +68,8 @@ LL |     MAX;
    |
 help: use the associated constant instead
    |
-LL -     MAX;
-LL +     u32::MAX;
-   |
+LL |     u32::MAX;
+   |     +++++
 
 error: usage of a legacy numeric method
   --> tests/ui/legacy_numeric_constants.rs:49:10

tests/ui/associated-types/defaults-suitability.current.stderr

@@ -134,9 +134,8 @@ LL |     type Baz = T;
    |          --- required by a bound in this associated type
 help: consider further restricting type parameter `T` with trait `Clone`
    |
-LL -     Self::Baz: Clone,
-LL +     Self::Baz: Clone, T: std::clone::Clone
-   |
+LL |     Self::Baz: Clone, T: std::clone::Clone
+   |                       ++++++++++++++++++++
 
 error: aborting due to 8 previous errors
 

tests/ui/associated-types/defaults-suitability.next.stderr

@@ -134,9 +134,8 @@ LL |     type Baz = T;
    |          --- required by a bound in this associated type
 help: consider further restricting type parameter `T` with trait `Clone`
    |
-LL -     Self::Baz: Clone,
-LL +     Self::Baz: Clone, T: std::clone::Clone
-   |
+LL |     Self::Baz: Clone, T: std::clone::Clone
+   |                       ++++++++++++++++++++
 
 error: aborting due to 8 previous errors
 

tests/ui/associated-types/issue-38821.stderr

@@ -13,9 +13,8 @@ LL | impl<T: NotNull> IntoNullable for T {
    |         unsatisfied trait bound introduced here
 help: consider extending the `where` clause, but there might be an alternative better way to express this requirement
    |
-LL -     Expr: Expression<SqlType=<Col::SqlType as IntoNullable>::Nullable>,
-LL +     Expr: Expression<SqlType=<Col::SqlType as IntoNullable>::Nullable>, <Col as Expression>::SqlType: NotNull
-   |
+LL |     Expr: Expression<SqlType=<Col::SqlType as IntoNullable>::Nullable>, <Col as Expression>::SqlType: NotNull
+   |                                                                         +++++++++++++++++++++++++++++++++++++
 
 error[E0277]: the trait bound `<Col as Expression>::SqlType: NotNull` is not satisfied
   --> $DIR/issue-38821.rs:40:1
@@ -38,9 +37,8 @@ LL | impl<T: NotNull> IntoNullable for T {
    |         unsatisfied trait bound introduced here
 help: consider extending the `where` clause, but there might be an alternative better way to express this requirement
    |
-LL -     Expr: Expression<SqlType=<Col::SqlType as IntoNullable>::Nullable>,
-LL +     Expr: Expression<SqlType=<Col::SqlType as IntoNullable>::Nullable>, <Col as Expression>::SqlType: NotNull
-   |
+LL |     Expr: Expression<SqlType=<Col::SqlType as IntoNullable>::Nullable>, <Col as Expression>::SqlType: NotNull
+   |                                                                         +++++++++++++++++++++++++++++++++++++
 
 error[E0277]: the trait bound `<Col as Expression>::SqlType: NotNull` is not satisfied
   --> $DIR/issue-38821.rs:23:10

tests/ui/associated-types/issue-54108.current.stderr

@@ -12,9 +12,8 @@ LL |     type Size: Add<Output = Self::Size>;
    |                ^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `Encoder::Size`
 help: consider further restricting the associated type
    |
-LL -     T: SubEncoder,
-LL +     T: SubEncoder, <T as SubEncoder>::ActualSize: Add
-   |
+LL |     T: SubEncoder, <T as SubEncoder>::ActualSize: Add
+   |                    ++++++++++++++++++++++++++++++++++
 
 error: aborting due to 1 previous error
 

tests/ui/associated-types/issue-54108.next.stderr

@@ -12,9 +12,8 @@ LL |     type Size: Add<Output = Self::Size>;
    |                    ^^^^^^^^^^^^^^^^^^^ required by this bound in `Encoder::Size`
 help: consider further restricting the associated type
    |
-LL -     T: SubEncoder,
-LL +     T: SubEncoder, <T as SubEncoder>::ActualSize: Add
-   |
+LL |     T: SubEncoder, <T as SubEncoder>::ActualSize: Add
+   |                    ++++++++++++++++++++++++++++++++++
 
 error: aborting due to 1 previous error
 

tests/ui/attributes/rustc_confusables.stderr

@@ -35,9 +35,8 @@ LL |     x.inser();
    |
 help: there is a method `insert` with a similar name
    |
-LL -     x.inser();
-LL +     x.insert();
-   |
+LL |     x.insert();
+   |            +
 
 error[E0599]: no method named `foo` found for struct `rustc_confusables_across_crate::BTreeSet` in the current scope
   --> $DIR/rustc_confusables.rs:15:7