[Since 0.4.1658] Cannot rename a non-local definition.

[Sewer56]

System Details

rust-analyzer version: 0.3.1665-standalone (first broken stable version)
rustc version: rustc 1.72.1 (d5c2e9c34 2023-09-13)

Details

Since v0.4.1658-standalone it is not possible to rename any item inside one of my projects. More details in reproduction section.

Regression

Broken since v0.4.1658-standalone (nightly). Earlier versions work as expected.

---

1658 (first broken ver.):
Actions Run: https://github.com/rust-lang/rust-analyzer/actions/runs/6153193088
Commit: cc6c820

1655 (previous functional release):
Commit: 994df3d

---

I believe this was introduced in #15232 .
However probably not directly caused by said commit (the commit seems to restrict needed functionality), but an unexpected edge case where a local item is believed to be non-local.

Reproduction

I have tried to create a minimal reproduction, however did not succeed in creating a minimal repro (I even tried copying dependency structure and workspace code settings).

So I am forced to instead link to one of my real [WIP] projects.

Reproduction Steps:

• Clone https://github.com/Reloaded-Project/Reloaded.Hooks-rs.git
• Checkout main
• Open main folder in VSCode (folder with README.MD)
  • Note: Projects are included via .vscode/settings.json -> rust-analyzer.linkedProjects.
• Open any file in reloaded-hooks-portable such as projects/reloaded-hooks-portable/src/api/wrapper_instruction_generator.rs.

It is not possible to rename any type or even local variable due to:

[Error - 03:04:27] Request textDocument/rename failed.
  Message: Cannot rename a non-local definition.
  Code: -32602

[alibektas]

@rustbot claim

[alibektas]

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

rust-analyzer/crates/base-db/src/input.rs

Lines 143 to 152 in f19479a

	pub enum CrateOrigin {
	/// Crates that are from the rustc workspace.
	Rustc { name: String },
	/// Crates that are workspace members.
	Local { repo: Option<String>, name: Option<String> },
	/// Crates that are non member libraries.
	Library { repo: Option<String>, name: String },
	/// Crates that are provided by the language, like std, core, proc-macro, ...
	Lang(LangCrateOrigin),
	}

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.

[Veykril]

Ah, that would explain that indeed. I think your merged crate graph in your examples is wrong though no? I would expect us to actually have two Bar crates in the graph, as origin is part of the crate data and it differing means we don't deduplicate the crate. This kind of duplication trips up a lot of things in general, I'm not sure what the best approach to take here is though.

[alibektas]

It is indeed wrong but the whole point was to show that Bar and BarAsLib are actually one and the same crate ( for the user ). I think we should revert #15232 for now until we have a solution to this.

[Veykril]

I think we should instead take a look at CrateOrigin and check if the current split of things makes sense. That is, cargo assigns is_local and is_member as properties to crates in a workspace, we use CrateOrigin::Local to mean member really. Now I don't remember what is_local implies for cargo but it might make sense to change stuff here depending on what it means.

Also, the duplicated crates only differ in the CrateOrigin, so it might make sense to consider the origin when deduplicating and when only the origin differs, try to deduplicate by merging the origin into the more sensible option. So in this case, keeping the local origin copy, and discarding the library one etc. But again, that depends on what the Local crate origin is being used for right now by other features. (A clear case of documentation on CrateOrigin and its variants is missing/not descriptive enough)

[Zalathar]

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

[Zalathar]

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

[alibektas]

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.

[Veykril]

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.

[ogapo]

"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."

