Add gated const item paths support for selected builtin attributes. by BarronKane · Pull Request #154708 · rust-lang/rust

BarronKane · 2026-04-02T09:22:51Z

Relevant: #52840
Zulip thread: #t-lang > #[repr(align(cacheline))] Support

Currently, there is no way to include any kind of language-level expressions within attributes. For example, if you want per-platform alignment based on the cacheline size of that platform, you would have to make a struct that is cfg gated with repr(align(n)), and then use that struct elsewhere for cache aligned values. Cache locality is important for cross-platform systems design, instead of just repr(align(SOME_CONST)). In an ideal world if this feature moves forward and expands, it would allow arbitrary expressions that resolve at instantiation such as [pseudocode]: repr(align(some_value * 16)).

This pull request opens the door to resolving expressions within attributes, but for now this only resolves consts for both simplicity and to reduce initial weight and reviewer burden while keeping the semantic door open for further expansion. I added machinery to grab symbols from an attribute and defer resolution to rustc_ast_lowering where the value can be retrieved and then measured against the constraints of the attribute at the attribute location.

I also added tests related to const resolution specifically, and wired this in as an unstable feature matching the standards set forward by the project at large. This also includes feature gates, unpretty, and presumed error codes to match errors within the gates.

Currently this supports:

#[repr(align(CONST))]
#[repr(packed(CONST))]
#[rustc_align(CONST)]
#[rustc_align_static(CONST)]

This adds the following types:

AttrResolutionKind
AttrResolved
AttrResolution
AttrResolutionRequest

This adds the following functions:

AttributeParser::parse_limited_attr_resolution_requests_should_emit
AttributeParse::parse_limited_attr_resolution_requests
parse_alignment_or_const_path

Example:

#![feature(const_attr_paths)]

#[cfg(target_arch = "x86_64")]
const CACHELINE_BYTES: usize = 64;

#[cfg(all(target_arch = "arm", target_feature = "mclass"))]
const CACHELINE_BYTES: usize = 32;

#[cfg(not(any(
    target_arch = "x86_64",
    all(target_arch = "arm", target_feature = "mclass"),
)))]
compile_error!("Unsupported target for CACHELINE_BYTES");

#[repr(align(CACHELINE_BYTES))]
struct Foo(u8);

rustbot · 2026-04-02T09:22:55Z

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

rustbot · 2026-04-02T09:22:57Z

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: compiler
compiler expanded to 69 candidates
Random selection from 13 candidates

mejrs

Thanks for opening the door on resolving things in attributes. I actually want this for other things so it's nice to see happening.

