From d53ce78903124e27dbb454d89c1fdb97ab93932b Mon Sep 17 00:00:00 2001 From: Scott Schafer Date: Mon, 22 Sep 2025 05:19:43 -0600 Subject: [PATCH 1/2] test: Add tests for trimming multiline suggestions --- tests/formatter.rs | 212 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 212 insertions(+) diff --git a/tests/formatter.rs b/tests/formatter.rs index 64e96b6..e7b986c 100644 --- a/tests/formatter.rs +++ b/tests/formatter.rs @@ -4329,3 +4329,215 @@ error: ensure single line at line 0 rendered correctly with group line lined up let renderer = renderer.decor_style(DecorStyle::Unicode); assert_data_eq!(renderer.render(input), expected_unicode); } + +#[test] +fn trimmed_multiline_suggestion() { + let source = r#"fn function_with_lots_of_arguments(a: i32, b: char, c: i32, d: i32, e: i32, f: i32) {} + +fn main() { + let variable_name = 42; + function_with_lots_of_arguments( + variable_name, + variable_name, + variable_name, + variable_name, + variable_name, + ); + //~^^^^^^^ ERROR this function takes 6 arguments but 5 arguments were supplied [E0061] +} +"#; + let path = "$DIR/trimmed_multiline_suggestion.rs"; + + let input = &[ + Group::with_title( + Level::ERROR + .primary_title("this function takes 6 arguments but 5 arguments were supplied") + .id("E0061"), + ) + .element( + Snippet::source(source) + .path(path) + .annotation( + AnnotationKind::Context + .span(196..209) + .label("argument #2 of type `char` is missing"), + ) + .annotation(AnnotationKind::Primary.span(132..163)), + ), + Group::with_title(Level::NOTE.secondary_title("function defined here")).element( + Snippet::source(source) + .path(path) + .annotation(AnnotationKind::Context.span(43..50).label("")) + .annotation(AnnotationKind::Primary.span(3..34)), + ), + Group::with_title(Level::HELP.secondary_title("provide the argument")).element( + Snippet::source(source).path(path).patch(Patch::new( + 163..285, + "( + variable_name, + /* char */, + variable_name, + variable_name, + variable_name, + variable_name, + )", + )), + ), + ]; + + let expected_ascii = str![[r#" +error[E0061]: this function takes 6 arguments but 5 arguments were supplied + --> $DIR/trimmed_multiline_suggestion.rs:5:5 + | + 5 | function_with_lots_of_arguments( + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + 6 | variable_name, + 7 | variable_name, + | ------------- argument #2 of type `char` is missing + | +note: function defined here + --> $DIR/trimmed_multiline_suggestion.rs:1:4 + | + 1 | fn function_with_lots_of_arguments(a: i32, b: char, c: i32, d: i32, e: i32, f: i32) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ------- +help: provide the argument + | + 7 | function_with_lots_of_arguments( + 8 | variable_name, + 9 ~ /* char */, +10 ~ variable_name, + | +"#]]; + let renderer_ascii = Renderer::plain(); + assert_data_eq!(renderer_ascii.render(input), expected_ascii); + + let expected_unicode = str![[r#" +error[E0061]: this function takes 6 arguments but 5 arguments were supplied + ╭▸ $DIR/trimmed_multiline_suggestion.rs:5:5 + │ + 5 │ function_with_lots_of_arguments( + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + 6 │ variable_name, + 7 │ variable_name, + │ ───────────── argument #2 of type `char` is missing + ╰╴ +note: function defined here + ╭▸ $DIR/trimmed_multiline_suggestion.rs:1:4 + │ + 1 │ fn function_with_lots_of_arguments(a: i32, b: char, c: i32, d: i32, e: i32, f: i32) {} + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ─────── +help: provide the argument + ╭╴ + 7 │ function_with_lots_of_arguments( + 8 │ variable_name, + 9 ± /* char */, +10 ± variable_name, + ╰╴ +"#]]; + let renderer_unicode = renderer_ascii.decor_style(DecorStyle::Unicode); + assert_data_eq!(renderer_unicode.render(input), expected_unicode); +} + +#[test] +fn trimmed_multiline_suggestion_elided_lines() { + let source_0 = r#" nums.iter().for_each(|x| { + if *x > 0 { + println!("Positive number"); + } else { + println!("Negative number"); + } + }) +"#; + let source_1 = r#"#![deny(clippy::semicolon_if_nothing_returned)] +"#; + + let input = &[ + Group::with_title(Level::ERROR.primary_title( + "consider adding a `;` to the last statement for consistent formatting", + )) + .element( + Snippet::source(source_0) + .path("tests/ui/semicolon_if_nothing_returned_testing.rs") + .line_start(4) + .annotation(AnnotationKind::Primary.span(4..166)), + ), + Group::with_title(Level::NOTE.secondary_title("the lint level is defined here")).element( + Snippet::source(source_1) + .path("tests/ui/semicolon_if_nothing_returned_testing.rs") + .line_start(2) + .annotation(AnnotationKind::Primary.span(8..45)), + ), + Group::with_title(Level::HELP.secondary_title("add a `;` here")).element( + Snippet::source(source_0) + .path("tests/ui/semicolon_if_nothing_returned_testing.rs") + .line_start(4) + .fold(true) + .patch(Patch::new( + 4..166, + r#"nums.iter().for_each(|x| { + if *x > 0 { + println!("Positive number"); + } else { + println!("Negative number"); + } + });"#, + )), + ), + ]; + + let expected_ascii = str![[r#" +error: consider adding a `;` to the last statement for consistent formatting + --> tests/ui/semicolon_if_nothing_returned_testing.rs:4:5 + | + 4 | / nums.iter().for_each(|x| { + 5 | | if *x > 0 { + 6 | | println!("Positive number"); + 7 | | } else { +... | +10 | | }) + | |______^ + | +note: the lint level is defined here + --> tests/ui/semicolon_if_nothing_returned_testing.rs:2:9 + | + 2 | #![deny(clippy::semicolon_if_nothing_returned)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: add a `;` here + | +10 | nums.iter().for_each(|x| { +... +15 | } +16 ~ }); + | +"#]]; + let renderer_ascii = Renderer::plain(); + assert_data_eq!(renderer_ascii.render(input), expected_ascii); + + let expected_unicode = str![[r#" +error: consider adding a `;` to the last statement for consistent formatting + ╭▸ tests/ui/semicolon_if_nothing_returned_testing.rs:4:5 + │ + 4 │ ┏ nums.iter().for_each(|x| { + 5 │ ┃ if *x > 0 { + 6 │ ┃ println!("Positive number"); + 7 │ ┃ } else { + ‡ ┃ +10 │ ┃ }) + │ ┗━━━━━━┛ + ╰╴ +note: the lint level is defined here + ╭▸ tests/ui/semicolon_if_nothing_returned_testing.rs:2:9 + │ + 2 │ #![deny(clippy::semicolon_if_nothing_returned)] + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +help: add a `;` here + ╭╴ +10 │ nums.iter().for_each(|x| { + … +15 │ } +16 ± }); + ╰╴ +"#]]; + let renderer_unicode = renderer_ascii.decor_style(DecorStyle::Unicode); + assert_data_eq!(renderer_unicode.render(input), expected_unicode); +} From 142438e527d90bb544ec37e4e8754dc9378b9791 Mon Sep 17 00:00:00 2001 From: Scott Schafer Date: Mon, 22 Sep 2025 05:19:43 -0600 Subject: [PATCH 2/2] fix: Use correct line num with trimmed suggestions --- src/renderer/render.rs | 3 +- src/renderer/source_map.rs | 85 ++++++++++++++++++++++++++++++++++---- src/snippet.rs | 78 ++++++++-------------------------- tests/formatter.rs | 28 ++++++------- 4 files changed, 111 insertions(+), 83 deletions(-) diff --git a/src/renderer/render.rs b/src/renderer/render.rs index 1f5bfc5..451e036 100644 --- a/src/renderer/render.rs +++ b/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). diff --git a/src/renderer/source_map.rs b/src/renderer/source_map.rs index 9af4178..0018d1b 100644 --- a/src/renderer/source_map.rs +++ b/src/renderer/source_map.rs @@ -370,7 +370,11 @@ impl<'a> SourceMap<'a> { pub(crate) fn splice_lines<'b>( &'b self, mut patches: Vec>, - ) -> Vec<(String, Vec>, Vec>)> { + ) -> Vec<( + String, + Vec>, + Vec>, + )> { 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::>(); 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, + pub(crate) span: Range, + 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 + } +} diff --git a/src/snippet.rs b/src/snippet.rs index 499c8a9..253cacc 100644 --- a/src/snippet.rs +++ b/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 - } -} diff --git a/tests/formatter.rs b/tests/formatter.rs index e7b986c..e715f63 100644 --- a/tests/formatter.rs +++ b/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);