internal: Rewrite attribute handling #20316
Conversation
rustbot
added
the
S-waiting-on-review
ChayimFriedman2
force-pushed
the
better-attrs
branch
2 times, most recently
from
e69ea8b to
b2ecf39
Compare
| @@ -64,6 +65,58 @@ impl ModPath { | |||
| ModPath { kind, segments: SmallVec::new_const() } | |||
| } | |||
|
|
|||
| pub fn from_tokens( | |||
There was a problem hiding this comment.
Ugh now we have 3 ways to parse these ...
There was a problem hiding this comment.
And all subtly different... Yeah I agree.
| // 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, | ||
| } |
There was a problem hiding this comment.
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.
| /// 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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I added a FIXME here too.
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... |
ChayimFriedman2
force-pushed
the
better-attrs
branch
from
b2ecf39 to
fccfb8c
Compare
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
force-pushed
the
better-attrs
branch
from
fccfb8c to
c42bd8d
Compare
|
Should we wait with this until the trait solver lands? It looks like it will cause a lot of conflicts |
|
The changes in hir-ty here are minimal, but I can wait until the switch to the next trait solver lands. |
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.