internal: remove input queries from `#[query_group]` by ada4a · Pull Request #22639 · rust-lang/rust-analyzer

ada4a · 2026-06-25T13:31:42Z

First two commits move the last remaining input queries
The third commit removes the input query support from #[query_group]

cc @Veykril

ada4a · 2026-06-25T13:32:14Z

Left some stylistic questions in the code, mainly for future such PRs

ada4a · 2026-06-25T13:43:28Z

Let me draft this while I investigate the test failures...

ada4a · 2026-06-25T15:29:31Z

in hir_def's macro_expansion_tests::mbe tests, there are several ones which have +syntaxctxt included, and they fail because the numeric SyntaxContext ids (?) have changed. Though the ids all seem to change consistently (17408->16384)
in hir_def's hir-def expr_store::tests::body::block::nested_module_scoping, the salsa id, crate id, and the block id of a module have changed
in ide_db's symbol_index::tests, the module id, EditionedFileIds, and struct ids have changed -- again, consistently (everything fell by 0x400)

While I have no idea why those ids would change as a result of this PR, the fact that the changes are all consistent seems like a good sign?

Veykril · 2026-06-25T16:46:27Z

Its because this change changes the salsa ingredient order which thus changes the page allocator and IDs of the allocation

ada4a · 2026-06-25T16:48:30Z

So.. it's fine? Should I just update the tests?

Veykril · 2026-06-25T16:52:16Z

Oh yea its fine and expected, meant to add that, so just update the tests

ChayimFriedman2 · 2026-06-26T07:08:09Z

+        // TODO: should I create `AnalysisHost::{update_,}expand_proc_attr_macros` to clean this up?
+        // TODO: we don't use `raw_db` mutably until we go into the `if` -- is it a problem that I
+        // still get it mutably in an eager manner? I did this to avoid needing two separate calls, to
+        // `raw_database` and `raw_database_mut`. Or maybe I should just inline `raw_db`?


the rust-analyzer crate does depend on analysis crates, but we should avoid adding new such dependencies where possible. But I also don't think exposing the struct wholesale is a good idea; instead, export a fn set_expand_proc_attr_macros(value: bool).

View changes since the review

Added a setter and a getter, but called the former set_expand_proc_attr_macros_with_durability, to match base_db::set_all_crates_with_durability. The change is in a separate commit to simplify review, but I'd like to squash the commit before merging

I think we should have only a setter, with no durability (that is, it sets the durability itself), that checks for equality with the previous value like here. hir is a high-level interface.

Agree with the proposal to change the setter, but I don't think we can quite get rid of the getter, because it has one legitimate user:

rust-analyzer/crates/hir-def/src/nameres/collector.rs

Line 619 in c0dfbc4

&& !crate::db::get_expand_proc_attr_macros(self.db) =>

That's in hir-def, it can access the input struct directly.

Veykril · 2026-06-25T13:34:32Z

+        // TODO: `ide`'s Cargo.toml says that it should avoid depending on `hir-*` crates directly,
+        // and go through `hir` re-exports instead. `rust-analyzer` OTOH does seems to depend on `hir-*`
+        // after all. So, should I import `ExpandProcAttrMacros` through `hir` or `hir_def`?


Generally speaking we should at the very minimum have a re-export from hir instead then. Though even there the question then arises whether we shouldn't have a more "stable" interface for the given thing. We have not been very dilligent with keeping the separation maintained at the moment.

View changes since the review

Veykril · 2026-06-26T08:33:48Z

Also coming back to the IDs in tests changing, ideally we wouldn't print those in tests at all / would "normalize" them such that they are stable wrt to the test, not to the salsa databass state, but no one has gotten around to changing that (also unsure how easy that is)

ada4a · 2026-06-26T09:22:12Z

Should I maybe combine this PR and #22641, and rename this to something like "internal: remove input queries from query_group", to match #22328? Since that's what these two PRs are really intending to do

Veykril · 2026-06-26T11:21:14Z

yea re-using one of these for both changes seems fine to me

This required several non-trivial changes: - Rename the existing `ProcMacros::get` to `ProcMacros::get_for_crate` (feel free to suggest a better name), to avoid clashing with the new getter created by salsa. - Change `ProcMacroBuilder::build` to take `db` and `durability`, as it will need to build `ProcMacros` inside the `db`. Also rename it to `build_in`, to reflect the change in functionality. - `ProcMacros` is now stored in the database directly, instead of being wrapped in an Arc. This should be fine though, since its `by_crate` field is marked with `#[returns(ref)]`.

ada4a

I went ahead and removed the support for #[salsa::input] after all, because it's nice to have the query-group-macro tests reflect the most current way of writing query groups (and just to test my own understanding).

View changes since this review

ada4a · 2026-06-26T19:00:47Z

-            "salsa_event(WillExecute { database_key: create_data_HelloWorldDatabase(Id(0)) })",
-            "salsa_event(WillCheckCancellation)",
-            "salsa_event(DidValidateMemoizedValue { database_key: create_data_HelloWorldDatabase(Id(0)) })",
-            "salsa_event(WillCheckCancellation)",
-            "salsa_event(WillExecute { database_key: invoke_length_query_shim(Id(800)) })",
+            "salsa_event(WillExecute { database_key: create_data_HelloWorldDatabase(Id(400)) })",
            "salsa_event(WillCheckCancellation)",
+            "salsa_event(WillExecute { database_key: invoke_length_query_shim(Id(c00)) })",


are these changes to be expected?..

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 25, 2026

ada4a force-pushed the unquerygroup-expand_proc_attr_macros branch from 7fe164e to 3656520 Compare June 25, 2026 13:32

ada4a marked this pull request as draft June 25, 2026 13:43

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 25, 2026

ada4a changed the title ~~move DefDatabased::expand_proc_attr_macros out of the query group~~ internal: move DefDatabased::expand_proc_attr_macros out of the query group Jun 25, 2026

ada4a marked this pull request as ready for review June 25, 2026 16:39

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 25, 2026

ada4a force-pushed the unquerygroup-expand_proc_attr_macros branch from 3656520 to 5f7510d Compare June 25, 2026 16:53

ada4a mentioned this pull request Jun 25, 2026

internal: move ExpandDatabase::proc_macros out of the query group #22641

Closed

ChayimFriedman2 requested changes Jun 26, 2026

View reviewed changes

Veykril reviewed Jun 26, 2026

View reviewed changes

ada4a changed the title ~~internal: move DefDatabased::expand_proc_attr_macros out of the query group~~ internal: move DefDatabase::expand_proc_attr_macros out of the query group Jun 26, 2026

ada4a changed the title ~~internal: move DefDatabase::expand_proc_attr_macros out of the query group~~ internal: remove input queries from #[query_group] Jun 26, 2026

ada4a added 3 commits June 26, 2026 20:37

move ExpandDatabase::proc_macros out of the query group

c6a5073

remove #[salsa::input] support in #[query_group]

5834f7a

ada4a force-pushed the unquerygroup-expand_proc_attr_macros branch from c0dfbc4 to 5834f7a Compare June 26, 2026 18:41

ada4a commented Jun 26, 2026

View reviewed changes

Uh oh!

Uh oh!

Conversation

ada4a commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ada4a commented Jun 25, 2026

Uh oh!

ada4a commented Jun 25, 2026

Uh oh!

ada4a commented Jun 25, 2026

Uh oh!

Veykril commented Jun 25, 2026

Uh oh!

ada4a commented Jun 25, 2026

Uh oh!

Veykril commented Jun 25, 2026

Uh oh!

ChayimFriedman2 Jun 26, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ada4a Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

ChayimFriedman2 Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

ada4a Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

ChayimFriedman2 Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

Veykril Jun 25, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Veykril commented Jun 26, 2026

Uh oh!

ada4a commented Jun 26, 2026

Uh oh!

Veykril commented Jun 26, 2026

Uh oh!

ada4a left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ada4a Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ada4a commented Jun 25, 2026 •

edited

Loading

ChayimFriedman2 Jun 26, 2026 •

edited by rustbot

Loading

Veykril Jun 25, 2026 •

edited by rustbot

Loading

ada4a left a comment •

edited by rustbot

Loading