warn on align on fields to avoid breaking changes by jdonszelmann · Pull Request #143868 · rust-lang/rust

jdonszelmann · 2025-07-13T06:14:42Z

rustbot · 2025-07-13T06:14:46Z

workingjubilee is currently at their maximum review capacity.
They may take a while to respond.

jieyouxu · 2025-07-13T07:25:23Z

compiler/rustc_passes/src/check_attr.rs

+            Target::Field => {
+                self.tcx.emit_node_span_lint(
+                    UNUSED_ATTRIBUTES,
+                    hir_id,
+                    attr_span,
+                    AlignOnFields { span },
+                );
+            }


Question: do we already have test coverage for this case?

We have about 20 of these cases and to the best of my knowledge no test coverage for any :ferris clueless:

Don't really have time to atm, am on holiday. Just knew how to fix this issue well

Yes, we may wish to land this and maybe open an issue about getting test coverage.

feel free to assign me or @JonathanBrouwer (who I think was interested in this sort of thing)

workingjubilee · 2025-07-13T23:03:21Z

I'm landing this to fix this right now on nightly, unsure if this is the right fix for beta.

@bors r+ rollup

bors · 2025-07-13T23:03:23Z

📌 Commit a5ab682 has been approved by workingjubilee

It is now in the queue for this repository.

…r=workingjubilee warn on align on fields to avoid breaking changes r? `@workingjubilee`

Rollup of 16 pull requests Successful merges: - #142885 (core: Add `BorrowedCursor::with_unfilled_buf`) - #143217 (Port #[link_ordinal] to the new attribute parsing infrastructure) - #143355 (wrapping shift: remove first bitmask and table) - #143448 (remote-test-client: Exit code `128 + <signal-number>` instead of `3`) - #143592 (UWP: link ntdll functions using raw-dylib) - #143681 (bootstrap/miri: avoid rebuilds for test builds) - #143710 (Updates to random number generation APIs) - #143724 (Tidy cleanup) - #143820 (Fixed a core crate compilation failure when enabling the `optimize_for_size` feature on some targets) - #143850 (Compiletest: Simplify {Html,Json}DocCk directive handling) - #143855 (Port `#[omit_gdb_pretty_printer_section]` to the new attribute parsing) - #143868 (warn on align on fields to avoid breaking changes) - #143875 (update issue number for `const_trait_impl`) - #143881 (Use zero for initialized Once state) - #143887 (Run bootstrap tests sooner in the `x test` pipeline) - #143893 (Don't require `eh_personality` lang item on targets that have a personality) Failed merges: - #143878 (Port `#[pointee]` to the new attribute parsing infrastructure) - #143891 (Port `#[coverage]` to the new attribute system) r? `@ghost` `@rustbot` modify labels: rollup

…r=workingjubilee warn on align on fields to avoid breaking changes r? ``@workingjubilee``

Rollup of 17 pull requests Successful merges: - #142885 (core: Add `BorrowedCursor::with_unfilled_buf`) - #143217 (Port #[link_ordinal] to the new attribute parsing infrastructure) - #143355 (wrapping shift: remove first bitmask and table) - #143448 (remote-test-client: Exit code `128 + <signal-number>` instead of `3`) - #143681 (bootstrap/miri: avoid rebuilds for test builds) - #143710 (Updates to random number generation APIs) - #143724 (Tidy cleanup) - #143738 (Move several float tests to floats/mod.rs) - #143820 (Fixed a core crate compilation failure when enabling the `optimize_for_size` feature on some targets) - #143850 (Compiletest: Simplify {Html,Json}DocCk directive handling) - #143855 (Port `#[omit_gdb_pretty_printer_section]` to the new attribute parsing) - #143868 (warn on align on fields to avoid breaking changes) - #143875 (update issue number for `const_trait_impl`) - #143881 (Use zero for initialized Once state) - #143887 (Run bootstrap tests sooner in the `x test` pipeline) - #143893 (Don't require `eh_personality` lang item on targets that have a personality) - #143901 (Region constraint nits) Failed merges: - #143878 (Port `#[pointee]` to the new attribute parsing infrastructure) - #143891 (Port `#[coverage]` to the new attribute system) r? `@ghost` `@rustbot` modify labels: rollup

jieyouxu · 2025-07-14T10:48:12Z

compiler/rustc_passes/src/check_attr.rs

+            Target::Field => {
+                self.tcx.emit_node_span_lint(
+                    UNUSED_ATTRIBUTES,
+                    hir_id,
+                    attr_span,
+                    AlignOnFields { span },
+                );
+            }


Question: wait hold on, what does this PR intend to do? If the intention was to not error on #[align] in various positions where we previously did not do, then this PR doesn't do that, because #142507 is a breaking change even if #[align] is an unstable built-in attribute, because unstable built-in attributes still participate in name resolution, and thus will still cause the ambiguity. This PR additionally produces another warning (i.e. unused_attributes) but does not prevent the name resolution ambiguity error from happening, cf. #134963.

Counter-example (I tried this test which fails on master, and also the commit from this PR, ignoring the irrelevant unstable fn_align feature gate warnings):

