First of all, I would like to say that I am very grateful for this amazing project. I’m aware it has a very complex codebase.

I don’t remember a single occasion where I accidentally renamed an item that wasn’t in my project. Now I can’t rename anything anymore. This is affecting our workflow. While we don’t have a bug fix, is it possible to create a configuration to not do this rename check?

So if everything goes well 🤞 with today’s nightly this config should be available. You will just need to set the following config to “true”, we defaulted it to “false” to make the feature visible to users, as it works just fine for most of the time. Here is the related piece in the documentation

[[rust-analyzer.rename.allowExternalItems]]rust-analyzer.rename.allowExternalItems (default: `false`)::
+
--
Allow renaming of items not belonging to the loaded workspaces.
--

Hi, @alibektas! I just tried. It worked like a charm! I’m proud of you and this community. Thank you very much.

Ah, this issue might be why Rename Symbol no longer works on local variables in some rustc crates.

We use a CrateGraph to represent how crates within a project relate to one another. With #15232 we forbade renaming of non-local items which means that if a crate’s origin

https://github.com/rust-lang/rust-analyzer/blob/f19479a2ad238ad861cfe8d57e63beeccb56169e/crates/base-db/src/input.rs#L143-L152

is something else other Local we cannot allow renaming to take place. The given example uses linkedProjects. This will create a “healthy” crate graph.

flowchart TB
    ProjectRoot -- CrateOrigin::Local --> Foo
    ProjectRoot -- CrateOrigin::Local --> Bar