I've just recently run into this issue (using v0.3.1713) when using multi-root Workspaces and with submodules declared between them via path. I'm not entirely certain what is causing it to trigger (I wasn't able to make a minimum repro project yet) but it would be really nice if there was an escape-hatch setting to simply disable this check and allow renaming "non-local" defs if opted in for complex projects?

[alibektas]

👋 We use configurations to customize behavior, in your case your request is more of a workaround kind. Although practically this could work, we'd rather see a solution that resolves this issue once and for all. We still couldn't merge the PR upstream so until then ( merged version will probably be available in nightly between 13.11-17.11) you could do two things

1. Go back to the commit where the breaking changes were first introduced ( see here

Or

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

Lines 78 to 82 in c1c9e10

	if let Some(krate) = self.krate(sema.db) {
	if !krate.origin(sema.db).is_local() {
	bail!("Cannot rename a non-local definition.")
	}
	}

delete lines 79-82 and build the project using the command cargo xtask install.

Sorry if you couldn't find the answer you were looking for.

EDIT : If it is actually the case that there are some cases where we cannot really do anything to prevent from producing false positives, then something like the proposed configuration may be a consideration. So there is that.

[ogapo]

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

[alibektas]

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. )

[TCROC]

I have also recently started experiencing this issue in one of the recent updates.

rust-analyzer version: 0.3.1730-standalone
rustc 1.73.0 (cc66ad468 2023-10-03)

[Zalathar]

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

[Zalathar]

Unfortunately I may have celebrated too soon.

Renaming does work in rustc_mir_transform, but it still doesn't work in rustc_mir_build.

Perhaps that crate triggers one of the more complicated scenarios not fixed by #15754.

[TCROC]

It is also not working in my crates

[Veykril]

That things break down in the rustc workspace is not too surprising to me, the workspace is cursed and r-a barely work in it in the first place.

@TCROC is your project public? Or can you figure out what project structure it is that still breaks here?

[Veykril]

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

Ugh, ye that is annoying... We can't really deduplicate these crates, since the out dirs should point into the respective target dirs here. Deduplicating might just work for one crate, but another might break because it now no longer finds its corresponding output.

[Veykril]

I think we might just wanna put the accidental library rename prevention behind a config and tell the user in the error message that they can disable it by setting "" to false. As I don't see a way to fix this concrete case here (though I am open for ideas), and I know that a bunch of people are annoyed by this issue. (Though at least we managed to push deduplication further thanks to this so thats a net win anyways)

[flodiebold]

I don't really understand why this is related to duplication at all. Why isn't our logic just "if it's inside the LSP root directory, we can rename it"?

[Veykril]

The problem is that the LSP Root directory can be completely unrelated to the workspaces, you can open a workspace in /foo/bar and /foo/baz but have the LSP root be in /qux/other. An example with vscode being you open a vscode workspace, then you proceed to add random folders (cargo workspaces) as workspace members that are unrelated to the opened workspace.

[Veykril]

One thing we should change still though is to treat path included dependencies as renamable (as was stated somewhere earlier), it doesn't really make sense to treat them as foreign.

[flodiebold]

An example with vscode being you open a vscode workspace, then you proceed to add random folders (cargo workspaces) as workspace members that are unrelated to the opened workspace.

What would be the problem with that though? I mean, we should allow renaming in all workspace folders of course. But to me it still seems like the VSCode workspace == the set of folders where the user wants to make changes.

[Veykril]

I'm saying that the the opened cargo workspaces may not lie inside the lsp workspace root, it technically is just an arbitrary path. But you raise a good point, maybe we need to change what we consider a local crate, that is currently the "CrateOrigin" is determined on a per cargo workspace basis, though really it should probably be set on a global LSP workspace basis? (That is consider everything local that is inside any cargo workspace member?) I think that is effectively what you are saying.

[flodiebold]

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.

[TCROC]

One thing we should change still though is to treat path included dependencies as renameable (as was stated somewhere earlier), it doesn't really make sense to treat them as foreign.

Do you mean like "rust-analyzer.linkedProjects" that are explicitly configured in the settings.json? If so, I agree. At the very least, those should be renameable since the developer has explicitly told ra these are the projects I am actively working on. I have control over them.

At least that's how I interpreted that setting.

[Veykril]

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

[vaniusrb]

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?

[TCROC]

I agree with @vaniusrb. This has also been quite detrimental to our workflow. We find most of our refactoring now consists of "ctrl + f" and "replace" which is less than ideal and prone to errors.

[alibektas]

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!

[TCROC]

Thank you @alibektas! :) No sorries necessary! :)

[alibektas]

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.
--

[alibektas]

ping @TCROC @vaniusrb

[TCROC]

@alibektas will I be able to test it out in the vs code pre-release version?

Or what is the best way to test it?

[alibektas]

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.

[TCROC]

@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! :)

[vaniusrb]

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