There are a lot of names like AttrConstResolution, attr_const_res_map, attr_const_resolution etc being used. I'm worried that if someone wants to extend this to resolve other things (not constants) they'll have to change all those names. Can you choose a more general naming scheme? (let's wait for consensus before making big changes though)

For example maybe just drop the const part everywhere and change AttrConstResolution to

enum AttrResolution {
     Const(...)
}

so others can add variants as needed.

I can't comment on the name resolution part - I'm not familiar with it.

View changes since this review

mejrs · 2026-04-02T13:23:40Z

compiler/rustc_resolve/src/late.rs

+    fn resolve_attr_const_paths(&mut self, attr: &'ast Attribute) {
+        match attr.name() {
+            Some(sym::repr) => {
+                let Some(items) = attr.meta_item_list() else {
+                    return;
+                };
+                for item in &items {
+                    let Some(meta) = item.meta_item() else {
+                        continue;
+                    };
+                    let Some(name) = meta.name() else {
+                        continue;
+                    };
+                    if !matches!(name, sym::align | sym::packed) {
+                        continue;
+                    }
+                    let Some([arg]) = meta.meta_item_list() else {
+                        continue;
+                    };
+                    let Some(path_meta) = arg.meta_item() else {
+                        continue;
+                    };
+                    if !path_meta.is_word() {
+                        continue;
+                    }
+                    if !self.should_resolve_attr_const_path(path_meta.path.span) {
+                        continue;
+                    }
+                    self.resolve_attr_const_path(attr.id, &path_meta.path);
+                }
+            }
+            Some(sym::rustc_align | sym::rustc_align_static) => {
+                let Some(items) = attr.meta_item_list() else {
+                    return;
+                };
+                let [arg] = items.as_slice() else {
+                    return;
+                };
+                let Some(path_meta) = arg.meta_item() else {
+                    return;
+                };
+                if !path_meta.is_word() {
+                    return;
+                }
+                if !self.should_resolve_attr_const_path(path_meta.path.span) {
+                    return;
+                }
+                self.resolve_attr_const_path(attr.id, &path_meta.path);
+            }
+            _ => {}
+        }


I strongly worry how this creates another place where an attribute is parsed. This is the exact thing we want to eliminate with the attribute parsing rework. Is it possible to use AttributeParser::parse_limited_should_emit here?

I missed that! I'll investigate. Do you want me to go forward in that direction or wait until a review threshold?

Please do go ahead. We definitely do not want this sort of duplicated parsing code.

Addressed! I folded everything into the AttributeParser machinery.

jieyouxu · 2026-04-02T14:05:05Z

Have a few more PRs that I need to investigate, don't have bandwidth to review this any time soon.
@rustbot reroll

tgross35 · 2026-04-02T18:04:32Z

Note that this still needs approval from the lang team as an experiment before going forward.

tgross35 · 2026-04-02T18:15:48Z

compiler/rustc_hir/src/attrs/data_structures.rs

+#[derive(PartialEq, Eq, Debug, Encodable, Decodable, Copy, Clone, HashStable_Generic)]
+pub enum AttrIntValue {
+    Lit(u128),
+    Const { def_id: DefId, span: Span },
+}
+
+#[derive(Debug, Copy, Clone, HashStable_Generic, Encodable, Decodable, PrintAttribute)]
+pub enum AttrConstResolved<Id = ast::NodeId> {
+    Resolved(Res<Id>),
+    Error,
+}
+
+#[derive(Debug, Copy, Clone, HashStable_Generic, Encodable, Decodable, PrintAttribute)]
+pub struct AttrConstResolution<Id = ast::NodeId> {
+    pub path_span: Span,
+    pub resolved: AttrConstResolved<Id>,
 }


Please make sure to document new types and functions you add, this isn't completely obvious. (I realize this is pretty much a draft, just something to do before merge)

Do you mean in the PR or in another part of the rustlang solution tree?

By "before merge“ I did mean here

BarronKane · 2026-04-03T05:47:34Z

Thanks for opening the door on resolving things in attributes. I actually want this for other things so it's nice to see happening.

There are a lot of names like AttrConstResolution, attr_const_res_map, attr_const_resolution etc being used. I'm worried that if someone wants to extend this to resolve other things (not constants) they'll have to change all those names. Can you choose a more general naming scheme? (let's wait for consensus before making big changes though)

For example maybe just drop the const part everywhere and change AttrConstResolution to
enum AttrResolution {
     Const(...)
}
so others can add variants as needed.

I can't comment on the name resolution part - I'm not familiar with it.

View changes since this review

Thanks! Yea as I get further and further into critical-safe aware library writing (offline I'm doing slab-allocator basaed green-thread fiber stacks, with a script that crawls the ELF for exact slab sizing for stack-based futures and async, no more pin box dyn trait), I'm finding more and more machinery that I think really should be rust-native. Being able to build per-target alignment is a BIG one, but I have a lot of others cooking.

I've taken the suggestions and code reviews I've gotten, and am about to push. I generally like the direction this has ended up going rather than my initial ideas.

BarronKane · 2026-04-03T07:55:39Z

☔ The latest upstream changes (presumably #154740) made this pull request unmergeable. Please resolve the merge conflicts.

This timing is diabolical. Fixing.

nnethercote · 2026-04-07T05:15:56Z

@BarronKane: I will probably pass this review on to somebody else because it's not an area of expertise for me. Before I do that, I have some questions. The PR description feels like is was generated by an LLM, was it? Also, this is your first PR and it's an usually complex and large PR for a first-timer, so I am wondering if you used an LLM for the code as well?

BarronKane · 2026-04-07T05:53:17Z

@BarronKane: I will probably pass this review on to somebody else because it's not an area of expertise for me. Before I do that, I have some questions. The PR description feels like is was generated by an LLM, was it? Also, this is your first PR and it's an usually complex and large PR for a first-timer, so I am wondering if you used an LLM for the code as well?

Hi, @nnethercote!

I do use LLMs as a tool for exploration, reviews, and ultimately to understand the pipeline as a whole. The code and approach are mine, and I did use assisted reviews for both code and the PR body to remain as correct and as unobtrusive to the project at large as possible.

My original intention was target-bound constants that resolved at parse time, which I did basically all myself to learn, but the machinery for actual expression resolution within attributes otherwise just wasn't there. I used LLMs to explore and gain deeper understanding of the tooling and compiler at large, and constructed an approach and wrote the code myself while reviewing and cross checking against the codebase and other PRs at large in order to learn the machinery myself. I reworked everything, including my approach, once I learned the lang team might want to go towards actual resolution inside attributes, and that required a lot more knowledge than I started with. Note: I intentionally scoped this to JUST const resolution to avoid trying to do everything at once, which aligns with the original intent, so as not to burden review and to allow incremental progress as desired.

My personal motivations include a very large personal project where I'm working on learning lower level systems development starting with the cortex-m, and sync/atomic primitives that work on a variety of systems including desktop and cortex-m. Not having compile time resolution of cacheline padding was a significant pain point for that alone. That's what brought me here.

Ultimately my goal is to learn and grow as an engineer.

BarronKane · 2026-04-07T07:51:32Z

Edit: Relocated to local code comment.

BarronKane · 2026-04-07T08:04:08Z

compiler/rustc_attr_parsing/src/attributes/repr.rs

+        // `parse_limited(sym::repr)` runs before lowering for callers that only care whether
+        // `repr(packed(...))` exists at all.
+        if matches!(cx.stage.should_emit(), ShouldEmit::Nothing) {
+            return Ok(AttrIntValue::Lit(1));


For who does end up reviewing this PR, I'm taking another look at things, and I want to note that in fn parse_alignment_or_const_path<S: Stage>, I'm returning 1 on ShouldEmit::Nothing. This was a deliberate decision because repr(packed) itself returns 1 by default elsewhere, and is effectively no-op otherwise, but part of not being obtrusive to the codebase I shimmed it in there in order to avoid a heavier refactor of repr.rs to make it typed. Categorically it is fake data, but I think it's defensible from this stance. If the lang and compiler teams want to go this direction of resolutions within attributes, this should be pulled out for a better solution so future work doesn't trip over it. Deferred state should be represented properly, so I wanted to note this now.

nnethercote · 2026-04-07T09:36:59Z

Thanks for the response. Can you rewrite the PR description to be shorter and less LLM-y? E.g. it should describe what's happening and why, but doesn't need to include every single details of what's changed -- a reviewer can just look at the diff for that. And PR descriptions usually don't include section headings. Thanks!

BarronKane · 2026-04-07T09:51:31Z

Thanks for the response. Can you rewrite the PR description to be shorter and less LLM-y? E.g. it should describe what's happening and why, but doesn't need to include every single details of what's changed -- a reviewer can just look at the diff for that. And PR descriptions usually don't include section headings. Thanks!

Absolutely! My thought was to keep it structured and formal, but I don't mind rewriting it with this in mind.

It was requested I document new types/functions, however, so I'll leave that at the bottom along with the example.

BarronKane · 2026-04-07T10:13:49Z

@nnethercote It's updated, please let me know if it's still malformed to taste. I wrote the new body by hand.

workingjubilee · 2026-04-07T18:05:02Z

We are no strangers to structured formality, but every PR description becomes a commit message in this repository when your PR is merged. It is longstanding convention that commit messages keep themselves brief but descriptive, so that people who are reading it later do not spend undue time wading through prose and instead can understand what it means as fast as possible.

jackh726 · 2026-04-07T19:03:45Z

It is longstanding convention that commit messages keep themselves brief but descriptive

This may be true often, but demonstrably doesn't always hold true (e.g. #147022, #152710, #153912, #153489, #153738, #153677).

BarronKane · 2026-04-07T19:12:34Z

We are no strangers to structured formality, but every PR description becomes a commit message in this repository when your PR is merged. It is longstanding convention that commit messages keep themselves brief but descriptive, so that people who are reading it later do not spend undue time wading through prose and instead can understand what it means as fast as possible.

Ahhh I see what you mean, I'll edit it further when I'm home tonight and pull out the rest of the fluff.

programmerjake · 2026-04-07T19:37:36Z

It is longstanding convention that commit messages keep themselves brief but descriptive

This may be true often, but demonstrably does always hold true (e.g. #147022, #152710, #153912, #153489, #153738, #153677).

I think you meant "doesn't".

BarronKane · 2026-04-07T20:10:21Z

It is longstanding convention that commit messages keep themselves brief but descriptive

This may be true often, but demonstrably doesn't always hold true (e.g. #147022, #152710, #153912, #153489, #153738, #153677).

Is the commit message ultimately manual, then?

I feel there should be a separation of concern between PRs and the commit message for the very fact that we have markdown for representing data in different ways within GitHub.

Either that or a more explicit guideline.

workingjubilee · 2026-04-07T22:56:06Z

@jackh726 I neither claimed it always held true, nor did I even suggest two goals could not be in tension. Indeed, the joining of them with "but" is usually used in English to directly suggest that they are. Sometimes a description must run long because to make it brief is to reduce the useful information to the point of uselessness. Likewise, sometimes a description being long also makes it less useful, because it creates noise without signal.

@BarronKane Do you mean the one for the PR's merge commit? No. It is because it is not manual that I note that the PR's description is probably better off reduced. The merge commits are generated by our automation.

BarronKane · 2026-04-07T22:59:58Z

@jackh726 I neither claimed it always held true, nor did I even suggest two goals could not be in tension. Indeed, the joining of them with "but" is usually used in English to directly suggest that they are. Sometimes a description must run long because to make it brief is to reduce the useful information to the point of uselessness. Likewise, sometimes a description being long also makes it less useful, because it creates noise without signal.

@BarronKane Do you mean the one for the PR's merge commit? No. It is because it is not manual that I note that the PR's description is probably better off reduced. The merge commits are generated by our automation.

Gotcha, thanks!

That was my primary concern if it was automated, because that gives everyone a but less precision for how PRs enter the trunk.

Alright, I'll do so and turn the pr body to conform to a commit style message rather than representing the changes as a report.

BarronKane · 2026-04-08T04:53:16Z

@workingjubilee I got rid of the markdown sugar, and made the body more or less commit message friendly. Anything else I should pull out explicitely?

Add gated support for const item paths in selected builtin attributes, including `repr(align)`, `repr(packed)`, `rustc_align`, and `rustc_align_static`. Resolve attribute const paths during late resolution, carry them through HIR as attr int values, and evaluate them at the layout/codegen use sites. This keeps the feature scoped to const item paths without adding general expression support in attributes. Also adds UI/codegen/incremental coverage and fixes the gated-syntax diagnostics so unresolved names do not leak before the feature gate.

rustbot · 2026-04-09T07:04:47Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) 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 Apr 2, 2026

rustbot assigned jieyouxu Apr 2, 2026