RFC: Add an attribute for raising the alignment of various items

[Jules-Bertholet]

trafficstars

Port C alignas to Rust.

Rendered

[traviscross]

We discussed this in the lang call today. We were feeling generally favorable about this, but all need to read it more closely.

[traviscross]

Having now had the chance to review this RFC thoroughly, I say that it's well written. Thanks to @Jules-Bertholet for that. It seems well motivated to me.

As mentioned above, I'd like for the document to speak more about the analysis of why one or another behavior is proper for async fn, and to speak to the arguments that would help us choose between let #[align(..)] mut x and let mut #[align(..)] x. I feel confident that whatever we do for placement here with respect to mut is also what we'd do with respect to other binding modes, so I'd prefer to just drop the restriction on other binding modes from this RFC. We could always later choose to partially stabilize.

If those things can perhaps be done, and the analysis with respect to async fn makes sense, then I plan to propose FCP merge on this RFC.

[Jules-Bertholet]

The arguments that would help us choose between let #[align(..)] mut x and let mut #[align(..)] x. I feel confident that whatever we do for placement here with respect to mut is also what we'd do with respect to other binding modes

This RFC proposes let #[align(…)] mut x, but I do not agree that we obviously want the same for ref and ref mut.

[traviscross]

This RFC proposes let #[align(…)] mut x, but I do not agree that we obviously want the same for ref and ref mut.

Then I'd appreciate if the RFC could discuss the reasons why we might want to make a different choice between these. It would surprise me a lot if we did, but I gather you must have a good argument in mind for this.

[traviscross]

cc @Nadrieril

[traviscross]

In my view, this RFC is well motivated and has a lot of correct details. Thanks to @Jules-Bertholet for writing this up. I think we should do it with one adjustment.

Proposal: Accept this RFC as written, modulo that:

As far as this RFC goes, we allow #[align(..)] on bindings with all binding modes, and we specify the syntactic grammar as:

IdentifierPattern -> OuterAttribute* `ref`? `mut`? IDENTIFIER ( `@` PatternNoTopAlt )?

Semantically, for the moment, we would disallow attributes other than align.

If maybe we want to hold back on allowing align semantically with non-move binding modes, I propose we consider that at stabilization time rather than as a carve-out in this RFC.

The current text contains an argument about how this question should be tied in with a potential decision we might later make about syntax for reference bindings where the binding itself is mutable, but I don't buy it. We might or might not ever do that, and even if we did, it wouldn't change my own feeling that all the tokens that control the binding mode should appear together in the grammar. I find the grammar above just too compelling.

In terms of whether OuterAttribute* should appear before or after the binding mode tokens, I've tried some code with it both ways, and I agree it makes more sense at the start of the IdentifierPattern.

@rfcbot fcp merge

[rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• [x] @joshtriplett
• [x] @nikomatsakis
• [x] @scottmcm
• [x] @tmandry
• [x] @traviscross

Concerns:

• specify-behavior-with-traits (https://github.com/rust-lang/rfcs/pull/3806#issuecomment-2992331685)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. See this document for info about what commands tagged team members can give me.

[Jules-Bertholet]

Two, the text specifies disallowing align with _ (non-)bindings, let #[align(..)] _ = ... This feels more like a warning to me, so I propose that we allow this and consider warning on it.

Strongly disagree. I’ve updated the RFC

[traviscross]

Thanks, yes, that's right. Adjusted the proposal.

[nikomatsakis]

@rfcbot reviewed

This makes sense to me. It seems like a capability we should have. I don't like using #[repr(align(22))] everywhere; it's too verbose and it's unclear what other "repr" attributes might make sense. We can always move that way later if needed.

I do recall us talking about #[align] in some other context where repr seemed odd -- I think on function items? I didn't notice whether they are covered by this RFC.

[rfcbot]

:bell: This is now entering its final comment period, as per the review above. :bell:

[folkertdev]

Functions are indeed included, in this section https://github.com/Jules-Bertholet/rfcs/blob/align-attr/text/3806-align-attr.md#on-function-items, and the stabilization PR I have up for alignment of functions is, for now, blocked on the decision here for whether to use #[repr(align(N))] or #[align(N)] or #[align = N]. This RFC proposes #[align(N)].

[tmandry]

@rfcbot reviewed

The part of this proposal that gives me the most pause is how it applies to async fn. It applies to the function returning the future, which is different from how #[inline] works, applying to the poll function. The justification given is reasonable: it should apply to the address of the named function as it does with other functions; any inconsistency here would lead to accidental UB. But it doesn't leave me completely satisfied; the inconsistency with #[inline] could just as well lead to confusion and accidental UB.

I think the RFC makes the right choice between the two options, but it leaves me wishing for a third.

repr(C) currently has two contradictory meanings: “a simple, linear layout algorithm that works the same everywhere” and “an ABI matching that of the target’s standard C compiler”. This RFC does not aim to resolve that conflict; that is being discussed as part of https://github.com/rust-lang/rfcs/pull/3718. Henceforth, we will use repr(C_for_real) to denote “match the system C compiler”, and repr(linear) to denote “simple, portable layout algorithm”; but those names are not normative.

This implies to me an unresolved question that needs to be resolved ahead of stabilization: What should the actual behavior be for repr(C)? Please add this to the RFC.

[Jules-Bertholet]

This implies to me an unresolved question that needs to be resolved ahead of stabilization: What should the actual behavior be for repr(C)? Please add this to the RFC.

Already there, under heading “MSVC”.

[workingjubilee]

Per my comments:

• https://github.com/rust-lang/rfcs/pull/3806#discussion_r2147021261
• https://github.com/rust-lang/rfcs/pull/3806#discussion_r2147022488

@rfcbot concern is-c-repr-enough-without-c++-semantics

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[traviscross]

@rfcbot concern specify-behavior-with-traits

As discussed in the stabilization of fn_align -- and thanks to @ehuss for catching this and pointing it out -- this RFC does not have language specifying the behavior of the attribute when applied to items within a trait definition. This is something we should specify.

I've asked @Jules-Bertholet to update the language in the RFC for this. I'll file a concern so that, on resolving it after we review this new language in a lang call, that we'll have restarted the FCP clock.

[Jules-Bertholet]

I’ve updated the RFC in response to comments received after FCP.

[scottmcm]

Thinking more and seeing updates, I think I'm fine to check a box here, subject to a note that I think this would be a really good thing to split into different features in the compiler for the different parts. I foresee stabilizing it on fields relatively promptly, but with much less urgency to figure it out for local variables, for example.

@rfcbot reviewed

[Jules-Bertholet]

@workingjubilee This feature is going to be implemented piecemeal anyway. Over-aligning trait impl methods can be considered a form of refinement (https://github.com/rust-lang/rust/issues/100706), and so could justifiably be held up until the rest of that feature is ready. But forbidding it forever would be inconsistent with allowing other forms of refinement.

[folkertdev]

That means that for now we should disallow it on function prototypes, right? E.g. inline currently behaves like that:

warning: `#[inline]` is ignored on function prototypes
 --> src/main.rs:2:5
  |
2 |     #[inline(always)]
  |     ^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_attributes)]` on by default

It doesn't look like refinement is moving that quickly, and currently the only attribute that I can find that is inherited from a trait is #[track_caller].

[Jules-Bertholet]

#[align(…)] is part of public API, #[inline] is an optimization hint.

[workingjubilee]

@Jules-Bertholet I am fine with us someday exploring it. I just know that #[align] is going to be stabilized almost immediately on functions after this, and I think that in principle, for refactoring purposes, you would definitely want to be able to go on a path from this:

#[align(16)]
fn something<T: Trait>(arg: &mut T) -> &mut T {
    // code
}

To this:

trait SomethingTrait: Trait {
    #[align(16)]
    fn call_me(&mut self) -> &mut Self;
}

impl<T> SomethingTrait for T where T: Trait {
    fn call_me(&mut self) -> &mut Self {
        // code
    }
}

That seems like a perfectly sensible evolution path, right? We would want parity there.

But I don't see any immediate value in these cases:

impl SomethingTrait for Type {
    #[align(32)]
    fn call_me(&mut self) -> &mut Self {
        // code
    }
}

impl SomethingTrait for OtherType {
    #[align(64)]
    fn call_me(&mut self) -> &mut Self {
        // code
    }
}

That would grant you a specific quality that is not present yet on the trait's generic API, which would be harder to reason about. So I don't think we should address the case of the impl refining the alignment immediately until we've got the rest of this nailed down hard.

And in general, people get very confused when an RFC is only in a partially-implemented state. It would be nice if we could at least be able to make clean and easy distinctions as to which parts are implemented, like "oh yes, it's completely implemented for functions and statics but not structs and locals".

[Jules-Bertholet]

I realized that this RFC currently fails to specify how #[align(…)] interacts with #[track_caller]. Unfortunately, it seems that #[track_caller] has problems (https://github.com/rust-lang/rust/issues/143162) that probably need to be resolved before I can make a proposal.

[Jules-Bertholet]

I’ve added a note that #[align(…)] is compatible with #[naked]. ~~(The compiler currently incorrectly forbids this.)~~ (Edit: it does work, I was using an old nightly)

[Jules-Bertholet]

I’ve added a note that #[align(…)] is allowed on function items in extern blocks.

[folkertdev]

I realized that this RFC currently fails to specify how #[align(…)] interacts with #[track_caller]. Unfortunately, it seems that #[track_caller] has problems (rust-lang/rust#143162) that probably need to be resolved before I can make a proposal.

I don't think that issue really matters? To me it makes most sense that the shim inherits the attribute, i.e. that both the shim and the real function are aligned to the given amount.

---

We're now using #[rustc_align] for the attribute because #[align] is already used as the name of procedural macros, causing conflicts when we add a builtin attribute with that name. How should we deal with that? It would be weird to accept an RFC that states that we'll use that attribute if it turns out that in practice it causes too much breakage, right? So should we do a proper crater run on that attribute name to make progress?

[Jules-Bertholet]

I don't think that issue really matters? To me it makes most sense that the shim inherits the attribute, i.e. that both the shim and the real function are aligned to the given amount.

I wasn’t certain whether the alignment of the “real function” would ever be observable. But it seems the answer is “yes” (https://github.com/rust-lang/rust/pull/145724). I’ve updated the RFC accordingly, and also noted another subtle interaction with FFI.

[Jules-Bertholet]

We're now using #[rustc_align] for the attribute because #[align] is already used as the name of procedural macros, causing conflicts when we add a builtin attribute with that name. How should we deal with that?

I’ve added a section on this.

[folkertdev]

I have static_align ready to go here https://github.com/rust-lang/rust/compare/master...folkertdev:rust:static-align, should I wait for the RFC to be re-approved, or can that be merged already.

Based on discussion so far everyone seemed on board with adding alignment on statics.