The trouble starts when Foo depends on Bar ( Following the repro foo is reloaded-hooks-x86-sys and Bar is reloaded-hooks-portable . This will cause Bar to be of CrateOrigin::Library from Foo’s perspective. This would have been OK had the linkedProjects not already registered Bar as CrateOrigin::Local. So now we have something like

flowchart TB
    ProjectRoot -- CrateOrigin::Local --> Foo
    ProjectRoot -- CrateOrigin::Local --> Bar
    Foo -- CrateOrigin::Library --> BarAsLib

Or in simplified terms :

flowchart TB
    ProjectRoot -- CrateOrigin::Local --> Foo
    ProjectRoot -- CrateOrigin::Local --> Bar
    Foo -- CrateOrigin::Library --> Bar

So resolving this issue will entail changes in CrateGraph which may/may not be easy. I think until then we can revert change from #15232 . Let’s ask for @Veykril 's opinion.

Here’s a test case where you can find a minimal repro.

ping @TCROC @vaniusrb

Adding a config was on my TODO list, let me work on it now and get back to you in a few hours. Sorry for the inconvenience!

refactor-broken-example.zip

Ok so I figured out how to reproduce this issue in minimized environments. There are 2 ways to cause it. Below are my findings:

Linking against 2 proc macros

The interesting thing is that in this case, only the 2nd proc macro crate breaks and doesn’t allow renaming vars:

example1-macros ✅

example2-macros ❌

And it seems to be directly correlated to the order of the projects that make use of the macros. If I rearrange the linked project settings to be like so:

{
    "rust-analyzer.linkedProjects": [
        "example2/example2/Cargo.toml",
        "example1/example1/Cargo.toml",
        "example2/example2-macros/Cargo.toml",
        "example1/example1-macros/Cargo.toml",
    ],
}

with the example2 being before example1, example2 will work and example1 will break. The order of the macro projects does not seem to matter, but the order of the project that references the macro project seems to break it.

So if I switched these 2:

        "example1/example1/Cargo.toml",
        "example2/example2/Cargo.toml",

To be:

        "example2/example2/Cargo.toml",
        "example1/example1/Cargo.toml",

Then example 2 will work and example 1 will break.

bitvec

Adding a reference to the bitvec crate in example1 or example2 seems to break refactoring in any procedural macro crates. This can be seen in example1/Cargo.toml. Uncomment the # bitvec = "1.0.1" and you will notice that you can no longer refactor things in example1-macros and start getting the “Cannot rename a non-local definition.” error.

I attached an archive of the project to reproduce on your machines. Let me know if you guys have any questions and / or need anything else from me.

Thanks again everyone! Ur all beautiful human beings and Merry Christmas! ♥️

Ok so starting up my projects in a brand new session, things are working correctly. I swear I was getting this error yesterday though. I’ll spend some time developing this week and see if it reappears.

You mightve been still using the old version. VSCode updates the extensions after startup, so you need to reload the window once to actually run the new version.

@shepmaster thanks, that should suffice to figure it out.

So first of all sorry for taking so much time I was sick for a week. I have addressed the reasons behind this issue and my PR solves most of them. The project given as a repro however will still not be able use renaming. I will address this in a following PR. But let me mention why it currently remains unresolved.

As I mentioned before we deal with multiple occurrences of the same crate and the dedup rules were until now too strict ( two crates must be equal to be merged into one but this is not possible in the given example, see comment above) . After I relaxed the conditions for merging I saw that there are inconsistencies in versions of one crate “hashbrown” in the given repro ( see metadata for both reloaded_hooks_{portable,x86_sys} @Sewer56 ). But I couldn’t have relaxed the condition to say if one project has a dependency that has a different version than its other occurrence then merge them anyway. So this still needs to be addressed somehow but until then it wouldn’t hurt to come up with a solution that solves this issue in most of cases.

@alibektas first initial test seems to indicate that its working! 😃 I’m going to try it for a bit and see if I run into any issues with it! 😃

Honestly I have always built from the source by running just cargo xtask install but I believe you are right with pre-release. In fact if you go to the extension tab and right click on rust-analyzer you can see that the version with the same version number (0.4.1810) has a 20 hours ago mark next to it.

And mind you that the feature will be available around 01:15 CET based on the most recent release time from yesterday.

Thank you @alibektas! 😃 No sorries necessary! 😃

Yes, all discovered and loaded projects, whether explicitly set via linkedProjects or implicitly loaded.

I think that’s what I’m saying, yes 😅 It just fundamentally doesn’t make sense to me that we could consider only one version of a crate in a specific folder local and the other not, it should be purely a property of the folder they’re in.

@alibektas I’m not a maintainer so I’m not familiar with the codebase at all, but looks like it was a really sneaky bug! Looks like a simple enough fix from the surface level 😃

I’ll be curious to see what others think as well! (Looks good to me tho! 😉)

I’ll take a sec to learn how to build ra from source and test on my main project as well to see if it fixes it 😃

Sure let me wrap things up and post the link here

So finally I found the reason which is a little peculiar. It is that we have for the “proc_macro2” crate different <Self as CrateData>.env["OUT_DIR"] values each time a new workspace is detected but for the deduplication we had previously assumed a one to one overlap of all Env hashes. When I relax this condition such that OUT_DIR plays no further role the issue is resolved. But before opening a PR I wanted to make sure that nothing speaks against it.

As to testing : Since the said crate is an external crate collecting metadata and extending them will not let use detect this error, same goes for the current slow-tests infra ( In both situations the error was undetected ). So any suggestions regarding a test case wouldn’t be so bad either

There is a linkedProjects setting which overwrites the scanning behavior

Ah, I can tell you why this fails (and why it works fine if the orchestrator is moved one level up). The project structure as is means that we only load the ui crate, but not the orchestrator (because it is too deep, we only scan the root and first level for cargo projects), so the orchestrator is considered non local as its not inside the workspace of ui (we might be able to cheat around a bit by respecting a crate’s position to the opened VSCode/LSP workspace root as well as any cargo workspaces). OTOH if you lift the orchestrator up one level we now load it as a cargo workspace as well, where the deduplication fix kicks in, making it just work.

@shepmaster @Veykril I was also able to reproduce this using the archive provided by @shepmaster

I can confirm that with 0.3.1748 I am now able to rename symbols in rustc_mir_transform once again.

Thanks for the instructions!

I went ahead and took a crack at a fix. I believe the correct course might be to relax the definition of CrateOrigin::is_local to include libraries which do not have a repo specified. Since we can’t know who the code at that path belongs to really, best to err on the conservative side. I had a look at all the current callsites and they seem to be using it to mean “is possibly user code”. It may be worth renaming is_local to is_possibly_user_code or something like that to convey what it really means (especially with the behavior change I’m proposing).

impl CrateOrigin {
    pub fn is_local(&self) -> bool {
        match self {
            Self::Local { .. } => true,
            Self::Library { repo, .. } => repo.is_none(),
            _ => false,
        }
    }
}

This change fixes my project. I am able to rename things in the cross-referenced crates but it still fails if I try to rename a symbol inside std and also fails if I try to rename something inside a regular crates.io dependency (I tried with tokio).

I made a PR here ogapo#1. I wasn’t sure enough about the change to want to PR against this repo (happy to do so if you recommend though). The change is small though, so might be better to be incorporated into PR #15754

👋 Thank you for your answer @ogapo . We were planning to make some changes to how we determine CrateOrigin. Let me get back to you in a couple of weeks once I got some time to work on this and I am personally in favor of you making a PR ( assuming that you would like to contribute to the project yourself. )

There is nothing to address in that case (aside from our PR relaxing things in regards to the origin), the root of the problem is that the crate that is being loaded in the linked projects while also being loaded as a dependency of another is subject to different Cargo.lock files. This makes it inherently impossible for rust-analyzer to dedup the crate if dependencies of said crate have differing versions in the respective lock files. As such, the project should either take care of trying to sync the lock files or only add the cargo workspace to the linked projects that contains the other crate in its dep graph as well as that will be effectively loaded due to that anyways.

The only thing that we should be looking into is recognizing this, and somehow inform the user that this kind of set up can cause IDE features like renaming to fail. Although I think there is a bug here in how we assign CrateOrigin’s still / how we forbid renaming as well. The crates in question where renaming fails here are still inside the project folders (presumably via path dependencies) in which case we shouldn’t forbid renames in the first place I think, since those are technically fine to modify. We only want to prevent the std lib sources and crates.io sources from being modified.

I’ve confirmed that reverting to 0.3.1657 allows Rename Symbol to work in rustc_mir_transform.