internal: Change StructId to be a tracked struct in salsa - #23170
Open
Wilfred wants to merge 1 commit into
Open
Conversation
rustbot
added
the
S-waiting-on-review
ChayimFriedman2
requested changes
Aug 17, 2026
Contributor
There was a problem hiding this comment.
Of course we cannot assume that. All analysis functions assume the IDs they get, of all kinds, are from the current revision. If something challenges that, we should fix that and not those methods (it'll also break when we add lifetimes to Salsa IDs).
Contributor
Author
|
Thanks for the review. My mental model of salsa is generally just 'fancy memoisation'. I think the right thing here is to use lifetimes properly in the relevant salsa-tracked structs, let me update the PR. |
Use a lifetime on StructId, defining a StructIdLt type following the
BlockLocLt pattern.
Ensure StructIdLt is a tracked type in salsa. This fixes a rare crash
where a user renames a struct and rust-analyzer panics:
Can't find ast id ErasedFileAstId { .. }
This seems to be because we could observe old interned StructId
values from previous salsa revisions.
We're moving to using lifetimes in these structs anyway, so this is
both a code cleanup and should be a fix for a crash.
AI disclosure: Code partly written by GPT-5.6 Sol, commit messages,
comments and code review entirely by me.
Wilfred
force-pushed
the
fix/stale-ast-id-after-rename
branch
from
August 21, 2026 17:14
d10b56c to
0090042
Compare
Wilfred
changed the title
Contributor
Author
|
OK, I've updated this to use salsa tracking, which I think is the right fix. I've copied the pattern in #22534, and this should be another step towards #22868. I've really struggled to get a satisfying Rust-code-level test for this, so I've just dropped unit test I had before. I do think the code is correct and fixes a real panic though. Let me know what you think. |
Contributor
|
Using lifetimes on its own cannot fix that, it'll just makes code that passes IDs between revisions not compile. We should do that anyway, but if you saw a real panic from this (and not just your test that was incorrect), this will not fix it. |
Contributor
Author
|
I am indeed seeing sporadic panics from this, they take the form: I wouldn't want to waste your time with theoretical bugs, I've kept all my PRs strictly to panics that I'm seeing in practice. |
Contributor
|
I didn't say you don't, what I said is that if you see real panics (like you confirmed) then this change won't fix them. |
Contributor
Author
|
Fair enough, just wanted to be explicit that I'm grateful for your regular and thorough reviews :) |
Contributor
|
IOW: you need to find the piece of code responsible for reusing IDs between revisions (which is a bug), and fix it. Using lifetimes is something we should do but it's tracked separately, while it could ease discovery of the problematic code (by making it not compile) it won't solve that by itself (and I don't know how much lifetimes we'll need to add). |
Contributor
|
An alternative (and perhaps more plausible) explanation is speculative completion: it does some sketchy things that could theoretically maybe cause things like that (we know it can mess the |
Contributor
|
Or, if you have a backtrace, maybe I can have a better idea. |
Contributor
Author
|
FWIW I'm seeing the panic on LSP requests textDocument/diagnostic, textDocument/codeAction and textDocument/semanticTokens/full, but I haven't seen any on completion requests. I'll dig up a stack trace too. |
Contributor
|
If you can have a stack trace with debuginfo, even better (we don't ship out binaries with debuginfo but maybe Meta does, that's what I'd do in your place - this helps debugging and the only reason we don't do that is that it increases binary size and thus download time, which is probably not a problem in an internal network). |
Contributor
Author
|
Argh, I've lost line numbers, but I've got function names. This is from rust-analyzer at commit 137005e: |
Contributor
|
Ouch it's cut - do you have the full backtrace? The start is especially important because it contains the IDE code invoking things. |
Contributor
Author
|
Argh, I think I need to fix my error collector, I don't seem to have more than that right now. I did see a salsa query backtrace if that's at all helpful: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Use a lifetime on StructId, defining a StructIdLt type following the
BlockLocLt pattern.
Ensure StructIdLt is a tracked type in salsa. This fixes a rare crash
where a user renames a struct and rust-analyzer panics:
This seems to be because we could observe old interned StructId
values from previous salsa revisions.
We're moving to using lifetimes in these structs anyway, so this is
both a code cleanup and should be a fix for a crash.
AI disclosure: Code partly written by GPT-5.6 Sol, commit messages,
comments and code review entirely by me.