-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix: don't duplicate lang items with overridden sysroot crates #20475
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: don't duplicate lang items with overridden sysroot crates #20475
Conversation
|
I think it will be better, if we're accessing the def map anyway, to iterate over ...although this will be weird given in other places we iterate over the Also I suppose conducting a test for this will be difficult? |
93168cb to
9dc287d
Compare
Sounds fair. I'll try with that
I'm not sure if I could manage to but I'll try that |
|
Actually, I don't see how this will solve the problem - if the solver is expecting a single lang item across the crate graph, won't we still produce twice, one for core and one for std, and the order will depend on which crate come first in the dependencies? What is the conflicting lang item? |
|
Re-exported things would end up into the same item, I guess? At least that's what happened when I tried analysis-stats on local. Basically, we cannot have duplicated lang-items in Rust, otherwise blocked by |
|
Reepxorts definitely aren't considered for lang_items, we consider only declarations. |
|
Then how would we produce duplicates? |
|
If there are actual duplicates? I don't know. What is the duplicated lang item in std? |
|
It is duplicated by the following manner. If we try to analyze something like Edit) More precisely, the duplicates here are in |
|
...Oh. That does explain it. But please add a comment linking to this issue, that's pretty subtle! |
|
Okay, I'll add some explanations and links once I'm finished |
9dc287d to
f4eabfb
Compare
| crate_local_def_map: Option<&'db LocalDefMap>, | ||
| // The dependencies of the current crate, including optional deps like `test`. | ||
| deps: FxHashMap<Name, BuiltDependency>, | ||
| deps: FxIndexMap<Name, BuiltDependency>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that the test and analysis-stats randomly succeeds before this fix due to non-determinism here 🤣
| } | ||
| //- /lib.rs crate:r#std deps:core | ||
| #[no_std] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#![no_std]Forgot the !.
| CrateOrigin::Lang(LangCrateOrigin::Std) => crate_data.no_std, | ||
| CrateOrigin::Lang(LangCrateOrigin::Std) => { | ||
| crate_data.no_core || crate_data.no_std | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no_core implies no_std
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our project-model inserts sysroot deps before the other deps. The changes on this file is to match with that behavior to test the issue.
| CrateOrigin::Library { repo, name } | ||
| } else { | ||
| CrateOrigin::Local { repo, name: Some(name) } | ||
| let origin = if force_non_lang_origin == ForceNoneLangOrigin::Yes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, our test suite considers crates with names std, core, alloc, ... as CrateOrigin::Lang. I made a bit ugly escape hatch to evade this like r#std
f4eabfb to
9c1918a
Compare
9c1918a to
15ac6a2
Compare
ChayimFriedman2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! That must've been a hassle to debug 😨
|
Testing a panic I had found running analysis-stats on the helix repo, and this seems to have fixed that as well. For extra context, this was the panic: |
I guess that improvement is from #20454 ? |
|
Oh, it's not a noise but a valuable test report on a sizable real world project. |
|
I haven't bisected yet but I guess the panics on helix was fixed with #20453 ? |
|
Ah, yep, that was the one. I thought it was something specific to the |
|
No worries! It still worth a lot to check how rust-analyzer runs on large projects. |


The panic here: #20443 (comment) originates from the "multiple instances" of lang items.
We can verify this running on
analysis-statsto the following code:So, depending on the "starting crate" of lang items query, it sometimes resolved as
crate::Pointee::Metadatabutcore::ptr::metadata::Pointee::Metadataotherwise, and if we try to compare those items, 💥This thing is happening in when we are running
analysis-statsonstd.Though
stdhas#![no_std]andcorehas#![no_core], we always insert sysroots dependencies no matter they are and they are dropped/overwritten on nameless, especially in the following linesdropping sysroots on
no_stdorno_core:rust-analyzer/crates/hir-def/src/nameres/collector.rs
Lines 316 to 346 in e10fa93
overwriting sysroots when we have
dependencies.std.path = ".."rust-analyzer/crates/hir-def/src/nameres/collector.rs
Lines 73 to 77 in e10fa93