Update substring match for substitutions

[faculerena]

Fixed as_substr function to check for matching prefixes too. This fixes this issue where td::s is matched instead of std::. This same behaviour can be seen with time and tokio::time and similar cases where the error import starts with the same prefix as the suggestion.

use sync;
fn main() {}

error[E0432]: unresolved import `sync`
 --> /tmp/test_import.rs:1:5
  |
1 | use sync;
  |     ^^^^ no `sync` in the root
  |
help: consider importing this module instead
  |
1 | use std::sync;
  |      +++++

After the changes:

error[E0432]: unresolved import `sync`
 --> /tmp/test_import.rs:1:5
  |
1 | use sync;
  |     ^^^^ no `sync` in the root
  |
help: consider importing this module instead
  |
1 | use std::sync;
  |     +++++

[Kivooeo]

Well, I guess we can't modify this as_substr, seems like even smallest change breaks everything else,

I guess we should somehow modify this function

rust/compiler/rustc_resolve/src/imports.rs

Line 736 in ab92564

fn throw_unresolved_import_error(

This function was pointed by -Ztrack-diagnostics

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 as_substr (haven't checked stack trace yet, but I guess you did this already, otherwise I have no idea how you found this as_substr function)

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:

1. crate::
2. rate::c or ate::cr

so if we got first we are fine with that, if we got second then we should somehow move cr to the start, so we will get crate::, here we have some advantage because we always know that everything after :: was stripped from start, so it should be moved

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

[Kivooeo]

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

[faculerena]

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.

[Kivooeo]

I guess it's possible to remove core and std check from here, I was just lazy to rerun tests once again to test it

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 ./x test ui you will see if there any regressions in test suite

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

[faculerena]

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.

13	help: consider importing this module instead
14	   |
15	LL | use test::test as y;
-	   |         ++++++
+	   |     ++++++
17	
18	error: aborting due to 2 previous errors
19	

I think this is an acceptable compromise (as is the case when there's a nested foo::foo), but I'm no one to decide over this. We could add a check for cases when the suggestion is foo:: and the original is foo

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()));
         }
     }

[Kivooeo]

Have you tried to run tests locally? Because to update them you to pass --bless flag, which will update "expected output"

[Kivooeo]

Like this ./x test ui --bless

[rustbot]

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[faculerena]

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.

[Kivooeo]

For author information: since I was mentoring this I can't approve this to merge, and we have to wait on someone else, but what can I do is write some more explanation on what is going on here

For reviewer information: This code is a hack, but there is no other way to do this, or needing a massive refactor in fundamental things? I'm not sure. this problem is so deeply inside that is hard to evaluate right fix for it, the problem is how multipart_suggestion generates suggestion and highlights diff, in its current implementation it will skip first same letters in original and suggestion strings, so if we got a sync and std::sync we will highlight td::s since first s letters are the same and it will shift on one letter, so roughly this PR adds a strict check (I'm not sure about false positives, should be whether very rare or impossible) that checks if we are in situation like above when we need suggest unresolved import with same prefix letters, and if so it correct processes this case otherwise just goes by default path which is works fine for all other cases

@BoxyUwU, please take a look on this or reroll if you dont have time on this, it needs some look from side