Skip to content

internal: Make HirFileId, EditionedFileId and macro files Salsa struct #19617

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

Merged
merged 1 commit into from
Apr 19, 2025

Conversation

ChayimFriedman2
Copy link
Contributor

@ChayimFriedman2 ChayimFriedman2 commented Apr 18, 2025

And make more queries non-interned.

Also flip the default for queries, now the default is to not intern and to intern a query you need to say invoke_interned.

This saves 161mb on analysis-stats ..

This is a big PR, but most of it is tedious replacements: the interesting changes are in span, base-db and hir-expand/src/lib.rs. Oh, and query-group-macro. The other files could use a skim review.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 18, 2025
@ChayimFriedman2 ChayimFriedman2 force-pushed the more-actual branch 4 times, most recently from c75f6d8 to 018402c Compare April 18, 2025 10:14
Comment on lines +1073 to +1078
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, salsa::Supertype)]
pub enum HirFileId {
FileId(EditionedFileId),
MacroFile(MacroCallId),
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that it would be nicei f we could shrink this to 4 bytes again (as was done with the bit packing before). Iirc back when I implemented that it saved ~10mb. Not relevant to this PR for now though.

Copy link
Contributor Author

@ChayimFriedman2 ChayimFriedman2 Apr 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do this if we store it in places as salsa::Id (or some newtype wrapper). Methods can still take HirFileId.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I believe Niko's initial supertype proposal wouldvebeen something like that, where instead of it being a derive it was an attribute that collapsed the variants into an erased wrapper. I wonder if we could implement that as a separate thing to the derive as well. I do like the derive in most cases, given it doesn't require you to extract the active variant manually.

Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit of a shame to see the distinction between macro call and macro file disappear but I imagine its annoying to get that to work with salsa nicely

And make more queries non-interned.

Also flip the default for queries, now the default is to not intern and to intern a query you need to say `invoke_interned`.
@ChayimFriedman2 ChayimFriedman2 added this pull request to the merge queue Apr 19, 2025
Merged via the queue into rust-lang:master with commit 150bb4a Apr 19, 2025
14 checks passed
@ChayimFriedman2 ChayimFriedman2 deleted the more-actual branch April 19, 2025 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants