Update substring match for substitutions #148061
Conversation
rustbot
added
S-waiting-on-author
faculerena
force-pushed
the
master
branch
3 times, most recently
from
6e1a961 to
3a0a76b
Compare
This comment has been minimized.
This comment has been minimized.
|
Well, I guess we can't modify this I guess we should somehow modify this function rust/compiler/rustc_resolve/src/imports.rs Line 736 in ab92564 This function was pointed by error[E0432]: unresolved import `sync`
--> /home/gh-Kivooeo/test_/src/main.rs:2:5
|
2 | use sync;
| ^^^^ no `sync` in the root
|
= note: -Ztrack-diagnostics: created at compiler/rustc_resolve/src/imports.rs:768:24
help: consider importing this module instead
|
2 | use std::sync;
| +++++This one generates a final suggestion, but I don't really sure how this tide with So, what is my idea about, in the place where we generating suggestion (I guess this is function above, but I'm not 100% sure) we need to check what we got:
so if we got first we are fine with that, if we got second then we should somehow move I will be honest that I have no idea how to implement that in that function, I maybe will took another look but later, so there is just some of my thoughts on the core idea how this should work |
diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs
index 17cd466f96b..2ed4e281adb 100644
--- a/compiler/rustc_errors/src/lib.rs
+++ b/compiler/rustc_errors/src/lib.rs
@@ -306,6 +306,18 @@ fn replaces_meaningful_content(&self, sm: &SourceMap) -> bool {
/// `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)> {
+ if suggestion.contains("::")
+ && ["core", "std"].iter().any(|&prefix| suggestion.starts_with(prefix))
+ && suggestion.ends_with(original)
+ && let Some(c1) = suggestion.chars().nth(0)
+ && let Some(c2) = original.chars().nth(0)
+ && c1 == c2
+ {
+ let prefix_len = suggestion.len() - original.len();
+ let prefix = &suggestion[..prefix_len];
+ return Some((0, prefix, original.len()));
+ }
+
let common_prefix = original
.chars()
.zip(suggestion.chars())Btw, just decided to check will this work or not... and sadly it is, all tests passed, and it gave correct suggestion But... I'd consider this as a last resort if we can't come up with a better solution |
|
Yeah, I was just trying to generalize that solution, as that does not catch third party crates. But yeah, it looks like the solution is that way. |
|
I guess it's possible to remove It's a bit late for me atm (3 am) so I would able to test it without this check later today Or if you want to test it yourself, you can copy this into your rustc fork and run But I'd be happier to see an implementation of my idea above, I also will try to investigate this direction more later on today |
This comment has been minimized.
This comment has been minimized.
|
This last potential fix I pushed has only one test failing. I missed that for some reason locally, I think I'm missing something for proper testing. I think this is an acceptable compromise (as is the case when there's a nested diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs
index 9cd34a8747f..5e4bd514af8 100644
--- a/compiler/rustc_errors/src/lib.rs
+++ b/compiler/rustc_errors/src/lib.rs
@@ -312,7 +312,10 @@ fn as_substr<'a>(original: &'a str, suggestion: &'a str) -> Option<(usize, &'a s
&& suggestion.len() > original.len()
{
let prefix = &suggestion[..suggestion.len() - original.len()];
- if prefix.ends_with("::") && suggestion.chars().next() == original.chars().next() {
+ if prefix.ends_with("::")
+ && suggestion.chars().next() == original.chars().next()
+ && suggestion.trim_suffix("::") != original
+ {
return Some((0, prefix, original.len()));
}
} |
|
Have you tried to run tests locally? Because to update them you to pass |
|
Like this |
rustbot
added
the
S-waiting-on-review
rustbot
removed
the
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
The change in this test is minimal, as this is a specific case. The benefits for other cases explained in issue 148070 outweight the change.
|
Ok, finally I updated a test to reflect this new behaviour, as I think it's just a test quirk more than a lint change. Thanks @Kivooeo for the help too. |
bors
added
S-waiting-on-author
|
Don't know why it's failing there. The logs show the wrong highlighting so I guess something is out of sync? |
|
As Jonathan said there is some soft conflict, so maybe something because of this, but I agree that this is weird because locally it does work correctly @JonathanBrouwer, can we try this in other rollup? |
|
Lets try the |
|
Actually I don't see this being a soft conflict, lets try immediately @bors try jobs=aarch64-gnu-llvm-20-1 |
Update substring match for substitutions try-job: aarch64-gnu-llvm-20-1
This comment has been minimized.
This comment has been minimized.
|
💔 Test for 9ffa1fb failed: CI. Failed jobs:
|
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
Wow |
|
Looks like the change is not being applied/recognized, even when previously it worked. It fails in the two new/changed tests. I actually don't know what's happening :( |
|
For some reason, I did Something had changed in before |
rustbot
removed
the
S-waiting-on-author
|
It's interesting because I even put a |
rustbot
added
the
S-waiting-on-review
|
Ok sorry, I closed the PR by accident renaming the fork branch. Yes, it's strange, I tried the same. I'll try later tracing the calls but is weird to have the same issue in another place, with a function that looks like it's not called anymore? (It looks like it's called in the same file, but not printing anything) |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
A few hours of debugging, here's when it's being filtered: diff --git a/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs b/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs
index 5d22a8b8e30..41a1432798a 100644
--- a/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs
+++ b/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs
@@ -408,6 +408,7 @@ fn emit_messages_default(
let mut parts = sub
.parts
.into_iter()
+ .map(|p| p.trim_trivial_replacements(sm))
.filter_map(|p| {
if is_different(sm, &p.snippet, p.span) {
Some((p.span, p.snippet))I'll check if there's something that can be done to avoid doing it that way, but there's the problem. (This makes tests fail) Trying to trace the execution when none of the functions I suspected were being called was weird! |
|
Oh there's the problem: rustc now uses annotate_snippets. I think the problem is now there instead of here. Reference, PR The string coming from |
|
Right, that makes sense. Note that annotate snippets is only used on nightly, so only fixing annotate snippets might make the test fail on stable |
|
Shouldn't the fix, as it is right now here, work for stable then? I don't fully understand that. |
|
Not really, just because you pushing changes for main branch, and you can't change or replace things in older versions, since main branch is around 1.93 version and stable is 1.91.1, so you can't directly change it, so in this case we have to wait when stable become 1.93 and catch this changes (I tried to explain that, it was pretty hard for non-native speaker lol, hope it makes at least little sense) |
|
Don't worry, I'm not either. I understood perfectly lol, thanks! I sent a PR to |
7 participants
Fixed
as_substrfunction to check for matching prefixes too. This fixes this issue wheretd::sis matched instead ofstd::. This same behaviour can be seen withtimeandtokio::timeand similar cases where the error import starts with the same prefix as the suggestion.After the changes: