Skip to content

Commit 0da3425

Browse files
committed
Fix incorrect span highlighting in E0432 import suggestions
When suggesting import corrections where the suggestion shares a prefix with the original import (e.g., 'sync' -> 'std::sync'), the diagnostic was incorrectly highlighting 'td::s' instead of 'std::'. This was caused by the as_substr function in rustc_errors/src/lib.rs using a flawed algorithm that didn't properly handle overlapping prefix/suffix patterns. The fix rewrites as_substr to: - Try all possible prefix/suffix splits - Select the split with the shortest insertion - Use longer suffix as tiebreaker for better context This not only fixes the reported bug but also improves highlighting accuracy in 7 other diagnostic scenarios, making the '+' markers point to the exact insertion location in all cases. Signed-off-by: Osama Abdelkader <[email protected]>
1 parent 19abae7 commit 0da3425

File tree

8 files changed

+52
-25
lines changed

8 files changed

+52
-25
lines changed

compiler/rustc_errors/src/lib.rs

Lines changed: 43 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -303,20 +303,50 @@ impl TrimmedSubstitutionPart {
303303
/// `BB` is. Return the length of the prefix, the "trimmed" suggestion, and the length
304304
/// of the suffix.
305305
fn as_substr<'a>(original: &'a str, suggestion: &'a str) -> Option<(usize, &'a str, usize)> {
306-
let common_prefix = original
307-
.chars()
308-
.zip(suggestion.chars())
309-
.take_while(|(c1, c2)| c1 == c2)
310-
.map(|(c, _)| c.len_utf8())
311-
.sum();
312-
let original = &original[common_prefix..];
313-
let suggestion = &suggestion[common_prefix..];
314-
if suggestion.ends_with(original) {
315-
let common_suffix = original.len();
316-
Some((common_prefix, &suggestion[..suggestion.len() - original.len()], common_suffix))
317-
} else {
318-
None
306+
// When the original string appears multiple times in the suggestion (e.g., "sync" in "std::sync"),
307+
// the previous algorithm would incorrectly split it. We need to find the split that results in
308+
// the shortest insertion, which is the most useful for diagnostics.
309+
310+
let mut best: Option<(usize, &'a str, usize)> = None;
311+
let mut min_insertion_len = usize::MAX;
312+
313+
// Try all possible prefix lengths
314+
let mut prefix_len = 0;
315+
for prefix_chars in 0..=original.chars().count() {
316+
if prefix_chars > 0 {
317+
prefix_len += original.chars().nth(prefix_chars - 1).unwrap().len_utf8();
318+
}
319+
320+
// Check if this prefix matches in the suggestion
321+
if !suggestion.is_char_boundary(prefix_len) || !suggestion[..prefix_len].ends_with(&original[..prefix_len]) {
322+
continue;
323+
}
324+
325+
let original_suffix = &original[prefix_len..];
326+
let suggestion_after_prefix = &suggestion[prefix_len..];
327+
328+
// Check if the remaining original appears as a suffix in the remaining suggestion
329+
if !suggestion_after_prefix.ends_with(original_suffix) {
330+
continue;
331+
}
332+
333+
// This is a valid split - calculate the insertion length
334+
let suffix_len = original_suffix.len();
335+
let insertion_len = suggestion_after_prefix.len() - suffix_len;
336+
337+
// Keep track of the split with the smallest insertion
338+
// If there's a tie, prefer the one with a longer suffix (more context)
339+
let is_better = insertion_len < min_insertion_len ||
340+
(insertion_len == min_insertion_len && best.map_or(true, |(_, _, old_suffix)| suffix_len > old_suffix));
341+
342+
if is_better {
343+
min_insertion_len = insertion_len;
344+
let insertion = &suggestion_after_prefix[..insertion_len];
345+
best = Some((prefix_len, insertion, suffix_len));
346+
}
319347
}
348+
349+
best
320350
}
321351

322352
impl CodeSuggestion {

tests/ui/argument-suggestions/issue-97197.stderr

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ LL | pub fn g(a1: (), a2: bool, a3: bool, a4: bool, a5: bool, a6: ()) -> () {}
1212
help: provide the arguments
1313
|
1414
LL | g((), /* bool */, /* bool */, /* bool */, /* bool */, ());
15-
| +++++++++++++++++++++++++++++++++++++++++++++++
15+
| ++++++++++++++++++++++++++++++++++++++++++++++++
1616

1717
error: aborting due to 1 previous error
1818

tests/ui/imports/issue-45799-bad-extern-crate-rename-suggestion-formatting.stderr

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ LL | extern crate std;
88
help: you can use `as` to change the binding name of the import
99
|
1010
LL | extern crate std as other_std;
11-
| ++++++++++++
11+
| +++++++++++++
1212

1313
error: aborting due to 1 previous error
1414

tests/ui/imports/no-std-inject.stderr

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ LL | extern crate core;
88
help: you can use `as` to change the binding name of the import
99
|
1010
LL | extern crate core as other_core;
11-
| +++++++++++++
11+
| ++++++++++++++
1212

1313
error: aborting due to 1 previous error
1414

tests/ui/suggestions/return-bindings.stderr

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ help: consider returning the local binding `s`
2424
|
2525
LL | let s: String = if let Some(s) = opt_str {
2626
LL ~ s
27-
LL ~
2827
|
2928

3029
error[E0308]: mismatched types
@@ -56,7 +55,6 @@ help: consider returning the local binding `s`
5655
|
5756
LL | let s: String = if let Some(s) = opt_str {
5857
LL ~ s
59-
LL ~
6058
|
6159

6260
error[E0308]: `if` and `else` have incompatible types
@@ -75,9 +73,8 @@ LL | | };
7573
|
7674
help: consider returning the local binding `s`
7775
|
78-
LL | let s = if let Some(s) = opt_str {
79-
LL ~ s
80-
LL ~ } else {
76+
LL ~ let s = if let Some(s) = opt_str {
77+
LL + s
8178
|
8279

8380
error[E0308]: mismatched types

tests/ui/suggestions/struct-field-type-including-single-colon.stderr

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ LL | a: foo:A,
77
help: write a path separator here
88
|
99
LL | a: foo::A,
10-
| +
10+
| +
1111

1212
error: expected `,`, or `}`, found `:`
1313
--> $DIR/struct-field-type-including-single-colon.rs:9:11
@@ -26,7 +26,7 @@ LL | b: foo::bar:B,
2626
help: write a path separator here
2727
|
2828
LL | b: foo::bar::B,
29-
| +
29+
| +
3030

3131
error: expected `,`, or `}`, found `:`
3232
--> $DIR/struct-field-type-including-single-colon.rs:15:16

tests/ui/suggestions/type-ascription-instead-of-path-2.stderr

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ LL | let _ = vec![Ok(2)].into_iter().collect:<Result<Vec<_>,_>>()?;
77
help: maybe write a path separator here
88
|
99
LL | let _ = vec![Ok(2)].into_iter().collect::<Result<Vec<_>,_>>()?;
10-
| +
10+
| +
1111

1212
error: aborting due to 1 previous error
1313

tests/ui/suggestions/type-ascription-instead-of-path-in-type.stderr

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ LL | let _: Vec<A:B> = A::B;
77
help: you might have meant to write a path instead of an associated type bound
88
|
99
LL | let _: Vec<A::B> = A::B;
10-
| +
10+
| +
1111

1212
error[E0107]: struct takes at least 1 generic argument but 0 generic arguments were supplied
1313
--> $DIR/type-ascription-instead-of-path-in-type.rs:6:12

0 commit comments

Comments
 (0)