internal: Rewrite attribute handling #20316

ChayimFriedman2 · 2025-07-27T17:25:37Z

Basically, we switch to expanding cfg_attr in AST form, filter irrelevant attributes from the item tree, and move hir-def attributes (non-item-tree) to be flag-based.

The main motivation is memory usage, although this also simplifies the code, and fixes some bugs around handling of cfg_attrs.

This saves 120mb on analysis-stats . 🎉

Fixes #10110 and maybe some other issues too.
Fixes #8434.

Veykril

You're killing me with these big PRs 😬 ..

I have some thought to note down on the EditionedFileId stuff but that will have to wait for later

crates/syntax/src/ast/traits.rs

Veykril · 2025-07-28T06:57:10Z

crates/hir-expand/src/mod_path.rs

        ModPath { kind, segments: SmallVec::new_const() }
    }

+    pub fn from_tokens(


Ugh now we have 3 ways to parse these ...

And all subtly different... Yeah I agree.

crates/hir-expand/src/lib.rs

crates/hir-def/src/item_tree/attrs.rs

Veykril · 2025-07-29T11:05:13Z

crates/hir-def/src/lib.rs

+// FIXME: We probably should use this in more places.
+/// This is used to avoid interning the whole `AttrDefId`, so we intern just modules and not everything.
+#[salsa_macros::interned(debug, no_lifetime)]
+pub struct InternedModuleId {
+    pub loc: ModuleId,
+}


Sounds like #20001

Yeah, except using interneds and not tracked structs, and being localized to attrs. At least currently, analysis doesn't create this type, only IDE needs to resolve attributes on modules, so the memory impact is low.

#20001 is that right path forward though.

Veykril · 2025-07-29T11:08:20Z

crates/hir-expand/src/attrs.rs

+    /// This can be empty if the path is not of 1 or 2 segments exactly.
+    pub segments: ArrayVec<SyntaxToken, 2>,
+    pub range: TextRange,
+    pub is_test: bool,


This doesn't really make sense, we cannot know whether this is the test attribute at this layer, we have no name resolution here after all?

I have a FIXME for this in hir-def/src/attrs.rs. The general point is that yes, we need proper name resolution for these, but it's hard to do. The reason I put this in Meta is because macros often expand to the full name ::core::prelude::vX::test, which is longer than 2 segments. I could make segments 4-length, but that may somewhat hurt perf. I don't feel strongly about that though.

I added a FIXME here too.

crates/hir-expand/src/attrs.rs

crates/ide-diagnostics/src/handlers/invalid_derive_target.rs

crates/ide/src/signature_help.rs

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

ChayimFriedman2 · 2025-07-29T12:00:07Z

You're killing me with these big PRs 😬 ..

Sorry! This one could probably be broken (at least to a rewrite of ast-based expand_cfg_attr then to flags-based attrs), but I didn't thought about that until it was too late...

Veykril · 2025-08-03T06:41:05Z

Should we wait with this until the trait solver lands? It looks like it will cause a lot of conflicts

ChayimFriedman2 · 2025-08-03T15:22:47Z

The changes in hir-ty here are minimal, but I can wait until the switch to the next trait solver lands.

ChayimFriedman2 · 2025-08-13T17:24:26Z

Rebased. This should be unblocked now.

ChayimFriedman2 · 2025-10-21T18:09:30Z

Rebased. @Veykril I think this should be ready for review now.

Veykril · 2025-10-21T19:41:14Z

Will take a look tomorrow 👍

Veykril · 2025-10-22T08:08:15Z

crates/base-db/src/editioned_file_id.rs

I see I never got back to my follow up on this part. I really do not like this, for one it feels wrong to bundle crates into this. And secondly, the crate being ignored for comparison and such gives me the fear that we might rely on the crate somewhere where we suddenly get a different editioned file id that compares the same but has the a different crate. Is there no way for us to go ab out things differently where we need this?

I dislike that too. In fact I doubted it is the best idea, and I am still not sure it is, and if there is an alternative I will eagerly adopt it. I did it anyway, because this simplifies the code a lot.

Basically, the (almost) only place we use this is to retrieve the cfg when lowering the item tree, to eager-expand #[cfg] and #[cfg_attr]. The alternative is that the item tree will not be crate-aware, and we will instead lazily expand cfgs on the def map. The problems with this approach are:

It requires us to maintain two codes for cfg expansion, both at AST-level and at tt level (item tree attrs)

Worse, they are extremely tightly coupled. Basically, they are required to match the behavior of each other precisely, including when they error. This is because of AttrId - we use the attribute index to refer to the attribute, and this is post cfg-expansion (it must be, since cfg_attr can expand to multiple attrs). That means that any mismatch between the two parsing codes will create very hard to detect bugs.

Also note that the crate is not ignored for comparisons. This is important. We ignore the crate in two places:

In hash checking, because we support ignoring the crate in comparisons, as per the next bullet. This shouldn't impact interning perf since usually each file belongs to only one crate.

In some handlers (those that use from_span_guess_origin()), at the following conditions:

The hasher is latency-sensitive (e.g. on enter), so it cannot afford checking the crate from the def map.

The hasher operates on AST only (this correlates with the previous point), so the crate doesn't matter

Veykril · 2025-10-22T08:14:53Z

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

-        let mut process = true;
-
        // Process other crate-level attributes.
        for attr in &*attrs {
-            if let Some(cfg) = attr.cfg()
-                && self.cfg_options.check(&cfg) == Some(false)
-            {
-                process = false;
-                break;
-            }


This still needs to be reverted #20316 (comment)

No? When hitting CfgDisabled we still process the attrs.

we process too many, once we hit the cfg we need to stop. That is

#![attr1] #![cfg(false)] #![attr2]

in this setup, rust only processes attr1, not attr2

The attributes in the CfgDisabled is only up to the cfg, it's even documented in the variant. This is because collect_item_tree_attrs() early-breaks on #[cfg].

crates/syntax/src/ast/traits.rs

Basically, we switch to expanding cfg_attr in AST form, filter irrelevant attributes from the item tree, and move hir-def attributes (non-item-tree) to be flag-based. The main motivation is memory usage, although this also simplifies the code, and fixes some bugs around handling of `cfg_attr`s.

ChayimFriedman2 · 2025-10-22T12:09:42Z

@Veykril do you want to merge this, or do you still want to review this?

Veykril · 2025-10-22T12:34:47Z

good to go, I don;t have a better idea for the editioned file id stuff and I don't want to block it on that either.

Veykril · 2025-10-22T13:58:03Z

This horribly broke something in the type system https://rust-analyzer.github.io/metrics/?start=2025-10-15&end=

ChayimFriedman2 · 2025-10-22T14:06:44Z

Both speed and type mismatches regression. Type mismatches is probably some thing I forgot - I'll try to investigate, speed is weirder as this didn't have any measurable effect on speed when I benchmarked, although that was before the trait solver migration. Will check that too.

ChayimFriedman2 · 2025-10-22T15:11:04Z

Okay, I identified the problem for both: for some reason, attribute macros are run twice. This is likely related to the def map changes - I'm reviewing them carefully now.

ChayimFriedman2 · 2025-10-23T08:28:04Z

Okay, I found the problem. The old behavior was subtly wrong, the new behavior is also wrong. I need to look at rustc now to see what it does.

ShoyuVanilla · 2025-10-23T08:35:53Z

FWIW rustc's attribute handling has been rewritten recently. cc rust-lang/rust#146389

lnicola · 2025-10-24T11:09:05Z

changelog skip

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 27, 2025

ChayimFriedman2 force-pushed the better-attrs branch 2 times, most recently from e69ea8b to b2ecf39 Compare July 28, 2025 19:22

Veykril reviewed Jul 29, 2025

View reviewed changes

ChayimFriedman2 force-pushed the better-attrs branch from b2ecf39 to fccfb8c Compare July 29, 2025 12:25

ChayimFriedman2 mentioned this pull request Jul 29, 2025

Rust analyzer uses so much RAM whenever I open vscode that it crashes my pc everytime #20339

Closed

ChayimFriedman2 force-pushed the better-attrs branch from fccfb8c to c42bd8d Compare August 2, 2025 18:30

ChayimFriedman2 force-pushed the better-attrs branch from c42bd8d to 541d210 Compare August 13, 2025 17:24

ChayimFriedman2 force-pushed the better-attrs branch from 541d210 to db870ed Compare August 13, 2025 17:25

ChayimFriedman2 mentioned this pull request Aug 21, 2025

Wrong error when derive(CoercePointee) is used with cfg_attr'ed pointee and 2+ generic argument type #20500

Open

ChayimFriedman2 force-pushed the better-attrs branch from db870ed to 3a58f2d Compare October 21, 2025 18:08

Veykril reviewed Oct 22, 2025

View reviewed changes

ChayimFriedman2 force-pushed the better-attrs branch from 3a58f2d to 455ca02 Compare October 22, 2025 08:47

Veykril approved these changes Oct 22, 2025

View reviewed changes

ChayimFriedman2 added this pull request to the merge queue Oct 22, 2025

Merged via the queue into rust-lang:master with commit 08c0dc6 Oct 22, 2025
15 checks passed

ChayimFriedman2 deleted the better-attrs branch October 22, 2025 12:56

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 22, 2025

ChayimFriedman2 mentioned this pull request Oct 22, 2025

Revert "internal: Rewrite attribute handling" #20887

Merged

internal: Rewrite attribute handling #20316

internal: Rewrite attribute handling #20316

Uh oh!

Conversation

ChayimFriedman2 commented Jul 27, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Veykril left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChayimFriedman2 commented Jul 29, 2025

Uh oh!

Veykril commented Aug 3, 2025

Uh oh!

ChayimFriedman2 commented Aug 3, 2025

Uh oh!

ChayimFriedman2 commented Aug 13, 2025

Uh oh!

ChayimFriedman2 commented Oct 21, 2025

Uh oh!

Veykril commented Oct 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ChayimFriedman2 commented Oct 22, 2025

Uh oh!

Veykril commented Oct 22, 2025

Uh oh!

Uh oh!

Veykril commented Oct 22, 2025

Uh oh!

ChayimFriedman2 commented Oct 22, 2025

Uh oh!

ChayimFriedman2 commented Oct 22, 2025

Uh oh!

ChayimFriedman2 commented Oct 23, 2025

Uh oh!

ShoyuVanilla commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lnicola commented Oct 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

ChayimFriedman2 commented Jul 27, 2025 •

edited by rustbot

Loading

ShoyuVanilla commented Oct 23, 2025 •

edited

Loading