// tests/ui/whatever_subdir/test.rs //@ proc-macro: my_derive.rs //@ edition: 2024 use my_derive::MyDerive; #[derive(MyDerive)] #[align] //~^ ERROR `align` is ambiguous pub struct Foo; fn main() {}

// tests/ui/whatever_subdir/auxiliary/my_derive.rs //@ edition: 2024 extern crate proc_macro; use proc_macro::{Span, TokenStream}; #[proc_macro_derive( MyDerive, attributes( align ) )] pub fn derive_custom(_item: TokenStream) -> TokenStream { TokenStream::new() }

I think if we wanted to prevent that ambiguity error from happening entirely, we'd need to pick one of the following options (or more) or some alternative solution:

Have to fix how built-in attributes are resolved versus user attributes

Revert use #[align] attribute for fn_align #142507

Rename #[align] to something less common.

(That is, I think this PR itself is correct, but doesn't address #143834.)

hrm. I misunderstood something then, sorry.

hrm. I misunderstood something then, sorry.

In some sense, this PR is itself correct -- I believe we should be warning on this unsupported position (on fields). It's just that, this warning does not prevent the name resolution ambiguity from occurring, unfortunately.

Rollup of 8 pull requests Successful merges: - #141809 (Don't call WSACleanup on process exit) - #143710 (Updates to random number generation APIs) - #143848 (Rename `stable_mir` and `rustc_smir`) - #143855 (Port `#[omit_gdb_pretty_printer_section]` to the new attribute parsing) - #143868 (warn on align on fields to avoid breaking changes) - #143870 ([COMPILETEST-UNTANGLE 6/N] Use `TestSuite` enum instead of stringly-typed test suites) - #143901 (Region constraint nits) - #143903 (Fix typos in documentation files) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #143868 - jdonszelmann:fix-align-on-fields, r=workingjubilee warn on align on fields to avoid breaking changes r? `@workingjubilee`

Rollup of 8 pull requests Successful merges: - rust-lang/rust#141809 (Don't call WSACleanup on process exit) - rust-lang/rust#143710 (Updates to random number generation APIs) - rust-lang/rust#143848 (Rename `stable_mir` and `rustc_smir`) - rust-lang/rust#143855 (Port `#[omit_gdb_pretty_printer_section]` to the new attribute parsing) - rust-lang/rust#143868 (warn on align on fields to avoid breaking changes) - rust-lang/rust#143870 ([COMPILETEST-UNTANGLE 6/N] Use `TestSuite` enum instead of stringly-typed test suites) - rust-lang/rust#143901 (Region constraint nits) - rust-lang/rust#143903 (Fix typos in documentation files) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned workingjubilee Jul 13, 2025

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 Jul 13, 2025

jdonszelmann mentioned this pull request Jul 13, 2025

Regression: align is ambiguous #143834

Closed

This comment has been minimized.

Sign in to view

warn on align on fields to avoid breaking changes

a5ab682

jdonszelmann force-pushed the fix-align-on-fields branch from e38322a to a5ab682 Compare July 13, 2025 06:26

jieyouxu reviewed Jul 13, 2025

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 13, 2025

workingjubilee mentioned this pull request Jul 13, 2025

Need test coverage for attr edge-cases (ambiguity, ignored positions, etc) #143911

Open

fmease added a commit to fmease/rust that referenced this pull request Jul 14, 2025

Rollup merge of rust-lang#143868 - jdonszelmann:fix-align-on-fields, …

98303f9

…r=workingjubilee warn on align on fields to avoid breaking changes r? `@workingjubilee`

fmease mentioned this pull request Jul 14, 2025

Rollup of 16 pull requests #143912

Closed

jhpratt added a commit to jhpratt/rust that referenced this pull request Jul 14, 2025

Rollup merge of rust-lang#143868 - jdonszelmann:fix-align-on-fields, …

7f7f2d4

…r=workingjubilee warn on align on fields to avoid breaking changes r? ``@workingjubilee``

jhpratt mentioned this pull request Jul 14, 2025

Rollup of 17 pull requests #143916

Closed

jieyouxu reviewed Jul 14, 2025

View reviewed changes

samueltardieu mentioned this pull request Jul 14, 2025

Rollup of 8 pull requests #143934

Merged

bors merged commit 16b3f47 into rust-lang:master Jul 14, 2025
11 checks passed

rustbot added this to the 1.90.0 milestone Jul 14, 2025

rust-timer added a commit that referenced this pull request Jul 14, 2025

Unrolled build for #143868

12a58fc

Rollup merge of #143868 - jdonszelmann:fix-align-on-fields, r=workingjubilee warn on align on fields to avoid breaking changes r? `@workingjubilee`

matthiaskrgr mentioned this pull request Jul 15, 2025

ICE: failed while formatting fluent string passes_align_on_fields: #143987

Closed

Uh oh!

Conversation

jdonszelmann commented Jul 13, 2025

Uh oh!

rustbot commented Jul 13, 2025

Uh oh!

This comment has been minimized.

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!

workingjubilee commented Jul 13, 2025

Uh oh!

bors commented Jul 13, 2025

Uh oh!

jieyouxu Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jieyouxu Jul 14, 2025 •

edited

Loading