Skip to content

Some refactorings around metadata encoding - #162900

Open
aerooneqq wants to merge 3 commits into
rust-lang:mainfrom
aerooneqq:record-tables-through-macros
Open

aerooneqq wants to merge 3 commits into
rust-lang:mainfrom
aerooneqq:record-tables-through-macros

Conversation

@aerooneqq

@aerooneqq aerooneqq commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Two refactorings that were extracted from #162809:

  • Use macros to record values in all tables, this unifies writing to tables and makes it much easier to modify or refactor writing process as it was needed in Remap def indices for deterministic metadata encoding #162809,
  • Moves proc_macro_quoted_spans table into ProcMacroData as it is filled only when ProcMacroData is Some (meaning crate type is proc-macro), so it is reasonable to place it here, moreover all tables left in LazyTables have DefIndex as key, this simplifies refactorings and modifications that are connected with DefIndexes.

r? @petrochenkov

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 17, 2026
Comment thread compiler/rustc_metadata/src/rmeta/encoder.rs Outdated
@petrochenkov

Copy link
Copy Markdown
Contributor

r? @Zalathar , for example, I've seen you refactoring the metadata infra recently

@rustbot rustbot assigned Zalathar and unassigned petrochenkov Sep 17, 2026
@rustbot

rustbot commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

Zalathar is not on the review rotation at the moment.
They may take a while to respond.

@Zalathar

Copy link
Copy Markdown
Member

Could you explain more about proc_macro_quoted_spans and why it's being moved?

From the PR description and commit message, I wasn't able to get a good sense of why this change is desirable, or whether it's even correct.

@aerooneqq

aerooneqq commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Could you explain more about proc_macro_quoted_spans and why it's being moved?

It was the only table that has usize as key, so we can't use macros to record values in it, and values in this table are set only under is_proc_macro condition in encode_proc_macros, so if ProcMacroData is None (meaning is_proc_macro is false) then all queries to this table will panic, so we can move this table into ProcMacroData, I thought that it is more appropriate place for this table.

@Zalathar

Copy link
Copy Markdown
Member

So if I understand correctly:

  • proc_macro_quoted_spans is only used in proc-macro crates, and ProcMacroData is specifically for holding data that is only used in proc-macro crates. So moving it to ProcMacroData is a reasonable change, regardless of why we want to move it out of LazyTables.
  • The reason we want to move it out of LazyTables is that it's the only one of those tables with a key type that isn't DefIndex. So having proc_macro_quoted_spans in LazyTables is getting in the way of changes to how DefIndex is treated.
  • The fact that proc_macro_quoted_spans happens to have a key type of usize is irrelevant, and prominently mentioning usize is misleading to future maintainers.

If so, could you please update the commit message and PR description to clearly explain the reasoning to future maintainers, and remove the misleading stuff about usize?

@aerooneqq
aerooneqq force-pushed the record-tables-through-macros branch from cc26ffb to 020ffe1 Compare September 18, 2026 09:04
@aerooneqq

aerooneqq commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

If so, could you please update the commit message and PR description to clearly explain the reasoning to future maintainers, and remove the misleading stuff about usize?

Updated commit message and PR description.

@Zalathar

Copy link
Copy Markdown
Member

Thanks.

I will also say that normally I would be against adding the macros, but I guess it makes sense if they’re specifically for enabling a tricky change to how def-indexes work. I do hope that we can remove the macros after those changes, though.

…ome` and `record_defaulted` -> `record_value`
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants