Skip to content

Fix presentation of purely "additive" replacement suggestion parts #136958

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 14, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions compiler/rustc_errors/src/emitter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
}

Expand Down
30 changes: 30 additions & 0 deletions compiler/rustc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
42 changes: 16 additions & 26 deletions src/tools/clippy/tests/ui/implicit_return.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this renders strangely since it has no addition +'s, but it's exactly how it rendered before #127541, so it's a preexisting bug in the emitter.

|

error: missing `return` statement
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
5 changes: 2 additions & 3 deletions src/tools/clippy/tests/ui/legacy_numeric_constants.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 2 additions & 3 deletions tests/ui/associated-types/defaults-suitability.current.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
5 changes: 2 additions & 3 deletions tests/ui/associated-types/defaults-suitability.next.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
10 changes: 4 additions & 6 deletions tests/ui/associated-types/issue-38821.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
5 changes: 2 additions & 3 deletions tests/ui/associated-types/issue-54108.current.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
5 changes: 2 additions & 3 deletions tests/ui/associated-types/issue-54108.next.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
5 changes: 2 additions & 3 deletions tests/ui/attributes/rustc_confusables.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 4 additions & 6 deletions tests/ui/attributes/rustc_confusables_std_cases.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,8 @@ LL | let mut x = VecDeque::new();
| ----- earlier `x` shadowed here with type `VecDeque`
help: you might have meant to use `push_back`
|
LL - x.push(1);
LL + x.push_back(1);
|
LL | x.push_back(1);
| +++++

error[E0599]: no method named `length` found for struct `Vec<{integer}>` in the current scope
--> $DIR/rustc_confusables_std_cases.rs:15:7
Expand Down Expand Up @@ -98,9 +97,8 @@ note: method defined here
--> $SRC_DIR/alloc/src/string.rs:LL:COL
help: you might have meant to use `push_str`
|
LL - String::new().push("");
LL + String::new().push_str("");
|
LL | String::new().push_str("");
| ++++

error[E0599]: no method named `append` found for struct `String` in the current scope
--> $DIR/rustc_confusables_std_cases.rs:24:19
Expand Down
5 changes: 2 additions & 3 deletions tests/ui/borrowck/issue-115259-suggest-iter-mut.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@ LL | self.layers.iter().fold(0, |result, mut layer| result + layer.proce
|
help: you may want to use `iter_mut` here
|
LL - self.layers.iter().fold(0, |result, mut layer| result + layer.process())
LL + self.layers.iter_mut().fold(0, |result, mut layer| result + layer.process())
|
LL | self.layers.iter_mut().fold(0, |result, mut layer| result + layer.process())
| ++++

error: aborting due to 1 previous error

Expand Down
5 changes: 2 additions & 3 deletions tests/ui/borrowck/issue-62387-suggest-iter-mut-2.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@ LL | vec.iter().flat_map(|container| container.things()).cloned().co
|
help: you may want to use `iter_mut` here
|
LL - vec.iter().flat_map(|container| container.things()).cloned().collect::<Vec<PathBuf>>();
LL + vec.iter_mut().flat_map(|container| container.things()).cloned().collect::<Vec<PathBuf>>();
|
LL | vec.iter_mut().flat_map(|container| container.things()).cloned().collect::<Vec<PathBuf>>();
| ++++

error: aborting due to 1 previous error

Expand Down
10 changes: 4 additions & 6 deletions tests/ui/borrowck/issue-62387-suggest-iter-mut.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@ LL | v.iter().for_each(|a| a.double());
|
help: you may want to use `iter_mut` here
|
LL - v.iter().for_each(|a| a.double());
LL + v.iter_mut().for_each(|a| a.double());
|
LL | v.iter_mut().for_each(|a| a.double());
| ++++

error[E0596]: cannot borrow `*a` as mutable, as it is behind a `&` reference
--> $DIR/issue-62387-suggest-iter-mut.rs:25:39
Expand All @@ -22,9 +21,8 @@ LL | v.iter().rev().rev().for_each(|a| a.double());
|
help: you may want to use `iter_mut` here
|
LL - v.iter().rev().rev().for_each(|a| a.double());
LL + v.iter_mut().rev().rev().for_each(|a| a.double());
|
LL | v.iter_mut().rev().rev().for_each(|a| a.double());
| ++++

error: aborting due to 2 previous errors

Expand Down
5 changes: 2 additions & 3 deletions tests/ui/c-variadic/issue-86053-1.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,8 @@ LL | self , ... , self , self , ... ) where F : FnOnce ( & 'a & 'b usize
|
help: a trait with a similar name exists
|
LL - self , ... , self , self , ... ) where F : FnOnce ( & 'a & 'b usize ) {
LL + self , ... , self , self , ... ) where Fn : FnOnce ( & 'a & 'b usize ) {
|
LL | self , ... , self , self , ... ) where Fn : FnOnce ( & 'a & 'b usize ) {
| +
help: you might be missing a type parameter
|
LL | fn ordering4 < 'a , 'b, F > ( a : , self , self , self ,
Expand Down
Loading
Loading