Skip to content

Commit 49fb61c

Browse files
matthiaskrgrmatthiaskrgr
authored
Rollup merge of #136958 - compiler-errors:additive-replacmeent, r=estebank
Fix presentation of purely "additive" replacement suggestion parts #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 #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
2 parents 2980f75 + 6d71251 commit 49fb61c

File tree

153 files changed

+640
-897
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

153 files changed

+640
-897
lines changed

Diff for: ‎compiler/rustc_errors/src/emitter.rs

+18-7
Original file line numberDiff line numberDiff line change
@@ -1976,13 +1976,16 @@ impl HumanEmitter {
19761976
Some(Style::HeaderMsg),
19771977
);
19781978

1979+
let other_suggestions = suggestions.len().saturating_sub(MAX_SUGGESTIONS);
1980+
19791981
let mut row_num = 2;
19801982
for (i, (complete, parts, highlights, _)) in
1981-
suggestions.iter().enumerate().take(MAX_SUGGESTIONS)
1983+
suggestions.into_iter().enumerate().take(MAX_SUGGESTIONS)
19821984
{
19831985
debug!(?complete, ?parts, ?highlights);
19841986

1985-
let has_deletion = parts.iter().any(|p| p.is_deletion(sm) || p.is_replacement(sm));
1987+
let has_deletion =
1988+
parts.iter().any(|p| p.is_deletion(sm) || p.is_destructive_replacement(sm));
19861989
let is_multiline = complete.lines().count() > 1;
19871990

19881991
if i == 0 {
@@ -2167,7 +2170,7 @@ impl HumanEmitter {
21672170
self.draw_code_line(
21682171
&mut buffer,
21692172
&mut row_num,
2170-
highlight_parts,
2173+
&highlight_parts,
21712174
line_pos + line_start,
21722175
line,
21732176
show_code_change,
@@ -2213,7 +2216,12 @@ impl HumanEmitter {
22132216
if let DisplaySuggestion::Diff | DisplaySuggestion::Underline | DisplaySuggestion::Add =
22142217
show_code_change
22152218
{
2216-
for part in parts {
2219+
for mut part in parts {
2220+
// If this is a replacement of, e.g. `"a"` into `"ab"`, adjust the
2221+
// suggestion and snippet to look as if we just suggested to add
2222+
// `"b"`, which is typically much easier for the user to understand.
2223+
part.trim_trivial_replacements(sm);
2224+
22172225
let snippet = if let Ok(snippet) = sm.span_to_snippet(part.span) {
22182226
snippet
22192227
} else {
@@ -2376,9 +2384,12 @@ impl HumanEmitter {
23762384
row_num = row + 1;
23772385
}
23782386
}
2379-
if suggestions.len() > MAX_SUGGESTIONS {
2380-
let others = suggestions.len() - MAX_SUGGESTIONS;
2381-
let msg = format!("and {} other candidate{}", others, pluralize!(others));
2387+
if other_suggestions > 0 {
2388+
let msg = format!(
2389+
"and {} other candidate{}",
2390+
other_suggestions,
2391+
pluralize!(other_suggestions)
2392+
);
23822393
buffer.puts(row_num, max_line_num_len + 3, &msg, Style::NoStyle);
23832394
}
23842395

Diff for: ‎compiler/rustc_errors/src/lib.rs

+30
Original file line numberDiff line numberDiff line change
@@ -230,10 +230,40 @@ impl SubstitutionPart {
230230
!self.snippet.is_empty() && self.replaces_meaningful_content(sm)
231231
}
232232

233+
/// Whether this is a replacement that overwrites source with a snippet
234+
/// in a way that isn't a superset of the original string. For example,
235+
/// replacing "abc" with "abcde" is not destructive, but replacing it
236+
/// it with "abx" is, since the "c" character is lost.
237+
pub fn is_destructive_replacement(&self, sm: &SourceMap) -> bool {
238+
self.is_replacement(sm)
239+
&& !sm.span_to_snippet(self.span).is_ok_and(|snippet| {
240+
self.snippet.trim_start().starts_with(snippet.trim_start())
241+
|| self.snippet.trim_end().ends_with(snippet.trim_end())
242+
})
243+
}
244+
233245
fn replaces_meaningful_content(&self, sm: &SourceMap) -> bool {
234246
sm.span_to_snippet(self.span)
235247
.map_or(!self.span.is_empty(), |snippet| !snippet.trim().is_empty())
236248
}
249+
250+
/// Try to turn a replacement into an addition when the span that is being
251+
/// overwritten matches either the prefix or suffix of the replacement.
252+
fn trim_trivial_replacements(&mut self, sm: &SourceMap) {
253+
if self.snippet.is_empty() {
254+
return;
255+
}
256+
let Ok(snippet) = sm.span_to_snippet(self.span) else {
257+
return;
258+
};
259+
if self.snippet.starts_with(&snippet) {
260+
self.span = self.span.shrink_to_hi();
261+
self.snippet = self.snippet[snippet.len()..].to_string();
262+
} else if self.snippet.ends_with(&snippet) {
263+
self.span = self.span.shrink_to_lo();
264+
self.snippet = self.snippet[..self.snippet.len() - snippet.len()].to_string();
265+
}
266+
}
237267
}
238268

239269
impl CodeSuggestion {

Diff for: ‎src/tools/clippy/tests/ui/implicit_return.stderr

+16-26
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,7 @@ LL | true
88
= help: to override `-D warnings` add `#[allow(clippy::implicit_return)]`
99
help: add `return` as shown
1010
|
11-
LL - true
12-
LL + return true
11+
LL | return true
1312
|
1413

1514
error: missing `return` statement
@@ -20,9 +19,8 @@ LL | if true { true } else { false }
2019
|
2120
help: add `return` as shown
2221
|
23-
LL - if true { true } else { false }
24-
LL + if true { return true } else { false }
25-
|
22+
LL | if true { return true } else { false }
23+
| ++++++
2624

2725
error: missing `return` statement
2826
--> tests/ui/implicit_return.rs:19:29
@@ -32,9 +30,8 @@ LL | if true { true } else { false }
3230
|
3331
help: add `return` as shown
3432
|
35-
LL - if true { true } else { false }
36-
LL + if true { true } else { return false }
37-
|
33+
LL | if true { true } else { return false }
34+
| ++++++
3835

3936
error: missing `return` statement
4037
--> tests/ui/implicit_return.rs:25:17
@@ -44,9 +41,8 @@ LL | true => false,
4441
|
4542
help: add `return` as shown
4643
|
47-
LL - true => false,
48-
LL + true => return false,
49-
|
44+
LL | true => return false,
45+
| ++++++
5046

5147
error: missing `return` statement
5248
--> tests/ui/implicit_return.rs:26:20
@@ -56,9 +52,8 @@ LL | false => { true },
5652
|
5753
help: add `return` as shown
5854
|
59-
LL - false => { true },
60-
LL + false => { return true },
61-
|
55+
LL | false => { return true },
56+
| ++++++
6257

6358
error: missing `return` statement
6459
--> tests/ui/implicit_return.rs:39:9
@@ -104,9 +99,8 @@ LL | let _ = || { true };
10499
|
105100
help: add `return` as shown
106101
|
107-
LL - let _ = || { true };
108-
LL + let _ = || { return true };
109-
|
102+
LL | let _ = || { return true };
103+
| ++++++
110104

111105
error: missing `return` statement
112106
--> tests/ui/implicit_return.rs:73:16
@@ -116,9 +110,8 @@ LL | let _ = || true;
116110
|
117111
help: add `return` as shown
118112
|
119-
LL - let _ = || true;
120-
LL + let _ = || return true;
121-
|
113+
LL | let _ = || return true;
114+
| ++++++
122115

123116
error: missing `return` statement
124117
--> tests/ui/implicit_return.rs:81:5
@@ -128,8 +121,7 @@ LL | format!("test {}", "test")
128121
|
129122
help: add `return` as shown
130123
|
131-
LL - format!("test {}", "test")
132-
LL + return format!("test {}", "test")
124+
LL | return format!("test {}", "test")
133125
|
134126

135127
error: missing `return` statement
@@ -140,8 +132,7 @@ LL | m!(true, false)
140132
|
141133
help: add `return` as shown
142134
|
143-
LL - m!(true, false)
144-
LL + return m!(true, false)
135+
LL | return m!(true, false)
145136
|
146137

147138
error: missing `return` statement
@@ -191,8 +182,7 @@ LL | true
191182
|
192183
help: add `return` as shown
193184
|
194-
LL - true
195-
LL + return true
185+
LL | return true
196186
|
197187

198188
error: aborting due to 16 previous errors

Diff for: ‎src/tools/clippy/tests/ui/legacy_numeric_constants.stderr

+2-3
Original file line numberDiff line numberDiff line change
@@ -68,9 +68,8 @@ LL | MAX;
6868
|
6969
help: use the associated constant instead
7070
|
71-
LL - MAX;
72-
LL + u32::MAX;
73-
|
71+
LL | u32::MAX;
72+
| +++++
7473

7574
error: usage of a legacy numeric method
7675
--> tests/ui/legacy_numeric_constants.rs:49:10

Diff for: ‎tests/ui/associated-types/defaults-suitability.current.stderr

+2-3
Original file line numberDiff line numberDiff line change
@@ -134,9 +134,8 @@ LL | type Baz = T;
134134
| --- required by a bound in this associated type
135135
help: consider further restricting type parameter `T` with trait `Clone`
136136
|
137-
LL - Self::Baz: Clone,
138-
LL + Self::Baz: Clone, T: std::clone::Clone
139-
|
137+
LL | Self::Baz: Clone, T: std::clone::Clone
138+
| ++++++++++++++++++++
140139

141140
error: aborting due to 8 previous errors
142141

Diff for: ‎tests/ui/associated-types/defaults-suitability.next.stderr

+2-3
Original file line numberDiff line numberDiff line change
@@ -134,9 +134,8 @@ LL | type Baz = T;
134134
| --- required by a bound in this associated type
135135
help: consider further restricting type parameter `T` with trait `Clone`
136136
|
137-
LL - Self::Baz: Clone,
138-
LL + Self::Baz: Clone, T: std::clone::Clone
139-
|
137+
LL | Self::Baz: Clone, T: std::clone::Clone
138+
| ++++++++++++++++++++
140139

141140
error: aborting due to 8 previous errors
142141

Diff for: ‎tests/ui/associated-types/issue-38821.stderr

+4-6
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,8 @@ LL | impl<T: NotNull> IntoNullable for T {
1313
| unsatisfied trait bound introduced here
1414
help: consider extending the `where` clause, but there might be an alternative better way to express this requirement
1515
|
16-
LL - Expr: Expression<SqlType=<Col::SqlType as IntoNullable>::Nullable>,
17-
LL + Expr: Expression<SqlType=<Col::SqlType as IntoNullable>::Nullable>, <Col as Expression>::SqlType: NotNull
18-
|
16+
LL | Expr: Expression<SqlType=<Col::SqlType as IntoNullable>::Nullable>, <Col as Expression>::SqlType: NotNull
17+
| +++++++++++++++++++++++++++++++++++++
1918

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

4543
error[E0277]: the trait bound `<Col as Expression>::SqlType: NotNull` is not satisfied
4644
--> $DIR/issue-38821.rs:23:10

Diff for: ‎tests/ui/associated-types/issue-54108.current.stderr

+2-3
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,8 @@ LL | type Size: Add<Output = Self::Size>;
1212
| ^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `Encoder::Size`
1313
help: consider further restricting the associated type
1414
|
15-
LL - T: SubEncoder,
16-
LL + T: SubEncoder, <T as SubEncoder>::ActualSize: Add
17-
|
15+
LL | T: SubEncoder, <T as SubEncoder>::ActualSize: Add
16+
| ++++++++++++++++++++++++++++++++++
1817

1918
error: aborting due to 1 previous error
2019

Diff for: ‎tests/ui/associated-types/issue-54108.next.stderr

+2-3
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,8 @@ LL | type Size: Add<Output = Self::Size>;
1212
| ^^^^^^^^^^^^^^^^^^^ required by this bound in `Encoder::Size`
1313
help: consider further restricting the associated type
1414
|
15-
LL - T: SubEncoder,
16-
LL + T: SubEncoder, <T as SubEncoder>::ActualSize: Add
17-
|
15+
LL | T: SubEncoder, <T as SubEncoder>::ActualSize: Add
16+
| ++++++++++++++++++++++++++++++++++
1817

1918
error: aborting due to 1 previous error
2019

Diff for: ‎tests/ui/attributes/rustc_confusables.stderr

+2-3
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,8 @@ LL | x.inser();
3535
|
3636
help: there is a method `insert` with a similar name
3737
|
38-
LL - x.inser();
39-
LL + x.insert();
40-
|
38+
LL | x.insert();
39+
| +
4140

4241
error[E0599]: no method named `foo` found for struct `rustc_confusables_across_crate::BTreeSet` in the current scope
4342
--> $DIR/rustc_confusables.rs:15:7

Diff for: ‎tests/ui/attributes/rustc_confusables_std_cases.stderr

+4-6
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,8 @@ LL | let mut x = VecDeque::new();
3838
| ----- earlier `x` shadowed here with type `VecDeque`
3939
help: you might have meant to use `push_back`
4040
|
41-
LL - x.push(1);
42-
LL + x.push_back(1);
43-
|
41+
LL | x.push_back(1);
42+
| +++++
4443

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

105103
error[E0599]: no method named `append` found for struct `String` in the current scope
106104
--> $DIR/rustc_confusables_std_cases.rs:24:19

Diff for: ‎tests/ui/borrowck/issue-115259-suggest-iter-mut.stderr

+2-3
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,8 @@ LL | self.layers.iter().fold(0, |result, mut layer| result + layer.proce
88
|
99
help: you may want to use `iter_mut` here
1010
|
11-
LL - self.layers.iter().fold(0, |result, mut layer| result + layer.process())
12-
LL + self.layers.iter_mut().fold(0, |result, mut layer| result + layer.process())
13-
|
11+
LL | self.layers.iter_mut().fold(0, |result, mut layer| result + layer.process())
12+
| ++++
1413

1514
error: aborting due to 1 previous error
1615

Diff for: ‎tests/ui/borrowck/issue-62387-suggest-iter-mut-2.stderr

+2-3
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,8 @@ LL | vec.iter().flat_map(|container| container.things()).cloned().co
88
|
99
help: you may want to use `iter_mut` here
1010
|
11-
LL - vec.iter().flat_map(|container| container.things()).cloned().collect::<Vec<PathBuf>>();
12-
LL + vec.iter_mut().flat_map(|container| container.things()).cloned().collect::<Vec<PathBuf>>();
13-
|
11+
LL | vec.iter_mut().flat_map(|container| container.things()).cloned().collect::<Vec<PathBuf>>();
12+
| ++++
1413

1514
error: aborting due to 1 previous error
1615

Diff for: ‎tests/ui/borrowck/issue-62387-suggest-iter-mut.stderr

+4-6
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,8 @@ LL | v.iter().for_each(|a| a.double());
88
|
99
help: you may want to use `iter_mut` here
1010
|
11-
LL - v.iter().for_each(|a| a.double());
12-
LL + v.iter_mut().for_each(|a| a.double());
13-
|
11+
LL | v.iter_mut().for_each(|a| a.double());
12+
| ++++
1413

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

2927
error: aborting due to 2 previous errors
3028

Diff for: ‎tests/ui/c-variadic/issue-86053-1.stderr

+2-3
Original file line numberDiff line numberDiff line change
@@ -63,9 +63,8 @@ LL | self , ... , self , self , ... ) where F : FnOnce ( & 'a & 'b usize
6363
|
6464
help: a trait with a similar name exists
6565
|
66-
LL - self , ... , self , self , ... ) where F : FnOnce ( & 'a & 'b usize ) {
67-
LL + self , ... , self , self , ... ) where Fn : FnOnce ( & 'a & 'b usize ) {
68-
|
66+
LL | self , ... , self , self , ... ) where Fn : FnOnce ( & 'a & 'b usize ) {
67+
| +
6968
help: you might be missing a type parameter
7069
|
7170
LL | fn ordering4 < 'a , 'b, F > ( a : , self , self , self ,

0 commit comments

Comments
 (0)