Skip to content
Closed
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
59 changes: 46 additions & 13 deletions compiler/rustc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,20 +303,53 @@ impl TrimmedSubstitutionPart {
/// `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 suggestion.ends_with(original) {
let common_suffix = original.len();
Some((common_prefix, &suggestion[..suggestion.len() - original.len()], common_suffix))
} else {
None
// When the original string appears multiple times in the suggestion (e.g., "sync" in "std::sync"),
// the previous algorithm would incorrectly split it. We need to find the split that results in
// the shortest insertion, which is the most useful for diagnostics.
Comment on lines +306 to +308
Copy link
Member

Choose a reason for hiding this comment

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

This is not a correct description of the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can improve that.


let mut best: Option<(usize, &'a str, usize)> = None;
let mut min_insertion_len = usize::MAX;

// Try all possible prefix lengths
let mut prefix_len = 0;
for prefix_chars in 0..=original.chars().count() {
if prefix_chars > 0 {
prefix_len += original.chars().nth(prefix_chars - 1).unwrap().len_utf8();
}

// Check if this prefix matches in the suggestion
if !suggestion.is_char_boundary(prefix_len)
|| !suggestion[..prefix_len].ends_with(&original[..prefix_len])
{
continue;
}

let original_suffix = &original[prefix_len..];
let suggestion_after_prefix = &suggestion[prefix_len..];

// Check if the remaining original appears as a suffix in the remaining suggestion
if !suggestion_after_prefix.ends_with(original_suffix) {
continue;
}

// This is a valid split - calculate the insertion length
let suffix_len = original_suffix.len();
let insertion_len = suggestion_after_prefix.len() - suffix_len;

// Keep track of the split with the smallest insertion
// If there's a tie, prefer the one with a longer suffix (more context)
let is_better = insertion_len < min_insertion_len
|| (insertion_len == min_insertion_len
&& best.map_or(true, |(_, _, old_suffix)| suffix_len > old_suffix));

if is_better {
min_insertion_len = insertion_len;
let insertion = &suggestion_after_prefix[..insertion_len];
best = Some((prefix_len, insertion, suffix_len));
}
}

best
}

impl CodeSuggestion {
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/argument-suggestions/issue-97197.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ LL | pub fn g(a1: (), a2: bool, a3: bool, a4: bool, a5: bool, a6: ()) -> () {}
help: provide the arguments
|
LL | g((), /* bool */, /* bool */, /* bool */, /* bool */, ());
| +++++++++++++++++++++++++++++++++++++++++++++++
| ++++++++++++++++++++++++++++++++++++++++++++++++

error: aborting due to 1 previous error

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ LL | extern crate std;
help: you can use `as` to change the binding name of the import
|
LL | extern crate std as other_std;
| ++++++++++++
| +++++++++++++
Comment on lines 10 to +11
Copy link
Member

Choose a reason for hiding this comment

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

This is like the linked issue but "from the other direction". So you just relocated the issue basically regressing this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, in this case the compiler suggests prepending "std as other_" to std to be "std as other_std", instead of appending "as other_std". Both result in same phrase. It's not a regression.

Choose a reason for hiding this comment

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

Is effectively the same error as before. We would want to be semantically correct, suggesting to import as alias instead of adding something in the middle of the phrase. It's not wrong, but it's weird. The previous error suggested adding td::s and that is actually correct, but semantically weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I totally agree, please let me know if you would like me to close it or try another approach. If you plan to fix it, then it makes sense to close this draft and I look for another issue to fix. Best regards.

Choose a reason for hiding this comment

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

I fixed it in the other PR. Still in draft to discuss a few things but I think it's ok.


error: aborting due to 1 previous error

Expand Down
2 changes: 1 addition & 1 deletion tests/ui/imports/no-std-inject.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ LL | extern crate core;
help: you can use `as` to change the binding name of the import
|
LL | extern crate core as other_core;
| +++++++++++++
| ++++++++++++++

error: aborting due to 1 previous error

Expand Down
7 changes: 2 additions & 5 deletions tests/ui/suggestions/return-bindings.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ help: consider returning the local binding `s`
|
LL | let s: String = if let Some(s) = opt_str {
LL ~ s
LL ~
|

error[E0308]: mismatched types
Expand Down Expand Up @@ -56,7 +55,6 @@ help: consider returning the local binding `s`
|
LL | let s: String = if let Some(s) = opt_str {
LL ~ s
LL ~
|

error[E0308]: `if` and `else` have incompatible types
Expand All @@ -75,9 +73,8 @@ LL | | };
|
help: consider returning the local binding `s`
|
LL | let s = if let Some(s) = opt_str {
LL ~ s
LL ~ } else {
LL ~ let s = if let Some(s) = opt_str {
LL + s
|

error[E0308]: mismatched types
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ LL | a: foo:A,
help: write a path separator here
|
LL | a: foo::A,
| +
| +

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

error: expected `,`, or `}`, found `:`
--> $DIR/struct-field-type-including-single-colon.rs:15:16
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ LL | let _ = vec![Ok(2)].into_iter().collect:<Result<Vec<_>,_>>()?;
help: maybe write a path separator here
|
LL | let _ = vec![Ok(2)].into_iter().collect::<Result<Vec<_>,_>>()?;
| +
| +

error: aborting due to 1 previous error

Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ LL | let _: Vec<A:B> = A::B;
help: you might have meant to write a path instead of an associated type bound
|
LL | let _: Vec<A::B> = A::B;
| +
| +

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