Warn on codegen attributes on required trait methods #148756
Merged
+154
−86
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
|
Some changes occurred in compiler/rustc_attr_parsing |
rustbot
added
A-attributes
Collaborator
rustbot
added
the
I-lang-nominated
JonathanBrouwer
changed the title
traviscross
added
the
P-lang-drag-1
Contributor
|
r? jdonszelmann |
Contributor
|
@rfcbot fcp merge lang |
Collaborator
|
Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns:
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. |
rust-rfcbot
added
proposed-final-comment-period
Member
|
@rfcbot reviewed |
rust-rfcbot
added
final-comment-period
Collaborator
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
Member
|
So, I'd like to file a concern here that we should do a crater run on this before landing. Here's my reasoning: This is landing as a FCW, which means that the team wants to remove this behavior, In my mind, this means that the team should know what the barriers are for that. If, for example, crater says that nobody is relying on it, it may mean that we want to move rather fast on that. If crater shows that a bunch of crates are relying on this, we should know that to be able to plan (I think Niko mentioned e.g. making the change over an edition in that case would be a possibility). Essentially, the concern from me is that a FCW here indicates it's important enough for people to think about, and I think that the lang team should have the data about who it effects. And, not blocking landing this on that data runs the (high) risk of it falling through the cracks. |
Contributor
|
@rfcbot concern jackh726-concern |
rust-rfcbot
added
proposed-final-comment-period
traviscross
added
the
I-lang-radar
Contributor
|
@JonathanBrouwer could you make a parallel PR that makes this a full error, then we crater that. Or at least I believe we wouldn't see this warning in a crate run right? |
rust-bors bot
added a commit
that referenced
this pull request
Nov 20, 2025
… r=<try> Crater for #148756
Contributor
Author
|
Crater run will happen here: #149137 |
Closed
rust-rfcbot
added
finished-final-comment-period
Collaborator
|
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. |
Signed-off-by: Jonathan Brouwer <[email protected]>
Signed-off-by: Jonathan Brouwer <[email protected]>
JonathanBrouwer
force-pushed
the
link_section_targets2
branch
from
c49ff7a to
8fa10c0
Compare
Collaborator
|
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. |
Contributor
Author
|
@rustbot ready |
rustbot
added
the
S-waiting-on-review
JonathanBrouwer
removed
the
S-waiting-on-fcp
Contributor
|
@bors r+ rollup |
Collaborator
bors
added
S-waiting-on-bors
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Dec 15, 2025
…ts2, r=jdonszelmann Warn on codegen attributes on required trait methods This PR turns applying the following attributes on required trait methods (that is, trait methods **without** a default implementation) into a FCW: - `#[cold]` - `#[link_section]` - `#[linkage]` (unstable) - `#[rustc_allow_const_fn_unstable]` (internal attribute) These attributes already had no effect when applied to a required trait method, this PR only adds a warning. Furthermore, it adds a comment in the code that the following codegen attributes are *inherited* when applied to a required trait method: - `#[track_caller]` - `#[align]` (unstable) `@rustbot` labels +I-lang-nominated `@rust-lang/lang` Two questions for the lang team: - Is adding this warning ok? - Does the current behaviour of these attributes align with that you would expect them to be? Fixes rust-lang#147432
bors
added a commit
that referenced
this pull request
Dec 15, 2025
Rollup of 9 pull requests Successful merges: - #148756 (Warn on codegen attributes on required trait methods) - #148790 (Add new Tier-3 target: riscv64im-unknown-none-elf) - #149271 (feat: dlopen Enzyme) - #149354 (Bootstrap config: libgccjit libs dir) - #149459 (std: sys: fs: uefi: Implement set_times and set_perm) - #149950 (Simplify how inline asm handles `MaybeUninit`) - #150000 (Port `#[rustc_legacy_const_generics]` to use attribute parser ) - #150014 (Metadata loader cleanups) - #150021 (document that mpmc channels deliver an item to (at most) one receiver) r? `@ghost` `@rustbot` modify labels: rollup
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Dec 15, 2025
…ts2, r=jdonszelmann Warn on codegen attributes on required trait methods This PR turns applying the following attributes on required trait methods (that is, trait methods **without** a default implementation) into a FCW: - `#[cold]` - `#[link_section]` - `#[linkage]` (unstable) - `#[rustc_allow_const_fn_unstable]` (internal attribute) These attributes already had no effect when applied to a required trait method, this PR only adds a warning. Furthermore, it adds a comment in the code that the following codegen attributes are *inherited* when applied to a required trait method: - `#[track_caller]` - `#[align]` (unstable) ``@rustbot`` labels +I-lang-nominated ``@rust-lang/lang`` Two questions for the lang team: - Is adding this warning ok? - Does the current behaviour of these attributes align with that you would expect them to be? Fixes rust-lang#147432
bors
added a commit
that referenced
this pull request
Dec 15, 2025
Rollup of 8 pull requests Successful merges: - #148756 (Warn on codegen attributes on required trait methods) - #148790 (Add new Tier-3 target: riscv64im-unknown-none-elf) - #149271 (feat: dlopen Enzyme) - #149459 (std: sys: fs: uefi: Implement set_times and set_perm) - #149950 (Simplify how inline asm handles `MaybeUninit`) - #150000 (Port `#[rustc_legacy_const_generics]` to use attribute parser ) - #150014 (Metadata loader cleanups) - #150021 (document that mpmc channels deliver an item to (at most) one receiver) r? `@ghost` `@rustbot` modify labels: rollup
Zalathar
added a commit
to Zalathar/rust
that referenced
this pull request
Dec 16, 2025
…ts2, r=jdonszelmann Warn on codegen attributes on required trait methods This PR turns applying the following attributes on required trait methods (that is, trait methods **without** a default implementation) into a FCW: - `#[cold]` - `#[link_section]` - `#[linkage]` (unstable) - `#[rustc_allow_const_fn_unstable]` (internal attribute) These attributes already had no effect when applied to a required trait method, this PR only adds a warning. Furthermore, it adds a comment in the code that the following codegen attributes are *inherited* when applied to a required trait method: - `#[track_caller]` - `#[align]` (unstable) ```@rustbot``` labels +I-lang-nominated ```@rust-lang/lang``` Two questions for the lang team: - Is adding this warning ok? - Does the current behaviour of these attributes align with that you would expect them to be? Fixes rust-lang#147432
bors
added a commit
that referenced
this pull request
Dec 16, 2025
Rollup of 14 pull requests Successful merges: - #148756 (Warn on codegen attributes on required trait methods) - #148790 (Add new Tier-3 target: riscv64im-unknown-none-elf) - #149271 (feat: dlopen Enzyme) - #149459 (std: sys: fs: uefi: Implement set_times and set_perm) - #149771 (bootstrap readme: make easy to read when editor wrapping is not enabled) - #149856 (Provide an extended framework for type visit, for use in rust-analyzer) - #149950 (Simplify how inline asm handles `MaybeUninit`) - #150014 (Metadata loader cleanups) - #150021 (document that mpmc channels deliver an item to (at most) one receiver) - #150022 (Generate macro expansion for rust compiler crates docs) - #150029 (Update books) - #150031 (assert impossible branch is impossible) - #150034 (do not add `I-prioritize` when `F-*` labels are present) - #150036 (Use the embeddable filename for coverage artifacts) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
that referenced
this pull request
Dec 16, 2025
Rollup of 13 pull requests Successful merges: - #148756 (Warn on codegen attributes on required trait methods) - #148790 (Add new Tier-3 target: riscv64im-unknown-none-elf) - #149271 (feat: dlopen Enzyme) - #149459 (std: sys: fs: uefi: Implement set_times and set_perm) - #149771 (bootstrap readme: make easy to read when editor wrapping is not enabled) - #149856 (Provide an extended framework for type visit, for use in rust-analyzer) - #149950 (Simplify how inline asm handles `MaybeUninit`) - #150014 (Metadata loader cleanups) - #150021 (document that mpmc channels deliver an item to (at most) one receiver) - #150029 (Update books) - #150031 (assert impossible branch is impossible) - #150034 (do not add `I-prioritize` when `F-*` labels are present) - #150036 (Use the embeddable filename for coverage artifacts) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
that referenced
this pull request
Dec 16, 2025
Rollup merge of #148756 - JonathanBrouwer:link_section_targets2, r=jdonszelmann Warn on codegen attributes on required trait methods This PR turns applying the following attributes on required trait methods (that is, trait methods **without** a default implementation) into a FCW: - `#[cold]` - `#[link_section]` - `#[linkage]` (unstable) - `#[rustc_allow_const_fn_unstable]` (internal attribute) These attributes already had no effect when applied to a required trait method, this PR only adds a warning. Furthermore, it adds a comment in the code that the following codegen attributes are *inherited* when applied to a required trait method: - `#[track_caller]` - `#[align]` (unstable) ````@rustbot```` labels +I-lang-nominated ````@rust-lang/lang```` Two questions for the lang team: - Is adding this warning ok? - Does the current behaviour of these attributes align with that you would expect them to be? Fixes #147432
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR turns applying the following attributes on required trait methods (that is, trait methods without a default implementation) into a FCW:
#[cold]#[link_section]#[linkage](unstable)#[rustc_allow_const_fn_unstable](internal attribute)These attributes already had no effect when applied to a required trait method, this PR only adds a warning.
Furthermore, it adds a comment in the code that the following codegen attributes are inherited when applied to a required trait method:
#[track_caller]#[align](unstable)@rustbot labels +I-lang-nominated
@rust-lang/lang
Two questions for the lang team:
Fixes #147432