feat!: create alias when renaming an import.

[UserIsntAvailable]

Closes #15858

Implemented:

• - Prevent using reserved keywords (e.g self) and _.
• - Rename other modules that might be referencing the import.
• - Fix "broken" tests.
• - Rename only "direct" references.
• - Test more cases.

Future possibilities:

1. Also support extern crate <name> syntax.
2. Allow aliasing self when it is inside an UseTreeList.
   3. If import path already has an alias, "rename" the alias.
   4. Create alias even if path is not the last path segment.

[alibektas]

👋 Thank you for this PR! I have also seen the your post in the related issue. I am not well acquainted with the inner workings of the renaming infra and honestly I have only taken a short glance at it but maybe you could enlighten me on sth: Why don't you have your aliasing implementation propagate all the way through ide_db::rename::rename_references so that,

• You don't have to rewrite some of the stuff that is already there
• If/when a certain thing about renaming changes, so your part of the code adapts to the changes too.
• The actual problem you named would probably be resolved by doing this too, or not? I will have to think about this last point so it may be possible that you see an newer version of this post sometime in the next couple of hours.

Ofc the editing that an alias would cause is very different to the import statement but on the other hand not so different for the defs affected by it, something we can leverage.

And one more thing to keep in mind : We don't like unwraps. Whenever possible you should avoid using it and handle it instead.

[UserIsntAvailable]

Why don't you have your aliasing implementation propagate all the way through ide_db::rename::rename_references

That is a good question; it just seemed easier to implement at the ide_db::rename abstraction, because it wasn't clear how to modify rename/rename_references without needing to change every call-site. Before merging I would like to do that, because as you noted "[I] don't have to rewrite some of the stuff that is already there", but for now, that is not that important.

The actual problem you named would probably be resolved by doing this too, or not? [...]

Not really; the way that I see it is that a rename is "absolute"; if an item gets renamed then all references needs to update their name too, because, no matter if the item path is qualified or not, they all need to point to the new_name to find the item.

To the contrary, an alias is "relative"; you only change references that directly depend on that particular item name. A more "formal" way to express this would be to say that, if Name is present on module X, then all references/usages that depend on X::Name should be "renamed" (of course, this would also work if Name is referenced with self, super and crate).

So, with the example of yesterday:

pub mod foo { pub struct Foo; }

use foo::Foo$0;

fn main() {
    let _: Foo;      // Changes, because directly depends (uses) `Foo`.
    let _: foo::Foo; // This doesn't get changed; points to same reference, but
                     // doesn't depend (use) the `Foo` that is in the module/scope.
}

But lets make this "minimal" example a little more complicated:

pub mod foo { pub struct Foo; }
pub mod foo2 { pub struct Foo; }

mod bar {
    use super::Foo; // Changes, because directly depends (uses) `Foo`.
}
mod baz {
    use crate::Foo; // Changes, because directly depends (uses) `Foo`.
}

use foo::Foo$0;

fn main() {
    let _: Foo;       // Changes, because directly depends (uses) `Foo`.
    let _: self::Foo; // Changes, because directly depends (uses) `Foo`. (`self`
                      // == current scope/mod).
    let _: foo::Foo;  // This doesn't get changed; points to same reference, but
                      // doesn't use the `Foo` that is in the module/scope.
    let _: foo2::Foo; // This doesn't get changed; points to different reference.
}

Currently, all but foo2::Foo (which is good) are being renamed when using "alias fallback".

Ofc the editing that an alias would cause is very different to the import statement but on the other hand not so different for the defs affected by it, something we can leverage.

I think is the other way around; the editing part (editing/updating the ast) is the same, but what defs that are affected is different (like I explained above). I think the only place that we would need to change on ide_db::rename::rename_reference would be:

rust-analyzer/crates/ide-db/src/rename.rs

Line 340 in 2c2a7a1

let usages = def.usages(sema).all();

If we are using alias as a fallback (the easiest way to check this is to just add a alias_fallback: bool parameter), then only find "direct" references.

I have been thinking that we might be able to add a new "parameter" to the builder pattern of FindUsages to allow only "direct" usages. FindUsages::search can already know that foo::Foo and foo2::Foo are different references, so I don't think it would that hard to allow this?

---

Sorry if I make this way more longer that it should have been; I just wanted to give a more detailed problem perspective, in the hopes that we can see if I'm missing something obvious 😅.

And one more thing to keep in mind : We don't like unwraps. Whenever possible you should avoid using it and handle it instead.

Yeah sure; I wasn't very sure how I should write "this option can't be None" type of thing, so I just unwrapped and put a "never!" assertion at the top. That code was a hack anyways, so it would be removed once I can properly find usages.

---

/cc @Veykril You might have something to say about all this?

[Veykril]

Yes, we should support that all the same

[UserIsntAvailable]

It seems that this is blocked on usages.all() not being able to find references to extern crate modules.

[Veykril]

4. Create alias even if path is not the last path segment.

This I don't think we should do as that is a more invasive change in the end (splits the imports etc).

[Veykril]

The problem of usages relying on the "re-export" of an import is tricky indeed. I am not sure if we already have everything in place to do that properly yet (I don't think we do), as that requires keeping track of what import a usage refers to (this we have), and then resolving the imports step by step (this we are still missing I believe) such that we can check whether the renamed import affects the usage or not.

Though we most likely want a new flag on the usage search for that.

[UserIsntAvailable]

I am not sure if we already have everything in place to do that properly yet (I don't think we do), as that requires keeping track of what import a usage refers to (this we have), and then resolving the imports step by step (this we are still missing I believe) such that we can check whether the renamed import affects the usage or not.

Ok that sounds unfortunate; I'm gonna try implement it on these next days. I gave a look at FindUsages::search and there is a lot to take into account 😅.

Thanks for the heads up though 👍.

[Veykril]

It's fine if we can't handle that case properly for now though,as long as we open an issue / fixme for that. Most people will use absolute paths for things, especially if they have the default rust-analyzer settings.

[alibektas]

@UserIsntAvailable this is mostly my fault, sorry! Whatever you can't/won't do I can do afterwards.

[UserIsntAvailable]

@Veykril

It's fine if we can't handle that case properly for now though,as long as we open an issue / fixme for that.

I'm fine with that; I just didn't want to leave this on a somewhat broken state.

@alibektas

this is mostly my fault, sorry! Whatever you can't/won't do I can do afterwards.

Don't worry about it! I wasn't really sure if it was fine to merge this as is, then do the other necessary changes on another PR. If you want to do it I don't have any problem with it!

[UserIsntAvailable]

@rustbot label -S-waiting-on-author +S-waiting-on-review

[Veykril]

Seems good to me, as noted by the FIXME, this can be done more properly via ide_db::rename_reference once we have #14079 fully done. I believe we can already implement the renaming for extern crate foo correctly as we have that in the Definition.

[Veykril]

Is this considered ready from your PoV? (asking since its still a draft)

[bors]

☔ The latest upstream changes (presumably #16586) made this pull request unmergeable. Please resolve the merge conflicts.

[UserIsntAvailable]

@Veykril Oops, I thought I marked it as ready. 👍

[Veykril]

Rebased it, thanks!
@bors r+

[bors]

📌 Commit 6dd5dc1 has been approved by Veykril

It is now in the queue for this repository.

[bors]

⌛ Testing commit 6dd5dc1 with merge ac1029f...

[bors]

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing ac1029f to master...