Add gated const item paths support for selected builtin attributes.

[BarronKane]

View all comments

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]

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

[rustbot]

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

[jieyouxu]

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

[tgross35]

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

[BarronKane]

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.

[nnethercote]

@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]

@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]

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]

@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]

@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]

@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?

[rustbot]

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.