fix: Use correct line num with trimmed suggestions

Author: Muscraft

src/renderer/render.rs

@@ -1511,7 +1511,8 @@ fn emit_suggestion_default(
         }
 
         let file_lines = sm.span_to_lines(parts[0].span.clone());
-        let (line_start, line_end) = sm.span_to_locations(parts[0].span.clone());
+        // We use the original span to get original line_start
+        let (line_start, line_end) = sm.span_to_locations(parts[0].original_span.clone());
         let mut lines = complete.lines();
         if lines.clone().next().is_none() {
             // Account for a suggestion to completely remove a line(s) with whitespace (#94192).

src/renderer/source_map.rs

@@ -370,7 +370,11 @@ impl<'a> SourceMap<'a> {
     pub(crate) fn splice_lines<'b>(
         &'b self,
         mut patches: Vec<Patch<'b>>,
-    ) -> Vec<(String, Vec<Patch<'b>>, Vec<Vec<SubstitutionHighlight>>)> {
+    ) -> Vec<(
+        String,
+        Vec<TrimmedPatch<'b>>,
+        Vec<Vec<SubstitutionHighlight>>,
+    )> {
         fn push_trailing(
             buf: &mut String,
             line_opt: Option<&str>,
@@ -450,15 +454,18 @@ impl<'a> SourceMap<'a> {
         let mut prev_line = lines.first().map(|line| line.line);
         let mut buf = String::new();
 
+        let trimmed_patches = patches
+            .into_iter()
+            // 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.
+            .map(|part| part.trim_trivial_replacements(self))
+            .collect::<Vec<_>>();
         let mut line_highlight = vec![];
         // We need to keep track of the difference between the existing code and the added
         // or deleted code in order to point at the correct column *after* substitution.
         let mut acc = 0;
-        for part in &mut patches {
-            // 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(self);
+        for part in &trimmed_patches {
             let (cur_lo, cur_hi) = self.span_to_locations(part.span.clone());
             if prev_hi.line == cur_lo.line {
                 let mut count = push_trailing(&mut buf, prev_line, &prev_hi, Some(&cur_lo));
@@ -540,7 +547,7 @@ impl<'a> SourceMap<'a> {
         if highlights.iter().all(|parts| parts.is_empty()) {
             Vec::new()
         } else {
-            vec![(buf, patches, highlights)]
+            vec![(buf, trimmed_patches, highlights)]
         }
     }
 }
@@ -704,3 +711,67 @@ pub(crate) struct SubstitutionHighlight {
     pub(crate) start: usize,
     pub(crate) end: usize,
 }
+
+#[derive(Clone, Debug)]
+pub(crate) struct TrimmedPatch<'a> {
+    pub(crate) original_span: Range<usize>,
+    pub(crate) span: Range<usize>,
+    pub(crate) replacement: Cow<'a, str>,
+}
+
+impl<'a> TrimmedPatch<'a> {
+    pub(crate) fn is_addition(&self, sm: &SourceMap<'_>) -> bool {
+        !self.replacement.is_empty() && !self.replaces_meaningful_content(sm)
+    }
+
+    pub(crate) fn is_deletion(&self, sm: &SourceMap<'_>) -> bool {
+        self.replacement.trim().is_empty() && self.replaces_meaningful_content(sm)
+    }
+
+    pub(crate) fn is_replacement(&self, sm: &SourceMap<'_>) -> bool {
+        !self.replacement.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(crate) fn is_destructive_replacement(&self, sm: &SourceMap<'_>) -> bool {
+        self.is_replacement(sm)
+            && !sm
+                .span_to_snippet(self.span.clone())
+                // This should use `is_some_and` when our MSRV is >= 1.70
+                .map_or(false, |s| {
+                    as_substr(s.trim(), self.replacement.trim()).is_some()
+                })
+    }
+
+    fn replaces_meaningful_content(&self, sm: &SourceMap<'_>) -> bool {
+        sm.span_to_snippet(self.span.clone())
+            .map_or(!self.span.is_empty(), |snippet| !snippet.trim().is_empty())
+    }
+}
+
+/// Given an original string like `AACC`, and a suggestion like `AABBCC`, try to detect
+/// the case where a substring of the suggestion is "sandwiched" in the original, like
+/// `BB` is. Return the length of the prefix, the "trimmed" suggestion, and the length
+/// of the suffix.
+pub(crate) fn as_substr<'a>(
+    original: &'a str,
+    suggestion: &'a str,
+) -> Option<(usize, &'a str, usize)> {
+    let common_prefix = original
+        .chars()
+        .zip(suggestion.chars())
+        .take_while(|(c1, c2)| c1 == c2)
+        .map(|(c, _)| c.len_utf8())
+        .sum();
+    let original = &original[common_prefix..];
+    let suggestion = &suggestion[common_prefix..];
+    if let Some(stripped) = suggestion.strip_suffix(original) {
+        let common_suffix = original.len();
+        Some((common_prefix, stripped, common_suffix))
+    } else {
+        None
+    }
+}

src/snippet.rs

@@ -1,6 +1,6 @@
 //! Structures used as an input for the library.
 
-use crate::renderer::source_map::SourceMap;
+use crate::renderer::source_map::{as_substr, SourceMap, TrimmedPatch};
 use crate::Level;
 use std::borrow::Cow;
 use std::ops::Range;
@@ -439,51 +439,28 @@ impl<'a> Patch<'a> {
         }
     }
 
-    pub(crate) fn is_addition(&self, sm: &SourceMap<'_>) -> bool {
-        !self.replacement.is_empty() && !self.replaces_meaningful_content(sm)
-    }
-
-    pub(crate) fn is_deletion(&self, sm: &SourceMap<'_>) -> bool {
-        self.replacement.trim().is_empty() && self.replaces_meaningful_content(sm)
-    }
-
-    pub(crate) fn is_replacement(&self, sm: &SourceMap<'_>) -> bool {
-        !self.replacement.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(crate) fn is_destructive_replacement(&self, sm: &SourceMap<'_>) -> bool {
-        self.is_replacement(sm)
-            && !sm
-                .span_to_snippet(self.span.clone())
-                // This should use `is_some_and` when our MSRV is >= 1.70
-                .map_or(false, |s| {
-                    as_substr(s.trim(), self.replacement.trim()).is_some()
-                })
-    }
-
-    fn replaces_meaningful_content(&self, sm: &SourceMap<'_>) -> bool {
-        sm.span_to_snippet(self.span.clone())
-            .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.
-    pub(crate) fn trim_trivial_replacements(&mut self, sm: &'a SourceMap<'a>) {
-        if self.replacement.is_empty() {
-            return;
+    pub(crate) fn trim_trivial_replacements(self, sm: &'a SourceMap<'a>) -> TrimmedPatch<'a> {
+        let mut trimmed = TrimmedPatch {
+            original_span: self.span.clone(),
+            span: self.span,
+            replacement: self.replacement,
+        };
+
+        if trimmed.replacement.is_empty() {
+            return trimmed;
         }
-        let Some(snippet) = sm.span_to_snippet(self.span.clone()) else {
-            return;
+        let Some(snippet) = sm.span_to_snippet(trimmed.original_span.clone()) else {
+            return trimmed;
         };
 
-        if let Some((prefix, substr, suffix)) = as_substr(snippet, &self.replacement) {
-            self.span = self.span.start + prefix..self.span.end.saturating_sub(suffix);
-            self.replacement = Cow::Owned(substr.to_owned());
+        if let Some((prefix, substr, suffix)) = as_substr(snippet, &trimmed.replacement) {
+            trimmed.span = trimmed.original_span.start + prefix
+                ..trimmed.original_span.end.saturating_sub(suffix);
+            trimmed.replacement = Cow::Owned(substr.to_owned());
         }
+        trimmed
     }
 }
 
@@ -587,24 +564,3 @@ impl<'a> From<&'a String> for OptionCow<'a> {
         Self(Some(Cow::Borrowed(value.as_str())))
     }
 }
-
-/// Given an original string like `AACC`, and a suggestion like `AABBCC`, try to detect
-/// the case where a substring of the suggestion is "sandwiched" in the original, like
-/// `BB` is. Return the length of the prefix, the "trimmed" suggestion, and the length
-/// of the suffix.
-fn as_substr<'a>(original: &'a str, suggestion: &'a str) -> Option<(usize, &'a str, usize)> {
-    let common_prefix = original
-        .chars()
-        .zip(suggestion.chars())
-        .take_while(|(c1, c2)| c1 == c2)
-        .map(|(c, _)| c.len_utf8())
-        .sum();
-    let original = &original[common_prefix..];
-    let suggestion = &suggestion[common_prefix..];
-    if let Some(stripped) = suggestion.strip_suffix(original) {
-        let common_suffix = original.len();
-        Some((common_prefix, stripped, common_suffix))
-    } else {
-        None
-    }
-}

tests/formatter.rs

@@ -4402,10 +4402,10 @@ note: function defined here
    |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^         -------
 help: provide the argument
    |
- 7 |     function_with_lots_of_arguments(
- 8 |         variable_name,
- 9 ~         /* char */,
-10 ~         variable_name,
+ 5 |     function_with_lots_of_arguments(
+ 6 |         variable_name,
+ 7 ~         /* char */,
+ 8 ~         variable_name,
    |
 "#]];
     let renderer_ascii = Renderer::plain();
@@ -4428,10 +4428,10 @@ note: function defined here
    ╰╴   ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━         ───────
 help: provide the argument
    ╭╴
- 7 │     function_with_lots_of_arguments(
- 8 │         variable_name,
- 9 ±         /* char */,
-10 ±         variable_name,
+ 5 │     function_with_lots_of_arguments(
+ 6 │         variable_name,
+ 7 ±         /* char */,
+ 8 ±         variable_name,
    ╰╴
 "#]];
     let renderer_unicode = renderer_ascii.decor_style(DecorStyle::Unicode);
@@ -4504,10 +4504,10 @@ note: the lint level is defined here
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 help: add a `;` here
    |
-10 |     nums.iter().for_each(|x| {
+ 4 |     nums.iter().for_each(|x| {
 ...
-15 |         }
-16 ~     });
+ 9 |         }
+10 ~     });
    |
 "#]];
     let renderer_ascii = Renderer::plain();
@@ -4532,10 +4532,10 @@ note: the lint level is defined here
    ╰╴        ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
 help: add a `;` here
    ╭╴
-10 │     nums.iter().for_each(|x| {
+ 4 │     nums.iter().for_each(|x| {
  …
-15 │         }
-16 ±     });
+ 9 │         }
+10 ±     });
    ╰╴
 "#]];
     let renderer_unicode = renderer_ascii.decor_style(DecorStyle::Unicode);