-
Notifications
You must be signed in to change notification settings - Fork 13.9k
Fix incorrect span highlighting in E0432 import suggestions #148081
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
Fix incorrect span highlighting in E0432 import suggestions #148081
Conversation
This comment has been minimized.
This comment has been minimized.
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 <osama.abdelkader@gmail.com>
0da3425 to
b2d2b36
Compare
| // 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can improve that.
|
Redundant with #148061 |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
| LL | extern crate std as other_std; | ||
| | ++++++++++++ | ||
| | +++++++++++++ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I didn't know that there is another draft PR for that issue, please let me know whether I should close this one or improve it. |
|
Closed since it's redundant. |
Fixes #148070